Xiubo Li <xiubli@xxxxxxxxxx> writes:
> On 3/9/22 1:47 AM, Luís Henriques wrote:
>> xiubli@xxxxxxxxxx writes:
>>
>>> From: Xiubo Li <xiubli@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>>
>>> 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
