Merge remote-tracking branch 'origin/develop' into threading_updates

qkoziol · Jun 13, 2024 · 9fbc4dc · 9fbc4dc
2 parents ba0ed23 + 4a0b1b6
commit 9fbc4dc
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 161 deletions.
diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
@@ -802,6 +802,16 @@ Bug Fixes since HDF5-1.14.0 release
 ===================================
     Library
     -------
+    - Fixed library to allow usage of page buffering feature for serial file
+      access with parallel builds of HDF5
+
+      When HDF5 is built with parallel support enabled, the library would previously
+      disallow any usage of page buffering, even if a file was not opened with
+      parallel access. The library now allows usage of page buffering for serial
+      file access with parallel builds of HDF5. Usage of page buffering is still
+      disabled for any form of parallel file access, even if only 1 MPI process
+      is used.
+
     - Fixed a leak of datatype IDs created internally during datatype conversion
 
       Fixed an issue where the library could leak IDs that it creates internally

diff --git a/src/H5CX.c b/src/H5CX.c
@@ -153,17 +153,30 @@
  *      application's (non-default) property list.  Getting / setting these
  *      properties within the library does _not_ affect the application's
  *      property list.  Note that the naming of these fields, <foo> and
- *      <foo>_valid, is important for the H5CX_RETRIEVE_PROP_VALID ahd
- *      H5CX_RETRIEVE_PROP_VALID_SET macros to work properly.
+ *      <foo>_valid, is important for the H5CX_RETRIEVE_PROP_VALID
+ *      macro to work properly.
  *
- * - "Return-only"" properties that are returned to the application, mainly
+ * - "Return-only" properties that are returned to the application, mainly
  *      for sending out "introspection" information ("Why did collective I/O
  *      get broken for this operation?", "Which filters are set on the chunk I
  *      just directly read in?", etc) Setting these fields will cause the
  *      corresponding property in the property list to be set when the API
  *      context is popped, when returning from the API routine.  Note that the
  *      naming of these fields, <foo> and <foo>_set, is important for the
  *      H5CX_TEST_SET_PROP and H5CX_SET_PROP macros to work properly.
+ *
+ * - "Return-and-read" properties that are returned to the application to send out introspection information,
+ *      but are also queried by the library internally. Internal queries always retrieve the original value
+ *      from the property list, and update the context's value to match. These properties have both a 'valid'
+ *      and 'set' flag. <foo>_valid is true if the field has ever been populated from its underlying property
+ *      list. <foo>_set flag is true if this field has ever been set on the context for application
+ *      introspection. The naming of these fields is important for the H5CX_RETRIEVE_PROP_VALID_SET macro to
+ *      work properly.
+ *
+ *      Note that if a set operation is followed by an internal read, it is possible for <foo>_set to be true
+ *      while the value in the context matches the underlying property list, resulting in a redundant write to
+ *      the property list when the context is popped. Similarly, if a field has been set on the context but
+ *      never read internally, <foo>_valid will be false despite the context containing a meaningful value.
  */
 typedef struct H5CX_t {
     /* DXPL */
@@ -1027,7 +1040,7 @@ H5CX_restore_state(const H5CX_state_t *api_state)
 /*-------------------------------------------------------------------------
  * Function:    H5CX_free_state
  *
- * Purpose:     Free a previously retrievedAPI context state
+ * Purpose:     Free a previously retrieved API context state
  *
  * Return:      Non-negative on success / Negative on failure
  *

diff --git a/src/H5Fint.c b/src/H5Fint.c
@@ -2008,15 +2008,6 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id)
     if (H5P_get(a_plist, H5F_ACS_PAGE_BUFFER_SIZE_NAME, &page_buf_size) < 0)
         HGOTO_ERROR(H5E_FILE, H5E_CANTGET, NULL, "can't get page buffer size");
     if (page_buf_size) {
-#ifdef H5_HAVE_PARALLEL
-        /* Collective metadata writes are not supported with page buffering */
-        if (file->shared->coll_md_write)
-            HGOTO_ERROR(H5E_FILE, H5E_CANTOPENFILE, NULL,
-                        "collective metadata writes are not supported with page buffering");
-
-        /* Temporary: fail file create when page buffering feature is enabled for parallel */
-        HGOTO_ERROR(H5E_FILE, H5E_CANTOPENFILE, NULL, "page buffering is disabled for parallel");
-#endif /* H5_HAVE_PARALLEL */
         /* Query for other page buffer cache properties */
         if (H5P_get(a_plist, H5F_ACS_PAGE_BUFFER_MIN_META_PERC_NAME, &page_buf_min_meta_perc) < 0)
             HGOTO_ERROR(H5E_FILE, H5E_CANTGET, NULL, "can't get minimum metadata fraction of page buffer");
@@ -2029,14 +2020,30 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id)
         HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, NULL, "can't get evict on close value");
 
 #ifdef H5_HAVE_PARALLEL
