| Message ID | 1646440633-3542-2-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: Initial implementation of NFSv4 Courteous Server | expand |
> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > Add helper locks_any_blockers to check if there is any blockers > for a file_lock. > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > include/linux/fs.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 831b20430d6e..7f5756bfcc13 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1200,6 +1200,11 @@ extern void lease_unregister_notifier(struct notifier_block *); > struct files_struct; > extern void show_fd_locks(struct seq_file *f, > struct file *filp, struct files_struct *files); > + > +static inline bool locks_has_blockers_locked(struct file_lock *lck) > +{ > + return !list_empty(&lck->fl_blocked_requests); > +} > #else /* !CONFIG_FILE_LOCKING */ > static inline int fcntl_getlk(struct file *file, unsigned int cmd, > struct flock __user *user) > @@ -1335,6 +1340,11 @@ static inline int lease_modify(struct file_lock *fl, int arg, > struct files_struct; > static inline void show_fd_locks(struct seq_file *f, > struct file *filp, struct files_struct *files) {} > + > +static inline bool locks_has_blockers_locked(struct file_lock *lck) > +{ > + return false; > +} > #endif /* !CONFIG_FILE_LOCKING */ > > static inline struct inode *file_inode(const struct file *f) Hm. This is not exactly what I had in mind. In order to be more kABI friendly, fl_blocked_requests should be dereferenced only in fs/locks.c. IMO you want to take the inner loop in nfs4_lockowner_has_blockers() and make that a function that lives in fs/locks.c. Something akin to: fs/locks.c: /** * locks_owner_has_blockers - Check for blocking lock requests * @flctx: file lock context * @owner: lock owner * * Return values: * %true: @ctx has at least one blocker * %false: @ctx has no blockers */ bool locks_owner_has_blockers(struct file_lock_context *flctx, fl_owner_t owner) { struct file_lock *fl; spin_lock(&flctx->flc_lock); list_for_each_entry(fl, &flctx->flc_posix, fl_list) { if (fl->fl_owner != owner) continue; if (!list_empty(&fl->fl_blocked_requests)) { spin_unlock(&flctx->flc_lock); return true; } } spin_unlock(&flctx->flc_lock); return false; } EXPORT_SYMBOL(locks_owner_has_blockers); As a subsequent clean up (which anyone can do at a later point), a similar change could be done to check_for_locks(). This bit of code seems to appear in several other filesystems, for example: 7643 inode = locks_inode(nf->nf_file); 7644 flctx = inode->i_flctx; 7645 7646 if (flctx && !list_empty_careful(&flctx->flc_posix)) { 7647 spin_lock(&flctx->flc_lock); 7648 list_for_each_entry(fl, &flctx->flc_posix, fl_list) { 7649 if (fl->fl_owner == (fl_owner_t)lowner) { 7650 status = true; 7651 break; 7652 } 7653 } 7654 spin_unlock(&flctx->flc_lock); 7655 } -- Chuck Lever
On 3/9/22 12:56 PM, Chuck Lever III wrote: > >> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >> >> Add helper locks_any_blockers to check if there is any blockers >> for a file_lock. >> >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> include/linux/fs.h | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index 831b20430d6e..7f5756bfcc13 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -1200,6 +1200,11 @@ extern void lease_unregister_notifier(struct notifier_block *); >> struct files_struct; >> extern void show_fd_locks(struct seq_file *f, >> struct file *filp, struct files_struct *files); >> + >> +static inline bool locks_has_blockers_locked(struct file_lock *lck) >> +{ >> + return !list_empty(&lck->fl_blocked_requests); >> +} >> #else /* !CONFIG_FILE_LOCKING */ >> static inline int fcntl_getlk(struct file *file, unsigned int cmd, >> struct flock __user *user) >> @@ -1335,6 +1340,11 @@ static inline int lease_modify(struct file_lock *fl, int arg, >> struct files_struct; >> static inline void show_fd_locks(struct seq_file *f, >> struct file *filp, struct files_struct *files) {} >> + >> +static inline bool locks_has_blockers_locked(struct file_lock *lck) >> +{ >> + return false; >> +} >> #endif /* !CONFIG_FILE_LOCKING */ >> >> static inline struct inode *file_inode(const struct file *f) > Hm. This is not exactly what I had in mind. > > In order to be more kABI friendly, fl_blocked_requests should be > dereferenced only in fs/locks.c. IMO you want to take the inner > loop in nfs4_lockowner_has_blockers() and make that a function > that lives in fs/locks.c. Something akin to: > > fs/locks.c: > > /** > * locks_owner_has_blockers - Check for blocking lock requests > * @flctx: file lock context > * @owner: lock owner > * > * Return values: > * %true: @ctx has at least one blocker > * %false: @ctx has no blockers > */ > bool locks_owner_has_blockers(struct file_lock_context *flctx, > fl_owner_t owner) > { > struct file_lock *fl; > > spin_lock(&flctx->flc_lock); > list_for_each_entry(fl, &flctx->flc_posix, fl_list) { > if (fl->fl_owner != owner) > continue; > if (!list_empty(&fl->fl_blocked_requests)) { > spin_unlock(&flctx->flc_lock); > return true; > } > } > spin_unlock(&flctx->flc_lock); > return false; > } > EXPORT_SYMBOL(locks_owner_has_blockers); > > As a subsequent clean up (which anyone can do at a later point), > a similar change could be done to check_for_locks(). This bit of > code seems to appear in several other filesystems, for example: I used check_for_locks as reference for locks_owner_has_blockers so both should be updated to be more kABI friendly as you suggested. Can I update both in a subsequent cleanup patch? I plan to have a number of small cleanup up patches for these and also some nits. Thanks, -Dai > > 7643 inode = locks_inode(nf->nf_file); > 7644 flctx = inode->i_flctx; > 7645 > 7646 if (flctx && !list_empty_careful(&flctx->flc_posix)) { > 7647 spin_lock(&flctx->flc_lock); > 7648 list_for_each_entry(fl, &flctx->flc_posix, fl_list) { > 7649 if (fl->fl_owner == (fl_owner_t)lowner) { > 7650 status = true; > 7651 break; > 7652 } > 7653 } > 7654 spin_unlock(&flctx->flc_lock); > 7655 } > > > -- > Chuck Lever > > >
> On Mar 9, 2022, at 10:11 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > On 3/9/22 12:56 PM, Chuck Lever III wrote: >> >>>> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >>> >>> Add helper locks_any_blockers to check if there is any blockers >>> for a file_lock. >>> >>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>> --- >>> include/linux/fs.h | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/include/linux/fs.h b/include/linux/fs.h >>> index 831b20430d6e..7f5756bfcc13 100644 >>> --- a/include/linux/fs.h >>> +++ b/include/linux/fs.h >>> @@ -1200,6 +1200,11 @@ extern void lease_unregister_notifier(struct notifier_block *); >>> struct files_struct; >>> extern void show_fd_locks(struct seq_file *f, >>> struct file *filp, struct files_struct *files); >>> + >>> +static inline bool locks_has_blockers_locked(struct file_lock *lck) >>> +{ >>> + return !list_empty(&lck->fl_blocked_requests); >>> +} >>> #else /* !CONFIG_FILE_LOCKING */ >>> static inline int fcntl_getlk(struct file *file, unsigned int cmd, >>> struct flock __user *user) >>> @@ -1335,6 +1340,11 @@ static inline int lease_modify(struct file_lock *fl, int arg, >>> struct files_struct; >>> static inline void show_fd_locks(struct seq_file *f, >>> struct file *filp, struct files_struct *files) {} >>> + >>> +static inline bool locks_has_blockers_locked(struct file_lock *lck) >>> +{ >>> + return false; >>> +} >>> #endif /* !CONFIG_FILE_LOCKING */ >>> >>> static inline struct inode *file_inode(const struct file *f) >> Hm. This is not exactly what I had in mind. >> >> In order to be more kABI friendly, fl_blocked_requests should be >> dereferenced only in fs/locks.c. IMO you want to take the inner >> loop in nfs4_lockowner_has_blockers() and make that a function >> that lives in fs/locks.c. Something akin to: >> >> fs/locks.c: >> >> /** >> * locks_owner_has_blockers - Check for blocking lock requests >> * @flctx: file lock context >> * @owner: lock owner >> * >> * Return values: >> * %true: @ctx has at least one blocker >> * %false: @ctx has no blockers >> */ >> bool locks_owner_has_blockers(struct file_lock_context *flctx, >> fl_owner_t owner) >> { >> struct file_lock *fl; >> >> spin_lock(&flctx->flc_lock); >> list_for_each_entry(fl, &flctx->flc_posix, fl_list) { >> if (fl->fl_owner != owner) >> continue; >> if (!list_empty(&fl->fl_blocked_requests)) { >> spin_unlock(&flctx->flc_lock); >> return true; >> } >> } >> spin_unlock(&flctx->flc_lock); >> return false; >> } >> EXPORT_SYMBOL(locks_owner_has_blockers); >> >> As a subsequent clean up (which anyone can do at a later point), >> a similar change could be done to check_for_locks(). This bit of >> code seems to appear in several other filesystems, for example: > > I used check_for_locks as reference for locks_owner_has_blockers > so both should be updated to be more kABI friendly as you suggested. > Can I update both in a subsequent cleanup patch? No. I prefer that you don’t introduce code and then clean it up later in the same series. You need to introduce locks_owner_has_blockers() in the same patch where you add the nfsd4_ function that will use it. I think that’s like 7 or 8/11 ? I don’t have it in front of me. Because it deduplicates code, cleaning up check_for_locks() will need to involve at least one other site (outside of NFSD) that can make use of this new helper. Therefore I prefer that you wait and do that work after courteous server is merged. > I plan to have a number of small cleanup up patches for these and also some nits. Clean-ups of areas not having to do with courteous server can go in separate patches, but I would prefer to keep clean-ups of the new code in this series integrated together in the same patches with the new code. Let’s stay focused on finishing the courteous server work and getting it merged. > Thanks, > -Dai > > >> >> 7643 inode = locks_inode(nf->nf_file); >> 7644 flctx = inode->i_flctx; >> 7645 >> 7646 if (flctx && !list_empty_careful(&flctx->flc_posix)) { >> 7647 spin_lock(&flctx->flc_lock); >> 7648 list_for_each_entry(fl, &flctx->flc_posix, fl_list) { >> 7649 if (fl->fl_owner == (fl_owner_t)lowner) { >> 7650 status = true; >> 7651 break; >> 7652 } >> 7653 } >> 7654 spin_unlock(&flctx->flc_lock); >> 7655 } >> >> >> -- >> Chuck Lever >> >> >>
On 3/9/22 8:08 PM, Chuck Lever III wrote: >> On Mar 9, 2022, at 10:11 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >> >> On 3/9/22 12:56 PM, Chuck Lever III wrote: >>>>> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >>>> Add helper locks_any_blockers to check if there is any blockers >>>> for a file_lock. >>>> >>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>>> --- >>>> include/linux/fs.h | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/include/linux/fs.h b/include/linux/fs.h >>>> index 831b20430d6e..7f5756bfcc13 100644 >>>> --- a/include/linux/fs.h >>>> +++ b/include/linux/fs.h >>>> @@ -1200,6 +1200,11 @@ extern void lease_unregister_notifier(struct notifier_block *); >>>> struct files_struct; >>>> extern void show_fd_locks(struct seq_file *f, >>>> struct file *filp, struct files_struct *files); >>>> + >>>> +static inline bool locks_has_blockers_locked(struct file_lock *lck) >>>> +{ >>>> + return !list_empty(&lck->fl_blocked_requests); >>>> +} >>>> #else /* !CONFIG_FILE_LOCKING */ >>>> static inline int fcntl_getlk(struct file *file, unsigned int cmd, >>>> struct flock __user *user) >>>> @@ -1335,6 +1340,11 @@ static inline int lease_modify(struct file_lock *fl, int arg, >>>> struct files_struct; >>>> static inline void show_fd_locks(struct seq_file *f, >>>> struct file *filp, struct files_struct *files) {} >>>> + >>>> +static inline bool locks_has_blockers_locked(struct file_lock *lck) >>>> +{ >>>> + return false; >>>> +} >>>> #endif /* !CONFIG_FILE_LOCKING */ >>>> >>>> static inline struct inode *file_inode(const struct file *f) >>> Hm. This is not exactly what I had in mind. >>> >>> In order to be more kABI friendly, fl_blocked_requests should be >>> dereferenced only in fs/locks.c. IMO you want to take the inner >>> loop in nfs4_lockowner_has_blockers() and make that a function >>> that lives in fs/locks.c. Something akin to: >>> >>> fs/locks.c: >>> >>> /** >>> * locks_owner_has_blockers - Check for blocking lock requests >>> * @flctx: file lock context >>> * @owner: lock owner >>> * >>> * Return values: >>> * %true: @ctx has at least one blocker >>> * %false: @ctx has no blockers >>> */ >>> bool locks_owner_has_blockers(struct file_lock_context *flctx, >>> fl_owner_t owner) >>> { >>> struct file_lock *fl; >>> >>> spin_lock(&flctx->flc_lock); >>> list_for_each_entry(fl, &flctx->flc_posix, fl_list) { >>> if (fl->fl_owner != owner) >>> continue; >>> if (!list_empty(&fl->fl_blocked_requests)) { >>> spin_unlock(&flctx->flc_lock); >>> return true; >>> } >>> } >>> spin_unlock(&flctx->flc_lock); >>> return false; >>> } >>> EXPORT_SYMBOL(locks_owner_has_blockers); >>> >>> As a subsequent clean up (which anyone can do at a later point), >>> a similar change could be done to check_for_locks(). This bit of >>> code seems to appear in several other filesystems, for example: >> I used check_for_locks as reference for locks_owner_has_blockers >> so both should be updated to be more kABI friendly as you suggested. >> Can I update both in a subsequent cleanup patch? > No. I prefer that you don’t introduce code and then clean it up later in the same series. > > You need to introduce locks_owner_has_blockers() in the same patch where you add the nfsd4_ function that will use it. I think that’s like 7 or 8/11 ? I don’t have it in front of me. ok, fix in v16. > > Because it deduplicates code, cleaning up check_for_locks() will need to involve at least one other site (outside of NFSD) that can make use of this new helper. Therefore I prefer that you wait and do that work after courteous server is merged. ok. > > >> I plan to have a number of small cleanup up patches for these and also some nits. > Clean-ups of areas not having to do with courteous server can go in separate patches, but I would prefer to keep clean-ups of the new code in this series integrated together in the same patches with the new code. > > Let’s stay focused on finishing the courteous server work and getting it merged. ok. Thanks, -Dai > > >> Thanks, >> -Dai >> >> >>> 7643 inode = locks_inode(nf->nf_file); >>> 7644 flctx = inode->i_flctx; >>> 7645 >>> 7646 if (flctx && !list_empty_careful(&flctx->flc_posix)) { >>> 7647 spin_lock(&flctx->flc_lock); >>> 7648 list_for_each_entry(fl, &flctx->flc_posix, fl_list) { >>> 7649 if (fl->fl_owner == (fl_owner_t)lowner) { >>> 7650 status = true; >>> 7651 break; >>> 7652 } >>> 7653 } >>> 7654 spin_unlock(&flctx->flc_lock); >>> 7655 } >>> >>> >>> -- >>> Chuck Lever >>> >>> >>>
diff --git a/include/linux/fs.h b/include/linux/fs.h index 831b20430d6e..7f5756bfcc13 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1200,6 +1200,11 @@ extern void lease_unregister_notifier(struct notifier_block *); struct files_struct; extern void show_fd_locks(struct seq_file *f, struct file *filp, struct files_struct *files); + +static inline bool locks_has_blockers_locked(struct file_lock *lck) +{ + return !list_empty(&lck->fl_blocked_requests); +} #else /* !CONFIG_FILE_LOCKING */ static inline int fcntl_getlk(struct file *file, unsigned int cmd, struct flock __user *user) @@ -1335,6 +1340,11 @@ static inline int lease_modify(struct file_lock *fl, int arg, struct files_struct; static inline void show_fd_locks(struct seq_file *f, struct file *filp, struct files_struct *files) {} + +static inline bool locks_has_blockers_locked(struct file_lock *lck) +{ + return false; +} #endif /* !CONFIG_FILE_LOCKING */ static inline struct inode *file_inode(const struct file *f)
Add helper locks_any_blockers to check if there is any blockers for a file_lock. Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- include/linux/fs.h | 10 ++++++++++ 1 file changed, 10 insertions(+)