Re: [v4 3/3] bcachefs: Implement casefolding

[Gabriel Krisman Bertazi <krisman@suse.de>]

Kent Overstreet <kent.overstreet@linux.dev> writes:

> On Mon, Aug 14, 2023 at 10:18:33PM +0100, Joshua Ashton wrote:
>
>> Grepping the test right now I already know we will fail the
>> `test_casefold_flag_removal` test as we don't support toggling the flag on
>> non-empty directories as it would require rehashing and thus restructuring
>> the btree.
>
> What was the motivation for supporting that on other filesystems? I'm
> not inclined to add that to bcachefs without a good reason.

I don't think there was a better reason than "it is user-friendly to
allow disabling the feature without having to remove the
directory". Note that it is trivial to do so in empty directories for
ext4/f2fs: it is just a matter of flipping a bit in the inode to
enable/disable casefolding.

I think it's ok for the Windows games to not allow disabling
case-insensitive once it is set for a directory.  For fstests, I suppose
the easiest solution would be to skip that subtest when running on
bcachefs.

>> > Sure, if you have both, dirent and dentry->d_name casefolded, then it is
>> > just a regular memcmp, but at least you don't have to change the disk
>> > format and your implementation becomes simpler.
>> > 
>> 
>> Given we have the opportunity to have support for this in the dirent format
>> for bcachefs, I think it makes sense to take it right now.
>
> It also means that we have the option of defining a casefolding map that
> handles ß correctly.

What do you mean by handling ß correctly?  do you intend to support NFKD
normalization?  We can extend fs/unicode/ for that fairly easily,
but iirc, other filesystems decided to stick with NFD, because NFKD is
a bit too eager when folding some characters.

>> > > +#if IS_ENABLED(CONFIG_UNICODE)
>> > > +	struct unicode_map 	*cf_encoding;
>> > > +#endif
>> > 
>> > This goes in struct super_block.
>> 
>> The code in libfs.c that uses the s_encoding in the super_block is never
>> used for bcachefs as it defines its own inode_operations and doesn't use any
>> of the libfs common code for lookups/hashes there (ie. the
>> dentry_operations).
>> 
>> One thing we wanted to do down the line was to use the bcachefs options
>> system to be able to pick the encoding on a per-inode basis.
>> 
>> Currently I am storing it in the struct bch_fs as currently there is no need
>> for other encodings right now and it is convenient, so there can only be one
>> -- but that could change at some point with an opt attached to the inode.
>> 
>> I am happy to bung it in the vfs super_block for now though -- but I need to
>> look at how that all works out with mounting and the multiple drives
>> situation. Just be aware that at some point, it will likely be moved back
>> again to our code.
>
> We also don't always have a super_block available when we have a running
> bch_fs object (e.g. when running in userspace, for fsck and various
> other tools). Since s_encoding is used in dirent.c, it probably needs to
> be in bch_fs.

Ah, I didn't know this is reused for userspace tools.  Still, as long as
you initialize sb->s_encoding at mount time, libfs helpers will work fine.

-- 
Gabriel Krisman Bertazi