-    /* Check for evict on close in parallel (currently unsupported) */
+    /* Check for unsupported settings in parallel */
     assert(file->shared);
     if (H5F_SHARED_HAS_FEATURE(file->shared, H5FD_FEAT_HAS_MPI)) {
         int mpi_size = H5F_shared_mpi_get_size(file->shared);
 
-        if ((mpi_size > 1) && evict_on_close)
-            HGOTO_ERROR(H5E_FILE, H5E_UNSUPPORTED, NULL,
-                        "evict on close is currently not supported in parallel HDF5");
+        /*
+         * While there shouldn't be any problems in general with using page buffering
+         * with only 1 MPI process, there are still some testing issues to be fixed.
+         * Until then, page buffering is disabled for any form of parallel access.
+         */
+        if (page_buf_size) {
+            /* Collective metadata writes are not supported with page buffering */
+            if (file->shared->coll_md_write)
+                HGOTO_ERROR(H5E_FILE, H5E_CANTOPENFILE, NULL,
+                            "collective metadata writes are not supported with page buffering");
+
+            HGOTO_ERROR(H5E_FILE, H5E_CANTOPENFILE, NULL, "page buffering is disabled for parallel");
+        }
+
+        if (mpi_size > 1) {
+            if (evict_on_close)
+                HGOTO_ERROR(H5E_FILE, H5E_UNSUPPORTED, NULL,
+                            "evict on close is currently not supported in parallel HDF5");
+        }
     }
 #endif
 

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
@@ -483,7 +483,7 @@ endforeach ()
 add_executable (chunk_info ${HDF5_TEST_SOURCE_DIR}/chunk_info.c)
 target_compile_options(chunk_info PRIVATE "${HDF5_CMAKE_C_FLAGS}")
 target_compile_definitions(chunk_info PRIVATE "${HDF5_TEST_COMPILE_DEFS_PRIVATE}")
-target_include_directories (chunk_info PRIVATE "${HDF5_SRC_INCLUDE_DIRS};${HDF5_SRC_BINARY_DIR};${HDF5_TEST_BINARY_DIR};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:${MPI_C_INCLUDE_DIRS}>")
+target_include_directories (chunk_info PRIVATE "${HDF5_SRC_INCLUDE_DIRS};${HDF5_COMP_INCLUDE_DIRECTORIES};${HDF5_SRC_BINARY_DIR};${HDF5_TEST_BINARY_DIR};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:${MPI_C_INCLUDE_DIRS}>")
 if (NOT BUILD_SHARED_LIBS)
   TARGET_C_PROPERTIES (chunk_info STATIC)
   target_link_libraries (chunk_info PRIVATE ${HDF5_TEST_LIB_TARGET} ${LINK_COMP_LIBS})
@@ -504,7 +504,7 @@ endif ()
 add_executable (direct_chunk ${HDF5_TEST_SOURCE_DIR}/direct_chunk.c)
 target_compile_options(direct_chunk PRIVATE "${HDF5_CMAKE_C_FLAGS}")
 target_compile_definitions(direct_chunk PRIVATE "${HDF5_TEST_COMPILE_DEFS_PRIVATE}")
