Nfs feature vfd swmr #1208

vchoi-hdfgroup · 2021-11-17T17:30:01Z

No description provided.

Feature/vfd swmr

VFD SWMR: Updates the mirror VFD tests so they compile and pass tests…

Feature/vfd swmr

src/H5Pfapl.c src/H5Fvfd_swmr.c src/H5Fpublic.h src/H5Fpkg.h src/H5Fprivate.h 2) For VFD SWMR testing, add private property for checksum generation of metadata files: src/H5Fint.c src/H5Fvfd_swmr.c src/H5Pfapl.c src/H5Fpkg.h src/H5Fprivate.h 3) Fix the following in H5F_vfd_swmr_init() and H5F_vfd_swmr_close_or_flush(): (a) Allocate metadata file index right after metadata file header. (b) Set tick number to 0 when creating header and index for file open case. (c) Remove tick number increment at file close. src/H5Fvfd_swmr.c src/H5Ftest.c 4) To be consistent with the RFC, change the name for field "chksum" to "checksum" in struct H5FD_vfd_swmr_idx_entry_t: src/H5FDprivate.h src/H5FDtest.c src/H5FDvfd_swmr.c src/H5Ftest.c src/H5PB.c 4) Add tests for NFS/updater test/vfd_swmr.c 5) Modify common routine init_vfd_swmr_config() to accept updater_file_path test/vfd_swmr_common.c test/vfd_swmr_common.h 6) Changes to the tests due to the common routine init_vfd_swmr_config(): test/vfd_swmr_addrem_writer.c test/vfd_swmr_attrdset_writer.c test/vfd_swmr_bigset_writer.c test/vfd_swmr_dsetchks_writer.c test/vfd_swmr_dsetops_writer.c test/vfd_swmr_generator.c test/vfd_swmr_gfail_writer.c test/vfd_swmr_gperf_writer.c test/vfd_swmr_group_writer.c test/vfd_swmr_reader.c test/vfd_swmr_remove_reader.c test/vfd_swmr_remove_writer.c test/vfd_swmr_sparse_reader.c test/vfd_swmr_sparse_writer.c test/vfd_swmr_vlstr_reader.c test/vfd_swmr_vlstr_writer.c test/vfd_swmr_writer.c test/page_buffer.c

kyang2014

My comments are mostly trivial. After Vailin check them and make change as appropriate. I don't need to review them again.

kyang2014 · 2021-11-17T19:32:58Z

src/H5Fint.c

+    hbool_t                 ci_write            = FALSE;    /* Whether MDC CI write requested */
+    hbool_t                 file_create         = FALSE;    /* Creating a new file or not */
+    H5F_vfd_swmr_config_t * vfd_swmr_config_ptr = NULL;     /* Points to VFD SMWR config info */
+    H5F_generate_md_ck_cb_t cb_info             = {NULL};


May add a comment on what this callback is for like the other variables?

kyang2014 · 2021-11-17T19:33:42Z

src/H5Fint.c

+    hbool_t                 evict_on_close;                 /* Evict on close value from plist */
+    hbool_t                 use_file_locking    = TRUE;     /* Using file locks? */
+    hbool_t                 ci_load             = FALSE;    /* Whether MDC ci load requested */
+    hbool_t                 ci_write            = FALSE;    /* Whether MDC CI write requested */


Minor: CI in the comment may ci like the last line

kyang2014 · 2021-11-17T20:03:04Z

src/H5Fprivate.h

+     + 4                 /* Page size */                                                                     \
+     + 8                 /* Sequence number */                                                               \
+     + 8                 /* Tick number */                                                                   \
+     + 8                 /* Chnage list offset */                                                            \


Minor: typo - Chnage should be Change

kyang2014 · 2021-11-17T20:05:19Z

src/H5Fprivate.h

+ *
+ *  An array of instances of H5F_vfd_swmr_updater_cl_entry_t of length equal to
+ *  the number of metadata pages and multi-page metadata entries modified in
+ *  the past tick is used ot aseemble the assoicated data in preparation for


Minor typo: "used ot aseemble the assoicated" should be "used to assemble the associated"

kyang2014 · 2021-11-17T20:12:33Z

src/H5Fprivate.h

+ *           array of H5F_vfd_swmr_updater_cl_ entry_t whose base address
+ *           is stored in the change_list field below.  This value is also the
+ *           number of metadata pages and multi-page metadata entries that have
+ *           been modified in the past tick


Minor add a '.' at the end of "past tick"

