Re: [PATCH RFC v15 01/11] fs/lock: add helper locks_any_blockers to check for blockers

[dai.ngo@oracle.com]

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
>
>
>