-target_include_directories (direct_chunk PRIVATE "${HDF5_SRC_INCLUDE_DIRS};${HDF5_SRC_BINARY_DIR};${HDF5_TEST_BINARY_DIR};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:${MPI_C_INCLUDE_DIRS}>")
+target_include_directories (direct_chunk PRIVATE "${HDF5_SRC_INCLUDE_DIRS};${HDF5_COMP_INCLUDE_DIRECTORIES};${HDF5_SRC_BINARY_DIR};${HDF5_TEST_BINARY_DIR};$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:${MPI_C_INCLUDE_DIRS}>")
 if (NOT BUILD_SHARED_LIBS)
   TARGET_C_PROPERTIES (direct_chunk STATIC)
   target_link_libraries (direct_chunk PRIVATE ${HDF5_TEST_LIB_TARGET} ${LINK_COMP_LIBS})

diff --git a/test/page_buffer.c b/test/page_buffer.c
@@ -38,9 +38,6 @@
 #define FILENAME_LEN 1024
 
 /* test routines */
-#ifdef H5_HAVE_PARALLEL
-static unsigned verify_page_buffering_disabled(hid_t orig_fapl, const char *driver_name);
-#else
 #define NUM_DSETS 5
 #define NX        100
 #define NY        50
@@ -54,12 +51,9 @@ static unsigned test_stats_collection(hid_t orig_fapl, const char *driver_name);
 /* helper routines */
 static unsigned create_file(char *filename, hid_t fcpl, hid_t fapl);
 static unsigned open_file(char *filename, hid_t fapl, hsize_t page_size, size_t page_buffer_size);
-#endif /* H5_HAVE_PARALLEL */
 
 static const char *FILENAME[] = {"filepaged", NULL};
 
-#ifndef H5_HAVE_PARALLEL
-
 /*-------------------------------------------------------------------------
  * Function:    create_file()
  *
@@ -289,7 +283,6 @@ open_file(char *filename, hid_t fapl, hsize_t page_size, size_t page_buffer_size
     H5E_END_TRY
     return 1;
 }
-#endif /* H5_HAVE_PARALLEL */
 
 /*
  *
@@ -353,8 +346,6 @@ set_multi_split(const char *driver_name, hid_t fapl, hsize_t pagesize)
 
 } /* set_multi_split() */
 
-#ifndef H5_HAVE_PARALLEL
-
 /*-------------------------------------------------------------------------
  * Function:    test_args()
  *
@@ -2043,121 +2034,6 @@ test_stats_collection(hid_t orig_fapl, const char *driver_name)
 
     return 1;
 } /* test_stats_collection */
