From 8f93a2cf9ab708c505671d1f6ba02de74802ff66 Mon Sep 17 00:00:00 2001 From: "vchoi-hdfgroup.org" Date: Mon, 23 Oct 2023 03:07:34 -0500 Subject: [PATCH 1/8] Fix for issue #3025: Save the MPI info in the file struct so H5Fget_access_plist() can retrieve it from there. --- src/H5Fint.c | 9 ++++--- src/H5Fpkg.h | 1 + testpar/t_file.c | 57 +++++++++++++++++++++++++++++++++++++++++++++ testpar/testphdf5.c | 2 ++ testpar/testphdf5.h | 1 + 5 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/H5Fint.c b/src/H5Fint.c index 014f619d8a9..544608899b5 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -402,7 +402,6 @@ H5F_get_access_plist(H5F_t *f, bool app_ref) HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID, "can't set collective metadata read flag"); if (H5F_HAS_FEATURE(f, H5FD_FEAT_HAS_MPI)) { MPI_Comm mpi_comm; - MPI_Info mpi_info; /* Retrieve and set MPI communicator */ if (MPI_COMM_NULL == (mpi_comm = H5F_mpi_get_comm(f))) @@ -410,10 +409,8 @@ H5F_get_access_plist(H5F_t *f, bool app_ref) if (H5P_set(new_plist, H5F_ACS_MPI_PARAMS_COMM_NAME, &mpi_comm) < 0) HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID, "can't set MPI communicator"); - /* Retrieve and set MPI info object */ - if (H5P_get(old_plist, H5F_ACS_MPI_PARAMS_INFO_NAME, &mpi_info) < 0) - HGOTO_ERROR(H5E_FILE, H5E_CANTGET, H5I_INVALID_HID, "can't get MPI info object"); - if (H5P_set(new_plist, H5F_ACS_MPI_PARAMS_INFO_NAME, &mpi_info) < 0) + /* Retrieve MPI info object */ + if (H5P_set(new_plist, H5F_ACS_MPI_PARAMS_INFO_NAME, &(f->shared->mpi_info)) < 0) HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID, "can't set MPI info object"); } #endif /* H5_HAVE_PARALLEL */ @@ -1209,6 +1206,8 @@ H5F__new(H5F_shared_t *shared, unsigned flags, hid_t fcpl_id, hid_t fapl_id, H5F HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, NULL, "can't get collective metadata read flag"); if (H5P_get(plist, H5F_ACS_COLL_MD_WRITE_FLAG_NAME, &(f->shared->coll_md_write)) < 0) HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, NULL, "can't get collective metadata write flag"); + if (H5P_get(plist, H5F_ACS_MPI_PARAMS_INFO_NAME, &(f->shared->mpi_info)) < 0) + HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, NULL, "can't set MPI info object"); #endif /* H5_HAVE_PARALLEL */ if (H5P_get(plist, H5F_ACS_META_CACHE_INIT_IMAGE_CONFIG_NAME, &(f->shared->mdc_initCacheImageCfg)) < 0) diff --git a/src/H5Fpkg.h b/src/H5Fpkg.h index bc5c90bd5da..e81b25072eb 100644 --- a/src/H5Fpkg.h +++ b/src/H5Fpkg.h @@ -359,6 +359,7 @@ struct H5F_shared_t { #ifdef H5_HAVE_PARALLEL H5P_coll_md_read_flag_t coll_md_read; /* Do all metadata reads collectively */ bool coll_md_write; /* Do all metadata writes collectively */ + MPI_Info mpi_info; /* MPI info */ #endif /* H5_HAVE_PARALLEL */ }; diff --git a/testpar/t_file.c b/testpar/t_file.c index a6a541becf3..7042a6f110d 100644 --- a/testpar/t_file.c +++ b/testpar/t_file.c @@ -1060,3 +1060,60 @@ test_invalid_libver_bounds_file_close_assert(void) ret = H5Pclose(fcpl_id); VRFY((SUCCEED == ret), "H5Pclose"); } + +/* + * Verify that MPI I/O hints are preserved after closing the file access property list + * as described in issue #3025 + * This is a test program from the user. + */ +void +test_fapl_preserve_hints(void) +{ + hid_t fid = H5I_INVALID_HID; /* HDF5 file ID */ + hid_t fapl_id = H5I_INVALID_HID; /* File access plist */ + const char *filename; + MPI_Info info = MPI_INFO_NULL; + MPI_Info info_used = MPI_INFO_NULL; + herr_t ret; /* Generic return value */ + int mpi_ret; /* MPI return value */ + + filename = (const char *)GetTestParameters(); + + /* set up MPI parameters */ + mpi_ret = MPI_Info_create(&info); + VRFY((mpi_ret >= 0), "MPI_Info_create succeeded"); + + mpi_ret = MPI_Info_set(info, "hdf_info_fapl", "true"); + VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_set succeeded"); + + fapl_id = H5Pcreate(H5P_FILE_ACCESS); + VRFY((fapl_id != H5I_INVALID_HID), "H5Pcreate"); + + ret = H5Pset_fapl_mpio(fapl_id, MPI_COMM_WORLD, info); + VRFY((ret >= 0), "H5Pset_fapl_mpio"); + + fid = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, fapl_id); + VRFY((fid != H5I_INVALID_HID), "H5Fcreate succeeded"); + + ret = H5Pclose(fapl_id); + VRFY((ret >= 0), "H5Pclose succeeded"); + + fapl_id = H5Fget_access_plist(fid); + VRFY((fapl_id != H5I_INVALID_HID), "H5Fget_access_plist succeeded"); + + ret = H5Pget_fapl_mpio(fapl_id, NULL, &info_used); + VRFY((ret >= 0), "H5Pget_fapl_mpio succeeded"); + + VRFY((info_used != MPI_INFO_NULL), "H5Pget_fapl_mpio"); + + ret = H5Pclose(fapl_id); + VRFY((ret >= 0), "H5Pclose succeeded"); + + ret = H5Fclose(fid); + VRFY((ret >= 0), "H5Fclose succeeded"); + + /* Free the MPI info object */ + mpi_ret = MPI_Info_free(&info); + VRFY((mpi_ret >= 0), "MPI_Info_free succeeded"); + +} /* end test_fapl_preserve_hints() */ diff --git a/testpar/testphdf5.c b/testpar/testphdf5.c index 584ca1f6107..b47f020cb25 100644 --- a/testpar/testphdf5.c +++ b/testpar/testphdf5.c @@ -366,6 +366,8 @@ main(int argc, char **argv) AddTest("invlibverassert", test_invalid_libver_bounds_file_close_assert, NULL, "Invalid libver bounds assertion failure", PARATESTFILE); + AddTest("fapl_preserve", test_fapl_preserve_hints, NULL, "preserve MPI I/O hints after fapl closed", PARATESTFILE); + AddTest("idsetw", dataset_writeInd, NULL, "dataset independent write", PARATESTFILE); AddTest("idsetr", dataset_readInd, NULL, "dataset independent read", PARATESTFILE); diff --git a/testpar/testphdf5.h b/testpar/testphdf5.h index 6ac8080c82a..b56d3656abd 100644 --- a/testpar/testphdf5.h +++ b/testpar/testphdf5.h @@ -233,6 +233,7 @@ void zero_dim_dset(void); void test_file_properties(void); void test_delete(void); void test_invalid_libver_bounds_file_close_assert(void); +void test_fapl_preserve_hints(void); void multiple_dset_write(void); void multiple_group_write(void); void multiple_group_read(void); From 3dec7d52fe1853f1632fa8be12cdb105ca7a48c4 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 23 Oct 2023 08:13:07 +0000 Subject: [PATCH 2/8] Committing clang-format changes --- testpar/t_file.c | 8 ++++---- testpar/testphdf5.c | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/testpar/t_file.c b/testpar/t_file.c index 7042a6f110d..c493d01a261 100644 --- a/testpar/t_file.c +++ b/testpar/t_file.c @@ -1069,10 +1069,10 @@ test_invalid_libver_bounds_file_close_assert(void) void test_fapl_preserve_hints(void) { - hid_t fid = H5I_INVALID_HID; /* HDF5 file ID */ - hid_t fapl_id = H5I_INVALID_HID; /* File access plist */ + hid_t fid = H5I_INVALID_HID; /* HDF5 file ID */ + hid_t fapl_id = H5I_INVALID_HID; /* File access plist */ const char *filename; - MPI_Info info = MPI_INFO_NULL; + MPI_Info info = MPI_INFO_NULL; MPI_Info info_used = MPI_INFO_NULL; herr_t ret; /* Generic return value */ int mpi_ret; /* MPI return value */ @@ -1105,7 +1105,7 @@ test_fapl_preserve_hints(void) VRFY((ret >= 0), "H5Pget_fapl_mpio succeeded"); VRFY((info_used != MPI_INFO_NULL), "H5Pget_fapl_mpio"); - + ret = H5Pclose(fapl_id); VRFY((ret >= 0), "H5Pclose succeeded"); diff --git a/testpar/testphdf5.c b/testpar/testphdf5.c index b47f020cb25..dd8459d0faa 100644 --- a/testpar/testphdf5.c +++ b/testpar/testphdf5.c @@ -366,7 +366,8 @@ main(int argc, char **argv) AddTest("invlibverassert", test_invalid_libver_bounds_file_close_assert, NULL, "Invalid libver bounds assertion failure", PARATESTFILE); - AddTest("fapl_preserve", test_fapl_preserve_hints, NULL, "preserve MPI I/O hints after fapl closed", PARATESTFILE); + AddTest("fapl_preserve", test_fapl_preserve_hints, NULL, "preserve MPI I/O hints after fapl closed", + PARATESTFILE); AddTest("idsetw", dataset_writeInd, NULL, "dataset independent write", PARATESTFILE); AddTest("idsetr", dataset_readInd, NULL, "dataset independent read", PARATESTFILE); From 833c6b8d91ac6cf44416c424faf7b1acc08759cc Mon Sep 17 00:00:00 2001 From: "vchoi-hdfgroup.org" Date: Mon, 23 Oct 2023 23:56:36 -0500 Subject: [PATCH 3/8] Modifications based on PR review comments: close MPI info when closing file. --- src/H5Fint.c | 8 ++++++++ testpar/t_file.c | 3 +++ 2 files changed, 11 insertions(+) diff --git a/src/H5Fint.c b/src/H5Fint.c index 544608899b5..2aef8964553 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1413,6 +1413,14 @@ H5F__dest(H5F_t *f, bool flush, bool free_on_failure) f->shared->efc = NULL; } /* end if */ +#ifdef H5_HAVE_PARALLEL + if (f->shared->mpi_info != MPI_INFO_NULL) { + /* Free MPI info saved in the file struct */ + if (H5_mpi_info_free(&f->shared->mpi_info) < 0) + HDONE_ERROR(H5E_FILE, H5E_CANTRELEASE, FAIL, "can't free MPI info"); + } +#endif + /* With the shutdown modifications, the contents of the metadata cache * should be clean at this point, with the possible exception of the * the superblock and superblock extension. diff --git a/testpar/t_file.c b/testpar/t_file.c index c493d01a261..fdb6631347c 100644 --- a/testpar/t_file.c +++ b/testpar/t_file.c @@ -1116,4 +1116,7 @@ test_fapl_preserve_hints(void) mpi_ret = MPI_Info_free(&info); VRFY((mpi_ret >= 0), "MPI_Info_free succeeded"); + mpi_ret = MPI_Info_free(&info_used); + VRFY((mpi_ret >= 0), "MPI_Info_free succeeded"); + } /* end test_fapl_preserve_hints() */ From 2806b3f1c474a9cce8810fe15b36f8b7310ccbb6 Mon Sep 17 00:00:00 2001 From: "vchoi-hdfgroup.org" Date: Tue, 24 Oct 2023 12:35:29 -0500 Subject: [PATCH 4/8] Modifications based on PR review comments: initialize mpi_info field in file struct. --- src/H5Fint.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/H5Fint.c b/src/H5Fint.c index 2aef8964553..0c85e10ba64 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1130,6 +1130,12 @@ H5F__new(H5F_shared_t *shared, unsigned flags, hid_t fcpl_id, hid_t fapl_id, H5F /* initialize point of no return */ f->shared->point_of_no_return = false; +#ifdef H5_HAVE_PARALLEL + /* Initialize this just in case we fail before setting this field and */ + /* we try to call H5_mpi_info_free() on uninitialized memory in H5F__dest() */ + f->shared->mpi_info = MPI_INFO_NULL; +#endif /* H5_HAVE_PARALLEL */ + /* Copy the file creation and file access property lists into the * new file handle. We do this early because some values might need * to change as the file is being opened. From 6bf3647790699bbad49e54c793776e997357e826 Mon Sep 17 00:00:00 2001 From: "vchoi-hdfgroup.org" Date: Tue, 24 Oct 2023 18:06:07 -0500 Subject: [PATCH 5/8] Modifications based on PR review comments: verify the key and value retrieved are same as what is set before. --- testpar/t_file.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/testpar/t_file.c b/testpar/t_file.c index 57f0d3a8336..f4011f9f6da 100644 --- a/testpar/t_file.c +++ b/testpar/t_file.c @@ -1131,8 +1131,16 @@ test_fapl_preserve_hints(void) hid_t fid = H5I_INVALID_HID; /* HDF5 file ID */ hid_t fapl_id = H5I_INVALID_HID; /* File access plist */ const char *filename; + MPI_Info info = MPI_INFO_NULL; + const char *key = "hdf_info_fapl"; + const char *value = "xyz"; + MPI_Info info_used = MPI_INFO_NULL; + int flag = -1; + char value_used[20]; + char key_used[20]; + herr_t ret; /* Generic return value */ int mpi_ret; /* MPI return value */ @@ -1142,7 +1150,7 @@ test_fapl_preserve_hints(void) mpi_ret = MPI_Info_create(&info); VRFY((mpi_ret >= 0), "MPI_Info_create succeeded"); - mpi_ret = MPI_Info_set(info, "hdf_info_fapl", "true"); + mpi_ret = MPI_Info_set(info, key, value); VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_set succeeded"); fapl_id = H5Pcreate(H5P_FILE_ACCESS); @@ -1165,6 +1173,21 @@ test_fapl_preserve_hints(void) VRFY((info_used != MPI_INFO_NULL), "H5Pget_fapl_mpio"); + memset(key_used, 0, 20); + memset(value_used, 0, 20); + + mpi_ret = MPI_Info_get_nthkey(info_used, 0, key_used); + VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_get_nthkey succeeded"); + + mpi_ret = MPI_Info_get(info_used, key_used, 20, value_used, &flag); + VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_get succeeded"); + + ret = strcmp(key_used, key); + VRFY(ret == 0, "strcmp key"); + + ret = strcmp(value_used, value); + VRFY(ret == 0, "strcmp value"); + ret = H5Pclose(fapl_id); VRFY((ret >= 0), "H5Pclose succeeded"); From e5fa4eb5e765f6ec03fc199e1baeb053979386a0 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 24 Oct 2023 23:21:57 +0000 Subject: [PATCH 6/8] Committing clang-format changes --- testpar/t_file.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/testpar/t_file.c b/testpar/t_file.c index f4011f9f6da..d1ef2966d9e 100644 --- a/testpar/t_file.c +++ b/testpar/t_file.c @@ -1132,17 +1132,17 @@ test_fapl_preserve_hints(void) hid_t fapl_id = H5I_INVALID_HID; /* File access plist */ const char *filename; - MPI_Info info = MPI_INFO_NULL; - const char *key = "hdf_info_fapl"; + MPI_Info info = MPI_INFO_NULL; + const char *key = "hdf_info_fapl"; const char *value = "xyz"; - MPI_Info info_used = MPI_INFO_NULL; - int flag = -1; - char value_used[20]; - char key_used[20]; + MPI_Info info_used = MPI_INFO_NULL; + int flag = -1; + char value_used[20]; + char key_used[20]; - herr_t ret; /* Generic return value */ - int mpi_ret; /* MPI return value */ + herr_t ret; /* Generic return value */ + int mpi_ret; /* MPI return value */ filename = (const char *)GetTestParameters(); From 38db446735a8bbc48ae48b52052001f1af126cd0 Mon Sep 17 00:00:00 2001 From: "vchoi-hdfgroup.org" Date: Tue, 24 Oct 2023 21:37:01 -0500 Subject: [PATCH 7/8] Modification based on PR review comments: get the # of keys and loop through all the keys to perform the comparison. --- testpar/t_file.c | 47 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/testpar/t_file.c b/testpar/t_file.c index d1ef2966d9e..40853b467f7 100644 --- a/testpar/t_file.c +++ b/testpar/t_file.c @@ -1132,8 +1132,11 @@ test_fapl_preserve_hints(void) hid_t fapl_id = H5I_INVALID_HID; /* File access plist */ const char *filename; - MPI_Info info = MPI_INFO_NULL; - const char *key = "hdf_info_fapl"; + int nkeys_used; + bool same = false; + + MPI_Info info = MPI_INFO_NULL; + const char *key = "hdf_info_fapl"; const char *value = "xyz"; MPI_Info info_used = MPI_INFO_NULL; @@ -1141,8 +1144,9 @@ test_fapl_preserve_hints(void) char value_used[20]; char key_used[20]; - herr_t ret; /* Generic return value */ - int mpi_ret; /* MPI return value */ + int i; + herr_t ret; /* Generic return value */ + int mpi_ret; /* MPI return value */ filename = (const char *)GetTestParameters(); @@ -1173,20 +1177,35 @@ test_fapl_preserve_hints(void) VRFY((info_used != MPI_INFO_NULL), "H5Pget_fapl_mpio"); - memset(key_used, 0, 20); - memset(value_used, 0, 20); + mpi_ret = MPI_Info_get_nkeys(info_used, &nkeys_used); + VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_get_nkeys succeeded"); + + /* Loop over the # of keys */ + for (i = 0; i < nkeys_used; i++) { + + /* Memset the buffers to zero */ + memset(key_used, 0, 20); + memset(value_used, 0, 20); - mpi_ret = MPI_Info_get_nthkey(info_used, 0, key_used); - VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_get_nthkey succeeded"); + /* Get the nth key */ + mpi_ret = MPI_Info_get_nthkey(info_used, i, key_used); + VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_get_nthkey succeeded"); - mpi_ret = MPI_Info_get(info_used, key_used, 20, value_used, &flag); - VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_get succeeded"); + if (!strcmp(key_used, key)) { - ret = strcmp(key_used, key); - VRFY(ret == 0, "strcmp key"); + mpi_ret = MPI_Info_get(info_used, key_used, 20, value_used, &flag); + VRFY((mpi_ret == MPI_SUCCESS), "MPI_Info_get succeeded"); + + if (!strcmp(value_used, value)) { + + /* Both key_used and value_used are the same */ + same = true; + break; + } + } + } /* end for */ - ret = strcmp(value_used, value); - VRFY(ret == 0, "strcmp value"); + VRFY((same == true), "key_used and value_used are the same"); ret = H5Pclose(fapl_id); VRFY((ret >= 0), "H5Pclose succeeded"); From 99da0e6188437aa137807a9e865ea8bc48273480 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 25 Oct 2023 02:41:28 +0000 Subject: [PATCH 8/8] Committing clang-format changes --- testpar/t_file.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/testpar/t_file.c b/testpar/t_file.c index 40853b467f7..ce55270cdd2 100644 --- a/testpar/t_file.c +++ b/testpar/t_file.c @@ -1132,11 +1132,11 @@ test_fapl_preserve_hints(void) hid_t fapl_id = H5I_INVALID_HID; /* File access plist */ const char *filename; - int nkeys_used; + int nkeys_used; bool same = false; - MPI_Info info = MPI_INFO_NULL; - const char *key = "hdf_info_fapl"; + MPI_Info info = MPI_INFO_NULL; + const char *key = "hdf_info_fapl"; const char *value = "xyz"; MPI_Info info_used = MPI_INFO_NULL; @@ -1144,9 +1144,9 @@ test_fapl_preserve_hints(void) char value_used[20]; char key_used[20]; - int i; - herr_t ret; /* Generic return value */ - int mpi_ret; /* MPI return value */ + int i; + herr_t ret; /* Generic return value */ + int mpi_ret; /* MPI return value */ filename = (const char *)GetTestParameters();