Re: [PATCH v2 00/11] vfs: recall-only directory delegations for knfsd

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2025-10-18 at 10:44 +1100, NeilBrown wrote:
> On Fri, 17 Oct 2025, Jeff Layton wrote:
> > A smaller variation of the v1 patchset that I posted earlier this week.
> > Neil's review inspired me to get rid of the lm_may_setlease operation
> > and to do the conflict resolution internally inside of nfsd. That means
> > a smaller VFS-layer change, and an overall reduction in code.
> > 
> > This patchset adds support for directory delegations to nfsd. This
> > version only supports recallable delegations. There is no CB_NOTIFY
> > support yet. I have patches for those, but we've decided to add that
> > support in a later kernel once we get some experience with this part.
> > Anna is working on the client-side pieces.
> > 
> > It would be great if we could get into linux-next soon so that it can be
> > merged for v6.19. Christian, could you pick up the vfs/filelock patches,
> > and Chuck pick up the nfsd patches?
> > 
> > Thanks!
> > Jeff
> > 
> > [1]: https://lore.kernel.org/all/20240315-dir-deleg-v1-0-a1d6209a3654@kernel.org/
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > Changes in v2:
> > - handle lease conflict resolution inside of nfsd
> > - drop the lm_may_setlease lock_manager operation
> > - just add extra argument to vfs_create() instead of creating wrapper
> > - don't allocate fsnotify_mark for open directories
> > - Link to v1: https://lore.kernel.org/r/20251013-dir-deleg-ro-v1-0-406780a70e5e@kernel.org
> > 
> > ---
> > Jeff Layton (11):
> >       filelock: push the S_ISREG check down to ->setlease handlers
> >       vfs: add try_break_deleg calls for parents to vfs_{link,rename,unlink}
> >       vfs: allow mkdir to wait for delegation break on parent
> >       vfs: allow rmdir to wait for delegation break on parent
> >       vfs: break parent dir delegations in open(..., O_CREAT) codepath
> >       vfs: make vfs_create break delegations on parent directory
> >       vfs: make vfs_mknod break delegations on parent directory
> >       filelock: lift the ban on directory leases in generic_setlease
> >       nfsd: allow filecache to hold S_IFDIR files
> >       nfsd: allow DELEGRETURN on directories
> >       nfsd: wire up GET_DIR_DELEGATION handling
> 
> vfs_symlink() is missing from the updated APIs.  Surely that needs to be
> able to wait for a delegation to break.
> 

Ouch! That's a major oversight. I'll fix that up.

> vfs_mkobj() maybe does too, but I could easily turn a blind eye to that.
> 
> I haven't looked properly at the last patch but all the other could have
>  Reviewed-by: NeilBrown <neil@brown.name>
> 
> once the vfs_symlink() omission is fixed.
> 
> NeilBrown


Chuck found a couple of potential leaks in there so those will also
need to be fixed. As I was writing some xfstests for the VFS pieces, I
found another problem too:

Currently the F_SETLEASE API sets FL_LEASE leases, but the new
delegation breaks that this set adds don't break FL_LEASE leases, since
these are FL_DELEG leases.

This distinction is mostly due to historical reasons. Leases were added
first (for Samba oplocks), but didn't break on metadata changes. When
Bruce added delegations, he wanted to ensure that the lease API didn't
suddenly change behavior.

I see several potential options to fix this:

1/ The simplest is to just make the F_SETLEASE command set FL_DELEG
leases when the inode is a directory. That makes for a messy userland
interface where files get FL_LEASE objects, but directories get
FL_DELEG. I think that will be less useful for userland.

2/ Don't expose this to userland at all (yet?), and just keep returning
EINVAL on attempts to set a lease on a directory. The downside there is
that this would require us to use nfsd for testing this functionality.
Less people will do that than would if it were an xfstest that ran on
most local filesystems. I do have some pynfs tests though which could
help cover the gap.

3/ Add new F_SETDELEG/F_GETDELEG fcntl() commands. The nice thing about
this is that it would also allow us to add a flags field to these
commands. The later patches that add CB_NOTIFY support add the ability
to ignore certain types of delegation break events. This option would
allow us to expose that functionality to userland too. NFS Ganesha and
Samba, for example, could make use of this.

#3 wouldn't be too difficult (aside from having to update the
manpages), so I kind of like that idea.

Thoughts?
-- 
Jeff Layton <jlayton@kernel.org>