-#endif /* #ifndef H5_HAVE_PARALLEL */
-
-/*-------------------------------------------------------------------------
- * Function:    verify_page_buffering_disabled()
- *
- * Purpose:     This function should only be called in parallel
- *              builds.
- *
- *              At present, page buffering should be disabled in parallel
- *              builds.  Verify this.
- *
- * Return:      0 if test is successful
- *              1 if test fails
- *
- *-------------------------------------------------------------------------
- */
-
-#ifdef H5_HAVE_PARALLEL
-static unsigned
-verify_page_buffering_disabled(hid_t orig_fapl, const char *driver_name)
-{
-    char  filename[FILENAME_LEN];    /* Filename to use */
-    hid_t file_id = H5I_INVALID_HID; /* File ID */
-    hid_t fcpl    = H5I_INVALID_HID;
-    hid_t fapl    = H5I_INVALID_HID;
-
-    TESTING("Page Buffering Disabled");
-    h5_fixname(FILENAME[0], orig_fapl, filename, sizeof(filename));
-
-    /* first, try to create a file with page buffering enabled */
-
-    if ((fapl = H5Pcopy(orig_fapl)) < 0)
-        TEST_ERROR;
-
-    if (set_multi_split(driver_name, fapl, 4096) != 0)
-        TEST_ERROR;
-
-    if ((fcpl = H5Pcreate(H5P_FILE_CREATE)) < 0)
-        FAIL_STACK_ERROR;
-
-    if (H5Pset_file_space_strategy(fcpl, H5F_FSPACE_STRATEGY_PAGE, 0, (hsize_t)1) < 0)
-        FAIL_STACK_ERROR;
-
-    if (H5Pset_file_space_page_size(fcpl, 4096) < 0)
-        FAIL_STACK_ERROR;
-
-    if (H5Pset_page_buffer_size(fapl, 4096 * 8, 0, 0) < 0)
-        FAIL_STACK_ERROR;
-
-    /* try to create  the file -- should fail */
-    H5E_BEGIN_TRY
-    {
-        file_id = H5Fcreate(filename, H5F_ACC_TRUNC, fcpl, fapl);
-    }
-    H5E_END_TRY
-
-    if (file_id >= 0)
-        TEST_ERROR;
-
-    /* now, create a file, close it, and then try to open it with page
-     * buffering enabled.
-     */
-    if ((fcpl = H5Pcreate(H5P_FILE_CREATE)) < 0)
-        FAIL_STACK_ERROR;
-
-    if (H5Pset_file_space_strategy(fcpl, H5F_FSPACE_STRATEGY_PAGE, 0, (hsize_t)1) < 0)
-        FAIL_STACK_ERROR;
-
-    if (H5Pset_file_space_page_size(fcpl, 4096) < 0)
-        FAIL_STACK_ERROR;
-
-    /* create the file */
-    if ((file_id = H5Fcreate(filename, H5F_ACC_TRUNC, fcpl, H5P_DEFAULT)) < 0)
-        FAIL_STACK_ERROR;
-
-    /* close the file */
-    if (H5Fclose(file_id) < 0)
-        FAIL_STACK_ERROR;
-
-    /* try to open the file using the fapl prepared above which enables
-     * page buffering.  Should fail.
-     */
-    H5E_BEGIN_TRY
-    {
-        file_id = H5Fopen(filename, H5F_ACC_RDWR, fapl);
-    }
-    H5E_END_TRY
-
-    if (file_id >= 0)
-        TEST_ERROR;
-
-    if (H5Pclose(fcpl) < 0)
-        FAIL_STACK_ERROR;
-
-    if (H5Pclose(fapl) < 0)
-        FAIL_STACK_ERROR;
-
-    PASSED();
-
-    return 0;
-
-error:
-
-    H5E_BEGIN_TRY
-    {
-        H5Pclose(fapl);
-        H5Pclose(fcpl);
-        H5Fclose(file_id);
-    }
-    H5E_END_TRY
-
-    return 1;
-
-} /* verify_page_buffering_disabled() */
-#endif /* H5_HAVE_PARALLEL */
 
 /*-------------------------------------------------------------------------
  * Function:    main()
@@ -2203,22 +2079,13 @@ main(void)
         FAIL_STACK_ERROR;
     api_ctx_pushed = true;
 
-#ifdef H5_HAVE_PARALLEL
-
-    puts("Page Buffering is disabled for parallel.");
-    nerrors += verify_page_buffering_disabled(fapl, driver_name);
-
-#else /* H5_HAVE_PARALLEL */
-
     nerrors += test_args(fapl, driver_name);
     nerrors += test_raw_data_handling(fapl, driver_name);
     nerrors += test_lru_processing(fapl, driver_name);
     nerrors += test_min_threshold(fapl, driver_name);
     nerrors += test_stats_collection(fapl, driver_name);
     nerrors += test_pb_fapl_tolerance_at_open();
 
-#endif /* H5_HAVE_PARALLEL */
-
     h5_clean_files(FILENAME, fapl);
 
     if (nerrors)