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:     hooanon05g@gmail.com
Cc:     trond.myklebust@hammerspace.com, linux-nfs@vger.kernel.org
Subject: Re: NFS, two d_delete() calls in nfs_unlink()
In-reply-to: <9451.1660793491@jrobl>
References: <7634.1660728564@jrobl>,
 <166079133167.5425.16635199337074058478@noble.neil.brown.name>,
 <9451.1660793491@jrobl>
Date:   Fri, 19 Aug 2022 09:05:48 +1000
Message-id: <166086394829.5425.9699631920964605772@noble.neil.brown.name>
Precedence: bulk

On Thu, 18 Aug 2022, hooanon05g@gmail.com wrote:
> "NeilBrown":
> > Thanks for the report.
> > This possibility of calling d_delete() twice has been present
> > since  9019fb391de0 in v5.16.
> 
> I don't think 9019fb391de0 is a problem.
> Before v6.0-rc1, the target dentry was unhashed by __d_drop() call in
> nfs_unlink(), and nfs_dentry_handle_enoent() skipped calling d_delete()
> by simple_positive(). d_delete() was called only once via
> nfs_dentry_remove_handle_error().

Ahhh - simple_positive() checks d_unhashed() - I didn't connect that.

So before my recent patch we needed the second call to d_delete() in
nfs_unlink() because the first wasn't effective.  However the second
call in nfs_rmdir() was still a problem.

So if the current fix (9a31abb1c009) gets ported back before the patch
that removed unhash (3c59366c207e) then it won't do the right thing
for nfs_unlink().  As it has a Fixes: tag, that is likely.

It would be better to protect the d_delete() with
d_really_is_positive().

Trond: can we drop that patch and replace it, or should I add the
d_really_is_positive() check with a new patch?


> 
> In v6.0-rc1, the dentry is not unhashed and nfs_dentry_handle_enoent()
> doesn't skip calling d_delete().
> 
> 
> > How did you discover this bug, and why do you think my patch
> > caused it?
> 
> I met this problem during a stress test aiming a filesystem I am
> developing.
> And I think unhashing causes nfs_dentry_handle_enoent() to call
> d_delete().

Thanks a lot for helping me understand!

Thanks,
NeilBrown