[PATCH 0/3] ceph: fix cephfs rsync kworker high load issue

[PATCH 0/3] ceph: fix cephfs rsync kworker high load issue

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

Xiubo Li (3):
  ceph: move to a dedicated slabcache for ceph_cap_snap
  ceph: move kzalloc under i_ceph_lock with GFP_ATOMIC flag
  ceph: do no update snapshot context when there is no new snapshot

 fs/ceph/snap.c               | 56 ++++++++++++++++++++++++------------
 fs/ceph/super.c              |  7 +++++
 fs/ceph/super.h              |  2 +-
 include/linux/ceph/libceph.h |  1 +
 4 files changed, 47 insertions(+), 19 deletions(-)

-- 
2.27.0

[PATCH 1/3] ceph: move to a dedicated slabcache for ceph_cap_snap

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

There could be huge number of capsnap queued in a short time, on
x86_64 it's 248 bytes, which will be rounded up to 256 bytes by
kzalloc. Move this to a dedicated slabcache to save 8 bytes for
each.

For the kmalloc-256 slab cache, the actual size will be 512 bytes:
kmalloc-256        21797  74656    512   32    4 : tunables, etc

For a dedicated slab cache the real size is 312 bytes:
ceph_cap_snap          0      0    312   52    4 : tunables, etc

So actually we can save 200 bytes for each.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/snap.c               | 5 +++--
 fs/ceph/super.c              | 7 +++++++
 fs/ceph/super.h              | 2 +-
 include/linux/ceph/libceph.h | 1 +
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index b41e6724c591..c787775eaf2a 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -482,7 +482,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
 	struct ceph_buffer *old_blob = NULL;
 	int used, dirty;
 
-	capsnap = kzalloc(sizeof(*capsnap), GFP_NOFS);
+	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
 	if (!capsnap) {
 		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
 		return;
@@ -603,7 +603,8 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
 	spin_unlock(&ci->i_ceph_lock);
 
 	ceph_buffer_put(old_blob);
-	kfree(capsnap);
+	if (capsnap)
+		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
 	ceph_put_snap_context(old_snapc);
 }
 
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index bf79f369aec6..978463fa822c 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -864,6 +864,7 @@ static void destroy_fs_client(struct ceph_fs_client *fsc)
  */
 struct kmem_cache *ceph_inode_cachep;
 struct kmem_cache *ceph_cap_cachep;
+struct kmem_cache *ceph_cap_snap_cachep;
 struct kmem_cache *ceph_cap_flush_cachep;
 struct kmem_cache *ceph_dentry_cachep;
 struct kmem_cache *ceph_file_cachep;
@@ -892,6 +893,9 @@ static int __init init_caches(void)
 	ceph_cap_cachep = KMEM_CACHE(ceph_cap, SLAB_MEM_SPREAD);
 	if (!ceph_cap_cachep)
 		goto bad_cap;
+	ceph_cap_snap_cachep = KMEM_CACHE(ceph_cap_snap, SLAB_MEM_SPREAD);
+	if (!ceph_cap_snap_cachep)
+		goto bad_cap_snap;
 	ceph_cap_flush_cachep = KMEM_CACHE(ceph_cap_flush,
 					   SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD);
 	if (!ceph_cap_flush_cachep)
@@ -931,6 +935,8 @@ static int __init init_caches(void)
 bad_dentry:
 	kmem_cache_destroy(ceph_cap_flush_cachep);
 bad_cap_flush:
+	kmem_cache_destroy(ceph_cap_snap_cachep);
+bad_cap_snap:
 	kmem_cache_destroy(ceph_cap_cachep);
 bad_cap:
 	kmem_cache_destroy(ceph_inode_cachep);
@@ -947,6 +953,7 @@ static void destroy_caches(void)
 
 	kmem_cache_destroy(ceph_inode_cachep);
 	kmem_cache_destroy(ceph_cap_cachep);
+	kmem_cache_destroy(ceph_cap_snap_cachep);
 	kmem_cache_destroy(ceph_cap_flush_cachep);
 	kmem_cache_destroy(ceph_dentry_cachep);
 	kmem_cache_destroy(ceph_file_cachep);
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index c0718d5a8fb8..2d08104c8955 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -231,7 +231,7 @@ static inline void ceph_put_cap_snap(struct ceph_cap_snap *capsnap)
 	if (refcount_dec_and_test(&capsnap->nref)) {
 		if (capsnap->xattr_blob)
 			ceph_buffer_put(capsnap->xattr_blob);
-		kfree(capsnap);
+		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
 	}
 }
 
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index edf62eaa6285..00af2c98da75 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -284,6 +284,7 @@ DEFINE_RB_LOOKUP_FUNC(name, type, keyfld, nodefld)
 
 extern struct kmem_cache *ceph_inode_cachep;
 extern struct kmem_cache *ceph_cap_cachep;
+extern struct kmem_cache *ceph_cap_snap_cachep;
 extern struct kmem_cache *ceph_cap_flush_cachep;
 extern struct kmem_cache *ceph_dentry_cachep;
 extern struct kmem_cache *ceph_file_cachep;
-- 
2.27.0

Re: [PATCH 1/3] ceph: move to a dedicated slabcache for ceph_cap_snap

[Jeff Layton]

On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> There could be huge number of capsnap queued in a short time, on
> x86_64 it's 248 bytes, which will be rounded up to 256 bytes by
> kzalloc. Move this to a dedicated slabcache to save 8 bytes for
> each.
> 
> For the kmalloc-256 slab cache, the actual size will be 512 bytes:
> kmalloc-256        21797  74656    512   32    4 : tunables, etc
> 
> For a dedicated slab cache the real size is 312 bytes:
> ceph_cap_snap          0      0    312   52    4 : tunables, etc
> 
> So actually we can save 200 bytes for each.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/snap.c               | 5 +++--
>  fs/ceph/super.c              | 7 +++++++
>  fs/ceph/super.h              | 2 +-
>  include/linux/ceph/libceph.h | 1 +
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index b41e6724c591..c787775eaf2a 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -482,7 +482,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  	struct ceph_buffer *old_blob = NULL;
>  	int used, dirty;
>  
> -	capsnap = kzalloc(sizeof(*capsnap), GFP_NOFS);
> +	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
>  	if (!capsnap) {
>  		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
>  		return;
> @@ -603,7 +603,8 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  	spin_unlock(&ci->i_ceph_lock);
>  
>  	ceph_buffer_put(old_blob);
> -	kfree(capsnap);
> +	if (capsnap)
> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>  	ceph_put_snap_context(old_snapc);
>  }
>  
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index bf79f369aec6..978463fa822c 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -864,6 +864,7 @@ static void destroy_fs_client(struct ceph_fs_client *fsc)
>   */
>  struct kmem_cache *ceph_inode_cachep;
>  struct kmem_cache *ceph_cap_cachep;
> +struct kmem_cache *ceph_cap_snap_cachep;
>  struct kmem_cache *ceph_cap_flush_cachep;
>  struct kmem_cache *ceph_dentry_cachep;
>  struct kmem_cache *ceph_file_cachep;
> @@ -892,6 +893,9 @@ static int __init init_caches(void)
>  	ceph_cap_cachep = KMEM_CACHE(ceph_cap, SLAB_MEM_SPREAD);
>  	if (!ceph_cap_cachep)
>  		goto bad_cap;
> +	ceph_cap_snap_cachep = KMEM_CACHE(ceph_cap_snap, SLAB_MEM_SPREAD);
> +	if (!ceph_cap_snap_cachep)
> +		goto bad_cap_snap;
>  	ceph_cap_flush_cachep = KMEM_CACHE(ceph_cap_flush,
>  					   SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD);
>  	if (!ceph_cap_flush_cachep)
> @@ -931,6 +935,8 @@ static int __init init_caches(void)
>  bad_dentry:
>  	kmem_cache_destroy(ceph_cap_flush_cachep);
>  bad_cap_flush:
> +	kmem_cache_destroy(ceph_cap_snap_cachep);
> +bad_cap_snap:
>  	kmem_cache_destroy(ceph_cap_cachep);
>  bad_cap:
>  	kmem_cache_destroy(ceph_inode_cachep);
> @@ -947,6 +953,7 @@ static void destroy_caches(void)
>  
>  	kmem_cache_destroy(ceph_inode_cachep);
>  	kmem_cache_destroy(ceph_cap_cachep);
> +	kmem_cache_destroy(ceph_cap_snap_cachep);
>  	kmem_cache_destroy(ceph_cap_flush_cachep);
>  	kmem_cache_destroy(ceph_dentry_cachep);
>  	kmem_cache_destroy(ceph_file_cachep);
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index c0718d5a8fb8..2d08104c8955 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -231,7 +231,7 @@ static inline void ceph_put_cap_snap(struct ceph_cap_snap *capsnap)
>  	if (refcount_dec_and_test(&capsnap->nref)) {
>  		if (capsnap->xattr_blob)
>  			ceph_buffer_put(capsnap->xattr_blob);
> -		kfree(capsnap);
> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>  	}
>  }
>  
> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> index edf62eaa6285..00af2c98da75 100644
> --- a/include/linux/ceph/libceph.h
> +++ b/include/linux/ceph/libceph.h
> @@ -284,6 +284,7 @@ DEFINE_RB_LOOKUP_FUNC(name, type, keyfld, nodefld)
>  
>  extern struct kmem_cache *ceph_inode_cachep;
>  extern struct kmem_cache *ceph_cap_cachep;
> +extern struct kmem_cache *ceph_cap_snap_cachep;
>  extern struct kmem_cache *ceph_cap_flush_cachep;
>  extern struct kmem_cache *ceph_dentry_cachep;
>  extern struct kmem_cache *ceph_file_cachep;

