| 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 |
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 > ====================== ============= ================= =========
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.
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 > > ====================== ============= ================= ========= >
> 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
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.
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 >>> ====================== ============= ================= =========
> 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
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.
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 --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 {
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(-)