On 3/9/22 5:57 PM, Luís Henriques wrote:
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?
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)
?
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,