Looks good. I'll plan to merge this into testing branch.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] ceph: move to a dedicated slabcache for ceph_cap_snap

[Jeff Layton]

On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> There could be huge number of capsnap queued in a short time, on
> x86_64 it's 248 bytes, which will be rounded up to 256 bytes by
> kzalloc. Move this to a dedicated slabcache to save 8 bytes for
> each.
> 
> For the kmalloc-256 slab cache, the actual size will be 512 bytes:
> kmalloc-256        21797  74656    512   32    4 : tunables, etc
> 
> For a dedicated slab cache the real size is 312 bytes:
> ceph_cap_snap          0      0    312   52    4 : tunables, etc
> 
> So actually we can save 200 bytes for each.
> 

I dropped everything but the top paragraph in the description above. On
non-debug kernels, kmalloc-256 is indeed 256 bytes. The inflation you're
seeing is almost certainly from sort of kernel debugging options.

> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/snap.c               | 5 +++--
>  fs/ceph/super.c              | 7 +++++++
>  fs/ceph/super.h              | 2 +-
>  include/linux/ceph/libceph.h | 1 +
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index b41e6724c591..c787775eaf2a 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -482,7 +482,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  	struct ceph_buffer *old_blob = NULL;
>  	int used, dirty;
>  
> -	capsnap = kzalloc(sizeof(*capsnap), GFP_NOFS);
> +	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
>  	if (!capsnap) {
>  		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
>  		return;
> @@ -603,7 +603,8 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  	spin_unlock(&ci->i_ceph_lock);
>  
>  	ceph_buffer_put(old_blob);
> -	kfree(capsnap);
> +	if (capsnap)
> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>  	ceph_put_snap_context(old_snapc);
>  }
>  
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index bf79f369aec6..978463fa822c 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -864,6 +864,7 @@ static void destroy_fs_client(struct ceph_fs_client *fsc)
>   */
>  struct kmem_cache *ceph_inode_cachep;
>  struct kmem_cache *ceph_cap_cachep;
> +struct kmem_cache *ceph_cap_snap_cachep;
>  struct kmem_cache *ceph_cap_flush_cachep;
>  struct kmem_cache *ceph_dentry_cachep;
>  struct kmem_cache *ceph_file_cachep;
> @@ -892,6 +893,9 @@ static int __init init_caches(void)
>  	ceph_cap_cachep = KMEM_CACHE(ceph_cap, SLAB_MEM_SPREAD);
>  	if (!ceph_cap_cachep)
>  		goto bad_cap;
> +	ceph_cap_snap_cachep = KMEM_CACHE(ceph_cap_snap, SLAB_MEM_SPREAD);
> +	if (!ceph_cap_snap_cachep)
> +		goto bad_cap_snap;
>  	ceph_cap_flush_cachep = KMEM_CACHE(ceph_cap_flush,
>  					   SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD);
>  	if (!ceph_cap_flush_cachep)
> @@ -931,6 +935,8 @@ static int __init init_caches(void)
>  bad_dentry:
>  	kmem_cache_destroy(ceph_cap_flush_cachep);
>  bad_cap_flush:
> +	kmem_cache_destroy(ceph_cap_snap_cachep);
> +bad_cap_snap:
>  	kmem_cache_destroy(ceph_cap_cachep);
>  bad_cap:
>  	kmem_cache_destroy(ceph_inode_cachep);
> @@ -947,6 +953,7 @@ static void destroy_caches(void)
>  
>  	kmem_cache_destroy(ceph_inode_cachep);
>  	kmem_cache_destroy(ceph_cap_cachep);
> +	kmem_cache_destroy(ceph_cap_snap_cachep);
>  	kmem_cache_destroy(ceph_cap_flush_cachep);
>  	kmem_cache_destroy(ceph_dentry_cachep);
>  	kmem_cache_destroy(ceph_file_cachep);
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index c0718d5a8fb8..2d08104c8955 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -231,7 +231,7 @@ static inline void ceph_put_cap_snap(struct ceph_cap_snap *capsnap)
>  	if (refcount_dec_and_test(&capsnap->nref)) {
>  		if (capsnap->xattr_blob)
>  			ceph_buffer_put(capsnap->xattr_blob);
> -		kfree(capsnap);
> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>  	}
>  }
>  
> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> index edf62eaa6285..00af2c98da75 100644
> --- a/include/linux/ceph/libceph.h
> +++ b/include/linux/ceph/libceph.h
> @@ -284,6 +284,7 @@ DEFINE_RB_LOOKUP_FUNC(name, type, keyfld, nodefld)
>  
>  extern struct kmem_cache *ceph_inode_cachep;
>  extern struct kmem_cache *ceph_cap_cachep;
> +extern struct kmem_cache *ceph_cap_snap_cachep;
>  extern struct kmem_cache *ceph_cap_flush_cachep;
>  extern struct kmem_cache *ceph_dentry_cachep;
>  extern struct kmem_cache *ceph_file_cachep;

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] ceph: move to a dedicated slabcache for ceph_cap_snap

[Xiubo Li]

On 2/16/22 10:58 PM, Jeff Layton wrote:
> On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> There could be huge number of capsnap queued in a short time, on
>> x86_64 it's 248 bytes, which will be rounded up to 256 bytes by
>> kzalloc. Move this to a dedicated slabcache to save 8 bytes for
>> each.
>>
>> For the kmalloc-256 slab cache, the actual size will be 512 bytes:
>> kmalloc-256        21797  74656    512   32    4 : tunables, etc
>>
>> For a dedicated slab cache the real size is 312 bytes:
>> ceph_cap_snap          0      0    312   52    4 : tunables, etc
>>
>> So actually we can save 200 bytes for each.
>>
> I dropped everything but the top paragraph in the description above. On
> non-debug kernels, kmalloc-256 is indeed 256 bytes. The inflation you're
> seeing is almost certainly from sort of kernel debugging options.

Yeah, checked it, you are right.


