Message-ID: <pkpzm6okfqchet42lhcebwaiuwrwil6wp76flnk3p3mgijtg2e@us7jkctbpsgc>
Date: Tue, 29 Apr 2025 12:21:34 -0400
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Patrick Donnelly <batrick@...bytes.com>
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 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).

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

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.

Powered by blists - more mailing lists