diff --git a/include/spdk/blob.h b/include/spdk/blob.h index 445032c48..f63f8ee21 100644 --- a/include/spdk/blob.h +++ b/include/spdk/blob.h @@ -132,6 +132,9 @@ typedef void (*spdk_blob_op_with_bs_dev)(void *cb_arg, struct spdk_bs_dev *bs_de * callback registered with the blobstore to create the external snapshot device. The blobstore * consumer must set this while loading the blobstore if it intends to support external snapshots. * + * If the blobstore consumer does not wish to load an external snapshot, it should set *bs_dev to + * NULL and return 0. + * * \param bs_ctx Context provided by the blobstore consumer via esnap_ctx member of struct * spdk_bs_opts. * \param blob_ctx Context provided to spdk_bs_open_ext() via esnap_ctx member of struct diff --git a/include/spdk_internal/lvolstore.h b/include/spdk_internal/lvolstore.h index 770a7bed5..6a9b9aca6 100644 --- a/include/spdk_internal/lvolstore.h +++ b/include/spdk_internal/lvolstore.h @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: BSD-3-Clause * Copyright (C) 2017 Intel Corporation. * All rights reserved. + * Copyright (c) 2022-2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. */ #ifndef SPDK_INTERNAL_LVOLSTORE_H @@ -74,10 +75,12 @@ struct spdk_lvol_store { TAILQ_HEAD(, spdk_lvol) lvols; TAILQ_HEAD(, spdk_lvol) pending_lvols; TAILQ_HEAD(, spdk_lvol) retry_open_lvols; + bool load_esnaps; bool on_list; TAILQ_ENTRY(spdk_lvol_store) link; char name[SPDK_LVS_NAME_MAX]; char new_name[SPDK_LVS_NAME_MAX]; + spdk_bs_esnap_dev_create esnap_bs_dev_create; }; struct spdk_lvol { diff --git a/lib/blob/blobstore.c b/lib/blob/blobstore.c index c9ec769f0..75e512f5f 100644 --- a/lib/blob/blobstore.c +++ b/lib/blob/blobstore.c @@ -372,6 +372,20 @@ blob_back_bs_destroy_esnap_done(void *ctx, struct spdk_blob *blob, int bserrno) assert(false); } + if (bs_dev == NULL) { + /* + * This check exists to make scanbuild happy. + * + * blob->back_bs_dev for an esnap is NULL during the first iteration of blobs while + * the blobstore is being loaded. It could also be NULL if there was an error + * opening the esnap device. In each of these cases, no channels could have been + * created because back_bs_dev->create_channel() would have led to a NULL pointer + * deref. + */ + assert(false); + return; + } + SPDK_DEBUGLOG(blob_esnap, "blob 0x%" PRIx64 ": calling destroy on back_bs_dev\n", blob->id); bs_dev->destroy(bs_dev); } @@ -1420,14 +1434,19 @@ blob_load_esnap(struct spdk_blob *blob, void *blob_ctx) return rc; } - assert(bs_dev != NULL); - SPDK_DEBUGLOG(blob_esnap, "blob 0x%" PRIx64 ": loaded back_bs_dev\n", blob->id); - if ((bs->io_unit_size % bs_dev->blocklen) != 0) { - SPDK_NOTICELOG("blob 0x%" PRIx64 " external snapshot device block size %u is not " - "compatible with blobstore block size %u\n", - blob->id, bs_dev->blocklen, bs->io_unit_size); - bs_dev->destroy(bs_dev); - return -EINVAL; + /* + * Note: bs_dev might be NULL if the consumer chose to not open the external snapshot. + * This especially might happen during spdk_bs_load() iteration. + */ + if (bs_dev != NULL) { + SPDK_DEBUGLOG(blob_esnap, "blob 0x%" PRIx64 ": loaded back_bs_dev\n", blob->id); + if ((bs->io_unit_size % bs_dev->blocklen) != 0) { + SPDK_NOTICELOG("blob 0x%" PRIx64 " external snapshot device block size %u " + "is not compatible with blobstore block size %u\n", + blob->id, bs_dev->blocklen, bs->io_unit_size); + bs_dev->destroy(bs_dev); + return -EINVAL; + } } blob->back_bs_dev = bs_dev; @@ -1447,7 +1466,6 @@ blob_load_backing_dev(spdk_bs_sequence_t *seq, void *cb_arg) if (blob_is_esnap_clone(blob)) { rc = blob_load_esnap(blob, seq->cpl.u.blob_handle.esnap_ctx); - assert((rc == 0) ^ (blob->back_bs_dev == NULL)); blob_load_final(ctx, rc); return; } @@ -8937,7 +8955,7 @@ blob_esnap_destroy_bs_dev_channels(struct spdk_blob *blob, bool abort_io, { struct blob_esnap_destroy_ctx *ctx; - if (!blob_is_esnap_clone(blob)) { + if (!blob_is_esnap_clone(blob) || blob->back_bs_dev == NULL) { if (cb_fn != NULL) { cb_fn(cb_arg, blob, 0); } diff --git a/lib/lvol/lvol.c b/lib/lvol/lvol.c index 909fa155c..ce7e887ab 100644 --- a/lib/lvol/lvol.c +++ b/lib/lvol/lvol.c @@ -22,6 +22,9 @@ static TAILQ_HEAD(, spdk_lvol_store) g_lvol_stores = TAILQ_HEAD_INITIALIZER(g_lv static pthread_mutex_t g_lvol_stores_mutex = PTHREAD_MUTEX_INITIALIZER; static inline int lvs_opts_copy(const struct spdk_lvs_opts *src, struct spdk_lvs_opts *dst); +static int lvs_esnap_bs_dev_create(void *bs_ctx, void *blob_ctx, struct spdk_blob *blob, + const void *esnap_id, uint32_t id_len, + struct spdk_bs_dev **_bs_dev); static int add_lvs_to_list(struct spdk_lvol_store *lvs) @@ -59,6 +62,8 @@ lvs_alloc(void) TAILQ_INIT(&lvs->pending_lvols); TAILQ_INIT(&lvs->retry_open_lvols); + lvs->load_esnaps = false; + return lvs; } @@ -188,6 +193,7 @@ load_next_lvol(void *cb_arg, struct spdk_blob *blob, int lvolerrno) if (lvolerrno == -ENOENT) { /* Finished iterating */ if (req->lvserrno == 0) { + lvs->load_esnaps = true; req->cb_fn(req->cb_arg, lvs, req->lvserrno); free(req); } else { @@ -441,7 +447,8 @@ lvs_load(struct spdk_bs_dev *bs_dev, const struct spdk_lvs_opts *_lvs_opts, snprintf(bs_opts.bstype.bstype, sizeof(bs_opts.bstype.bstype), "LVOLSTORE"); if (lvs_opts.esnap_bs_dev_create != NULL) { - bs_opts.esnap_bs_dev_create = lvs_opts.esnap_bs_dev_create; + req->lvol_store->esnap_bs_dev_create = lvs_opts.esnap_bs_dev_create; + bs_opts.esnap_bs_dev_create = lvs_esnap_bs_dev_create; bs_opts.esnap_ctx = req->lvol_store; } @@ -1687,3 +1694,36 @@ spdk_lvs_grow(struct spdk_bs_dev *bs_dev, spdk_lvs_op_with_handle_complete cb_fn spdk_bs_grow(bs_dev, &opts, lvs_load_cb, req); } + +static int +lvs_esnap_bs_dev_create(void *bs_ctx, void *blob_ctx, struct spdk_blob *blob, + const void *esnap_id, uint32_t id_len, + struct spdk_bs_dev **bs_dev) +{ + struct spdk_lvol_store *lvs = bs_ctx; + struct spdk_lvol *lvol = blob_ctx; + spdk_blob_id blob_id = spdk_blob_get_id(blob); + + if (lvs == NULL) { + if (lvol == NULL) { + SPDK_ERRLOG("Blob 0x%" PRIx64 ": no lvs context nor lvol context\n", + blob_id); + return -EINVAL; + } + lvs = lvol->lvol_store; + } + + /* + * When spdk_lvs_load() is called, it iterates through all blobs in its blobstore building + * up a list of lvols (lvs->lvols). During this initial iteration, each blob is opened, + * passed to load_next_lvol(), then closed. There is no need to open the external snapshot + * during this phase. Once the blobstore is loaded, lvs->load_esnaps is set to true so that + * future lvol opens cause the external snapshot to be loaded. + */ + if (!lvs->load_esnaps) { + *bs_dev = NULL; + return 0; + } + + return lvs->esnap_bs_dev_create(lvs, lvol, blob, esnap_id, id_len, bs_dev); +} diff --git a/test/unit/lib/lvol/lvol.c/lvol_ut.c b/test/unit/lib/lvol/lvol.c/lvol_ut.c index 9e1536e45..8b90db1c8 100644 --- a/test/unit/lib/lvol/lvol.c/lvol_ut.c +++ b/test/unit/lib/lvol/lvol.c/lvol_ut.c @@ -28,8 +28,6 @@ #define SPDK_BLOB_THIN_PROV (1ULL << 0) -DEFINE_STUB(spdk_blob_get_esnap_id, int, - (struct spdk_blob *blob, const void **id, size_t *len), -ENOTSUP); DEFINE_STUB(spdk_bdev_get_name, const char *, (const struct spdk_bdev *bdev), NULL); DEFINE_STUB(spdk_bdev_get_by_name, struct spdk_bdev *, (const char *name), NULL); DEFINE_STUB(spdk_bdev_create_bs_dev_ro, int, @@ -496,6 +494,21 @@ spdk_bs_create_clone(struct spdk_blob_store *bs, spdk_blob_id blobid, spdk_bs_create_blob_ext(bs, NULL, cb_fn, cb_arg); } +static int g_spdk_blob_get_esnap_id_errno; +static bool g_spdk_blob_get_esnap_id_called; +static void *g_spdk_blob_get_esnap_id; +static size_t g_spdk_blob_get_esnap_id_len; +int +spdk_blob_get_esnap_id(struct spdk_blob *blob, const void **id, size_t *len) +{ + g_spdk_blob_get_esnap_id_called = true; + if (g_spdk_blob_get_esnap_id_errno == 0) { + *id = g_spdk_blob_get_esnap_id; + *len = g_spdk_blob_get_esnap_id_len; + } + return g_spdk_blob_get_esnap_id_errno; +} + static void lvol_store_op_with_handle_complete(void *cb_arg, struct spdk_lvol_store *lvol_store, int lvserrno) { @@ -2152,13 +2165,16 @@ lvol_get_xattr(void) free_dev(&dev); } +struct spdk_bs_dev *g_esnap_bs_dev; +int g_esnap_bs_dev_errno = -ENOTSUP; + static int ut_esnap_bs_dev_create(void *bs_ctx, void *blob_ctx, struct spdk_blob *blob, const void *esnap_id, uint32_t id_len, struct spdk_bs_dev **_bs_dev) { - CU_ASSERT(false); - return -ENOTSUP; + *_bs_dev = g_esnap_bs_dev; + return g_esnap_bs_dev_errno; } static void @@ -2169,6 +2185,9 @@ lvol_esnap_reload(void) struct spdk_lvs_opts opts; int rc; + g_esnap_bs_dev = NULL; + g_esnap_bs_dev_errno = -ENOTSUP; + req = calloc(1, sizeof(*req)); SPDK_CU_ASSERT_FATAL(req != NULL); @@ -2345,6 +2364,67 @@ lvol_esnap_create_delete(void) g_lvol_store = NULL; } +static void +lvol_esnap_load_esnaps(void) +{ + struct spdk_blob blob = { .id = 42 }; + struct spdk_lvol_store *lvs; + struct spdk_lvol *lvol; + struct spdk_bs_dev *bs_dev = NULL; + struct spdk_bs_dev esnap_bs_dev = { 0 }; + int rc; + uint64_t esnap_id = 42; + + lvs = lvs_alloc(); + SPDK_CU_ASSERT_FATAL(lvs != NULL); + lvs->esnap_bs_dev_create = ut_esnap_bs_dev_create; + lvol = lvol_alloc(lvs, __func__, true, LVOL_CLEAR_WITH_DEFAULT); + SPDK_CU_ASSERT_FATAL(lvol != NULL); + + /* Handle missing bs_ctx and blob_ctx gracefully */ + rc = lvs_esnap_bs_dev_create(NULL, NULL, &blob, &esnap_id, sizeof(esnap_id), &bs_dev); + CU_ASSERT(rc == -EINVAL); + + /* Do not try to load external snapshot when load_esnaps is false */ + g_spdk_blob_get_esnap_id_called = false; + bs_dev = NULL; + rc = lvs_esnap_bs_dev_create(lvs, lvol, &blob, &esnap_id, sizeof(esnap_id), &bs_dev); + CU_ASSERT(rc == 0); + CU_ASSERT(bs_dev == NULL); + CU_ASSERT(!g_spdk_blob_get_esnap_id_called); + + /* Same, with only lvs */ + bs_dev = NULL; + rc = lvs_esnap_bs_dev_create(lvs, NULL, &blob, &esnap_id, sizeof(esnap_id), &bs_dev); + CU_ASSERT(rc == 0); + CU_ASSERT(bs_dev == NULL); + CU_ASSERT(!g_spdk_blob_get_esnap_id_called); + + /* Same, with only lvol */ + bs_dev = NULL; + rc = lvs_esnap_bs_dev_create(NULL, lvol, &blob, &esnap_id, sizeof(esnap_id), &bs_dev); + CU_ASSERT(rc == 0); + CU_ASSERT(bs_dev == NULL); + CU_ASSERT(!g_spdk_blob_get_esnap_id_called); + + /* Happy path */ + g_esnap_bs_dev = &esnap_bs_dev; + g_esnap_bs_dev_errno = 0; + + lvs->load_esnaps = true; + ut_spdk_bdev_create_bs_dev_ro = 0; + g_spdk_blob_get_esnap_id_errno = 0; + bs_dev = NULL; + rc = lvs_esnap_bs_dev_create(lvs, lvol, &blob, &esnap_id, sizeof(esnap_id), &bs_dev); + CU_ASSERT(rc == 0); + + /* Clean up */ + lvol_free(lvol); + lvs_free(lvs); + g_esnap_bs_dev = NULL; + g_esnap_bs_dev_errno = -ENOTSUP; +} + int main(int argc, char **argv) { @@ -2386,6 +2466,7 @@ main(int argc, char **argv) CU_ADD_TEST(suite, lvol_esnap_reload); CU_ADD_TEST(suite, lvol_esnap_create_bad_args); CU_ADD_TEST(suite, lvol_esnap_create_delete); + CU_ADD_TEST(suite, lvol_esnap_load_esnaps); allocate_threads(1); set_thread(0);