>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/snap.c               | 5 +++--
>>   fs/ceph/super.c              | 7 +++++++
>>   fs/ceph/super.h              | 2 +-
>>   include/linux/ceph/libceph.h | 1 +
>>   4 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
>> index b41e6724c591..c787775eaf2a 100644
>> --- a/fs/ceph/snap.c
>> +++ b/fs/ceph/snap.c
>> @@ -482,7 +482,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>>   	struct ceph_buffer *old_blob = NULL;
>>   	int used, dirty;
>>   
>> -	capsnap = kzalloc(sizeof(*capsnap), GFP_NOFS);
>> +	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
>>   	if (!capsnap) {
>>   		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
>>   		return;
>> @@ -603,7 +603,8 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>>   	spin_unlock(&ci->i_ceph_lock);
>>   
>>   	ceph_buffer_put(old_blob);
>> -	kfree(capsnap);
>> +	if (capsnap)
>> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>>   	ceph_put_snap_context(old_snapc);
>>   }
>>   
>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>> index bf79f369aec6..978463fa822c 100644
>> --- a/fs/ceph/super.c
>> +++ b/fs/ceph/super.c
>> @@ -864,6 +864,7 @@ static void destroy_fs_client(struct ceph_fs_client *fsc)
>>    */
>>   struct kmem_cache *ceph_inode_cachep;
>>   struct kmem_cache *ceph_cap_cachep;
>> +struct kmem_cache *ceph_cap_snap_cachep;
>>   struct kmem_cache *ceph_cap_flush_cachep;
>>   struct kmem_cache *ceph_dentry_cachep;
>>   struct kmem_cache *ceph_file_cachep;
>> @@ -892,6 +893,9 @@ static int __init init_caches(void)
>>   	ceph_cap_cachep = KMEM_CACHE(ceph_cap, SLAB_MEM_SPREAD);
>>   	if (!ceph_cap_cachep)
>>   		goto bad_cap;
>> +	ceph_cap_snap_cachep = KMEM_CACHE(ceph_cap_snap, SLAB_MEM_SPREAD);
>> +	if (!ceph_cap_snap_cachep)
>> +		goto bad_cap_snap;
>>   	ceph_cap_flush_cachep = KMEM_CACHE(ceph_cap_flush,
>>   					   SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD);
>>   	if (!ceph_cap_flush_cachep)
>> @@ -931,6 +935,8 @@ static int __init init_caches(void)
>>   bad_dentry:
>>   	kmem_cache_destroy(ceph_cap_flush_cachep);
>>   bad_cap_flush:
>> +	kmem_cache_destroy(ceph_cap_snap_cachep);
>> +bad_cap_snap:
>>   	kmem_cache_destroy(ceph_cap_cachep);
>>   bad_cap:
>>   	kmem_cache_destroy(ceph_inode_cachep);
>> @@ -947,6 +953,7 @@ static void destroy_caches(void)
>>   
>>   	kmem_cache_destroy(ceph_inode_cachep);
>>   	kmem_cache_destroy(ceph_cap_cachep);
>> +	kmem_cache_destroy(ceph_cap_snap_cachep);
>>   	kmem_cache_destroy(ceph_cap_flush_cachep);
>>   	kmem_cache_destroy(ceph_dentry_cachep);
>>   	kmem_cache_destroy(ceph_file_cachep);
>> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
>> index c0718d5a8fb8..2d08104c8955 100644
>> --- a/fs/ceph/super.h
>> +++ b/fs/ceph/super.h
>> @@ -231,7 +231,7 @@ static inline void ceph_put_cap_snap(struct ceph_cap_snap *capsnap)
>>   	if (refcount_dec_and_test(&capsnap->nref)) {
>>   		if (capsnap->xattr_blob)
>>   			ceph_buffer_put(capsnap->xattr_blob);
>> -		kfree(capsnap);
>> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>>   	}
>>   }
>>   
>> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
>> index edf62eaa6285..00af2c98da75 100644
>> --- a/include/linux/ceph/libceph.h
>> +++ b/include/linux/ceph/libceph.h
>> @@ -284,6 +284,7 @@ DEFINE_RB_LOOKUP_FUNC(name, type, keyfld, nodefld)
>>   
>>   extern struct kmem_cache *ceph_inode_cachep;
>>   extern struct kmem_cache *ceph_cap_cachep;
>> +extern struct kmem_cache *ceph_cap_snap_cachep;
>>   extern struct kmem_cache *ceph_cap_flush_cachep;
>>   extern struct kmem_cache *ceph_dentry_cachep;
>>   extern struct kmem_cache *ceph_file_cachep;

Re: [PATCH 1/3] ceph: move to a dedicated slabcache for ceph_cap_snap

[Luís Henriques]

xiubli@redhat.com writes:

> From: Xiubo Li <xiubli@redhat.com>
>
> There could be huge number of capsnap queued in a short time, on
> x86_64 it's 248 bytes, which will be rounded up to 256 bytes by
> kzalloc. Move this to a dedicated slabcache to save 8 bytes for
> each.
>
> For the kmalloc-256 slab cache, the actual size will be 512 bytes:
> kmalloc-256        21797  74656    512   32    4 : tunables, etc
>
> For a dedicated slab cache the real size is 312 bytes:
> ceph_cap_snap          0      0    312   52    4 : tunables, etc
>
> So actually we can save 200 bytes for each.
>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/snap.c               | 5 +++--
>  fs/ceph/super.c              | 7 +++++++
>  fs/ceph/super.h              | 2 +-
>  include/linux/ceph/libceph.h | 1 +
>  4 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index b41e6724c591..c787775eaf2a 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -482,7 +482,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  	struct ceph_buffer *old_blob = NULL;
>  	int used, dirty;
>  
> -	capsnap = kzalloc(sizeof(*capsnap), GFP_NOFS);
> +	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);

Unfortunately, this is causing issues in my testing.  Looks like there are
several fields that are assumed to be initialised to zero.  I've seen two
BUGs so far, in functions ceph_try_drop_cap_snap (capsnap->cap_flush.tid > 0)
and __ceph_finish_cap_snap (capsnap->writing).

I guess you'll have to either zero out all that memory, or manually
initialise the fields (not sure which ones really require that).

Cheers,
-- 
Luís

>  	if (!capsnap) {
>  		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
>  		return;
> @@ -603,7 +603,8 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  	spin_unlock(&ci->i_ceph_lock);
>  
>  	ceph_buffer_put(old_blob);
> -	kfree(capsnap);
> +	if (capsnap)
> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>  	ceph_put_snap_context(old_snapc);
>  }
>  
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index bf79f369aec6..978463fa822c 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -864,6 +864,7 @@ static void destroy_fs_client(struct ceph_fs_client *fsc)
>   */
>  struct kmem_cache *ceph_inode_cachep;
>  struct kmem_cache *ceph_cap_cachep;
> +struct kmem_cache *ceph_cap_snap_cachep;
>  struct kmem_cache *ceph_cap_flush_cachep;
>  struct kmem_cache *ceph_dentry_cachep;
>  struct kmem_cache *ceph_file_cachep;
> @@ -892,6 +893,9 @@ static int __init init_caches(void)
>  	ceph_cap_cachep = KMEM_CACHE(ceph_cap, SLAB_MEM_SPREAD);
>  	if (!ceph_cap_cachep)
>  		goto bad_cap;
> +	ceph_cap_snap_cachep = KMEM_CACHE(ceph_cap_snap, SLAB_MEM_SPREAD);
> +	if (!ceph_cap_snap_cachep)
> +		goto bad_cap_snap;
>  	ceph_cap_flush_cachep = KMEM_CACHE(ceph_cap_flush,
>  					   SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD);
>  	if (!ceph_cap_flush_cachep)
> @@ -931,6 +935,8 @@ static int __init init_caches(void)
>  bad_dentry:
>  	kmem_cache_destroy(ceph_cap_flush_cachep);
>  bad_cap_flush:
> +	kmem_cache_destroy(ceph_cap_snap_cachep);
> +bad_cap_snap:
>  	kmem_cache_destroy(ceph_cap_cachep);
>  bad_cap:
>  	kmem_cache_destroy(ceph_inode_cachep);
> @@ -947,6 +953,7 @@ static void destroy_caches(void)
>  
>  	kmem_cache_destroy(ceph_inode_cachep);
>  	kmem_cache_destroy(ceph_cap_cachep);
> +	kmem_cache_destroy(ceph_cap_snap_cachep);
>  	kmem_cache_destroy(ceph_cap_flush_cachep);
>  	kmem_cache_destroy(ceph_dentry_cachep);
>  	kmem_cache_destroy(ceph_file_cachep);
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index c0718d5a8fb8..2d08104c8955 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -231,7 +231,7 @@ static inline void ceph_put_cap_snap(struct ceph_cap_snap *capsnap)
>  	if (refcount_dec_and_test(&capsnap->nref)) {
>  		if (capsnap->xattr_blob)
>  			ceph_buffer_put(capsnap->xattr_blob);
> -		kfree(capsnap);
> +		kmem_cache_free(ceph_cap_snap_cachep, capsnap);
>  	}
>  }
>  
> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> index edf62eaa6285..00af2c98da75 100644
> --- a/include/linux/ceph/libceph.h
> +++ b/include/linux/ceph/libceph.h
> @@ -284,6 +284,7 @@ DEFINE_RB_LOOKUP_FUNC(name, type, keyfld, nodefld)
>  
>  extern struct kmem_cache *ceph_inode_cachep;
>  extern struct kmem_cache *ceph_cap_cachep;
> +extern struct kmem_cache *ceph_cap_snap_cachep;
>  extern struct kmem_cache *ceph_cap_flush_cachep;
>  extern struct kmem_cache *ceph_dentry_cachep;
>  extern struct kmem_cache *ceph_file_cachep;
> -- 
>
> 2.27.0
>

Re: [PATCH 1/3] ceph: move to a dedicated slabcache for ceph_cap_snap

[Jeff Layton]

