linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Christian Brauner <brauner@kernel.org>, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] dcache: rename d_genocide()
Date: Sat, 10 Feb 2024 23:27:18 +0000	[thread overview]
Message-ID: <20240210232718.GG608142@ZenIV> (raw)
In-Reply-To: <20240210100643.2207350-1-amir73il@gmail.com>

On Sat, Feb 10, 2024 at 12:06:43PM +0200, Amir Goldstein wrote:

> I am not usually for PC culture and I know that you are on team
> "freedom of speech" ;-), but IMO this one stood out for its high ratio
> of bad taste vs. usefulness.
> 
> The patch is based on your revert of "get rid of DCACHE_GENOCIDE".
> I was hoping that you could queue my patch along with the revert.
> 
> BTW, why was d_genocide() only dropping refcounts on the s_root tree
> and not on the s_roots trees like shrink_dcache_for_umount()?
> Is it because dentries on s_roots are not supposed to be hashed?

Because secondary roots make no sense for "everything's in dcache"
kind of filesystems, mostly.

FWIW, I don't believe that cosmetic renaming is the right thing to
do here.  The real issue here is that those fs-pinned dentries are
hard to distinguish.  The rule is "if dentry on such filesystem is
positive and hashed, that contributes 1 to its refcount".

Unfortunately, that doesn't come with sane locking rules.
If nothing else, I would rather have an explicit flag set along
with bumping ->d_count on creation side and cleared along with
dropping refcount on removal, both under ->d_lock.

Another piece of ugliness is the remaining places that try to
open-code simple_recursive_removal(); they get it wrong more
often than not.  Connected to the previous, since that's where
those games with simple_unlink()/simple_rmdir() and associated
fun with refcounts tend to happen.

I'm trying to untangle that mess - on top of that revert,
obviously.  Interposing your patch in there is doable, of course,
but it's not particularly useful, IMO, especially since the
whole d_genocide() thing is quite likely to disappear, turning
kill_litter_super() into an alias for kill_anon_super().

  reply	other threads:[~2024-02-10 23:27 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-10 10:06 [PATCH] dcache: rename d_genocide() Amir Goldstein
2024-02-10 23:27 ` Al Viro [this message]
2024-02-11  8:00   ` Amir Goldstein
2024-02-11 18:44     ` Al Viro
2024-02-12  7:02       ` Amir Goldstein
2024-02-12  8:09         ` Al Viro
2024-02-13  4:42           ` Al Viro
2024-02-13  6:42             ` Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240210232718.GG608142@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=amir73il@gmail.com \
    --cc=brauner@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).