diff mbox series

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

Message ID 1644468729-30383-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

Commit Message

Dai Ngo Feb. 10, 2022, 4:52 a.m. UTC
Add new callback, lm_lock_conflict, to lock_manager_operations to allow
the lock manager to take appropriate action to resolve the lock conflict
if possible. The callback takes 1 argument, the file_lock of the blocker
and returns true if the conflict was resolved else returns false. Note
that the lock manager has to be able to resolve the conflict while
the spinlock flc_lock is held.

Lock manager, such as NFSv4 courteous server, uses this callback to
resolve conflict by destroying lock owner, or the NFSv4 courtesy client
(client that has expired but allowed to maintains its states) that owns
the lock.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 Documentation/filesystems/locking.rst |  2 ++
 fs/locks.c                            | 14 ++++++++++----
 include/linux/fs.h                    |  8 ++++++++
 3 files changed, 20 insertions(+), 4 deletions(-)

Comments

J. Bruce Fields Feb. 10, 2022, 2:21 p.m. UTC | #1
Jeff, this table of locking rules seems out of date since 6109c85037e5
"locks: add a dedicated spinlock to protect i_flctx lists".  Are any of
those callbacks still called with the i_lock?  Should that column be
labeled "flc_lock" instead?  Or is that even still useful information?

--b.

On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> index d36fe79167b3..57ce0fbc8ab1 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -439,6 +439,7 @@ prototypes::
>  	void (*lm_break)(struct file_lock *); /* break_lease callback */
>  	int (*lm_change)(struct file_lock **, int);
>  	bool (*lm_breaker_owns_lease)(struct file_lock *);
> +	bool (*lm_lock_conflict)(struct file_lock *);
>  
>  locking rules:
>  
> @@ -450,6 +451,7 @@ lm_grant:		no		no			no
>  lm_break:		yes		no			no
>  lm_change		yes		no			no
>  lm_breaker_owns_lease:	no		no			no
> +lm_lock_conflict:       no		no			no
>  ======================	=============	=================	=========
J. Bruce Fields Feb. 10, 2022, 2:28 p.m. UTC | #2
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.

(Apologies if this has already been woodshedded to death, I haven't been
following.)

--b.
Jeff Layton Feb. 10, 2022, 2:29 p.m. UTC | #3
On Thu, 2022-02-10 at 09:21 -0500, J. Bruce Fields wrote:
> Jeff, this table of locking rules seems out of date since 6109c85037e5
> "locks: add a dedicated spinlock to protect i_flctx lists".  Are any of
> those callbacks still called with the i_lock?  Should that column be
> labeled "flc_lock" instead?  Or is that even still useful information?
> 
> --b.


Yeah, that should probably be the flc_lock. I don't think we protect
anything in the file locking code with the i_lock anymore.

> 
> On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
> > diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> > index d36fe79167b3..57ce0fbc8ab1 100644
> > --- a/Documentation/filesystems/locking.rst
> > +++ b/Documentation/filesystems/locking.rst
> > @@ -439,6 +439,7 @@ prototypes::
> >  	void (*lm_break)(struct file_lock *); /* break_lease callback */
> >  	int (*lm_change)(struct file_lock **, int);
> >  	bool (*lm_breaker_owns_lease)(struct file_lock *);
> > +	bool (*lm_lock_conflict)(struct file_lock *);
> >  
> >  locking rules:
> >  
> > @@ -450,6 +451,7 @@ lm_grant:		no		no			no
> >  lm_break:		yes		no			no
> >  lm_change		yes		no			no
> >  lm_breaker_owns_lease:	no		no			no
> > +lm_lock_conflict:       no		no			no
> >  ======================	=============	=================	=========
>
Chuck Lever III Feb. 10, 2022, 4:50 p.m. UTC | #4
> On Feb 10, 2022, at 9:28 AM, J. Bruce Fields <bfields@fieldses.org> 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
Jeff Layton Feb. 10, 2022, 5:32 p.m. UTC | #5
On Thu, 2022-02-10 at 16:50 +0000, Chuck Lever III wrote:
> 
> > On Feb 10, 2022, at 9:28 AM, J. Bruce Fields <bfields@fieldses.org> 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?
> 