On Fri, 2022-02-18 at 18:11 +0000, Luís Henriques wrote:
> xiubli@redhat.com writes:
> 
> > From: Xiubo Li <xiubli@redhat.com>
> > 
> > There could be huge number of capsnap queued in a short time, on
> > x86_64 it's 248 bytes, which will be rounded up to 256 bytes by
> > kzalloc. Move this to a dedicated slabcache to save 8 bytes for
> > each.
> > 
> > For the kmalloc-256 slab cache, the actual size will be 512 bytes:
> > kmalloc-256        21797  74656    512   32    4 : tunables, etc
> > 
> > For a dedicated slab cache the real size is 312 bytes:
> > ceph_cap_snap          0      0    312   52    4 : tunables, etc
> > 
> > So actually we can save 200 bytes for each.
> > 
> > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > ---
> >  fs/ceph/snap.c               | 5 +++--
> >  fs/ceph/super.c              | 7 +++++++
> >  fs/ceph/super.h              | 2 +-
> >  include/linux/ceph/libceph.h | 1 +
> >  4 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> > index b41e6724c591..c787775eaf2a 100644
> > --- a/fs/ceph/snap.c
> > +++ b/fs/ceph/snap.c
> > @@ -482,7 +482,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
> >  	struct ceph_buffer *old_blob = NULL;
> >  	int used, dirty;
> >  
> > -	capsnap = kzalloc(sizeof(*capsnap), GFP_NOFS);
> > +	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
> 
> Unfortunately, this is causing issues in my testing.  Looks like there are
> several fields that are assumed to be initialised to zero.  I've seen two
> BUGs so far, in functions ceph_try_drop_cap_snap (capsnap->cap_flush.tid > 0)
> and __ceph_finish_cap_snap (capsnap->writing).
> 
> I guess you'll have to either zero out all that memory, or manually
> initialise the fields (not sure which ones really require that).


Good catch. That memory is expected to be zeroed. I switched it to use
kmem_cache_zalloc in testing branch, which should fix this. Please let
me know if it doesn't.

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 2/3] ceph: move kzalloc under i_ceph_lock with GFP_ATOMIC flag

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

There has one case that the snaprealm has been updated and then
it will iterate all the inode under it and try to queue a cap
snap for it. But in some case there has millions of subdirectries
or files under it and most of them no any Fw or dirty pages and
then will just be skipped.