kyang2014 · 2021-11-17T20:59:36Z

test/vfd_swmr.c

+
+    /* Remove all the updater files if exist: <updater_file_path>.<i> */
+    for (i = 0;; i++) {
+        sprintf(ud_name, "%s.%lu", updater_file_path, i);


May still use HDsprintf.

kyang2014 · 2021-11-17T21:00:46Z

test/vfd_swmr.c

+static herr_t
+verify_ud_chk(char *md_file_path, char *ud_file_path)
+{
+    char     chk_name[1024 + 4];      /* Checksum file name */


Minor: Should we use a constant to replace 1024 here? Also should the local variables be initialized? Should the ";" in 'FAIL_STACK_ERROR; ' and 'TEST_ERROR;' be removed or make them consistent?

Fix the constant for chk_name[].
As for the ";" after FAIL_STACK_ERROR and TEST_ERROR, I will add ";" to be consistent with most of those calls in this file. I checked the files in the test directory, some of those calls have ";" and some without.

kyang2014 · 2021-11-17T21:04:19Z

test/vfd_swmr.c

+ * Purpose:     This is the callback function used by
+ *              test_updater_generate_md_checksums() when the
+ *              H5F_ACS_GENERATE_MD_CK_CB_NAME property is set in fapl.
+ *                  --Opens and read the metadata file into a buffer.


Typo: Open instead of Opens

kyang2014 · 2021-11-17T21:07:18Z

test/vfd_swmr.c

+    cb_info.func = md_ck_cb;
+
+    /* Activate private property to generate checksums for updater's metadata file */
+    H5Pset(fapl, H5F_ACS_GENERATE_MD_CK_CB_NAME, &cb_info);


Should we check the return value?

kyang2014 · 2021-11-17T21:08:34Z

test/vfd_swmr.c

+            TEST_ERROR;
+    }
+    else {
+        fid = H5Fcreate(FILENAME4, H5F_ACC_TRUNC, fcpl, H5P_DEFAULT);


Should we check the returned values of APIs in this block?

raylu-hdf

I tried this branch. It worked with some basic tests.

…oup/hdf5 into nfs_feature_vfd_swmr

vchoi-hdfgroup and others added 7 commits August 27, 2021 11:20

Merge pull request #38 from HDFGroup/feature/vfd_swmr

d134859

Feature/vfd swmr

Merge pull request #40 from HDFGroup/feature/vfd_swmr

bb54464

VFD SWMR: Updates the mirror VFD tests so they compile and pass tests…

Merge pull request #42 from HDFGroup/feature/vfd_swmr

33630ee

Feature/vfd swmr

Merge pull request #43 from vchoi-hdfgroup/feature/vfd_swmr

b92c7e5

Feature/vfd swmr

Merge pull request #44 from HDFGroup/feature/vfd_swmr

d4bd58d

Feature/vfd swmr

Merge pull request #45 from vchoi-hdfgroup/feature/vfd_swmr

f329355

Feature/vfd swmr

vchoi-hdfgroup requested review from derobins, fortnern, jhendersonHDF, jrmainzer, qkoziol and soumagne as code owners November 17, 2021 17:30

Committing clang-format changes

68ddc30

vchoi-hdfgroup requested review from kyang2014 and raylu-hdf November 17, 2021 17:32

kyang2014 approved these changes Nov 17, 2021

View reviewed changes

raylu-hdf approved these changes Dec 6, 2021

View reviewed changes

vchoi and others added 6 commits December 6, 2021 18:52

Changes to address feedback from PR review.

7253d77

Committing clang-format changes

cf7f42c

Clang format changes.

03047ba

Merge branch 'nfs_feature_vfd_swmr' of https://github.com/vchoi-hdfgr…

395453c

…oup/hdf5 into nfs_feature_vfd_swmr

Trying to fix the PR's window test failures.

ae56804

Add comments about the window test failure.

ca8180f

vchoi-hdfgroup merged commit 1ca806a into HDFGroup:feature/vfd_swmr Dec 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nfs feature vfd swmr #1208

Nfs feature vfd swmr #1208

vchoi-hdfgroup commented Nov 17, 2021

kyang2014 left a comment

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

kyang2014 Nov 17, 2021

vchoi-hdfgroup Nov 19, 2021

raylu-hdf left a comment

Nfs feature vfd swmr #1208

Nfs feature vfd swmr #1208

Conversation

vchoi-hdfgroup commented Nov 17, 2021

kyang2014 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raylu-hdf left a comment

Choose a reason for hiding this comment