-
Notifications
You must be signed in to change notification settings - Fork 859
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.
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
fs/ufs: change default locking protocol #12756
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
* Copyright (c) 2015-2018 Research Organization for Information Science | ||
* and Technology (RIST). All rights reserved. | ||
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved. | ||
* Copyright (c) 2024 Advanced Micro Devices, Inc. All rights reserverd. | ||
* $COPYRIGHT$ | ||
* | ||
* Additional copyrights may follow | ||
|
@@ -105,12 +106,6 @@ mca_fs_ufs_file_open (struct ompi_communicator_t *comm, | |
component. */ | ||
fh->f_flags |= OMPIO_LOCK_ENTIRE_FILE; | ||
} | ||
else { | ||
fh->f_flags |= OMPIO_LOCK_NEVER; | ||
} | ||
} | ||
else { | ||
fh->f_flags |= OMPIO_LOCK_NEVER; | ||
} | ||
free (fstype); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not knowing this code path very well, it looks to me like you handled Lustre and NFS just fine. However, is there any case other than NFS and Lustre? Seems that previously such a case would have set OMPIO_LOCK_NEVER but now it would be unset (LOCK_RANGES). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the fs/ufs component handles everything that is not Lustre at the moment (e.g. NFS, local file systems, BeeGFS, etc.). We treat all of them equal except for NFS. Lustre requires special handling because it allows us to modify some additional parameters (stripe size, stripe count, etc.), hence it has its own component. So from the fs/ufs component perspective its either NFS (in which case we activate locking) or everything else (in which case we now do range locking instead of no locking). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot that we also have separate components for GPFS and IME, not sure how much they are in use however. We also used to have separate components for PVFS2/OrangeFS, but that was removed a few weeks ago. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just re-read the commit message. Your intent is to change those paths from LOCK_NEVER to LOCK_RANGE, which is what you have done. 👍 |
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (feel free to ignore) This could be more user friendly if it was implemented with mca_base_var_enum_t, but requires more code.