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

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

[Theodore Ts'o]

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?

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.

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.

Cheers,

					- Ted

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

[Gabriel Krisman Bertazi]

"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