| Message ID | 20240315-dir-deleg-v1-2-a1d6209a3654@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | vfs, nfsd, nfs: implement directory delegations | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Mar 15, 2024 at 12:52:53PM -0400, Jeff Layton wrote: > The NFSv4.1 protocol adds support for directory delegations, but it > specifies that if you already have a delegation and try to request a new > one on the same filehandle, the server must reply that the delegation is > unavailable. > > Add a new lease_manager callback to allow the lease manager (nfsd in > this case) to impose extra checks when performing a setlease. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/locks.c | 5 +++++ > include/linux/filelock.h | 10 ++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/fs/locks.c b/fs/locks.c > index cb4b35d26162..415cca8e9565 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1822,6 +1822,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr > continue; > } > > + /* Allow the lease manager to veto the setlease */ > + if (lease->fl_lmops->lm_set_conflict && > + lease->fl_lmops->lm_set_conflict(lease, fl)) > + goto out; > + > /* > * No exclusive leases if someone else has a lease on > * this file: > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > index daee999d05f3..c5fc768087df 100644 > --- a/include/linux/filelock.h > +++ b/include/linux/filelock.h > @@ -49,6 +49,16 @@ struct lease_manager_operations { > int (*lm_change)(struct file_lease *, int, struct list_head *); > void (*lm_setup)(struct file_lease *, void **); > bool (*lm_breaker_owns_lease)(struct file_lease *); > + > + /** > + * lm_set_conflict - extra conditions for setlease > + * @new: new file_lease being set > + * @old: old (extant) file_lease > + * > + * This allows the lease manager to add extra conditions when > + * setting a lease. To make it clear which return value causes add_lease() to abort, I'd rather see API contract-style descriptions of the meaning of the return values instead of this design note. Something like: * Return values: * %true: @new and @old conflict * %false: No conflict detected > + */ > + bool (*lm_set_conflict)(struct file_lease *new, struct file_lease *old); > }; > > struct lock_manager { > > -- > 2.44.0 >
On Sun, 2024-03-17 at 10:56 -0400, Chuck Lever wrote: > On Fri, Mar 15, 2024 at 12:52:53PM -0400, Jeff Layton wrote: > > The NFSv4.1 protocol adds support for directory delegations, but it > > specifies that if you already have a delegation and try to request a new > > one on the same filehandle, the server must reply that the delegation is > > unavailable. > > > > Add a new lease_manager callback to allow the lease manager (nfsd in > > this case) to impose extra checks when performing a setlease. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/locks.c | 5 +++++ > > include/linux/filelock.h | 10 ++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index cb4b35d26162..415cca8e9565 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -1822,6 +1822,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr > > continue; > > } > > > > + /* Allow the lease manager to veto the setlease */ > > + if (lease->fl_lmops->lm_set_conflict && > > + lease->fl_lmops->lm_set_conflict(lease, fl)) > > + goto out; > > + > > /* > > * No exclusive leases if someone else has a lease on > > * this file: > > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > > index daee999d05f3..c5fc768087df 100644 > > --- a/include/linux/filelock.h > > +++ b/include/linux/filelock.h > > @@ -49,6 +49,16 @@ struct lease_manager_operations { > > int (*lm_change)(struct file_lease *, int, struct list_head *); > > void (*lm_setup)(struct file_lease *, void **); > > bool (*lm_breaker_owns_lease)(struct file_lease *); > > + > > + /** > > + * lm_set_conflict - extra conditions for setlease > > + * @new: new file_lease being set > > + * @old: old (extant) file_lease > > + * > > + * This allows the lease manager to add extra conditions when > > + * setting a lease. > > To make it clear which return value causes add_lease() to abort, I'd > rather see API contract-style descriptions of the meaning of the > return values instead of this design note. Something like: > > * Return values: > * %true: @new and @old conflict > * %false: No conflict detected > > Thanks. I added this to the patch in my tree. > > + */ > > + bool (*lm_set_conflict)(struct file_lease *new, struct file_lease *old); > > }; > > > > struct lock_manager { > > > > -- > > 2.44.0 > > >
On Fri, Mar 15, 2024 at 12:52:53PM -0400, Jeff Layton wrote: > The NFSv4.1 protocol adds support for directory delegations, but it > specifies that if you already have a delegation and try to request a new > one on the same filehandle, the server must reply that the delegation is > unavailable. > > Add a new lease_manager callback to allow the lease manager (nfsd in > this case) to impose extra checks when performing a setlease. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/locks.c | 5 +++++ > include/linux/filelock.h | 10 ++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/fs/locks.c b/fs/locks.c > index cb4b35d26162..415cca8e9565 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1822,6 +1822,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr > continue; > } > > + /* Allow the lease manager to veto the setlease */ > + if (lease->fl_lmops->lm_set_conflict && > + lease->fl_lmops->lm_set_conflict(lease, fl)) > + goto out; > + > /* > * No exclusive leases if someone else has a lease on > * this file: > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > index daee999d05f3..c5fc768087df 100644 > --- a/include/linux/filelock.h > +++ b/include/linux/filelock.h > @@ -49,6 +49,16 @@ struct lease_manager_operations { > int (*lm_change)(struct file_lease *, int, struct list_head *); > void (*lm_setup)(struct file_lease *, void **); > bool (*lm_breaker_owns_lease)(struct file_lease *); > + > + /** > + * lm_set_conflict - extra conditions for setlease > + * @new: new file_lease being set > + * @old: old (extant) file_lease > + * > + * This allows the lease manager to add extra conditions when > + * setting a lease. > + */ > + bool (*lm_set_conflict)(struct file_lease *new, struct file_lease *old); Minor, but it seems a bit misnamed to me. I'd recommend calling this lm_may_set_lease() or lm_may_lease().
On Wed, 2024-03-20 at 14:13 +0100, Christian Brauner wrote: > On Fri, Mar 15, 2024 at 12:52:53PM -0400, Jeff Layton wrote: > > The NFSv4.1 protocol adds support for directory delegations, but it > > specifies that if you already have a delegation and try to request a new > > one on the same filehandle, the server must reply that the delegation is > > unavailable. > > > > Add a new lease_manager callback to allow the lease manager (nfsd in > > this case) to impose extra checks when performing a setlease. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/locks.c | 5 +++++ > > include/linux/filelock.h | 10 ++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index cb4b35d26162..415cca8e9565 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -1822,6 +1822,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr > > continue; > > } > > > > + /* Allow the lease manager to veto the setlease */ > > + if (lease->fl_lmops->lm_set_conflict && > > + lease->fl_lmops->lm_set_conflict(lease, fl)) > > + goto out; > > + > > /* > > * No exclusive leases if someone else has a lease on > > * this file: > > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > > index daee999d05f3..c5fc768087df 100644 > > --- a/include/linux/filelock.h > > +++ b/include/linux/filelock.h > > @@ -49,6 +49,16 @@ struct lease_manager_operations { > > int (*lm_change)(struct file_lease *, int, struct list_head *); > > void (*lm_setup)(struct file_lease *, void **); > > bool (*lm_breaker_owns_lease)(struct file_lease *); > > + > > + /** > > + * lm_set_conflict - extra conditions for setlease > > + * @new: new file_lease being set > > + * @old: old (extant) file_lease > > + * > > + * This allows the lease manager to add extra conditions when > > + * setting a lease. > > + */ > > + bool (*lm_set_conflict)(struct file_lease *new, struct file_lease *old); > > Minor, but it seems a bit misnamed to me. I'd recommend calling this > lm_may_set_lease() or lm_may_lease(). > Yeah, I wasn't thrilled with the name either. Your suggestions sound reasonable to me. I'll change it to something along those lines. Thanks,
diff --git a/fs/locks.c b/fs/locks.c index cb4b35d26162..415cca8e9565 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1822,6 +1822,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr continue; } + /* Allow the lease manager to veto the setlease */ + if (lease->fl_lmops->lm_set_conflict && + lease->fl_lmops->lm_set_conflict(lease, fl)) + goto out; + /* * No exclusive leases if someone else has a lease on * this file: diff --git a/include/linux/filelock.h b/include/linux/filelock.h index daee999d05f3..c5fc768087df 100644 --- a/include/linux/filelock.h +++ b/include/linux/filelock.h @@ -49,6 +49,16 @@ struct lease_manager_operations { int (*lm_change)(struct file_lease *, int, struct list_head *); void (*lm_setup)(struct file_lease *, void **); bool (*lm_breaker_owns_lease)(struct file_lease *); + + /** + * lm_set_conflict - extra conditions for setlease + * @new: new file_lease being set + * @old: old (extant) file_lease + * + * This allows the lease manager to add extra conditions when + * setting a lease. + */ + bool (*lm_set_conflict)(struct file_lease *new, struct file_lease *old); }; struct lock_manager {
The NFSv4.1 protocol adds support for directory delegations, but it specifies that if you already have a delegation and try to request a new one on the same filehandle, the server must reply that the delegation is unavailable. Add a new lease_manager callback to allow the lease manager (nfsd in this case) to impose extra checks when performing a setlease. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/locks.c | 5 +++++ include/linux/filelock.h | 10 ++++++++++ 2 files changed, 15 insertions(+)