Re: [PATCH v5] ceph: do not dencrypt the dentry name twice for readdir

["Luís Henriques" <lhenriques@suse.de>]

Xiubo Li <xiubli@redhat.com> writes:

> On 3/9/22 1:47 AM, Luís Henriques wrote:
>> xiubli@redhat.com writes:
>>
>>> From: Xiubo Li <xiubli@redhat.com>
>>>
>>> For the readdir request the dentries will be pasred and dencrypted
>>> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
>>> get the dentry name from the dentry cache instead of parsing and
>>> dencrypting them again. This could improve performance.
>>>
>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>> ---
>>>
>>> V5:
>>> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
>>> - release the rde->dentry in destroy_reply_info
>>>
>>>
>>>   fs/ceph/crypto.h     |  8 ++++++
>>>   fs/ceph/dir.c        | 59 +++++++++++++++++++++-----------------------
>>>   fs/ceph/inode.c      |  7 ++++++
>>>   fs/ceph/mds_client.c |  2 ++
>>>   fs/ceph/mds_client.h |  1 +
>>>   5 files changed, 46 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
>>> index 1e08f8a64ad6..c85cb8c8bd79 100644
>>> --- a/fs/ceph/crypto.h
>>> +++ b/fs/ceph/crypto.h
>>> @@ -83,6 +83,14 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>>>    */
>>>   #define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
>>>   +/*
>>> + * The encrypted long snap name will be in format of
>>> + * "_${ENCRYPTED-LONG-SNAP-NAME}_${INODE-NUM}". And will set the max longth
>>> + * to sizeof('_') + NAME_MAX + sizeof('_') + max of sizeof(${INO}) + extra 7
>>> + * bytes to align the total size to 8 bytes.
>>> + */
>>> +#define CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX (1 + 255 + 1 + 16 + 7)
>>> +
>> I think this constant needs to be defined in a different way and we need
>> to keep the snapshots names length a bit shorter than NAME_MAX.  And I'm
>> not talking just about the encrypted snapshots.
>>
>> Right now, ceph PR#45192 fixes an MDS limitation that is keeping long
>> snapshot names smaller than 80 characters.  With this limitation we would
>> need to keep the snapshot names < 64:
>>
>>     '_' + <name> + '_' + '<inode#>' '\0'
>>      1  +   64   +  1  +    12     +  1 = 80
>>
>> Note however that currently clients *do* allow to create snapshots with
>> bigger names.  And if we do that we'll get an error when doing an LSSNAP
>> on a .snap subdirectory that will contain the corresponding long name:
>>
>>    # mkdir a/.snap/123456qwertasdfgzxcvb7890yuiophjklnm123456qwertasdfgzxcvb78912345
>>    # ls -li a/b/.snap
>>    ls: a/b/.snap/_123456qwertasdfgzxcvb7890yuiophjklnm123456qwertasdfgzxcvb78912345_109951162777: No such file or directory
>>
>> We can limit the snapshot names on creation, but this should probably be
>> handled on the MDS side (so that old clients won't break anything).  Does
>> this make sense?  I can work on an MDS patch for this but... to which
>> length should names be limited? NAME_MAX - (2*'_' + <inode len>)?  Or
>> should we take base64-encoded names already into account?
>>
>> (Sorry, I'm jumping around between PRs and patches, and trying to make any
>> sense out of the snapshots code :-/ )
>
> For fscrypt case I think it's okay, because the max len of the encrypted name
> will be 189 bytes, so even plusing the extra 2 * sizeof('_') - sizeof(<inode#>) 
> == 15 bytes with ceph PR#41592 it should work well.

Is it really 189 bytes, or 252, which is the result of base64 encoding 189
bytes?  Reading the documentation in the CEPH_NOHASH_NAME_MAX definition
it seems to be 252.  And in that case we need to limit the names length
even further.

>
> But for none fscrypt case, we must limit the max len to NAME_MAX - 2 *
> sizeof('_') - sizeof(<inode#>) == 255 - 2 - 13 == 240. So fixing this in 
> MDS side makes sense IMO.

Yeah, I suppose this makes sense.  I can send out a PR soon with this, and
try to document it somewhere.  But it may make sense to merge both PRs at
the same time and *backport* them to older releases.

Cheers,
-- 
Luís