URL: https://tracker.ceph.com/issues/44100
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/snap.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index c787775eaf2a..d075d3ce5f6d 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -477,19 +477,21 @@ static bool has_new_snaps(struct ceph_snap_context *o,
 static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
 {
 	struct inode *inode = &ci->vfs_inode;
-	struct ceph_cap_snap *capsnap;
+	struct ceph_cap_snap *capsnap = NULL;
 	struct ceph_snap_context *old_snapc, *new_snapc;
 	struct ceph_buffer *old_blob = NULL;
 	int used, dirty;
-
-	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
-	if (!capsnap) {
-		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
-		return;
+	bool need_flush = false;
+	bool atomic_alloc_mem_failed = false;
+
+retry:
+	if (unlikely(atomic_alloc_mem_failed)) {
+	        capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
+		if (!capsnap) {
+			pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
+			return;
+		}
 	}
-	capsnap->cap_flush.is_capsnap = true;
-	INIT_LIST_HEAD(&capsnap->cap_flush.i_list);
-	INIT_LIST_HEAD(&capsnap->cap_flush.g_list);
 
 	spin_lock(&ci->i_ceph_lock);
 	used = __ceph_caps_used(ci);
@@ -532,7 +534,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
 	 */
 	if (has_new_snaps(old_snapc, new_snapc)) {
 		if (dirty & (CEPH_CAP_ANY_EXCL|CEPH_CAP_FILE_WR))
-			capsnap->need_flush = true;
+			need_flush = true;
 	} else {
 		if (!(used & CEPH_CAP_FILE_WR) &&
 		    ci->i_wrbuffer_ref_head == 0) {
@@ -542,6 +544,21 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
 		}
 	}
 
+	if (!capsnap) {
+	        capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_ATOMIC);
+		if (unlikely(!capsnap)) {
+			pr_err("ENOMEM atomic allocating ceph_cap_snap on %p\n",
+			       inode);
+			spin_unlock(&ci->i_ceph_lock);
+			atomic_alloc_mem_failed = true;
+			goto retry;
+		}
+	}
+	capsnap->need_flush = need_flush;
+	capsnap->cap_flush.is_capsnap = true;
+	INIT_LIST_HEAD(&capsnap->cap_flush.i_list);
+	INIT_LIST_HEAD(&capsnap->cap_flush.g_list);
+
 	dout("queue_cap_snap %p cap_snap %p queuing under %p %s %s\n",
 	     inode, capsnap, old_snapc, ceph_cap_string(dirty),
 	     capsnap->need_flush ? "" : "no_flush");
-- 
2.27.0

Re: [PATCH 2/3] ceph: move kzalloc under i_ceph_lock with GFP_ATOMIC flag

[Jeff Layton]

On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> There has one case that the snaprealm has been updated and then
> it will iterate all the inode under it and try to queue a cap
> snap for it. But in some case there has millions of subdirectries
> or files under it and most of them no any Fw or dirty pages and
> then will just be skipped.
> 
> URL: https://tracker.ceph.com/issues/44100
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/snap.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index c787775eaf2a..d075d3ce5f6d 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -477,19 +477,21 @@ static bool has_new_snaps(struct ceph_snap_context *o,
>  static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  {
>  	struct inode *inode = &ci->vfs_inode;
> -	struct ceph_cap_snap *capsnap;
> +	struct ceph_cap_snap *capsnap = NULL;
>  	struct ceph_snap_context *old_snapc, *new_snapc;
>  	struct ceph_buffer *old_blob = NULL;
>  	int used, dirty;
> -
> -	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
> -	if (!capsnap) {
> -		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
> -		return;
> +	bool need_flush = false;
> +	bool atomic_alloc_mem_failed = false;
> +
> +retry:
> +	if (unlikely(atomic_alloc_mem_failed)) {
> +	        capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
> +		if (!capsnap) {
> +			pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
> +			return;
> +		}
>  	}
> -	capsnap->cap_flush.is_capsnap = true;
> -	INIT_LIST_HEAD(&capsnap->cap_flush.i_list);
> -	INIT_LIST_HEAD(&capsnap->cap_flush.g_list);
>  
>  	spin_lock(&ci->i_ceph_lock);
>  	used = __ceph_caps_used(ci);
> @@ -532,7 +534,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  	 */
>  	if (has_new_snaps(old_snapc, new_snapc)) {
>  		if (dirty & (CEPH_CAP_ANY_EXCL|CEPH_CAP_FILE_WR))
> -			capsnap->need_flush = true;
> +			need_flush = true;
>  	} else {
>  		if (!(used & CEPH_CAP_FILE_WR) &&
>  		    ci->i_wrbuffer_ref_head == 0) {
> @@ -542,6 +544,21 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>  		}
>  	}
>  
> +	if (!capsnap) {
> +	        capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_ATOMIC);
> +		if (unlikely(!capsnap)) {
> +			pr_err("ENOMEM atomic allocating ceph_cap_snap on %p\n",
> +			       inode);
> +			spin_unlock(&ci->i_ceph_lock);
> +			atomic_alloc_mem_failed = true;
> +			goto retry;
> +		}
> +	}
> +	capsnap->need_flush = need_flush;
> +	capsnap->cap_flush.is_capsnap = true;
> +	INIT_LIST_HEAD(&capsnap->cap_flush.i_list);
> +	INIT_LIST_HEAD(&capsnap->cap_flush.g_list);
> +
>  	dout("queue_cap_snap %p cap_snap %p queuing under %p %s %s\n",
>  	     inode, capsnap, old_snapc, ceph_cap_string(dirty),
>  	     capsnap->need_flush ? "" : "no_flush");

I'm not so thrilled with this patch.

First, are you sure you want GFP_ATOMIC here? Something like GFP_NOWAIT
may be better since you have a fallback so the kernel can still make
forward progress on reclaim if this returns NULL.

That said, this is pretty kludgey. I'd much prefer to see something that
didn't require this sort of hack. Maybe instead you could have
queue_realm_cap_snaps do the allocation and pass a (struct ceph_cap_snap
**) pointer in, and it can set the thing to NULL if it ends up using it?

That way, we still don't do the allocation under spinlock and you only
end up allocating the number you need (plus maybe one or two on the
edges).

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] ceph: move kzalloc under i_ceph_lock with GFP_ATOMIC flag

[Xiubo Li]

On 2/16/22 12:57 AM, Jeff Layton wrote:
> On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> There has one case that the snaprealm has been updated and then
>> it will iterate all the inode under it and try to queue a cap
>> snap for it. But in some case there has millions of subdirectries
>> or files under it and most of them no any Fw or dirty pages and
>> then will just be skipped.
>>
>> URL: https://tracker.ceph.com/issues/44100
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/snap.c | 37 +++++++++++++++++++++++++++----------
>>   1 file changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
>> index c787775eaf2a..d075d3ce5f6d 100644
>> --- a/fs/ceph/snap.c
>> +++ b/fs/ceph/snap.c
>> @@ -477,19 +477,21 @@ static bool has_new_snaps(struct ceph_snap_context *o,
>>   static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>>   {
>>   	struct inode *inode = &ci->vfs_inode;
>> -	struct ceph_cap_snap *capsnap;
>> +	struct ceph_cap_snap *capsnap = NULL;
>>   	struct ceph_snap_context *old_snapc, *new_snapc;
>>   	struct ceph_buffer *old_blob = NULL;
>>   	int used, dirty;
>> -
>> -	capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
>> -	if (!capsnap) {
>> -		pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
>> -		return;
>> +	bool need_flush = false;
>> +	bool atomic_alloc_mem_failed = false;
>> +
>> +retry:
>> +	if (unlikely(atomic_alloc_mem_failed)) {
>> +	        capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_NOFS);
>> +		if (!capsnap) {
>> +			pr_err("ENOMEM allocating ceph_cap_snap on %p\n", inode);
>> +			return;
>> +		}
>>   	}
>> -	capsnap->cap_flush.is_capsnap = true;
>> -	INIT_LIST_HEAD(&capsnap->cap_flush.i_list);
>> -	INIT_LIST_HEAD(&capsnap->cap_flush.g_list);
>>   
>>   	spin_lock(&ci->i_ceph_lock);
>>   	used = __ceph_caps_used(ci);
>> @@ -532,7 +534,7 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>>   	 */
>>   	if (has_new_snaps(old_snapc, new_snapc)) {
>>   		if (dirty & (CEPH_CAP_ANY_EXCL|CEPH_CAP_FILE_WR))
>> -			capsnap->need_flush = true;
>> +			need_flush = true;
>>   	} else {
>>   		if (!(used & CEPH_CAP_FILE_WR) &&
>>   		    ci->i_wrbuffer_ref_head == 0) {
>> @@ -542,6 +544,21 @@ static void ceph_queue_cap_snap(struct ceph_inode_info *ci)
>>   		}
>>   	}
>>   
>> +	if (!capsnap) {
>> +	        capsnap = kmem_cache_alloc(ceph_cap_snap_cachep, GFP_ATOMIC);
>> +		if (unlikely(!capsnap)) {
>> +			pr_err("ENOMEM atomic allocating ceph_cap_snap on %p\n",
>> +			       inode);
>> +			spin_unlock(&ci->i_ceph_lock);
>> +			atomic_alloc_mem_failed = true;
>> +			goto retry;
>> +		}
>> +	}
>> +	capsnap->need_flush = need_flush;
>> +	capsnap->cap_flush.is_capsnap = true;
>> +	INIT_LIST_HEAD(&capsnap->cap_flush.i_list);
>> +	INIT_LIST_HEAD(&capsnap->cap_flush.g_list);
>> +
>>   	dout("queue_cap_snap %p cap_snap %p queuing under %p %s %s\n",
>>   	     inode, capsnap, old_snapc, ceph_cap_string(dirty),
>>   	     capsnap->need_flush ? "" : "no_flush");
> I'm not so thrilled with this patch.
>
> First, are you sure you want GFP_ATOMIC here? Something like GFP_NOWAIT
> may be better since you have a fallback so the kernel can still make
> forward progress on reclaim if this returns NULL.
>
> That said, this is pretty kludgey. I'd much prefer to see something that
> didn't require this sort of hack. Maybe instead you could have
> queue_realm_cap_snaps do the allocation and pass a (struct ceph_cap_snap
> **) pointer in, and it can set the thing to NULL if it ends up using it?

Sounds good, I will switch to this approach in V2.

Thanks.


> That way, we still don't do the allocation under spinlock and you only
> end up allocating the number you need (plus maybe one or two on the
> edges).
>

[PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

No need to update snapshot context when any of the following two
cases happens:
1: if my context seq matches realm's seq and realm has no parent.
2: if my context seq equals or is larger than my parent's, this
   works because we rebuild_snap_realms() works _downward_ in
   hierarchy after each update.

This fix will avoid those inodes which accidently calling
ceph_queue_cap_snap() and make no sense, for exmaple:

There have 6 directories like:

/dir_X1/dir_X2/dir_X3/
/dir_Y1/dir_Y2/dir_Y3/

Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
make a root snapshot under /.snap/root_snap. And every time when
we make snapshots under /dir_Y1/..., the kclient will always try
to rebuild the snap context for snap_X2 realm and finally will
always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
no sense.

That's because the snap_X2's seq is 2 and root_snap's seq is 3.
So when creating a new snapshot under /dir_Y1/... the new seq
will be 4, and then the mds will send kclient a snapshot backtrace
in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
it will always rebuild the from the last realm, that's the root_snap.
So later when rebuilding the snap context it will always rebuild
the snap_X2 realm and then try to queue cap snaps for all the inodes
related in snap_X2 realm, and we are seeing the logs like:

"ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"

URL: https://tracker.ceph.com/issues/44100
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/snap.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index d075d3ce5f6d..1f24a5de81e7 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
 		num += parent->cached_context->num_snaps;
 	}
 
-	/* do i actually need to update?  not if my context seq
-	   matches realm seq, and my parents' does to.  (this works
-	   because we rebuild_snap_realms() works _downward_ in
-	   hierarchy after each update.) */
+	/* do i actually need to update? No need when any of the following
+	 * two cases:
+	 * #1: if my context seq matches realm's seq and realm has no parent.
+	 * #2: if my context seq equals or is larger than my parent's, this
+	 *     works because we rebuild_snap_realms() works _downward_ in
+	 *     hierarchy after each update.
+	 */
 	if (realm->cached_context &&
-	    realm->cached_context->seq == realm->seq &&
-	    (!parent ||
-	     realm->cached_context->seq >= parent->cached_context->seq)) {
+	    ((realm->cached_context->seq == realm->seq && !parent) ||
+	     (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
 		dout("build_snap_context %llx %p: %p seq %lld (%u snaps)"
 		     " (unchanged)\n",
 		     realm->ino, realm, realm->cached_context,
-- 
2.27.0

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Jeff Layton]

On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> No need to update snapshot context when any of the following two
> cases happens:
> 1: if my context seq matches realm's seq and realm has no parent.
> 2: if my context seq equals or is larger than my parent's, this
>    works because we rebuild_snap_realms() works _downward_ in
>    hierarchy after each update.
> 
> This fix will avoid those inodes which accidently calling
> ceph_queue_cap_snap() and make no sense, for exmaple:
> 
> There have 6 directories like:
> 
> /dir_X1/dir_X2/dir_X3/
> /dir_Y1/dir_Y2/dir_Y3/
> 
> Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
> make a root snapshot under /.snap/root_snap. And every time when
> we make snapshots under /dir_Y1/..., the kclient will always try
> to rebuild the snap context for snap_X2 realm and finally will
> always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
> no sense.
> 
> That's because the snap_X2's seq is 2 and root_snap's seq is 3.
> So when creating a new snapshot under /dir_Y1/... the new seq
> will be 4, and then the mds will send kclient a snapshot backtrace
> in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
> it will always rebuild the from the last realm, that's the root_snap.
> So later when rebuilding the snap context it will always rebuild
> the snap_X2 realm and then try to queue cap snaps for all the inodes
> related in snap_X2 realm, and we are seeing the logs like:
> 
> "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
> 
> URL: https://tracker.ceph.com/issues/44100
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/snap.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index d075d3ce5f6d..1f24a5de81e7 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>  		num += parent->cached_context->num_snaps;
>  	}
>  
> -	/* do i actually need to update?  not if my context seq
> -	   matches realm seq, and my parents' does to.  (this works
> -	   because we rebuild_snap_realms() works _downward_ in
> -	   hierarchy after each update.) */
> +	/* do i actually need to update? No need when any of the following
> +	 * two cases:
> +	 * #1: if my context seq matches realm's seq and realm has no parent.
> +	 * #2: if my context seq equals or is larger than my parent's, this
> +	 *     works because we rebuild_snap_realms() works _downward_ in
> +	 *     hierarchy after each update.
> +	 */
>  	if (realm->cached_context &&
> -	    realm->cached_context->seq == realm->seq &&
> -	    (!parent ||
> -	     realm->cached_context->seq >= parent->cached_context->seq)) {
> +	    ((realm->cached_context->seq == realm->seq && !parent) ||
> +	     (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
>  		dout("build_snap_context %llx %p: %p seq %lld (%u snaps)"
>  		     " (unchanged)\n",
>  		     realm->ino, realm, realm->cached_context,

I've never had a good feel for the snaprealm handling code, so I'll
leave it to others that do to comment on whether your logic makes sense.

Either way, I don't think this patch depends on the earlier two, does
it? The comment is a nice addition though.

Acked-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Xiubo Li]

On 2/16/22 1:05 AM, Jeff Layton wrote:
> On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> No need to update snapshot context when any of the following two
>> cases happens:
>> 1: if my context seq matches realm's seq and realm has no parent.
>> 2: if my context seq equals or is larger than my parent's, this
>>     works because we rebuild_snap_realms() works _downward_ in
>>     hierarchy after each update.
>>
>> This fix will avoid those inodes which accidently calling
>> ceph_queue_cap_snap() and make no sense, for exmaple:
>>
>> There have 6 directories like:
>>
>> /dir_X1/dir_X2/dir_X3/
>> /dir_Y1/dir_Y2/dir_Y3/
>>
>> Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
>> make a root snapshot under /.snap/root_snap. And every time when
>> we make snapshots under /dir_Y1/..., the kclient will always try
>> to rebuild the snap context for snap_X2 realm and finally will
>> always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
>> no sense.
>>
>> That's because the snap_X2's seq is 2 and root_snap's seq is 3.
>> So when creating a new snapshot under /dir_Y1/... the new seq
>> will be 4, and then the mds will send kclient a snapshot backtrace
>> in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
>> it will always rebuild the from the last realm, that's the root_snap.
>> So later when rebuilding the snap context it will always rebuild
>> the snap_X2 realm and then try to queue cap snaps for all the inodes
>> related in snap_X2 realm, and we are seeing the logs like:
>>
>> "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
>>
>> URL: https://tracker.ceph.com/issues/44100
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/snap.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
>> index d075d3ce5f6d..1f24a5de81e7 100644
>> --- a/fs/ceph/snap.c
>> +++ b/fs/ceph/snap.c
>> @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>>   		num += parent->cached_context->num_snaps;
>>   	}
>>   
>> -	/* do i actually need to update?  not if my context seq
>> -	   matches realm seq, and my parents' does to.  (this works
>> -	   because we rebuild_snap_realms() works _downward_ in
>> -	   hierarchy after each update.) */
>> +	/* do i actually need to update? No need when any of the following
>> +	 * two cases:
>> +	 * #1: if my context seq matches realm's seq and realm has no parent.
>> +	 * #2: if my context seq equals or is larger than my parent's, this
>> +	 *     works because we rebuild_snap_realms() works _downward_ in
>> +	 *     hierarchy after each update.
>> +	 */
>>   	if (realm->cached_context &&
>> -	    realm->cached_context->seq == realm->seq &&
>> -	    (!parent ||
>> -	     realm->cached_context->seq >= parent->cached_context->seq)) {
>> +	    ((realm->cached_context->seq == realm->seq && !parent) ||
>> +	     (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
>>   		dout("build_snap_context %llx %p: %p seq %lld (%u snaps)"
>>   		     " (unchanged)\n",
>>   		     realm->ino, realm, realm->cached_context,
> I've never had a good feel for the snaprealm handling code, so I'll
> leave it to others that do to comment on whether your logic makes sense.
>
> Either way, I don't think this patch depends on the earlier two, does
> it? The comment is a nice addition though.

Right.

Thanks.

>
> Acked-by: Jeff Layton <jlayton@kernel.org>
>

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Jeff Layton]

On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> No need to update snapshot context when any of the following two
> cases happens:
> 1: if my context seq matches realm's seq and realm has no parent.
> 2: if my context seq equals or is larger than my parent's, this
>    works because we rebuild_snap_realms() works _downward_ in
>    hierarchy after each update.
> 
> This fix will avoid those inodes which accidently calling
> ceph_queue_cap_snap() and make no sense, for exmaple:
> 
> There have 6 directories like:
> 
> /dir_X1/dir_X2/dir_X3/
> /dir_Y1/dir_Y2/dir_Y3/
> 
> Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
> make a root snapshot under /.snap/root_snap. And every time when
> we make snapshots under /dir_Y1/..., the kclient will always try
> to rebuild the snap context for snap_X2 realm and finally will
> always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
> no sense.
> 
> That's because the snap_X2's seq is 2 and root_snap's seq is 3.
> So when creating a new snapshot under /dir_Y1/... the new seq
> will be 4, and then the mds will send kclient a snapshot backtrace
> in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
> it will always rebuild the from the last realm, that's the root_snap.
> So later when rebuilding the snap context it will always rebuild
> the snap_X2 realm and then try to queue cap snaps for all the inodes
> related in snap_X2 realm, and we are seeing the logs like:
> 
> "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
> 
> URL: https://tracker.ceph.com/issues/44100
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/snap.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index d075d3ce5f6d..1f24a5de81e7 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>  		num += parent->cached_context->num_snaps;
>  	}
>  
> -	/* do i actually need to update?  not if my context seq
> -	   matches realm seq, and my parents' does to.  (this works
> -	   because we rebuild_snap_realms() works _downward_ in
> -	   hierarchy after each update.) */
> +	/* do i actually need to update? No need when any of the following
> +	 * two cases:
> +	 * #1: if my context seq matches realm's seq and realm has no parent.
> +	 * #2: if my context seq equals or is larger than my parent's, this
> +	 *     works because we rebuild_snap_realms() works _downward_ in
> +	 *     hierarchy after each update.
> +	 */

This may be, but that downward building is done via unbounded recursion
in rebuild_snap_realms().

I'm ok with taking this patch in the short term, but I would really like
to see steps made to eliminate recursion from this code altogether. A
sufficiently deep hierarchy could blow out the stack. How could we
redesign this code to avoid it?

>  	if (realm->cached_context &&
> -	    realm->cached_context->seq == realm->seq &&
> -	    (!parent ||
> -	     realm->cached_context->seq >= parent->cached_context->seq)) {
> +	    ((realm->cached_context->seq == realm->seq && !parent) ||
> +	     (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
>  		dout("build_snap_context %llx %p: %p seq %lld (%u snaps)"
>  		     " (unchanged)\n",
>  		     realm->ino, realm, realm->cached_context,

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Xiubo Li]

On 2/16/22 2:35 AM, Jeff Layton wrote:
> On Tue, 2022-02-15 at 20:23 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> No need to update snapshot context when any of the following two
>> cases happens:
>> 1: if my context seq matches realm's seq and realm has no parent.
>> 2: if my context seq equals or is larger than my parent's, this
>>     works because we rebuild_snap_realms() works _downward_ in
>>     hierarchy after each update.
>>
>> This fix will avoid those inodes which accidently calling
>> ceph_queue_cap_snap() and make no sense, for exmaple:
>>
>> There have 6 directories like:
>>
>> /dir_X1/dir_X2/dir_X3/
>> /dir_Y1/dir_Y2/dir_Y3/
>>
>> Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
>> make a root snapshot under /.snap/root_snap. And every time when
>> we make snapshots under /dir_Y1/..., the kclient will always try
>> to rebuild the snap context for snap_X2 realm and finally will
>> always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
>> no sense.
>>
>> That's because the snap_X2's seq is 2 and root_snap's seq is 3.
>> So when creating a new snapshot under /dir_Y1/... the new seq
>> will be 4, and then the mds will send kclient a snapshot backtrace
>> in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
>> it will always rebuild the from the last realm, that's the root_snap.
>> So later when rebuilding the snap context it will always rebuild
>> the snap_X2 realm and then try to queue cap snaps for all the inodes
>> related in snap_X2 realm, and we are seeing the logs like:
>>
>> "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
>>
>> URL: https://tracker.ceph.com/issues/44100
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/snap.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
>> index d075d3ce5f6d..1f24a5de81e7 100644
>> --- a/fs/ceph/snap.c
>> +++ b/fs/ceph/snap.c
>> @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>>   		num += parent->cached_context->num_snaps;
>>   	}
>>   
>> -	/* do i actually need to update?  not if my context seq
>> -	   matches realm seq, and my parents' does to.  (this works
>> -	   because we rebuild_snap_realms() works _downward_ in
>> -	   hierarchy after each update.) */
>> +	/* do i actually need to update? No need when any of the following
>> +	 * two cases:
>> +	 * #1: if my context seq matches realm's seq and realm has no parent.
>> +	 * #2: if my context seq equals or is larger than my parent's, this
>> +	 *     works because we rebuild_snap_realms() works _downward_ in
>> +	 *     hierarchy after each update.
>> +	 */
> This may be, but that downward building is done via unbounded recursion
> in rebuild_snap_realms().
>
> I'm ok with taking this patch in the short term, but I would really like
> to see steps made to eliminate recursion from this code altogether. A
> sufficiently deep hierarchy could blow out the stack. How could we
> redesign this code to avoid it?

Yeah, the recursion really a potential issue here, let me figure out one 
method to eliminate it later in the following patches, there has some 
other code also may need to improve, and I am still going through the 
code, not very clear yet. But not related to the perfermance issue this 
patch is trying to fix.

-- Xiubo

>
>>   	if (realm->cached_context &&
>> -	    realm->cached_context->seq == realm->seq &&
>> -	    (!parent ||
>> -	     realm->cached_context->seq >= parent->cached_context->seq)) {
>> +	    ((realm->cached_context->seq == realm->seq && !parent) ||
>> +	     (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
>>   		dout("build_snap_context %llx %p: %p seq %lld (%u snaps)"
>>   		     " (unchanged)\n",
>>   		     realm->ino, realm, realm->cached_context,

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Yan, Zheng]

On Tue, Feb 15, 2022 at 11:04 PM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> No need to update snapshot context when any of the following two
> cases happens:
> 1: if my context seq matches realm's seq and realm has no parent.
> 2: if my context seq equals or is larger than my parent's, this
>    works because we rebuild_snap_realms() works _downward_ in
>    hierarchy after each update.
>
> This fix will avoid those inodes which accidently calling
> ceph_queue_cap_snap() and make no sense, for exmaple:
>
> There have 6 directories like:
>
> /dir_X1/dir_X2/dir_X3/
> /dir_Y1/dir_Y2/dir_Y3/
>
> Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
> make a root snapshot under /.snap/root_snap. And every time when
> we make snapshots under /dir_Y1/..., the kclient will always try
> to rebuild the snap context for snap_X2 realm and finally will
> always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
> no sense.
>
> That's because the snap_X2's seq is 2 and root_snap's seq is 3.
> So when creating a new snapshot under /dir_Y1/... the new seq
> will be 4, and then the mds will send kclient a snapshot backtrace
> in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
> it will always rebuild the from the last realm, that's the root_snap.
> So later when rebuilding the snap context it will always rebuild
> the snap_X2 realm and then try to queue cap snaps for all the inodes
> related in snap_X2 realm, and we are seeing the logs like:
>
> "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
>
> URL: https://tracker.ceph.com/issues/44100
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/snap.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index d075d3ce5f6d..1f24a5de81e7 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>                 num += parent->cached_context->num_snaps;
>         }
>
> -       /* do i actually need to update?  not if my context seq
> -          matches realm seq, and my parents' does to.  (this works
> -          because we rebuild_snap_realms() works _downward_ in
> -          hierarchy after each update.) */
> +       /* do i actually need to update? No need when any of the following
> +        * two cases:
> +        * #1: if my context seq matches realm's seq and realm has no parent.
> +        * #2: if my context seq equals or is larger than my parent's, this
> +        *     works because we rebuild_snap_realms() works _downward_ in
> +        *     hierarchy after each update.
> +        */
>         if (realm->cached_context &&
> -           realm->cached_context->seq == realm->seq &&
> -           (!parent ||
> -            realm->cached_context->seq >= parent->cached_context->seq)) {
> +           ((realm->cached_context->seq == realm->seq && !parent) ||
> +            (parent && realm->cached_context->seq >= parent->cached_context->seq))) {

With this change. When you mksnap on  /dir_Y1/, its snap context keeps
unchanged. In ceph_update_snap_trace, reset the 'invalidate' variable
for each realm should fix this issue.

>                 dout("build_snap_context %llx %p: %p seq %lld (%u snaps),
>                      " (unchanged)\n",
>                      realm->ino, realm, realm->cached_context,
> --
> 2.27.0
>

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Jeff Layton]

On Thu, 2022-02-17 at 11:03 +0800, Yan, Zheng wrote:
> On Tue, Feb 15, 2022 at 11:04 PM <xiubli@redhat.com> wrote:
> > 
> > From: Xiubo Li <xiubli@redhat.com>
> > 
> > No need to update snapshot context when any of the following two
> > cases happens:
> > 1: if my context seq matches realm's seq and realm has no parent.
> > 2: if my context seq equals or is larger than my parent's, this
> >    works because we rebuild_snap_realms() works _downward_ in
> >    hierarchy after each update.
> > 
> > This fix will avoid those inodes which accidently calling
> > ceph_queue_cap_snap() and make no sense, for exmaple:
> > 
> > There have 6 directories like:
> > 
> > /dir_X1/dir_X2/dir_X3/
> > /dir_Y1/dir_Y2/dir_Y3/
> > 
> > Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
> > make a root snapshot under /.snap/root_snap. And every time when
> > we make snapshots under /dir_Y1/..., the kclient will always try
> > to rebuild the snap context for snap_X2 realm and finally will
> > always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
> > no sense.
> > 
> > That's because the snap_X2's seq is 2 and root_snap's seq is 3.
> > So when creating a new snapshot under /dir_Y1/... the new seq
> > will be 4, and then the mds will send kclient a snapshot backtrace
> > in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
> > it will always rebuild the from the last realm, that's the root_snap.
> > So later when rebuilding the snap context it will always rebuild
> > the snap_X2 realm and then try to queue cap snaps for all the inodes
> > related in snap_X2 realm, and we are seeing the logs like:
> > 
> > "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
> > 
> > URL: https://tracker.ceph.com/issues/44100
> > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > ---
> >  fs/ceph/snap.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> > index d075d3ce5f6d..1f24a5de81e7 100644
> > --- a/fs/ceph/snap.c
> > +++ b/fs/ceph/snap.c
> > @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
> >                 num += parent->cached_context->num_snaps;
> >         }
> > 
> > -       /* do i actually need to update?  not if my context seq
> > -          matches realm seq, and my parents' does to.  (this works
> > -          because we rebuild_snap_realms() works _downward_ in
> > -          hierarchy after each update.) */
> > +       /* do i actually need to update? No need when any of the following
> > +        * two cases:
> > +        * #1: if my context seq matches realm's seq and realm has no parent.
> > +        * #2: if my context seq equals or is larger than my parent's, this
> > +        *     works because we rebuild_snap_realms() works _downward_ in
> > +        *     hierarchy after each update.
> > +        */
> >         if (realm->cached_context &&
> > -           realm->cached_context->seq == realm->seq &&
> > -           (!parent ||
> > -            realm->cached_context->seq >= parent->cached_context->seq)) {
> > +           ((realm->cached_context->seq == realm->seq && !parent) ||
> > +            (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
> 
> With this change. When you mksnap on  /dir_Y1/, its snap context keeps
> unchanged. In ceph_update_snap_trace, reset the 'invalidate' variable
> for each realm should fix this issue.
> 

This comment is terribly vague. "invalidate" is a local variable in that
function and isn't set on a per-realm basis.

Could you suggest a patch on top of Xiubo's patch instead?


> >                 dout("build_snap_context %llx %p: %p seq %lld (%u snaps),
> >                      " (unchanged)\n",
> >                      realm->ino, realm, realm->cached_context,
> > --
> > 2.27.0
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Yan, Zheng]

On Thu, Feb 17, 2022 at 6:55 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Thu, 2022-02-17 at 11:03 +0800, Yan, Zheng wrote:
> > On Tue, Feb 15, 2022 at 11:04 PM <xiubli@redhat.com> wrote:
> > >
> > > From: Xiubo Li <xiubli@redhat.com>
> > >
> > > No need to update snapshot context when any of the following two
> > > cases happens:
> > > 1: if my context seq matches realm's seq and realm has no parent.
> > > 2: if my context seq equals or is larger than my parent's, this
> > >    works because we rebuild_snap_realms() works _downward_ in
> > >    hierarchy after each update.
> > >
> > > This fix will avoid those inodes which accidently calling
> > > ceph_queue_cap_snap() and make no sense, for exmaple:
> > >
> > > There have 6 directories like:
> > >
> > > /dir_X1/dir_X2/dir_X3/
> > > /dir_Y1/dir_Y2/dir_Y3/
> > >
> > > Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
> > > make a root snapshot under /.snap/root_snap. And every time when
> > > we make snapshots under /dir_Y1/..., the kclient will always try
> > > to rebuild the snap context for snap_X2 realm and finally will
> > > always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
> > > no sense.
> > >
> > > That's because the snap_X2's seq is 2 and root_snap's seq is 3.
> > > So when creating a new snapshot under /dir_Y1/... the new seq
> > > will be 4, and then the mds will send kclient a snapshot backtrace
> > > in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
> > > it will always rebuild the from the last realm, that's the root_snap.
> > > So later when rebuilding the snap context it will always rebuild
> > > the snap_X2 realm and then try to queue cap snaps for all the inodes
> > > related in snap_X2 realm, and we are seeing the logs like:
> > >
> > > "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
> > >
> > > URL: https://tracker.ceph.com/issues/44100
> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > ---
> > >  fs/ceph/snap.c | 16 +++++++++-------
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> > > index d075d3ce5f6d..1f24a5de81e7 100644
> > > --- a/fs/ceph/snap.c
> > > +++ b/fs/ceph/snap.c
> > > @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
> > >                 num += parent->cached_context->num_snaps;
> > >         }
> > >
> > > -       /* do i actually need to update?  not if my context seq
> > > -          matches realm seq, and my parents' does to.  (this works
> > > -          because we rebuild_snap_realms() works _downward_ in
> > > -          hierarchy after each update.) */
> > > +       /* do i actually need to update? No need when any of the following
> > > +        * two cases:
> > > +        * #1: if my context seq matches realm's seq and realm has no parent.
> > > +        * #2: if my context seq equals or is larger than my parent's, this
> > > +        *     works because we rebuild_snap_realms() works _downward_ in
> > > +        *     hierarchy after each update.
> > > +        */
> > >         if (realm->cached_context &&
> > > -           realm->cached_context->seq == realm->seq &&
> > > -           (!parent ||
> > > -            realm->cached_context->seq >= parent->cached_context->seq)) {
> > > +           ((realm->cached_context->seq == realm->seq && !parent) ||
> > > +            (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
> >
> > With this change. When you mksnap on  /dir_Y1/, its snap context keeps
> > unchanged. In ceph_update_snap_trace, reset the 'invalidate' variable
> > for each realm should fix this issue.
> >
>
> This comment is terribly vague. "invalidate" is a local variable in that
> function and isn't set on a per-realm basis.
>
> Could you suggest a patch on top of Xiubo's patch instead?
>

something like this (not tested)

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index af502a8245f0..6ef41764008b 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -704,7 +704,8 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
        __le64 *prior_parent_snaps;        /* encoded */
        struct ceph_snap_realm *realm = NULL;
        struct ceph_snap_realm *first_realm = NULL;
-       int invalidate = 0;
+       struct ceph_snap_realm *realm_to_inval = NULL;
+       int invalidate;
        int err = -ENOMEM;
        LIST_HEAD(dirty_realms);

@@ -712,6 +713,7 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,

        dout("update_snap_trace deletion=%d\n", deletion);
 more:
+       invalidate = 0;
        ceph_decode_need(&p, e, sizeof(*ri), bad);
        ri = p;
        p += sizeof(*ri);
@@ -774,8 +776,10 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
             realm, invalidate, p, e);

        /* invalidate when we reach the _end_ (root) of the trace */
-       if (invalidate && p >= e)
-               rebuild_snap_realms(realm, &dirty_realms);
+       if (invalidate)
+               realm_to_inval = realm;
+       if (realm_to_inval && p >= e)
+               rebuild_snap_realms(realm_to_inval, &dirty_realms);

        if (!first_realm)
                first_realm = realm;



>
> > >                 dout("build_snap_context %llx %p: %p seq %lld (%u snaps),
> > >                      " (unchanged)\n",
> > >                      realm->ino, realm, realm->cached_context,
> > > --
> > > 2.27.0
> > >
>
> --
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/3] ceph: do no update snapshot context when there is no new snapshot

[Xiubo Li]

On 2/17/22 11:28 PM, Yan, Zheng wrote:
> On Thu, Feb 17, 2022 at 6:55 PM Jeff Layton <jlayton@kernel.org> wrote:
>> On Thu, 2022-02-17 at 11:03 +0800, Yan, Zheng wrote:
>>> On Tue, Feb 15, 2022 at 11:04 PM <xiubli@redhat.com> wrote:
>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>
>>>> No need to update snapshot context when any of the following two
>>>> cases happens:
>>>> 1: if my context seq matches realm's seq and realm has no parent.
>>>> 2: if my context seq equals or is larger than my parent's, this
>>>>     works because we rebuild_snap_realms() works _downward_ in
>>>>     hierarchy after each update.
>>>>
>>>> This fix will avoid those inodes which accidently calling
>>>> ceph_queue_cap_snap() and make no sense, for exmaple:
>>>>
>>>> There have 6 directories like:
>>>>
>>>> /dir_X1/dir_X2/dir_X3/
>>>> /dir_Y1/dir_Y2/dir_Y3/
>>>>
>>>> Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then
>>>> make a root snapshot under /.snap/root_snap. And every time when
>>>> we make snapshots under /dir_Y1/..., the kclient will always try
>>>> to rebuild the snap context for snap_X2 realm and finally will
>>>> always try to queue cap snaps for dir_Y2 and dir_Y3, which makes
>>>> no sense.
>>>>
>>>> That's because the snap_X2's seq is 2 and root_snap's seq is 3.
>>>> So when creating a new snapshot under /dir_Y1/... the new seq
>>>> will be 4, and then the mds will send kclient a snapshot backtrace
>>>> in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace()
>>>> it will always rebuild the from the last realm, that's the root_snap.
>>>> So later when rebuilding the snap context it will always rebuild
>>>> the snap_X2 realm and then try to queue cap snaps for all the inodes
>>>> related in snap_X2 realm, and we are seeing the logs like:
>>>>
>>>> "ceph:  queue_cap_snap 00000000a42b796b nothing dirty|writing"
>>>>
>>>> URL: https://tracker.ceph.com/issues/44100
>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>> ---
>>>>   fs/ceph/snap.c | 16 +++++++++-------
>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
>>>> index d075d3ce5f6d..1f24a5de81e7 100644
>>>> --- a/fs/ceph/snap.c
>>>> +++ b/fs/ceph/snap.c
>>>> @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>>>>                  num += parent->cached_context->num_snaps;
>>>>          }
>>>>
>>>> -       /* do i actually need to update?  not if my context seq
>>>> -          matches realm seq, and my parents' does to.  (this works
>>>> -          because we rebuild_snap_realms() works _downward_ in
>>>> -          hierarchy after each update.) */
>>>> +       /* do i actually need to update? No need when any of the following
>>>> +        * two cases:
>>>> +        * #1: if my context seq matches realm's seq and realm has no parent.
>>>> +        * #2: if my context seq equals or is larger than my parent's, this
>>>> +        *     works because we rebuild_snap_realms() works _downward_ in
>>>> +        *     hierarchy after each update.
>>>> +        */
>>>>          if (realm->cached_context &&
>>>> -           realm->cached_context->seq == realm->seq &&
>>>> -           (!parent ||
>>>> -            realm->cached_context->seq >= parent->cached_context->seq)) {
>>>> +           ((realm->cached_context->seq == realm->seq && !parent) ||
>>>> +            (parent && realm->cached_context->seq >= parent->cached_context->seq))) {
>>> With this change. When you mksnap on  /dir_Y1/, its snap context keeps
>>> unchanged. In ceph_update_snap_trace, reset the 'invalidate' variable
>>> for each realm should fix this issue.
>>>
Thanks Zheng for your feedback.

Yeah, there has one case this will happen. Your approach is simpler I 
will post a V2 for this.

-- Xiubo






>> This comment is terribly vague. "invalidate" is a local variable in that
>> function and isn't set on a per-realm basis.
>>
>> Could you suggest a patch on top of Xiubo's patch instead?
>>
> something like this (not tested)
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index af502a8245f0..6ef41764008b 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -704,7 +704,8 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
>          __le64 *prior_parent_snaps;        /* encoded */
>          struct ceph_snap_realm *realm = NULL;
>          struct ceph_snap_realm *first_realm = NULL;
> -       int invalidate = 0;
> +       struct ceph_snap_realm *realm_to_inval = NULL;
> +       int invalidate;
>          int err = -ENOMEM;
>          LIST_HEAD(dirty_realms);
>
> @@ -712,6 +713,7 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
>
>          dout("update_snap_trace deletion=%d\n", deletion);
>   more:
> +       invalidate = 0;
>          ceph_decode_need(&p, e, sizeof(*ri), bad);
>          ri = p;
>          p += sizeof(*ri);
> @@ -774,8 +776,10 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
>               realm, invalidate, p, e);
>
>          /* invalidate when we reach the _end_ (root) of the trace */
> -       if (invalidate && p >= e)
> -               rebuild_snap_realms(realm, &dirty_realms);
> +       if (invalidate)
> +               realm_to_inval = realm;
> +       if (realm_to_inval && p >= e)
> +               rebuild_snap_realms(realm_to_inval, &dirty_realms);
>
>          if (!first_realm)
>                  first_realm = realm;
>
>
>
>>>>                  dout("build_snap_context %llx %p: %p seq %lld (%u snaps),
>>>>                       " (unchanged)\n",
>>>>                       realm->ino, realm, realm->cached_context,
>>>> --
>>>> 2.27.0
>>>>
>> --
>> Jeff Layton <jlayton@kernel.org>