Re: [PATCH] ext4: Optimize case-insensitive lookups

[Gabriel Krisman Bertazi <krisman@collabora.com>]

"Theodore Ts'o" <tytso@mit.edu> writes:

> On Wed, May 29, 2019 at 02:54:46PM -0400, Gabriel Krisman Bertazi wrote:
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index c18ab748d20d..e3809cfda9f4 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -2078,6 +2078,10 @@ struct ext4_filename {
>>  #ifdef CONFIG_FS_ENCRYPTION
>>  	struct fscrypt_str crypto_buf;
>>  #endif
>> +#ifdef CONFIG_UNICODE
>> +	int cf_len;
>> +	unsigned char cf_name[EXT4_NAME_LEN];
>> +#endif
>>  };
>>  
>>  #define fname_name(p) ((p)->disk_name.name)
>
> EXT4_NAME_LEN is 256, and struct ext4_filename is allocated on the
> stack.  So this is going to increase the stack usage by 258 bytes.
> Perhaps should we just kmalloc the temporary buffer when it's needed?

I wanted to avoid adding an allocation to this path, but maybe that was
misguided, since this is out of the dcache critical path.  I also wanted
to remove the allocation from d_hash, but we'd require a similar size
allocation in the stack. Is that a good idea?

> The other thing that this patch reminds me is that there is great
> interest in supporting case folded directories and fscrypt at the same
> time.  Today fscrypt works by encrypting the filename, and stashes it
> in fname->crypto_buf, and this allows for a byte-for-byte comparison
> of the encrypted name.  To support fscrypt && casefold, what we would
> need to do is to change the htree hash so that the hash is caluclated
> on the normalized form, and then we'll have to decrypt each filename
> in the directory block and then compare it against the normalized form
> that stashed in cf_name.  So that means we'll never need to allocate
> memory for cf_name and crypto_buf at the same time.

fscrypt and case-insensitive is getting to the top of my to-do list,
i'll something there early next week.  Thanks for the explanation on
it.

>
> We can also use struct fscrypt_str for cf_name; it's defined as a
> combined unsighed char *name and u32 len.  We already use fscrypt_str
> even the !CONFIG_FS_ENCRYPTION case, since it's a convenient way of
> handling a non-NULL terminated filename blob.  And this will hopefully
> make it simpler to deal with integrating casefolding and fscrypt in
> the future.

I will send a v2 with this change already, to simplify
fscrypt+casefolding support.
>
> Cheers,
>
> 					- Ted

-- 
Gabriel Krisman Bertazi