Re: [PATCH RFC v12 1/3] fs/lock: add new callback, lm_lock_conflict, to lock_manager_operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On Feb 10, 2022, at 9:28 AM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> 
> On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index bbf812ce89a8..726d0005e32f 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1068,6 +1068,14 @@ struct lock_manager_operations {
>> 	int (*lm_change)(struct file_lock *, int, struct list_head *);
>> 	void (*lm_setup)(struct file_lock *, void **);
>> 	bool (*lm_breaker_owns_lease)(struct file_lock *);
>> +	/*
>> +	 * This callback function is called after a lock conflict is
>> +	 * detected. This allows the lock manager of the lock that
>> +	 * causes the conflict to see if the conflict can be resolved
>> +	 * somehow. If it can then this callback returns false; the
>> +	 * conflict was resolved, else returns true.
>> +	 */
>> +	bool (*lm_lock_conflict)(struct file_lock *cfl);
>> };
> 
> I don't love that name.  The function isn't checking for a lock
> conflict--it'd have to know *what* the lock is conflicting with.  It's
> being told whether the lock is still valid.
> 
> I'd prefer lm_lock_expired(), with the opposite return values.

Or even lm_lock_is_expired(). I agree that the sense of the
return values should be reversed.


The block comment does not belong in struct lock_manager_operations,
IMO.

Jeff's previous review comment was:

>> @@ -1059,6 +1062,9 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
>> 		list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
>> 			if (!posix_locks_conflict(request, fl))
>> 				continue;
>> +			if (fl->fl_lmops && fl->fl_lmops->lm_lock_conflict &&
>> +				!fl->fl_lmops->lm_lock_conflict(fl))
>> +				continue;
> 
> The naming of this op is a little misleading. We already know that there
> is a lock confict in this case. The question is whether it's resolvable
> by expiring a tardy client. That said, I don't have a better name to
> suggest at the moment.
> 
> A comment about what this function actually tells us would be nice here.


I agree that a comment that spells out the API contract would be
useful. But it doesn't belong in the middle of struct
lock_manager_operations, IMO.

I usually put such information in the block comment that precedes
the individual functions (nfsd4_fl_lock_conflict in this case).

Even so, the patch description has this information already.
Jeff, I think the patch description is adequate for this
purpose -- more information appears later in 3/3. What do you
think?


--
Chuck Lever







[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux