Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix H5F_get_access_plist to copy file locking settings #4030

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Fix H5F_get_access_plist to copy file locking settings #4030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,33 @@ Bug Fixes since HDF5-1.14.0 release
===================================
Library
-------
- Fixed H5Fget_access_plist so that it returns the file locking
settings for a file

When H5Fget_access_plist (and the internal H5F_get_access_plist)
is called on a file, the returned File Access Property List has
the library's default file locking settings rather than any
settings set for the file. This causes two problems:

- Opening an HDF5 file through an external link using H5Gopen,
H5Dopen, etc. with H5P_DEFAULT for the Dataset/Group/etc.
Access Property List will cause the external file to be opened
with the library's default file locking settings rather than
inheriting them from the parent file. This can be surprising
when a file is opened with file locking disabled, but its
external files are opened with file locking enabled.

- An application cannot make use of the H5Pset_elink_fapl
function to match file locking settings between an external
file and its parent file without knowing the correct setting
ahead of time, as calling H5Fget_access_plist on the parent
file will not return the correct settings.

This has been fixed by copying a file's file locking settings
into the newly-created File Access Property List in H5F_get_access_plist.

This fix partially addresses GitHub issue #4011

- Memory usage growth issue

Starting with the HDF5 1.12.1 release, an issue (GitHub issue #1256)
Expand Down
103 changes: 71 additions & 32 deletions src/H5Fint.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ static int H5F__get_objects_cb(void *obj_ptr, hid_t obj_id, void *key);
static herr_t H5F__build_name(const char *prefix, const char *file_name, char **full_name /*out*/);
static char *H5F__getenv_prefix_name(char **env_prefix /*in,out*/);
static H5F_t *H5F__new(H5F_shared_t *shared, unsigned flags, hid_t fcpl_id, hid_t fapl_id, H5FD_t *lf);
static herr_t H5F__check_if_using_file_locks(H5P_genplist_t *fapl, bool *use_file_locking);
static herr_t H5F__check_if_using_file_locks(H5P_genplist_t *fapl, bool *use_file_locking,
bool *ignore_disabled_locks);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will adding the extra parameter break binary compatibility if merged to hdf5_1_14?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a static function, so changing the parameters shouldn't be a problem. Nothing directly in the public API is touched here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, good

static herr_t H5F__dest(H5F_t *f, bool flush, bool free_on_failure);
static herr_t H5F__build_actual_name(const H5F_t *f, const H5P_genplist_t *fapl, const char *name,
char ** /*out*/ actual_name);
Expand All @@ -94,7 +95,8 @@ static herr_t H5F__flush_phase2(H5F_t *f, bool closing);
* true/false have obvious meanings. FAIL means the environment variable was
* not set, so the code should ignore it and use the fapl value instead.
*/
htri_t use_locks_env_g = FAIL;
htri_t use_locks_env_g = FAIL;
htri_t ignore_disabled_locks_g = FAIL;

/*****************************/
/* Library Private Variables */
Expand Down Expand Up @@ -140,7 +142,7 @@ H5F_init(void)
HGOTO_ERROR(H5E_FILE, H5E_CANTINIT, FAIL, "unable to initialize interface");

/* Check the file locking environment variable */
if (H5F__parse_file_lock_env_var(&use_locks_env_g) < 0)
if (H5F__parse_file_lock_env_var(&use_locks_env_g, &ignore_disabled_locks_g) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "unable to parse file locking environment variable");

done:
Expand Down Expand Up @@ -237,21 +239,31 @@ H5F__close_cb(H5VL_object_t *file_vol_obj, void **request)
*-------------------------------------------------------------------------
*/
herr_t
H5F__parse_file_lock_env_var(htri_t *use_locks)
H5F__parse_file_lock_env_var(htri_t *use_locks, htri_t *ignore_disabled_locks)
{
char *lock_env_var = NULL; /* Environment variable pointer */

FUNC_ENTER_PACKAGE_NOERR

/* Check the file locking environment variable */
lock_env_var = getenv(HDF5_USE_FILE_LOCKING);
if (lock_env_var && (!strcmp(lock_env_var, "FALSE") || !strcmp(lock_env_var, "0")))
*use_locks = false; /* Override: Never use locks */
else if (lock_env_var && (!strcmp(lock_env_var, "TRUE") || !strcmp(lock_env_var, "BEST_EFFORT") ||
!strcmp(lock_env_var, "1")))
*use_locks = true; /* Override: Always use locks */
else
*use_locks = FAIL; /* Environment variable not set, or not set correctly */
if (lock_env_var && (!strcmp(lock_env_var, "FALSE") || !strcmp(lock_env_var, "0"))) {
*use_locks = false; /* Override: Never use locks */
*ignore_disabled_locks = FAIL;
}
else if (lock_env_var && !strcmp(lock_env_var, "BEST_EFFORT")) {
*use_locks = true; /* Override: Always use locks */
*ignore_disabled_locks = true; /* Override: Ignore disabled locks */
}
else if (lock_env_var && (!strcmp(lock_env_var, "TRUE") || !strcmp(lock_env_var, "1"))) {
*use_locks = true; /* Override: Always use locks */
*ignore_disabled_locks = false; /* Override: Don't ignore disabled locks */
}
else {
/* Environment variable not set, or not set correctly */
*use_locks = FAIL;
*ignore_disabled_locks = FAIL;
}

FUNC_LEAVE_NOAPI(SUCCEED)
} /* end H5F__parse_file_lock_env_var() */
Expand Down Expand Up @@ -374,8 +386,13 @@ H5F_get_access_plist(H5F_t *f, bool app_ref)
if (H5P_set(new_plist, H5F_ACS_LIBVER_HIGH_BOUND_NAME, &f->shared->high_bound) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID,
"can't set 'high' bound for library format versions");
if (H5P_set(new_plist, H5F_ACS_USE_FILE_LOCKING_NAME, &f->shared->use_file_locking) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID, "can't set file locking property");
if (H5P_set(new_plist, H5F_ACS_IGNORE_DISABLED_FILE_LOCKS_NAME, &f->shared->ignore_disabled_locks) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID,
"can't set 'ignore disabled file locks' property");
if (H5P_set(new_plist, H5F_ACS_METADATA_READ_ATTEMPTS_NAME, &(f->shared->read_attempts)) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID, "can't set 'read attempts ' flag");
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID, "can't set 'read attempts' flag");
if (H5P_set(new_plist, H5F_ACS_OBJECT_FLUSH_CB_NAME, &(f->shared->object_flush)) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, H5I_INVALID_HID, "can't set object flush callback");

