diff mbox series

[v2] ceph: eliminate the recursion when rebuilding the snap context

Message ID 20220219065617.43718-1-xiubli@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] ceph: eliminate the recursion when rebuilding the snap context | expand

Commit Message

Xiubo Li Feb. 19, 2022, 6:56 a.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

Use a list instead of recursion to avoid possible stack overflow.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---

V2:
- Do not insert the child realms when building snapc for the parents


 fs/ceph/snap.c  | 57 +++++++++++++++++++++++++++++++++++++++++--------
 fs/ceph/super.h |  2 ++
 2 files changed, 50 insertions(+), 9 deletions(-)

Comments

Jeff Layton Feb. 19, 2022, 12:57 p.m. UTC | #1 
On Sat, 2022-02-19 at 14:56 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> Use a list instead of recursion to avoid possible stack overflow.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
> 
> V2:
> - Do not insert the child realms when building snapc for the parents
> 
> 
>  fs/ceph/snap.c  | 57 +++++++++++++++++++++++++++++++++++++++++--------
>  fs/ceph/super.h |  2 ++
>  2 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index bc5ec72d958c..722ddd166013 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -127,6 +127,7 @@ static struct ceph_snap_realm *ceph_create_snap_realm(
>  	INIT_LIST_HEAD(&realm->child_item);
>  	INIT_LIST_HEAD(&realm->empty_item);
>  	INIT_LIST_HEAD(&realm->dirty_item);
> +	INIT_LIST_HEAD(&realm->rebuild_item);
>  	INIT_LIST_HEAD(&realm->inodes_with_caps);
>  	spin_lock_init(&realm->inodes_with_caps_lock);
>  	__insert_snap_realm(&mdsc->snap_realms, realm);
> @@ -320,7 +321,8 @@ static int cmpu64_rev(const void *a, const void *b)
>   * build the snap context for a given realm.
>   */
>  static int build_snap_context(struct ceph_snap_realm *realm,
> -			      struct list_head* dirty_realms)
> +			      struct list_head *realm_queue,
> +			      struct list_head *dirty_realms)
>  {
>  	struct ceph_snap_realm *parent = realm->parent;
>  	struct ceph_snap_context *snapc;
> @@ -334,9 +336,9 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>  	 */
>  	if (parent) {
>  		if (!parent->cached_context) {
> -			err = build_snap_context(parent, dirty_realms);
> -			if (err)
> -				goto fail;
> +			/* add to the queue head */
> +			list_add(&parent->rebuild_item, realm_queue);
> +			return 1;
>  		}
>  		num += parent->cached_context->num_snaps;
>  	}
> @@ -420,13 +422,50 @@ static int build_snap_context(struct ceph_snap_realm *realm,
>  static void rebuild_snap_realms(struct ceph_snap_realm *realm,
>  				struct list_head *dirty_realms)
>  {
> -	struct ceph_snap_realm *child;
> +	LIST_HEAD(realm_queue);
> +	int last = 0;
> +	bool skip = false;
>  
> -	dout("rebuild_snap_realms %llx %p\n", realm->ino, realm);
> -	build_snap_context(realm, dirty_realms);
> +	list_add_tail(&realm->rebuild_item, &realm_queue);
>  
> -	list_for_each_entry(child, &realm->children, child_item)
> -		rebuild_snap_realms(child, dirty_realms);
> +	while (!list_empty(&realm_queue)) {
> +		struct ceph_snap_realm *_realm, *child;
> +
> +		_realm = list_first_entry(&realm_queue,
> +					  struct ceph_snap_realm,
> +					  rebuild_item);
> +
> +		/*
> +		 * If the last building failed dues to memory
> +		 * issue, just empty the realm_queue and return
> +		 * to avoid infinite loop.
> +		 */
> +		if (last < 0) {
> +			list_del_init(&_realm->rebuild_item);
> +			continue;
> +		}
> +
> +		last = build_snap_context(_realm, &realm_queue, dirty_realms);
> +		dout("rebuild_snap_realms %llx %p, %s\n", _realm->ino, _realm,
> +		     last > 0 ? "is deferred" : !last ? "succeeded" : "failed");
> +
> +		/* is any child in the list ? */
> +		list_for_each_entry(child, &_realm->children, child_item) {
> +			if (!list_empty(&child->rebuild_item)) {
> +				skip = true;
> +				break;
> +			}
> +		}
> +
> +		if (!skip) {
> +			list_for_each_entry(child, &_realm->children, child_item)
> +				list_add_tail(&child->rebuild_item, &realm_queue);
> +		}
> +
> +		/* last == 1 means need to build parent first */
> +		if (last <= 0)
> +			list_del_init(&_realm->rebuild_item);
> +	}
>  }
>  
>  
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index a17bd01a8957..baac800a6d11 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -885,6 +885,8 @@ struct ceph_snap_realm {
>  
>  	struct list_head dirty_item;     /* if realm needs new context */
>  
> +	struct list_head rebuild_item;   /* rebuild snap realms _downward_ in hierarchy */
> +
>  	/* the current set of snaps for this realm */
>  	struct ceph_snap_context *cached_context;
>  

Looks good. I dropped the one in the testing branch and merged this one.
diff mbox series

Patch

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index bc5ec72d958c..722ddd166013 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -127,6 +127,7 @@  static struct ceph_snap_realm *ceph_create_snap_realm(
 	INIT_LIST_HEAD(&realm->child_item);
 	INIT_LIST_HEAD(&realm->empty_item);
 	INIT_LIST_HEAD(&realm->dirty_item);
+	INIT_LIST_HEAD(&realm->rebuild_item);
 	INIT_LIST_HEAD(&realm->inodes_with_caps);
 	spin_lock_init(&realm->inodes_with_caps_lock);
 	__insert_snap_realm(&mdsc->snap_realms, realm);
@@ -320,7 +321,8 @@  static int cmpu64_rev(const void *a, const void *b)
  * build the snap context for a given realm.
  */
 static int build_snap_context(struct ceph_snap_realm *realm,
-			      struct list_head* dirty_realms)
+			      struct list_head *realm_queue,
+			      struct list_head *dirty_realms)
 {
 	struct ceph_snap_realm *parent = realm->parent;
 	struct ceph_snap_context *snapc;
@@ -334,9 +336,9 @@  static int build_snap_context(struct ceph_snap_realm *realm,
 	 */
 	if (parent) {
 		if (!parent->cached_context) {
-			err = build_snap_context(parent, dirty_realms);
-			if (err)
-				goto fail;
+			/* add to the queue head */
+			list_add(&parent->rebuild_item, realm_queue);
+			return 1;
 		}
 		num += parent->cached_context->num_snaps;
 	}
@@ -420,13 +422,50 @@  static int build_snap_context(struct ceph_snap_realm *realm,
 static void rebuild_snap_realms(struct ceph_snap_realm *realm,
 				struct list_head *dirty_realms)
 {
-	struct ceph_snap_realm *child;
+	LIST_HEAD(realm_queue);
+	int last = 0;
+	bool skip = false;
 
-	dout("rebuild_snap_realms %llx %p\n", realm->ino, realm);
-	build_snap_context(realm, dirty_realms);
+	list_add_tail(&realm->rebuild_item, &realm_queue);
 
-	list_for_each_entry(child, &realm->children, child_item)
-		rebuild_snap_realms(child, dirty_realms);
+	while (!list_empty(&realm_queue)) {
+		struct ceph_snap_realm *_realm, *child;
+
+		_realm = list_first_entry(&realm_queue,
+					  struct ceph_snap_realm,
+					  rebuild_item);
+
+		/*
+		 * If the last building failed dues to memory
+		 * issue, just empty the realm_queue and return
+		 * to avoid infinite loop.
+		 */
+		if (last < 0) {
+			list_del_init(&_realm->rebuild_item);
+			continue;
+		}
+
+		last = build_snap_context(_realm, &realm_queue, dirty_realms);
+		dout("rebuild_snap_realms %llx %p, %s\n", _realm->ino, _realm,
+		     last > 0 ? "is deferred" : !last ? "succeeded" : "failed");
+
+		/* is any child in the list ? */
+		list_for_each_entry(child, &_realm->children, child_item) {
+			if (!list_empty(&child->rebuild_item)) {
+				skip = true;
+				break;
+			}
+		}
+
+		if (!skip) {
+			list_for_each_entry(child, &_realm->children, child_item)
+				list_add_tail(&child->rebuild_item, &realm_queue);
+		}
+
+		/* last == 1 means need to build parent first */
+		if (last <= 0)
+			list_del_init(&_realm->rebuild_item);
+	}
 }
 
 
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index a17bd01a8957..baac800a6d11 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -885,6 +885,8 @@  struct ceph_snap_realm {
 
 	struct list_head dirty_item;     /* if realm needs new context */
 
+	struct list_head rebuild_item;   /* rebuild snap realms _downward_ in hierarchy */
+
 	/* the current set of snaps for this realm */
 	struct ceph_snap_context *cached_context;