I'd be fine with that.
Dai Ngo Feb. 10, 2022, 7:41 p.m. UTC | #6
On 2/10/22 6:29 AM, Jeff Layton wrote:
> On Thu, 2022-02-10 at 09:21 -0500, J. Bruce Fields wrote:
>> Jeff, this table of locking rules seems out of date since 6109c85037e5
>> "locks: add a dedicated spinlock to protect i_flctx lists".  Are any of
>> those callbacks still called with the i_lock?  Should that column be
>> labeled "flc_lock" instead?  Or is that even still useful information?
>>
>> --b.
>
> Yeah, that should probably be the flc_lock. I don't think we protect
> anything in the file locking code with the i_lock anymore.

Will replace inode->i_lock with flc_lock in v13.

-Dai

>
>> On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
>>> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
>>> index d36fe79167b3..57ce0fbc8ab1 100644
>>> --- a/Documentation/filesystems/locking.rst
>>> +++ b/Documentation/filesystems/locking.rst
>>> @@ -439,6 +439,7 @@ prototypes::
>>>   	void (*lm_break)(struct file_lock *); /* break_lease callback */
>>>   	int (*lm_change)(struct file_lock **, int);
>>>   	bool (*lm_breaker_owns_lease)(struct file_lock *);
>>> +	bool (*lm_lock_conflict)(struct file_lock *);
>>>   
>>>   locking rules:
>>>   
>>> @@ -450,6 +451,7 @@ lm_grant:		no		no			no
>>>   lm_break:		yes		no			no
>>>   lm_change		yes		no			no
>>>   lm_breaker_owns_lease:	no		no			no
>>> +lm_lock_conflict:       no		no			no
>>>   ======================	=============	=================	=========
Chuck Lever III Feb. 10, 2022, 7:44 p.m. UTC | #7
> On Feb 10, 2022, at 2:41 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> 
> On 2/10/22 6:29 AM, Jeff Layton wrote:
>> On Thu, 2022-02-10 at 09:21 -0500, J. Bruce Fields wrote:
>>> Jeff, this table of locking rules seems out of date since 6109c85037e5
>>> "locks: add a dedicated spinlock to protect i_flctx lists".  Are any of
>>> those callbacks still called with the i_lock?  Should that column be
>>> labeled "flc_lock" instead?  Or is that even still useful information?
>>> 
>>> --b.
>> 
>> Yeah, that should probably be the flc_lock. I don't think we protect
>> anything in the file locking code with the i_lock anymore.
> 
> Will replace inode->i_lock with flc_lock in v13.

To be clear, if you're fixing the documentation, that would need
to be a clean-up patch before your 1/3. Thanks!


> -Dai
> 
>> 
>>> On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
>>>> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
>>>> index d36fe79167b3..57ce0fbc8ab1 100644
>>>> --- a/Documentation/filesystems/locking.rst
>>>> +++ b/Documentation/filesystems/locking.rst
>>>> @@ -439,6 +439,7 @@ prototypes::
>>>>  	void (*lm_break)(struct file_lock *); /* break_lease callback */
>>>>  	int (*lm_change)(struct file_lock **, int);
>>>>  	bool (*lm_breaker_owns_lease)(struct file_lock *);
>>>> +	bool (*lm_lock_conflict)(struct file_lock *);
>>>>    locking rules:
>>>>  @@ -450,6 +451,7 @@ lm_grant:		no		no			no
>>>>  lm_break:		yes		no			no
>>>>  lm_change		yes		no			no
>>>>  lm_breaker_owns_lease:	no		no			no
>>>> +lm_lock_conflict:       no		no			no
>>>>  ======================	=============	=================	=========

--
Chuck Lever
Dai Ngo Feb. 10, 2022, 7:47 p.m. UTC | #8
On 2/10/22 6:28 AM, J. Bruce Fields 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.

Will replace lm_lock_conflict with lm_lock_expired with the opposite
return values: return true if lock was expired (conflict resolved)
else return false.

