On Wed, 06 Jul 2022, Jeff Layton wrote:
> On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote:
> > nfsd_lookup() takes an exclusive lock on the parent inode, but many
> > callers don't want the lock and may not need to lock at all if the
> > result is in the dcache.
> >
> > Change nfsd_lookup() to be passed a bool flag.
> > If false, don't take the lock.
> > If true, do take an exclusive lock, and return with it held if
> > successful.
> > If nfsd_lookup() returns an error, the lock WILL NOT be held.
> >
> > Only nfsd4_open() requests the lock to be held, and does so to block
> > rename until it decides whether to return a delegation.
> >
> > NOTE: when nfsd4_open() creates a file, the directory does *NOT* remain
> > locked and never has. So it is possible (though unlikely) for the
> > newly created file to be renamed before a delegation is handed out,
> > and that would be bad. This patch does *NOT* fix that, but *DOES*
> > take the directory lock immediately after creating the file, which
> > reduces the size of the window and ensure that the lock is held
> > consistently. More work is needed to guarantee no rename happens
> > before the delegation.
> >
>
> Interesting. Maybe after taking the lock, we could re-vet the dentry vs.
> the info in the OPEN request? That way, we'd presumably know that the
> above race didn't occur.
I would lean towards revalidating the dentry after getting the lease.
However I don't think "revalidate the dentry" is quite as easy as I
would like it to be, particularly if you care about bind-mounts of
regular files.
> > /*
> > - * We don't need the i_mutex after all. It's
> > - * still possible we could open this (regular
> > - * files can be mountpoints too), but the
> > - * i_mutex is just there to prevent renames of
> > - * something that we might be about to delegate,
> > - * and a mountpoint won't be renamed:
> > + * nfsd_cross_mnt() may wait for an upcall
> > + * to userspace, and holding i_sem across that
>
> s/i_sem/i_rwsem/
But ... fs/nilfs2/nilfs.h calls it i_sem, as does
fs/jffs2/README.Locking
And
$ git grep -w i_mutex | wc
180 1878 13728
But yes, I should spell it i_rwsem... or maybe just "the inode lock".
>
> Other than minor comment nit...
>
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>
Thanks,
NeilBrown
