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:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Jeff Layton" <jlayton@kernel.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 5/8] NFSD: reduce locking in nfsd_lookup()
In-reply-to: <0A6DEF8A-232B-45FE-8A45-C403DE4E4342@oracle.com>
References: <165708033167.1940.3364591321728458949.stgit@noble.brown>,
 <165708109258.1940.1095066282860556838.stgit@noble.brown>,
 <0A6DEF8A-232B-45FE-8A45-C403DE4E4342@oracle.com>
Date:   Thu, 07 Jul 2022 11:29:09 +1000
Message-id: <165715734903.17141.8779762171235676843@noble.neil.brown.name>
Precedence: bulk

On Thu, 07 Jul 2022, Chuck Lever III wrote:
> > +		/* We want to keep the directory locked until we've had a chance
> > +		 * to acquire a delegation if appropriate, so request that
> > +		 * nfsd_lookup() hold on to the lock.
> > 		 */
> > 		status = nfsd_lookup(rqstp, current_fh,
> > -				     open->op_fname, open->op_fnamelen, *resfh);
> > +				     open->op_fname, open->op_fnamelen, *resfh,
> > +				     true);
> > +		if (!status) {
> > +			/* NFSv4 protocol requires change attributes even though
> > +			 * no change happened.
> > +			 */
> > +			fh_fill_pre_attrs(current_fh);
> > +			fh_fill_post_attrs(current_fh);
> 
> If this is really correct, the comment should also state that
> no concurrent changes to the parent are possible during
> the lookup, and thus the pre and post attributes are expected
> to be the same always.

The earlier comment notes that nfsd_lookup() is called in a way in which
it takes and keeps the lock - that should imply that no other changes
can happen.  But with such insane looking code, it doesn't hurt to be
extra explicit.

> 
> Otherwise, this code paragraph looks just a little insane ;-)
> 
:-)

Thanks,
NeilBrown