>
> (Apologies if this has already been woodshedded to death, I haven't been
> following.)

Yes, the name has been changed couples of times :-) hopefully
lm_lock_expired will stick this time.


-Dai

>
> --b.
Dai Ngo Feb. 10, 2022, 7:56 p.m. UTC | #9
On 2/10/22 11:44 AM, Chuck Lever III wrote:
>
>> On Feb 10, 2022, at 2:41 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>>
>> On 2/10/22 6:29 AM, Jeff Layton wrote:
>>> On Thu, 2022-02-10 at 09:21 -0500, J. Bruce Fields wrote:
>>>> Jeff, this table of locking rules seems out of date since 6109c85037e5
>>>> "locks: add a dedicated spinlock to protect i_flctx lists".  Are any of
>>>> those callbacks still called with the i_lock?  Should that column be
>>>> labeled "flc_lock" instead?  Or is that even still useful information?
>>>>
>>>> --b.
>>> Yeah, that should probably be the flc_lock. I don't think we protect
>>> anything in the file locking code with the i_lock anymore.
>> Will replace inode->i_lock with flc_lock in v13.
> To be clear, if you're fixing the documentation, that would need
> to be a clean-up patch before your 1/3. Thanks!

Got it Chuck,

Thanks,
-Dai

>
>
>> -Dai
>>
>>>> On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
>>>>> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
>>>>> index d36fe79167b3..57ce0fbc8ab1 100644
>>>>> --- a/Documentation/filesystems/locking.rst
>>>>> +++ b/Documentation/filesystems/locking.rst
>>>>> @@ -439,6 +439,7 @@ prototypes::
>>>>>   	void (*lm_break)(struct file_lock *); /* break_lease callback */
>>>>>   	int (*lm_change)(struct file_lock **, int);
>>>>>   	bool (*lm_breaker_owns_lease)(struct file_lock *);
>>>>> +	bool (*lm_lock_conflict)(struct file_lock *);
>>>>>     locking rules:
>>>>>   @@ -450,6 +451,7 @@ lm_grant:		no		no			no
>>>>>   lm_break:		yes		no			no
>>>>>   lm_change		yes		no			no
>>>>>   lm_breaker_owns_lease:	no		no			no
>>>>> +lm_lock_conflict:       no		no			no
>>>>>   ======================	=============	=================	=========
> --
> Chuck Lever
>
>
>
diff mbox series

Patch

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index d36fe79167b3..57ce0fbc8ab1 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -439,6 +439,7 @@  prototypes::
 	void (*lm_break)(struct file_lock *); /* break_lease callback */
 	int (*lm_change)(struct file_lock **, int);
 	bool (*lm_breaker_owns_lease)(struct file_lock *);
+	bool (*lm_lock_conflict)(struct file_lock *);
 
 locking rules:
 
@@ -450,6 +451,7 @@  lm_grant:		no		no			no
 lm_break:		yes		no			no
 lm_change		yes		no			no
 lm_breaker_owns_lease:	no		no			no
+lm_lock_conflict:       no		no			no
 ======================	=============	=================	=========
 
 buffer_head
diff --git a/fs/locks.c b/fs/locks.c
index 0fca9d680978..052b42cc7f25 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -853,10 +853,13 @@  posix_test_lock(struct file *filp, struct file_lock *fl)
 
 	spin_lock(&ctx->flc_lock);
 	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
-		if (posix_locks_conflict(fl, cfl)) {
-			locks_copy_conflock(fl, cfl);
-			goto out;
-		}
+		if (!posix_locks_conflict(fl, cfl))
+			continue;
+		if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_conflict &&
+			!cfl->fl_lmops->lm_lock_conflict(cfl))
+			continue;
+		locks_copy_conflock(fl, cfl);
+		goto out;
 	}
 	fl->fl_type = F_UNLCK;
 out:
@@ -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;
 			if (conflock)
 				locks_copy_conflock(conflock, fl);
 			error = -EAGAIN;
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);
 };
 
 struct lock_manager {