Message-ID: <CACh33Fr4-53wOgciKSUTwsCwyb-L7A9fscQmLvbHynYX2j2brg@mail.gmail.com>
Date: Tue, 29 Apr 2025 12:48:49 -0400
From: Patrick Donnelly <batrick@...bytes.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, linux-bcachefs@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] bcachefs fixes for 6.15-rc4

On Tue, Apr 29, 2025 at 12:21 PM Kent Overstreet
<kent.overstreet@...ux.dev> wrote:
>
> On Tue, Apr 29, 2025 at 11:36:44AM -0400, Patrick Donnelly wrote:
> > I would not consider myself a kernel developer but I assume this
> > terminology (dentry aliases) refers to multiple dentries in the dcache
> > referring to the same physical dentry on the backing file system?
>
> This issue turned out to be my own pebcak; I'd missed the
> dentry_operations that do normalized lookups (to be fair, they were
> rather hidden) and I'd just pulled a 14 hour day so was a tad bitchy
> about it on the list.
>
> > If so, I can't convince myself that's a real problem. Wouldn't this be
> > beneficial because each application/process may utilize a different
> > name for the backing file system dentry? This keeps the cache hot with
> > relevant names without any need to do transformations on the dentry
> > names. Happy to learn otherwise because I expected this situation to
> > occur in practice with ceph-fuse. I just tested and the dcache entries
> > (/proc/sys/fs/dentry-state) increases as expected when performing case
> > permutations on a case-insensitive file name. I didn't observe any
> > cache inconsistencies when editing/removing these dentries. The danger
> > perhaps is cache pollution and some kind of DoS? That should be a
> > solvable problem but perhaps I misunderstand some complexity.
>
> Dentry aliases are fine when they're intended, they're properly
> supported by the dcache.
>
> The issue with caching an alias for the un-normalized lookup name is
> (as you note) that by permuting the different combinations of upper and
> lower case characters in a filename, userspace would be able to create
> an unbounded (technically, exponential bound in the length of the
> filename) number of aliases, and that's not what we want.
>
> (e.g. d_prune_aliases processes the whole list of aliases for an inode
> under a spinlock, so it's an easy way to produce unbounded latencies).

Well, if aliases are "supported" by the dcache, that seems like a bug to me.

> So it sounds like you probably didn't find the dentry_operations for
> normalized lookups, same as me.

I didn't look to begin with. :)

> Have a look at generic_set_sb_d_ops().
>
> > > Also, originally this was all in the same core dcache lookup path. So
> > > the whole "we have to check if the filesystem has its own hash
> > > function" ended up slowing down the normal case. It's obviously been
> > > massively modified since 1997 ("No, really?"), and now the code is
> > > very much set up so that the straight-line normal case is all the
> > > non-CI cases, and then case idnependence ends up out-of-line with its
> > > own dcache hash lookup loops so that it doesn't affect the normal good
> > > case.
> >
> > It's seems to me this is a good argument for keeping case-sensitivity
> > awareness out of the dcache. Let the fs do the namespace mapping and
> > accept that you may have dentry aliases.
> >
> > FWIW, I also wish we didn't have to deal with case-sensitivity but we
> > have users/protocols to support (as usual).
>
> The next issue I'm looking at is that the "keep d_ops out of the
> fastpath" only works if case sensitivity isn't enabled on the filesystem
> as a whole - i.e. if case sensitivity is enabled on even a single
> directory, we'll be flagging all the dentries to hit the d_ops methods.
>
> dcache.c doesn't check IS_CASEFOLD(inode) - d_init can probably be used
> for this, but we need to be careful when flipping casefolding on and off
> on an (empty) directory, there might still be cached negative dentries.
>
> ext4 has a filesystem wide flag for "case sensitive directories are
> allowed", so that enables/disables that optimization. But bcachefs
> doesn't have that kind of filesystem level flag, and I'm not going to
> add one - that sort of "you have to enable this option to have access to
> this other option" is a pita for end users.

In CephFS it's as simple as setting an extended attribute on a
directory, by admins.

-- 
Patrick Donnelly

Powered by blists - more mailing lists