Re: [PATCH 03/11] VFS/nfsd/cachefiles/ovl: add start_creating() and end_creating()

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-09-30 at 15:37 +1000, NeilBrown wrote:
> On Mon, 29 Sep 2025, Jeff Layton wrote:
> > On Fri, 2025-09-26 at 12:49 +1000, NeilBrown wrote:
> > > From: NeilBrown <neil@brown.name>
> > > 
> > > start_creating() is similar to simple_start_creating() but is not so
> > > simple.
> > > It takes a qstr for the name, includes permission checking, and does NOT
> > > report an error if the name already exists, returning a positive dentry
> > > instead.
> 
> 
> 
> > >  
> > > -	if (!d_is_negative(dentry)) {
> > > +	while (!d_is_negative(dentry)) {
> > 
> > Can you explain why this changed from an if to a while? The existing
> > code doesn't seem to ever retry this operation.
> 
> I tried to explain that in the commit message:
> 
> > Occasionally this change means that the parent lock is held for a
> > shorter period of time, for example in cachefiles_commit_tmpfile().
> > As this function now unlocks after an unlink and before the following
> > lookup, it is possible that the lookup could again find a positive
> > dentry, so a while loop is introduced there.
> 
> Is there something I could do to make that clearer?
> 

Clearly I didn't read the commit message well enough. Nothing I can
think of.

> ....
> > > @@ -1828,12 +1822,6 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> > >  	}
> > >  out:
> > >  	return err != nfs_ok ? err : nfserrno(host_err);
> > > -
> > > -out_dput:
> > > -	dput(dnew);
> > > -out_unlock:
> > > -	inode_unlock(dirp);
> > > -	goto out_drop_write;
> > >  }
> > > 
> > > 
> > 
> > 
> > I do quite like the nfsd cleanup though!
> > 
> > 
> 
> Thanks!
> 
> NeilBrown

You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>