Expand Down Expand Up @@ -1644,7 +1661,9 @@ H5F__dest(H5F_t *f, bool flush, bool free_on_failure)
/*-------------------------------------------------------------------------
* Function: H5F__check_if_using_file_locks
*
* Purpose: Determines if this file will use file locks.
* Purpose: Determines if this file will use file locks and whether or
* not to ignore the case where file locking is disabled on
* the file system.
*
* There are three ways that file locking can be controlled:
*
Expand All @@ -1665,22 +1684,35 @@ H5F__dest(H5F_t *f, bool flush, bool free_on_failure)
*-------------------------------------------------------------------------
*/
static herr_t
H5F__check_if_using_file_locks(H5P_genplist_t *fapl, bool *use_file_locking)
H5F__check_if_using_file_locks(H5P_genplist_t *fapl, bool *use_file_locking, bool *ignore_disabled_locks)
{
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_PACKAGE

/* Make sure the out parameter has a value */
*use_file_locking = true;
/* Make sure the out parameters have a value */
*use_file_locking = true;
*ignore_disabled_locks = false;

/* Check the fapl property */
if (H5P_get(fapl, H5F_ACS_USE_FILE_LOCKING_NAME, use_file_locking) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't get use file locking flag");
/* Check file locking environment variable first */
if (use_locks_env_g != FAIL) {
*use_file_locking = (use_locks_env_g == true);
}
else {
/* Check the file locking fapl property */
if (H5P_get(fapl, H5F_ACS_USE_FILE_LOCKING_NAME, use_file_locking) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't get use file locking flag");
}

/* Check the environment variable */
if (use_locks_env_g != FAIL)
*use_file_locking = (use_locks_env_g == true) ? true : false;
/* Check "ignore disabled file locks" environment variable first */
if (ignore_disabled_locks_g != FAIL) {
*ignore_disabled_locks = (ignore_disabled_locks_g == true);
}
else {
/* Check the "ignore disabled file locks" fapl property */
if (H5P_get(fapl, H5F_ACS_IGNORE_DISABLED_FILE_LOCKS_NAME, ignore_disabled_locks) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't get ignore disabled file locks property");
}

done:
FUNC_LEAVE_NOAPI(ret_value)
Expand Down Expand Up @@ -1775,10 +1807,11 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id)
bool set_flag = false; /*set the status_flags in the superblock */
bool clear = false; /*clear the status_flags */
bool evict_on_close; /* evict on close value from plist */
bool use_file_locking = true; /* Using file locks? */
bool ci_load = false; /* whether MDC ci load requested */
bool ci_write = false; /* whether MDC CI write requested */
H5F_t *ret_value = NULL; /*actual return value */
bool use_file_locking = true; /* Using file locks? */
bool ignore_disabled_locks = false; /* Ignore disabled file locks? */
bool ci_load = false; /* whether MDC ci load requested */
bool ci_write = false; /* whether MDC CI write requested */
H5F_t *ret_value = NULL; /*actual return value */

FUNC_ENTER_NOAPI(NULL)

Expand All @@ -1798,8 +1831,8 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id)
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, NULL, "not file access property list");

/* Check if we are using file locking */
if (H5F__check_if_using_file_locks(a_plist, &use_file_locking) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, NULL, "unable to get file locking flag");
if (H5F__check_if_using_file_locks(a_plist, &use_file_locking, &ignore_disabled_locks) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, NULL, "unable to get file locking flags");

/*
* Opening a file is a two step process. First we try to open the
Expand Down Expand Up @@ -1951,14 +1984,20 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id)
shared = file->shared;
lf = shared->lf;

/* Set the file locking flag. If the file is already open, the file
/* Set the file locking flags. If the file is already open, the file
* requested file locking flag must match that of the open file.
*/
if (shared->nrefs == 1)
file->shared->use_file_locking = use_file_locking;
else if (shared->nrefs > 1)
if (shared->nrefs == 1) {
file->shared->use_file_locking = use_file_locking;
file->shared->ignore_disabled_locks = ignore_disabled_locks;
}
else if (shared->nrefs > 1) {
if (file->shared->use_file_locking != use_file_locking)
HGOTO_ERROR(H5E_FILE, H5E_CANTINIT, NULL, "file locking flag values don't match");
if (file->shared->use_file_locking && (file->shared->ignore_disabled_locks != ignore_disabled_locks))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't quite sure on this addition, but it seems reasonable that, similar to how we check the file locking setting, we would also want to check that a file which is opened multiple times has the same "ignore disabled file locks" setting each time.

HGOTO_ERROR(H5E_FILE, H5E_CANTINIT, NULL,
"file locking 'ignore disabled locks' flag values don't match");
}

/* Check if page buffering is enabled */
if (H5P_get(a_plist, H5F_ACS_PAGE_BUFFER_SIZE_NAME, &page_buf_size) < 0)
Expand Down
27 changes: 15 additions & 12 deletions src/H5Fpkg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,16 +293,17 @@ struct H5F_shared_t {
hsize_t threshold; /* Threshold for alignment */
hsize_t alignment; /* Alignment */
unsigned gc_ref; /* Garbage-collect references? */
H5F_libver_t low_bound; /* The 'low' bound of library format versions */
H5F_libver_t high_bound; /* The 'high' bound of library format versions */
bool store_msg_crt_idx; /* Store creation index for object header messages? */
unsigned ncwfs; /* Num entries on cwfs list */
struct H5HG_heap_t **cwfs; /* Global heap cache */
struct H5G_t *root_grp; /* Open root group */
H5FO_t *open_objs; /* Open objects in file */
H5UC_t *grp_btree_shared; /* Ref-counted group B-tree node info */
bool use_file_locking; /* Whether or not to use file locking */
bool closing; /* File is in the process of being closed */
H5F_libver_t low_bound; /* The 'low' bound of library format versions */
H5F_libver_t high_bound; /* The 'high' bound of library format versions */
bool store_msg_crt_idx; /* Store creation index for object header messages? */
unsigned ncwfs; /* Num entries on cwfs list */
struct H5HG_heap_t **cwfs; /* Global heap cache */
struct H5G_t *root_grp; /* Open root group */
H5FO_t *open_objs; /* Open objects in file */
H5UC_t *grp_btree_shared; /* Ref-counted group B-tree node info */
bool use_file_locking; /* Whether or not to use file locking */
bool ignore_disabled_locks; /* Whether or not to ignore disabled file locking */
bool closing; /* File is in the process of being closed */

/* Cached VOL connector ID & info */
hid_t vol_id; /* ID of VOL connector for the container */
Expand Down Expand Up @@ -391,9 +392,11 @@ H5FL_EXTERN(H5F_t);
H5FL_EXTERN(H5F_shared_t);

/* Whether or not to use file locking (based on the environment variable)
* FAIL means ignore the environment variable.
* and whether or not to ignore disabled file locking. FAIL means ignore
* the environment variable.
*/
H5_DLLVAR htri_t use_locks_env_g;
H5_DLLVAR htri_t ignore_disabled_locks_g;

/******************************/
/* Package Private Prototypes */
Expand All @@ -411,7 +414,7 @@ H5_DLL herr_t H5F__start_swmr_write(H5F_t *f);
H5_DLL herr_t H5F__close(H5F_t *f);
H5_DLL herr_t H5F__set_libver_bounds(H5F_t *f, H5F_libver_t low, H5F_libver_t high);
H5_DLL herr_t H5F__get_cont_info(const H5F_t *f, H5VL_file_cont_info_t *info);
H5_DLL herr_t H5F__parse_file_lock_env_var(htri_t *use_locks);
H5_DLL herr_t H5F__parse_file_lock_env_var(htri_t *use_locks, htri_t *ignore_disabled_locks);
H5_DLL herr_t H5F__delete(const char *filename, hid_t fapl_id);

/* File mount related routines */
Expand Down
2 changes: 1 addition & 1 deletion src/H5Ftest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ H5F__reparse_file_lock_variable_test(void)
FUNC_ENTER_PACKAGE

/* Check the file locking environment variable */
if (H5F__parse_file_lock_env_var(&use_locks_env_g) < 0)
if (H5F__parse_file_lock_env_var(&use_locks_env_g, &ignore_disabled_locks_g) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "unable to parse file locking environment variable");

done:
Expand Down
4 changes: 3 additions & 1 deletion src/H5Pfapl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4978,7 +4978,9 @@ H5Pget_file_locking(hid_t fapl_id, hbool_t *use_file_locking /*out*/, hbool_t *i
H5TRACE3("e", "ixx", fapl_id, use_file_locking, ignore_when_disabled);

/* Make sure this is a fapl */
if (true != H5P_isa_class(fapl_id, H5P_FILE_ACCESS))
if (H5P_DEFAULT == fapl_id)
fapl_id = H5P_FILE_ACCESS_DEFAULT;
else if (true != H5P_isa_class(fapl_id, H5P_FILE_ACCESS))
HGOTO_ERROR(H5E_PLIST, H5E_CANTREGISTER, FAIL, "property list is not an access plist");

/* Get the plist structure */
Expand Down