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

Joshua Ashton
2023-08-23T09:49:24Z

On 8/15/23 19:00, Gabriel Krisman Bertazi wrote:
> 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.
>

Makes sense. And yeah, we would never unset it in Steam/Proton.

>>>> 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.

Yes, I believe we wanted to add support for other normalizations as
bcachefs options.

Or at the very least, ensure we structure what we do around being able
to support that in the future.

>
>>>>> +#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.
>

Cool.

- Joshie 🐸✨