From 1392b9fc17bfa74f0661000de212a5f54ff41956 Mon Sep 17 00:00:00 2001 From: jhendersonHDF Date: Fri, 17 Mar 2023 15:45:07 -0500 Subject: Subfiling VFD - fix issues with I/O concentrator selection strategies (#2571) Fix multiple bugs with the SELECT_IOC_EVERY_NTH_RANK and SELECT_IOC_TOTAL I/O concentrator selection strategies and add a regression test for them --- release_docs/RELEASE.txt | 20 ++++ src/H5FDsubfiling/H5subfiling_common.c | 110 ++++++++++++------- testpar/t_subfiling_vfd.c | 195 +++++++++++++++++++++++++++++++++ 3 files changed, 286 insertions(+), 39 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index d6ab00e..c8637f0 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -244,6 +244,26 @@ Bug Fixes since HDF5-1.13.3 release =================================== Library ------- + - Fixed issues in the Subfiling VFD when using the SELECT_IOC_EVERY_NTH_RANK + or SELECT_IOC_TOTAL I/O concentrator selection strategies + + Multiple bugs involving these I/O concentrator selection strategies + were fixed, including: + + * A bug that caused the selection strategy to be altered when + criteria for the strategy was specified in the + H5FD_SUBFILING_IOC_SELECTION_CRITERIA environment variable as + a single value, rather than in the old and undocumented + 'integer:integer' format + * Two bugs which caused a request for 'N' I/O concentrators to + result in 'N - 1' I/O concentrators being assigned, which also + lead to issues if only 1 I/O concentrator was requested + + Also added a regression test for these two I/O concentrator selection + strategies to prevent future issues. + + (JTH - 2023/03/15) + - Fix CVE-2021-37501 / GHSA-rfgw-5vq3-wrjf Check for overflow when calculating on-disk attribute data size. diff --git a/src/H5FDsubfiling/H5subfiling_common.c b/src/H5FDsubfiling/H5subfiling_common.c index b58c4d3..e4dcf25 100644 --- a/src/H5FDsubfiling/H5subfiling_common.c +++ b/src/H5FDsubfiling/H5subfiling_common.c @@ -992,17 +992,24 @@ init_app_topology(H5FD_subfiling_params_t *subfiling_config, MPI_Comm comm, MPI_ HDprintf("invalid IOC selection strategy string '%s' for strategy " "SELECT_IOC_EVERY_NTH_RANK; defaulting to SELECT_IOC_ONE_PER_NODE\n", ioc_sel_str); + ioc_select_val = 1; ioc_selection_type = SELECT_IOC_ONE_PER_NODE; + + H5_CHECK_OVERFLOW(iocs_per_node, long, int); + ioc_count = (int)iocs_per_node; + + break; } } + if (ioc_select_val > comm_size) + ioc_select_val = comm_size; + H5_CHECK_OVERFLOW(ioc_select_val, long, int); - ioc_count = (comm_size / (int)ioc_select_val); + ioc_count = ((comm_size - 1) / (int)ioc_select_val) + 1; - if ((comm_size % ioc_select_val) != 0) { - ioc_count++; - } + rank_multiple = (int)ioc_select_val; break; } @@ -1017,19 +1024,31 @@ init_app_topology(H5FD_subfiling_params_t *subfiling_config, MPI_Comm comm, MPI_ if (ioc_sel_str) { errno = 0; ioc_select_val = HDstrtol(ioc_sel_str, NULL, 0); - if ((ERANGE == errno) || (ioc_select_val <= 0) || (ioc_select_val >= comm_size)) { + if ((ERANGE == errno) || (ioc_select_val <= 0)) { HDprintf("invalid IOC selection strategy string '%s' for strategy SELECT_IOC_TOTAL; " "defaulting to SELECT_IOC_ONE_PER_NODE\n", ioc_sel_str); + ioc_select_val = 1; ioc_selection_type = SELECT_IOC_ONE_PER_NODE; + + H5_CHECK_OVERFLOW(iocs_per_node, long, int); + ioc_count = (int)iocs_per_node; + + break; } } + if (ioc_select_val > comm_size) + ioc_select_val = comm_size; + H5_CHECK_OVERFLOW(ioc_select_val, long, int); ioc_count = (int)ioc_select_val; - rank_multiple = (comm_size / ioc_count); + if (ioc_select_val > 1) + rank_multiple = (comm_size - 1) / ((int)ioc_select_val - 1); + else + rank_multiple = 1; break; } @@ -1145,34 +1164,48 @@ get_ioc_selection_criteria_from_env(H5FD_subfiling_ioc_select_t *ioc_selection_t *ioc_sel_info_str = NULL; if (env_value) { - long check_value; - /* - * For non-default options, the environment variable - * should have the following form: integer:[integer|string] - * In particular, EveryNthRank == 1:64 or every 64 ranks assign an IOC - * or WithConfig == 2:/ + * Parse I/O Concentrator selection strategy criteria as + * either a single value or two colon-separated values of + * the form 'integer:[integer|string]'. If two values are + * given, the first value specifies the I/O Concentrator + * selection strategy to use (given as the integer value + * corresponding to the H5FD_subfiling_ioc_select_t enum + * value for that strategy) and the second value specifies + * the criteria for that strategy. + * + * For example, to assign every 64th MPI rank as an I/O + * Concentrator, the criteria string should have the format + * '1:64' to specify the "every Nth rank" strategy with a + * criteria of '64'. */ - if ((opt_value = HDstrchr(env_value, ':'))) + opt_value = HDstrchr(env_value, ':'); + if (opt_value) { + long check_value; + *opt_value++ = '\0'; - errno = 0; - check_value = HDstrtol(env_value, NULL, 0); + errno = 0; + check_value = HDstrtol(env_value, NULL, 0); - if (errno == ERANGE) - H5_SUBFILING_SYS_GOTO_ERROR(H5E_VFL, H5E_CANTGET, FAIL, - "couldn't parse value from " H5FD_SUBFILING_IOC_SELECTION_CRITERIA - " environment variable"); + if (errno == ERANGE) + H5_SUBFILING_SYS_GOTO_ERROR(H5E_VFL, H5E_CANTGET, FAIL, + "couldn't parse value from " H5FD_SUBFILING_IOC_SELECTION_CRITERIA + " environment variable"); - if ((check_value < 0) || (check_value >= ioc_selection_options)) - H5_SUBFILING_GOTO_ERROR( - H5E_VFL, H5E_BADVALUE, FAIL, - "invalid IOC selection type value %ld from " H5FD_SUBFILING_IOC_SELECTION_CRITERIA - " environment variable", - check_value); + if ((check_value < 0) || (check_value >= ioc_selection_options)) + H5_SUBFILING_GOTO_ERROR( + H5E_VFL, H5E_BADVALUE, FAIL, + "invalid IOC selection type value %ld from " H5FD_SUBFILING_IOC_SELECTION_CRITERIA + " environment variable", + check_value); - *ioc_selection_type = (H5FD_subfiling_ioc_select_t)check_value; - *ioc_sel_info_str = opt_value; + *ioc_selection_type = (H5FD_subfiling_ioc_select_t)check_value; + *ioc_sel_info_str = opt_value; + } + else { + *ioc_sel_info_str = env_value; + } } done: @@ -1562,8 +1595,8 @@ identify_ioc_ranks(sf_topology_t *app_topology, int rank_stride) max_iocs = app_topology->n_io_concentrators; - if (NULL == (app_topology->io_concentrators = HDmalloc((size_t)app_topology->n_io_concentrators * - sizeof(*app_topology->io_concentrators)))) + if (NULL == (app_topology->io_concentrators = + HDmalloc((size_t)max_iocs * sizeof(*app_topology->io_concentrators)))) H5_SUBFILING_GOTO_ERROR(H5E_RESOURCE, H5E_CANTALLOC, FAIL, "couldn't allocate array of I/O concentrator ranks"); @@ -1622,30 +1655,29 @@ identify_ioc_ranks(sf_topology_t *app_topology, int rank_stride) case SELECT_IOC_EVERY_NTH_RANK: case SELECT_IOC_TOTAL: { - int world_size = app_layout->world_size; - int ioc_next = 0; + int num_iocs_assigned = 0; + int world_size = app_layout->world_size; HDassert(rank_stride > 0); - for (int i = 0; ioc_next < app_topology->n_io_concentrators; ioc_next++) { + for (int i = 0; num_iocs_assigned < max_iocs; num_iocs_assigned++) { int ioc_index = rank_stride * i++; + if (num_iocs_assigned >= max_iocs) + break; if (ioc_index >= world_size) break; - io_concentrators[ioc_next] = app_layout->layout[ioc_index].rank; + io_concentrators[num_iocs_assigned] = app_layout->layout[ioc_index].rank; - if (app_layout->world_rank == io_concentrators[ioc_next]) { - app_topology->ioc_idx = ioc_next; + if (app_layout->world_rank == io_concentrators[num_iocs_assigned]) { + app_topology->ioc_idx = num_iocs_assigned; app_topology->rank_is_ioc = TRUE; } - - if (ioc_next + 1 >= max_iocs) - break; } /* Set final number of I/O concentrators after adjustments */ - app_topology->n_io_concentrators = ioc_next; + app_topology->n_io_concentrators = num_iocs_assigned; break; } diff --git a/testpar/t_subfiling_vfd.c b/testpar/t_subfiling_vfd.c index e1f9e05..380c068 100644 --- a/testpar/t_subfiling_vfd.c +++ b/testpar/t_subfiling_vfd.c @@ -95,6 +95,7 @@ static hid_t create_subfiling_ioc_fapl(MPI_Comm comm, MPI_Info info, hbool_t cus static void test_create_and_close(void); static void test_config_file(void); static void test_stripe_sizes(void); +static void test_selection_strategies(void); static void test_read_different_stripe_size(void); static void test_subfiling_precreate_rank_0(void); static void test_subfiling_write_many_read_one(void); @@ -105,6 +106,7 @@ static test_func tests[] = { test_create_and_close, test_config_file, test_stripe_sizes, + test_selection_strategies, test_read_different_stripe_size, test_subfiling_precreate_rank_0, test_subfiling_write_many_read_one, @@ -795,6 +797,199 @@ test_stripe_sizes(void) #undef SUBF_NITER /* + * Test the different I/O Concentator selection strategies + * for the Subfiling VFD + */ +#define SUBF_FILENAME "test_subfiling_selection_strategies.h5" +#define NUM_RANKS_CHOICES 2 +#define NUM_CRITERIA_FORMATS 2 +static void +test_selection_strategies(void) +{ + H5FD_subfiling_params_t cfg; + hid_t file_id = H5I_INVALID_HID; + hid_t fapl_id = H5I_INVALID_HID; + char *tmp_filename = NULL; + + curr_nerrors = nerrors; + + if (MAINPROCESS) + TESTING_2("I/O concentrator selection strategies"); + + tmp_filename = HDmalloc(PATH_MAX); + VRFY(tmp_filename, "HDmalloc succeeded"); + + for (H5FD_subfiling_ioc_select_t strategy = 0; strategy < ioc_selection_options; strategy++) { + /* Skip 1 IOC per node strategy since we assume it's + * the default strategy tested in this file. Skip + * "with config" strategy since it isn't supported. + */ + if (strategy == SELECT_IOC_ONE_PER_NODE || strategy == SELECT_IOC_WITH_CONFIG) + continue; + + /* Test with 1 MPI rank and then all MPI ranks */ + for (size_t num_ranks_choice = 0; num_ranks_choice < NUM_RANKS_CHOICES; num_ranks_choice++) { + int num_active_ranks = mpi_size; + + if (num_ranks_choice == 0) + num_active_ranks = 1; + + /* Test with a selection strategy criteria string + * in the 'integer:[integer|string]' form and in + * the form of just a single value. + */ + for (size_t criteria_format_choice = 0; criteria_format_choice < NUM_CRITERIA_FORMATS; + criteria_format_choice++) { + MPI_Comm file_comm = comm_g; + char criteria_buf[256]; + char sel_criteria[128]; /* Use char buffer for criteria as we may support + the "with config" strategy in the future */ + int expected_num_subfiles; + + cfg.ioc_selection = strategy; + cfg.stripe_size = H5FD_SUBFILING_DEFAULT_STRIPE_SIZE; + cfg.stripe_count = H5FD_SUBFILING_DEFAULT_STRIPE_COUNT; + + switch (strategy) { + case SELECT_IOC_EVERY_NTH_RANK: { + int stride; + + /* Try to select a reasonable stride value */ + if (num_active_ranks <= 2) + stride = 1; + else if (num_active_ranks <= 8) + stride = 2; + else if (num_active_ranks <= 32) + stride = 4; + else if (num_active_ranks <= 128) + stride = 8; + else + stride = 16; + + HDsnprintf(sel_criteria, 128, "%d", stride); + + expected_num_subfiles = ((num_active_ranks - 1) / stride) + 1; + + break; + } + + case SELECT_IOC_TOTAL: { + int n_iocs; + + /* Try to select a reasonable number of IOCs */ + if (num_active_ranks <= 2) + n_iocs = 1; + else if (num_active_ranks <= 8) + n_iocs = 2; + else if (num_active_ranks <= 32) + n_iocs = 4; + else if (num_active_ranks <= 128) + n_iocs = 8; + else + n_iocs = 16; + + HDsnprintf(sel_criteria, 128, "%d", n_iocs); + + expected_num_subfiles = n_iocs; + + break; + } + + case SELECT_IOC_ONE_PER_NODE: + case SELECT_IOC_WITH_CONFIG: + default: + HDprintf("invalid IOC selection strategy\n"); + MPI_Abort(comm_g, -1); + } + + if (criteria_format_choice == 0) { + HDsnprintf(criteria_buf, 256, "%d:%s", strategy, sel_criteria); + } + else if (criteria_format_choice == 1) { + HDsnprintf(criteria_buf, 256, "%s", sel_criteria); + } + + VRFY(HDsetenv(H5FD_SUBFILING_IOC_SELECTION_CRITERIA, criteria_buf, 1) >= 0, + "HDsetenv succeeded"); + + HDassert(num_active_ranks == mpi_size || num_active_ranks == 1); + + if ((num_active_ranks == mpi_size) || (mpi_rank == 0)) { + h5_stat_t file_info; + FILE *subfile_ptr; + int num_digits; + + if (num_active_ranks < mpi_size) + file_comm = MPI_COMM_SELF; + + fapl_id = create_subfiling_ioc_fapl(file_comm, info_g, TRUE, &cfg, + H5FD_IOC_DEFAULT_THREAD_POOL_SIZE); + VRFY((fapl_id >= 0), "FAPL creation succeeded"); + + file_id = H5Fcreate(SUBF_FILENAME, H5F_ACC_TRUNC, H5P_DEFAULT, fapl_id); + VRFY((file_id >= 0), "H5Fcreate succeeded"); + + /* + * Get the file inode value so we can construct the subfile names + */ + VRFY((HDstat(SUBF_FILENAME, &file_info) >= 0), "HDstat succeeded"); + + num_digits = (int)(HDlog10(expected_num_subfiles) + 1); + + /* Ensure all the subfiles are present */ + for (int i = 0; i < expected_num_subfiles; i++) { + HDsnprintf(tmp_filename, PATH_MAX, H5FD_SUBFILING_FILENAME_TEMPLATE, SUBF_FILENAME, + (uint64_t)file_info.st_ino, num_digits, i + 1, expected_num_subfiles); + + /* Ensure file exists */ + subfile_ptr = HDfopen(tmp_filename, "r"); + VRFY(subfile_ptr, "HDfopen on subfile succeeded"); + VRFY((HDfclose(subfile_ptr) >= 0), "HDfclose on subfile succeeded"); + } + + /* Ensure no extra subfiles are present */ + HDsnprintf(tmp_filename, PATH_MAX, H5FD_SUBFILING_FILENAME_TEMPLATE, SUBF_FILENAME, + (uint64_t)file_info.st_ino, num_digits, expected_num_subfiles + 1, + expected_num_subfiles); + + /* Ensure file doesn't exist */ + subfile_ptr = HDfopen(tmp_filename, "r"); + VRFY(subfile_ptr == NULL, "HDfopen on subfile correctly failed"); + + VRFY((H5Fclose(file_id) >= 0), "File close succeeded"); + + mpi_code_g = MPI_Barrier(file_comm); + VRFY((mpi_code_g == MPI_SUCCESS), "MPI_Barrier succeeded"); + + H5E_BEGIN_TRY + { + H5Fdelete(SUBF_FILENAME, fapl_id); + } + H5E_END_TRY; + + VRFY((H5Pclose(fapl_id) >= 0), "FAPL close succeeded"); + + VRFY(HDunsetenv(H5FD_SUBFILING_IOC_SELECTION_CRITERIA) >= 0, "HDunsetenv succeeded"); + } + + mpi_code_g = MPI_Barrier(comm_g); + VRFY((mpi_code_g == MPI_SUCCESS), "MPI_Barrier succeeded"); + } + } + } + + mpi_code_g = MPI_Barrier(comm_g); + VRFY((mpi_code_g == MPI_SUCCESS), "MPI_Barrier succeeded"); + + HDfree(tmp_filename); + + CHECK_PASSED(); +} +#undef SUBF_FILENAME +#undef NUM_RANKS_CHOICES +#undef NUM_CRITERIA_FORMATS + +/* * Test that opening a file with a different stripe * size/count than was used when creating the file * results in the original stripe size/count being -- cgit v0.12