fs/dcache.c: bogus comment re LIST_POISON2 around __d_drop()

fs/dcache.c: bogus comment re LIST_POISON2 around __d_drop()

[Al Viro]

[Caught while documenting ->d_hash uses]

The second line in
 * ___d_drop doesn't mark dentry as "unhashed"
 * (dentry->d_hash.pprev will be LIST_POISON2, not NULL).
is incorrect; what ___d_drop() does is
	__hlist_bl_del()
which never included poisoning - that's only done by hlist_bl_del()
and we hadn't been using that since 2011, six years prior to
___d_drop() introduction.

	I've missed that on review back then; the first line _is_
true, so functionality is all right; it's just that LIST_POISON2
reference there is confusing (and incorrect).

	For now I'm dropping the following into a local branch;
might or might not get combined with other changes in the area...

diff --git a/fs/dcache.c b/fs/dcache.c
index 0c8faeee02e2..c9995545d8d7 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -604,7 +604,7 @@ EXPORT_SYMBOL(__d_drop);
  * __d_drop requires dentry->d_lock
  *
  * ___d_drop doesn't mark dentry as "unhashed"
- * (dentry->d_hash.pprev will be LIST_POISON2, not NULL).
+ * (dentry->d_hash.pprev will not be set to NULL).
  */
 void d_drop(struct dentry *dentry)
 {

Re: fs/dcache.c: bogus comment re LIST_POISON2 around __d_drop()

[NeilBrown]

On Mon, 06 Apr 2026, Al Viro wrote:
> [Caught while documenting ->d_hash uses]
> 
> The second line in
>  * ___d_drop doesn't mark dentry as "unhashed"
>  * (dentry->d_hash.pprev will be LIST_POISON2, not NULL).
> is incorrect; what ___d_drop() does is
> 	__hlist_bl_del()
> which never included poisoning - that's only done by hlist_bl_del()
> and we hadn't been using that since 2011, six years prior to
> ___d_drop() introduction.
> 
> 	I've missed that on review back then; the first line _is_
> true, so functionality is all right; it's just that LIST_POISON2
> reference there is confusing (and incorrect).
> 
> 	For now I'm dropping the following into a local branch;
> might or might not get combined with other changes in the area...
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 0c8faeee02e2..c9995545d8d7 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -604,7 +604,7 @@ EXPORT_SYMBOL(__d_drop);
>   * __d_drop requires dentry->d_lock
>   *
>   * ___d_drop doesn't mark dentry as "unhashed"
> - * (dentry->d_hash.pprev will be LIST_POISON2, not NULL).
> + * (dentry->d_hash.pprev will not be set to NULL).

It might be help to add *why* it doesn't.  Presumably this is an
optimisation for when the dentry will soon be added into a different
list.  There is no point setting pprev when it will soon be set to
something else while the lock is still held.
Do we know that the compiler won't work out this optimisation itself?

Maybe

    ___d_drop doesn't even mark the dentry as "unhashed"
    (dentry->d_hash.prev is unchanged).  This is optimal when
    the dentry will shortly be added to another list as in __d_move().

??

Thanks,
NeilBrown


>   */
>  void d_drop(struct dentry *dentry)
>  {
> 
> 

Re: fs/dcache.c: bogus comment re LIST_POISON2 around __d_drop()

[Al Viro]

On Mon, Apr 06, 2026 at 12:09:20PM +1000, NeilBrown wrote:

> It might be help to add *why* it doesn't.  Presumably this is an
> optimisation for when the dentry will soon be added into a different
> list.  There is no point setting pprev when it will soon be set to
> something else while the lock is still held.
> Do we know that the compiler won't work out this optimisation itself?

Er...  Neil, it was your patch in the first place ;-)  I realize that this
was over 8 years ago; for context see 61647823aa92.  The short version of
the story: we want to avoid a window during d_move() when d_unhashed()
(actually, d_unlinked()) called without ->d_lock would give false
positives.

Re: fs/dcache.c: bogus comment re LIST_POISON2 around __d_drop()

[NeilBrown]

On Mon, 06 Apr 2026, Al Viro wrote:
> On Mon, Apr 06, 2026 at 12:09:20PM +1000, NeilBrown wrote:
> 
> > It might be help to add *why* it doesn't.  Presumably this is an
> > optimisation for when the dentry will soon be added into a different
> > list.  There is no point setting pprev when it will soon be set to
> > something else while the lock is still held.
> > Do we know that the compiler won't work out this optimisation itself?
> 
> Er...  Neil, it was your patch in the first place ;-)  I realize that this
> was over 8 years ago; for context see 61647823aa92.  The short version of
> the story: we want to avoid a window during d_move() when d_unhashed()
> (actually, d_unlinked()) called without ->d_lock would give false
> positives.
> 

Right ... I clearly could have documented that better.

diff --git a/fs/dcache.c b/fs/dcache.c
index 7ba1801d8132..76726325fb28 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -595,7 +595,8 @@ EXPORT_SYMBOL(__d_drop);
  * __d_drop requires dentry->d_lock
  *
  * ___d_drop doesn't mark dentry as "unhashed"
- * (dentry->d_hash.pprev will be LIST_POISON2, not NULL).
+ * (dentry->d_hash.pprev is unchanged) so a racing d_unlinked()
+ * won't see a dentry undergoing d_move() as being unhashed.
  */
 void d_drop(struct dentry *dentry)
 {


??

Thanks,
NeilBrown