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 5:57 PM, Luís Henriques wrote:
>> 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?
>
> Yeah, you are right, I misread that. The 252 is from 189 * 4 / 3, which will be
> the base64 encoded name.
>
> So, I think you should fix this to make the totally of base64 encode name won't
> bigger than 240 = 255 - 15. So the CEPH_NOHASH_NAME_MAX should be:
>
> #define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>
> ?

That could be done, but maybe it's not worth it if the MDS returns -EINVAL
when creating a snapshot with a name that breaks this rule.  Thus, we can
still have regular file with 189 bytes names and only the snapshots will
have this extra limitation.  (I've already created PR#45312 for this.)

But I'm OK either way.

Cheers,
-- 
Luís

>
>>   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.
>
> Yeah, make sense.
>
> - Xiubo
>
>> Cheers,
>