From d31f2be55fa510c68a5d9308eb05c32c69782088 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Mon, 29 Oct 2018 14:19:38 -0700 Subject: [PATCH] reduce: have library create pmem file Currently the API requires the caller to open the pmem file and pass the mapped buffer pointer, length and pmem flag to libreduce using the spdk_reduce_pm_file structure. Let's have the library just do the pmem_map_file() instead. Users then just pass the path to create the pmem file. The library will still use the spdk_reduce_pm_file structure internally - so move its definition out of the public header and into reduce.c. Signed-off-by: Jim Harris Change-Id: I81fcbfdfbb3211dca016d6aa422cf2e1ab16d84d Reviewed-on: https://review.gerrithub.io/432593 Chandler-Test-Pool: SPDK Automated Test System Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Changpeng Liu Reviewed-by: Shuhei Matsumoto --- include/spdk/reduce.h | 23 +---- lib/reduce/reduce.c | 65 ++++++++++++-- test/unit/lib/reduce/reduce.c/reduce_ut.c | 105 +++++++++++----------- 3 files changed, 111 insertions(+), 82 deletions(-) diff --git a/include/spdk/reduce.h b/include/spdk/reduce.h index 5a1a154a0..cd6ae706c 100644 --- a/include/spdk/reduce.h +++ b/include/spdk/reduce.h @@ -88,23 +88,6 @@ int64_t spdk_reduce_get_backing_device_size(struct spdk_reduce_vol_params *param struct spdk_reduce_vol; -#define REDUCE_PATH_MAX 4096 - -/** - * Describes a persistent memory file used to hold metadata associated with a - * compressed volume. - */ -struct spdk_reduce_pm_file { - char path[REDUCE_PATH_MAX]; - void *pm_buf; - bool pm_is_pmem; - - /** Size of the persistent memory file in bytes. */ - uint64_t size; - - void (*close)(struct spdk_reduce_pm_file *); -}; - typedef void (*spdk_reduce_vol_op_complete)(void *ctx, int ziperrno); typedef void (*spdk_reduce_vol_op_with_handle_complete)(void *ctx, struct spdk_reduce_vol *vol, @@ -146,13 +129,15 @@ const struct spdk_uuid *spdk_reduce_vol_get_uuid(struct spdk_reduce_vol *vol); * * \param params Parameters for the new volume. * \param backing_dev Structure describing the backing device to use for the new volume. - * \param pm_file Structure describing the persistent memory file to use for the new volume. + * \param pm_file_dir Directory to use for creation of the persistent memory file to + * use for the new volume. This function will append the UUID as + * the filename to create in this directory. * \param cb_fn Callback function to signal completion of the initialization process. * \param cb_arg Argument to pass to the callback function. */ void spdk_reduce_vol_init(struct spdk_reduce_vol_params *params, struct spdk_reduce_backing_dev *backing_dev, - struct spdk_reduce_pm_file *pm_file, + const char *pm_file_dir, spdk_reduce_vol_op_with_handle_complete cb_fn, void *cb_arg); diff --git a/lib/reduce/reduce.c b/lib/reduce/reduce.c index afc47d3e2..e5d4e8adc 100644 --- a/lib/reduce/reduce.c +++ b/lib/reduce/reduce.c @@ -56,6 +56,19 @@ struct spdk_reduce_vol_superblock { }; SPDK_STATIC_ASSERT(sizeof(struct spdk_reduce_vol_superblock) == 4096, "size incorrect"); +#define REDUCE_PATH_MAX 4096 + +/** + * Describes a persistent memory file used to hold metadata associated with a + * compressed volume. + */ +struct spdk_reduce_pm_file { + char path[REDUCE_PATH_MAX]; + void *pm_buf; + int pm_is_pmem; + uint64_t size; +}; + struct spdk_reduce_vol { struct spdk_uuid uuid; struct spdk_reduce_pm_file pm_file; @@ -211,14 +224,26 @@ _init_write_path_cpl(void *cb_arg, int ziperrno) void spdk_reduce_vol_init(struct spdk_reduce_vol_params *params, struct spdk_reduce_backing_dev *backing_dev, - struct spdk_reduce_pm_file *pm_file, + const char *pm_file_dir, spdk_reduce_vol_op_with_handle_complete cb_fn, void *cb_arg) { struct spdk_reduce_vol *vol; struct spdk_reduce_vol_superblock *pm_super; struct reduce_init_load_ctx *init_ctx; int64_t size, size_needed; - int rc; + size_t mapped_len; + int dir_len, max_dir_len, rc; + + /* We need to append a path separator and the UUID to the supplied + * path. + */ + max_dir_len = REDUCE_PATH_MAX - SPDK_UUID_STRING_LEN - 1; + dir_len = strnlen(pm_file_dir, max_dir_len); + if (dir_len == max_dir_len) { + SPDK_ERRLOG("pm_file_dir (%s) too long\n", pm_file_dir); + cb_fn(cb_arg, NULL, -EINVAL); + return; + } rc = _validate_vol_params(params); if (rc != 0) { @@ -236,8 +261,6 @@ spdk_reduce_vol_init(struct spdk_reduce_vol_params *params, return; } - size_needed = spdk_reduce_get_pm_file_size(params); - size = pm_file->size; if (size_needed > size) { SPDK_ERRLOG("pm file size %" PRIi64 " but %" PRIi64 " needed\n", size, size_needed); @@ -258,8 +281,6 @@ spdk_reduce_vol_init(struct spdk_reduce_vol_params *params, return; } - memcpy(&vol->pm_file, pm_file, sizeof(*pm_file)); - vol->backing_super = spdk_dma_zmalloc(sizeof(*vol->backing_super), 0, NULL); if (vol->backing_super == NULL) { free(vol); @@ -288,6 +309,34 @@ spdk_reduce_vol_init(struct spdk_reduce_vol_params *params, spdk_uuid_generate(¶ms->uuid); } + memcpy(vol->pm_file.path, pm_file_dir, dir_len); + vol->pm_file.path[dir_len] = '/'; + spdk_uuid_fmt_lower(&vol->pm_file.path[dir_len + 1], SPDK_UUID_STRING_LEN, + ¶ms->uuid); + vol->pm_file.size = spdk_reduce_get_pm_file_size(params); + vol->pm_file.pm_buf = pmem_map_file(vol->pm_file.path, vol->pm_file.size, + PMEM_FILE_CREATE | PMEM_FILE_EXCL, 0600, + &mapped_len, &vol->pm_file.pm_is_pmem); + if (vol->pm_file.pm_buf == NULL) { + SPDK_ERRLOG("could not pmem_map_file(%s): %s\n", + vol->pm_file.path, strerror(errno)); + free(init_ctx); + spdk_dma_free(vol->backing_super); + free(vol); + cb_fn(cb_arg, NULL, -errno); + return; + } + + if (vol->pm_file.size != mapped_len) { + SPDK_ERRLOG("could not map entire pmem file (size=%" PRIu64 " mapped=%" PRIu64 ")\n", + vol->pm_file.size, mapped_len); + free(init_ctx); + spdk_dma_free(vol->backing_super); + free(vol); + cb_fn(cb_arg, NULL, -ENOMEM); + return; + } + memcpy(&vol->uuid, ¶ms->uuid, sizeof(params->uuid)); vol->backing_dev = backing_dev; @@ -335,9 +384,7 @@ spdk_reduce_vol_unload(struct spdk_reduce_vol *vol, return; } - if (vol->pm_file.close != NULL) { - vol->pm_file.close(&vol->pm_file); - } + pmem_unmap(vol->pm_file.pm_buf, vol->pm_file.size); vol->backing_dev->close(vol->backing_dev); diff --git a/test/unit/lib/reduce/reduce.c/reduce_ut.c b/test/unit/lib/reduce/reduce.c/reduce_ut.c index 31345f625..985fe6ac2 100644 --- a/test/unit/lib/reduce/reduce.c/reduce_ut.c +++ b/test/unit/lib/reduce/reduce.c/reduce_ut.c @@ -42,11 +42,13 @@ static struct spdk_reduce_vol *g_vol; static int g_ziperrno; static char *g_volatile_pm_buf; +static size_t g_volatile_pm_buf_len; static char *g_persistent_pm_buf; static bool g_backing_dev_closed; static char *g_backing_dev_buf; +static const char *g_path; -#define TEST_MD_PATH "/tmp/meta.data" +#define TEST_MD_PATH "/tmp" static void sync_pm_buf(const void *addr, size_t length) @@ -151,43 +153,45 @@ get_backing_device_size(void) CU_ASSERT(backing_size == expected_backing_size); } -static void -pm_file_close(struct spdk_reduce_pm_file *pm_file) +void * +pmem_map_file(const char *path, size_t len, int flags, mode_t mode, + size_t *mapped_lenp, int *is_pmemp) { + CU_ASSERT(g_volatile_pm_buf == NULL); + g_volatile_pm_buf = calloc(1, len); + g_volatile_pm_buf_len = len; + g_path = path; + SPDK_CU_ASSERT_FATAL(g_volatile_pm_buf != NULL); + *mapped_lenp = len; + *is_pmemp = 1; + + if (g_persistent_pm_buf == NULL) { + g_persistent_pm_buf = calloc(1, len); + SPDK_CU_ASSERT_FATAL(g_persistent_pm_buf != NULL); + } + + return g_volatile_pm_buf; +} + +int +pmem_unmap(void *addr, size_t len) +{ + CU_ASSERT(addr == g_volatile_pm_buf); + CU_ASSERT(len == g_volatile_pm_buf_len); free(g_volatile_pm_buf); g_volatile_pm_buf = NULL; + + return 0; } static void -pm_file_destroy(void) +persistent_pm_buf_destroy(void) { CU_ASSERT(g_persistent_pm_buf != NULL); free(g_persistent_pm_buf); g_persistent_pm_buf = NULL; } -static int -pm_file_init(struct spdk_reduce_pm_file *pm_file, struct spdk_reduce_vol_params *params) -{ - pm_file->size = spdk_reduce_get_pm_file_size(params); - - CU_ASSERT(g_persistent_pm_buf == NULL); - g_persistent_pm_buf = calloc(1, pm_file->size); - SPDK_CU_ASSERT_FATAL(g_persistent_pm_buf != NULL); - - CU_ASSERT(g_volatile_pm_buf == NULL); - g_volatile_pm_buf = calloc(1, pm_file->size); - SPDK_CU_ASSERT_FATAL(g_volatile_pm_buf != NULL); - - memcpy(pm_file->path, TEST_MD_PATH, strlen(TEST_MD_PATH)); - pm_file->path[strlen(TEST_MD_PATH)] = '\0'; - - pm_file->pm_buf = g_volatile_pm_buf; - pm_file->close = pm_file_close; - - return 0; -} - static void init_cb(void *cb_arg, struct spdk_reduce_vol *vol, int ziperrno) { @@ -206,7 +210,6 @@ init_failure(void) { struct spdk_reduce_vol_params params = {}; struct spdk_reduce_backing_dev backing_dev = {}; - struct spdk_reduce_pm_file pm_file = {}; backing_dev.blocklen = 512; @@ -217,34 +220,20 @@ init_failure(void) /* backing_dev and pm_file have an invalid size. This should fail. */ g_vol = NULL; g_ziperrno = 0; - spdk_reduce_vol_init(¶ms, &backing_dev, &pm_file, init_cb, NULL); + spdk_reduce_vol_init(¶ms, &backing_dev, TEST_MD_PATH, init_cb, NULL); CU_ASSERT(g_ziperrno == -EINVAL); SPDK_CU_ASSERT_FATAL(g_vol == NULL); - /* backing_dev now has valid size, but pm_file is still invalid. - * This should fail. + /* backing_dev now has valid size, but backing_dev still has null + * function pointers. This should fail. */ backing_dev.blockcnt = spdk_reduce_get_backing_device_size(¶ms) / backing_dev.blocklen; g_vol = NULL; g_ziperrno = 0; - spdk_reduce_vol_init(¶ms, &backing_dev, &pm_file, init_cb, NULL); + spdk_reduce_vol_init(¶ms, &backing_dev, TEST_MD_PATH, init_cb, NULL); CU_ASSERT(g_ziperrno == -EINVAL); SPDK_CU_ASSERT_FATAL(g_vol == NULL); - - /* pm_file now has valid size, but backing_dev still has null function - * pointers. This should fail. - */ - pm_file_init(&pm_file, ¶ms); - - g_vol = NULL; - g_ziperrno = 0; - spdk_reduce_vol_init(¶ms, &backing_dev, &pm_file, init_cb, NULL); - CU_ASSERT(g_ziperrno == -EINVAL); - SPDK_CU_ASSERT_FATAL(g_vol == NULL); - - pm_file_close(&pm_file); - pm_file_destroy(); } static void @@ -327,18 +316,17 @@ init_md(void) struct spdk_reduce_vol_params params = {}; struct spdk_reduce_vol_params *persistent_params; struct spdk_reduce_backing_dev backing_dev = {}; - struct spdk_reduce_pm_file pm_file = {}; + struct spdk_uuid uuid; params.vol_size = 1024 * 1024; /* 1MB */ params.chunk_size = 16 * 1024; params.backing_io_unit_size = 512; backing_dev_init(&backing_dev, ¶ms); - pm_file_init(&pm_file, ¶ms); g_vol = NULL; g_ziperrno = -1; - spdk_reduce_vol_init(¶ms, &backing_dev, &pm_file, init_cb, NULL); + spdk_reduce_vol_init(¶ms, &backing_dev, TEST_MD_PATH, init_cb, NULL); CU_ASSERT(g_ziperrno == 0); SPDK_CU_ASSERT_FATAL(g_vol != NULL); /* Confirm that reduce persisted the params to metadata. */ @@ -346,6 +334,15 @@ init_md(void) persistent_params = (struct spdk_reduce_vol_params *)(g_persistent_pm_buf + 8); CU_ASSERT(memcmp(persistent_params, ¶ms, sizeof(params)) == 0); + /* Check that the pm file path was constructed correctly. It should be in + * the form: + * TEST_MD_PATH + "/" + + */ + CU_ASSERT(strncmp(&g_path[0], TEST_MD_PATH, strlen(TEST_MD_PATH)) == 0); + CU_ASSERT(g_path[strlen(TEST_MD_PATH)] == '/'); + CU_ASSERT(spdk_uuid_parse(&uuid, &g_path[strlen(TEST_MD_PATH) + 1]) == 0); + CU_ASSERT(spdk_uuid_compare(&uuid, spdk_reduce_vol_get_uuid(g_vol)) == 0); + g_ziperrno = -1; g_backing_dev_closed = false; spdk_reduce_vol_unload(g_vol, unload_cb, NULL); @@ -353,7 +350,7 @@ init_md(void) CU_ASSERT(g_backing_dev_closed == true); CU_ASSERT(g_volatile_pm_buf == NULL); - pm_file_destroy(); + persistent_pm_buf_destroy(); backing_dev_destroy(&backing_dev); } @@ -363,7 +360,6 @@ init_backing_dev(void) struct spdk_reduce_vol_params params = {}; struct spdk_reduce_vol_params *persistent_params; struct spdk_reduce_backing_dev backing_dev = {}; - struct spdk_reduce_pm_file pm_file = {}; params.vol_size = 1024 * 1024; /* 1MB */ params.chunk_size = 16 * 1024; @@ -371,13 +367,14 @@ init_backing_dev(void) spdk_uuid_generate(¶ms.uuid); backing_dev_init(&backing_dev, ¶ms); - pm_file_init(&pm_file, ¶ms); g_vol = NULL; + g_path = NULL; g_ziperrno = -1; - spdk_reduce_vol_init(¶ms, &backing_dev, &pm_file, init_cb, NULL); + spdk_reduce_vol_init(¶ms, &backing_dev, TEST_MD_PATH, init_cb, NULL); CU_ASSERT(g_ziperrno == 0); SPDK_CU_ASSERT_FATAL(g_vol != NULL); + SPDK_CU_ASSERT_FATAL(g_path != NULL); /* Confirm that libreduce persisted the params to the backing device. */ CU_ASSERT(memcmp(g_backing_dev_buf, SPDK_REDUCE_SIGNATURE, 8) == 0); persistent_params = (struct spdk_reduce_vol_params *)(g_backing_dev_buf + 8); @@ -386,7 +383,7 @@ init_backing_dev(void) /* Confirm that the path to the persistent memory metadata file was persisted to * the backing device. */ - CU_ASSERT(strncmp(TEST_MD_PATH, + CU_ASSERT(strncmp(g_path, g_backing_dev_buf + REDUCE_BACKING_DEV_PATH_OFFSET, REDUCE_PATH_MAX) == 0); @@ -396,7 +393,7 @@ init_backing_dev(void) CU_ASSERT(g_ziperrno == 0); CU_ASSERT(g_backing_dev_closed == true); - pm_file_destroy(); + persistent_pm_buf_destroy(); backing_dev_destroy(&backing_dev); }