Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Jeff Layton" <jlayton@kernel.org>
Cc:     "Chuck Lever" <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 5/8] NFSD: reduce locking in nfsd_lookup()
In-reply-to: <9e62ac672697225ac1859cac2c0cd58665d7b4fb.camel@kernel.org>
References: <165708033167.1940.3364591321728458949.stgit@noble.brown>,
 <165708109258.1940.1095066282860556838.stgit@noble.brown>,
 <9e62ac672697225ac1859cac2c0cd58665d7b4fb.camel@kernel.org>
Date:   Thu, 07 Jul 2022 11:26:38 +1000
Message-id: <165715719815.17141.11997557184323519099@noble.neil.brown.name>
Precedence: bulk

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@kernel.org>
> 

Thanks,
NeilBrown