[PATCH] Improving readability of copy_tree

[PATCH] Improving readability of copy_tree

[Jemmy]

Hello everyone,

I'm new to Linux kernel development
and excited to make my first contribution.
While working with the copy_tree function,
I noticed some unclear variable names e.g., p, q, r.
I've updated them to be more descriptive,
aiming to make the code easier to understand.

Changes:

p       -> o_parent, old parent
q       -> n_mnt, new mount
r       -> o_mnt, old child
s       -> o_child, old child
parent  -> n_parent, new parent

Thanks for the opportunity to be part of this community!

BR,
Jemmy

Signed-off-by: Jemmy <jemmywong512@gmail.com>
---
 fs/namespace.c | 51 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5a51315c6678..b1cf95ddfb87 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1969,7 +1969,7 @@ static bool mnt_ns_loop(struct dentry *dentry)
 struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
 					int flag)
 {
-	struct mount *res, *p, *q, *r, *parent;
+	struct mount *res, *o_parent, *o_child, *o_mnt, *n_parent, *n_mnt;
 
 	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
 		return ERR_PTR(-EINVAL);
@@ -1977,47 +1977,46 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
 	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
 		return ERR_PTR(-EINVAL);
 
-	res = q = clone_mnt(mnt, dentry, flag);
-	if (IS_ERR(q))
-		return q;
+	res = n_mnt = clone_mnt(mnt, dentry, flag);
+	if (IS_ERR(n_mnt))
+		return n_mnt;
 
-	q->mnt_mountpoint = mnt->mnt_mountpoint;
+	n_mnt->mnt_mountpoint = mnt->mnt_mountpoint;
 
-	p = mnt;
-	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
-		struct mount *s;
-		if (!is_subdir(r->mnt_mountpoint, dentry))
+	o_parent = mnt;
+	list_for_each_entry(o_mnt, &mnt->mnt_mounts, mnt_child) {
+		if (!is_subdir(o_mnt->mnt_mountpoint, dentry))
 			continue;
 
-		for (s = r; s; s = next_mnt(s, r)) {
+		for (o_child = o_mnt; o_child; o_child = next_mnt(o_child, o_mnt)) {
 			if (!(flag & CL_COPY_UNBINDABLE) &&
-			    IS_MNT_UNBINDABLE(s)) {
-				if (s->mnt.mnt_flags & MNT_LOCKED) {
+			    IS_MNT_UNBINDABLE(o_child)) {
+				if (o_child->mnt.mnt_flags & MNT_LOCKED) {
 					/* Both unbindable and locked. */
-					q = ERR_PTR(-EPERM);
+					n_mnt = ERR_PTR(-EPERM);
 					goto out;
 				} else {
-					s = skip_mnt_tree(s);
+					o_child = skip_mnt_tree(o_child);
 					continue;
 				}
 			}
 			if (!(flag & CL_COPY_MNT_NS_FILE) &&
-			    is_mnt_ns_file(s->mnt.mnt_root)) {
-				s = skip_mnt_tree(s);
+			    is_mnt_ns_file(o_child->mnt.mnt_root)) {
+				o_child = skip_mnt_tree(o_child);
 				continue;
 			}
-			while (p != s->mnt_parent) {
-				p = p->mnt_parent;
-				q = q->mnt_parent;
+			while (o_parent != o_child->mnt_parent) {
+				o_parent = o_parent->mnt_parent;
+				n_mnt = n_mnt->mnt_parent;
 			}
-			p = s;
-			parent = q;
-			q = clone_mnt(p, p->mnt.mnt_root, flag);
-			if (IS_ERR(q))
+			o_parent = o_child;
+			n_parent = n_mnt;
+			n_mnt = clone_mnt(o_parent, o_parent->mnt.mnt_root, flag);
+			if (IS_ERR(n_mnt))
 				goto out;
 			lock_mount_hash();
-			list_add_tail(&q->mnt_list, &res->mnt_list);
-			attach_mnt(q, parent, p->mnt_mp, false);
+			list_add_tail(&n_mnt->mnt_list, &res->mnt_list);
+			attach_mnt(n_mnt, n_parent, o_parent->mnt_mp, false);
 			unlock_mount_hash();
 		}
 	}
@@ -2028,7 +2027,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
 		umount_tree(res, UMOUNT_SYNC);
 		unlock_mount_hash();
 	}
-	return q;
+	return n_mnt;
 }
 
 /* Caller should check returned pointer for errors */
-- 
2.34.1

Re: [PATCH] Improving readability of copy_tree

[Christian Brauner]

On Tue, Jun 04, 2024 at 09:43:47PM +0800, Jemmy wrote:
> Hello everyone,
> 
> I'm new to Linux kernel development
> and excited to make my first contribution.
> While working with the copy_tree function,
> I noticed some unclear variable names e.g., p, q, r.
> I've updated them to be more descriptive,
> aiming to make the code easier to understand.
> 
> Changes:
> 
> p       -> o_parent, old parent
> q       -> n_mnt, new mount
> r       -> o_mnt, old child
> s       -> o_child, old child
> parent  -> n_parent, new parent

Hey, seems worth to me but if so please spell out "old_*" and "new_*".

Re: [PATCH] Improving readability of copy_tree

[Jan Kara]

Hello!

On Tue 04-06-24 21:43:47, Jemmy wrote:
> I'm new to Linux kernel development
> and excited to make my first contribution.
> While working with the copy_tree function,
> I noticed some unclear variable names e.g., p, q, r.
> I've updated them to be more descriptive,
> aiming to make the code easier to understand.
> 
> Changes:
> 
> p       -> o_parent, old parent
> q       -> n_mnt, new mount
> r       -> o_mnt, old child
> s       -> o_child, old child
> parent  -> n_parent, new parent
> 
> Thanks for the opportunity to be part of this community!

So I agree more descriptive names would help readability of this code. But
I'd pick different names which IMHO better capture the purpose.

mnt -> root (root of the tree to copy)
r -> root_child (direct child of the root we are cloning)
s -> cur_mnt (current mount we are copying)
p -> cur_parent (parent of cur_mnt)
q -> cloned_mnt (freshly cloned mount)
parent -> cloned_parent

								Honza

> Signed-off-by: Jemmy <jemmywong512@gmail.com>
> ---
>  fs/namespace.c | 51 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5a51315c6678..b1cf95ddfb87 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1969,7 +1969,7 @@ static bool mnt_ns_loop(struct dentry *dentry)
>  struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
>  					int flag)
>  {
> -	struct mount *res, *p, *q, *r, *parent;
> +	struct mount *res, *o_parent, *o_child, *o_mnt, *n_parent, *n_mnt;
>  
>  	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
>  		return ERR_PTR(-EINVAL);
> @@ -1977,47 +1977,46 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
>  	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
>  		return ERR_PTR(-EINVAL);
>  
> -	res = q = clone_mnt(mnt, dentry, flag);
> -	if (IS_ERR(q))
> -		return q;
> +	res = n_mnt = clone_mnt(mnt, dentry, flag);
> +	if (IS_ERR(n_mnt))
> +		return n_mnt;
>  
> -	q->mnt_mountpoint = mnt->mnt_mountpoint;
> +	n_mnt->mnt_mountpoint = mnt->mnt_mountpoint;
>  
> -	p = mnt;
> -	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
> -		struct mount *s;
> -		if (!is_subdir(r->mnt_mountpoint, dentry))
> +	o_parent = mnt;
> +	list_for_each_entry(o_mnt, &mnt->mnt_mounts, mnt_child) {
> +		if (!is_subdir(o_mnt->mnt_mountpoint, dentry))
>  			continue;
>  
> -		for (s = r; s; s = next_mnt(s, r)) {
> +		for (o_child = o_mnt; o_child; o_child = next_mnt(o_child, o_mnt)) {
>  			if (!(flag & CL_COPY_UNBINDABLE) &&
> -			    IS_MNT_UNBINDABLE(s)) {
> -				if (s->mnt.mnt_flags & MNT_LOCKED) {
> +			    IS_MNT_UNBINDABLE(o_child)) {
> +				if (o_child->mnt.mnt_flags & MNT_LOCKED) {
>  					/* Both unbindable and locked. */
> -					q = ERR_PTR(-EPERM);
> +					n_mnt = ERR_PTR(-EPERM);
>  					goto out;
>  				} else {
> -					s = skip_mnt_tree(s);
> +					o_child = skip_mnt_tree(o_child);
>  					continue;
>  				}
>  			}
>  			if (!(flag & CL_COPY_MNT_NS_FILE) &&
> -			    is_mnt_ns_file(s->mnt.mnt_root)) {
> -				s = skip_mnt_tree(s);
> +			    is_mnt_ns_file(o_child->mnt.mnt_root)) {
> +				o_child = skip_mnt_tree(o_child);
>  				continue;
>  			}
> -			while (p != s->mnt_parent) {
> -				p = p->mnt_parent;
> -				q = q->mnt_parent;
> +			while (o_parent != o_child->mnt_parent) {
> +				o_parent = o_parent->mnt_parent;
> +				n_mnt = n_mnt->mnt_parent;
>  			}
> -			p = s;
> -			parent = q;
> -			q = clone_mnt(p, p->mnt.mnt_root, flag);
> -			if (IS_ERR(q))
> +			o_parent = o_child;
> +			n_parent = n_mnt;
> +			n_mnt = clone_mnt(o_parent, o_parent->mnt.mnt_root, flag);
> +			if (IS_ERR(n_mnt))
>  				goto out;
>  			lock_mount_hash();
> -			list_add_tail(&q->mnt_list, &res->mnt_list);
> -			attach_mnt(q, parent, p->mnt_mp, false);
> +			list_add_tail(&n_mnt->mnt_list, &res->mnt_list);
> +			attach_mnt(n_mnt, n_parent, o_parent->mnt_mp, false);
>  			unlock_mount_hash();
>  		}
>  	}
> @@ -2028,7 +2027,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
>  		umount_tree(res, UMOUNT_SYNC);
>  		unlock_mount_hash();
>  	}
> -	return q;
> +	return n_mnt;
>  }
>  
>  /* Caller should check returned pointer for errors */
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] Improving readability of copy_tree

[Waiman Long]

On 6/4/24 09:43, Jemmy wrote:
> Hello everyone,
>
> I'm new to Linux kernel development
> and excited to make my first contribution.
> While working with the copy_tree function,
> I noticed some unclear variable names e.g., p, q, r.
> I've updated them to be more descriptive,
> aiming to make the code easier to understand.
>
> Changes:
>
> p       -> o_parent, old parent
> q       -> n_mnt, new mount
> r       -> o_mnt, old child
> s       -> o_child, old child
> parent  -> n_parent, new parent
>
> Thanks for the opportunity to be part of this community!
>
> BR,
> Jemmy

I don't know why I am on the to-list as I haven't worked on this file 
before. Anyway, making meaning name is helpful, but I believe a 
functional level documentation on top of copy_tree() to explain what 
this function tries to accomplish can be helpful too.

Cheers,
Longman

>
> Signed-off-by: Jemmy <jemmywong512@gmail.com>
> ---
>   fs/namespace.c | 51 +++++++++++++++++++++++++-------------------------
>   1 file changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5a51315c6678..b1cf95ddfb87 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1969,7 +1969,7 @@ static bool mnt_ns_loop(struct dentry *dentry)
>   struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
>   					int flag)
>   {
> -	struct mount *res, *p, *q, *r, *parent;
> +	struct mount *res, *o_parent, *o_child, *o_mnt, *n_parent, *n_mnt;
>   
>   	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
>   		return ERR_PTR(-EINVAL);
> @@ -1977,47 +1977,46 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
>   	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
>   		return ERR_PTR(-EINVAL);
>   
> -	res = q = clone_mnt(mnt, dentry, flag);
> -	if (IS_ERR(q))
> -		return q;
> +	res = n_mnt = clone_mnt(mnt, dentry, flag);
> +	if (IS_ERR(n_mnt))
> +		return n_mnt;
>   
> -	q->mnt_mountpoint = mnt->mnt_mountpoint;
> +	n_mnt->mnt_mountpoint = mnt->mnt_mountpoint;
>   
> -	p = mnt;
> -	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
> -		struct mount *s;
> -		if (!is_subdir(r->mnt_mountpoint, dentry))
> +	o_parent = mnt;
> +	list_for_each_entry(o_mnt, &mnt->mnt_mounts, mnt_child) {
> +		if (!is_subdir(o_mnt->mnt_mountpoint, dentry))
>   			continue;
>   
> -		for (s = r; s; s = next_mnt(s, r)) {
> +		for (o_child = o_mnt; o_child; o_child = next_mnt(o_child, o_mnt)) {
>   			if (!(flag & CL_COPY_UNBINDABLE) &&
> -			    IS_MNT_UNBINDABLE(s)) {
> -				if (s->mnt.mnt_flags & MNT_LOCKED) {
> +			    IS_MNT_UNBINDABLE(o_child)) {
> +				if (o_child->mnt.mnt_flags & MNT_LOCKED) {
>   					/* Both unbindable and locked. */
> -					q = ERR_PTR(-EPERM);
> +					n_mnt = ERR_PTR(-EPERM);
>   					goto out;
>   				} else {
> -					s = skip_mnt_tree(s);
> +					o_child = skip_mnt_tree(o_child);
>   					continue;
>   				}
>   			}
>   			if (!(flag & CL_COPY_MNT_NS_FILE) &&
> -			    is_mnt_ns_file(s->mnt.mnt_root)) {
> -				s = skip_mnt_tree(s);
> +			    is_mnt_ns_file(o_child->mnt.mnt_root)) {
> +				o_child = skip_mnt_tree(o_child);
>   				continue;
>   			}
> -			while (p != s->mnt_parent) {
> -				p = p->mnt_parent;
> -				q = q->mnt_parent;
> +			while (o_parent != o_child->mnt_parent) {
> +				o_parent = o_parent->mnt_parent;
> +				n_mnt = n_mnt->mnt_parent;
>   			}
> -			p = s;
> -			parent = q;
> -			q = clone_mnt(p, p->mnt.mnt_root, flag);
> -			if (IS_ERR(q))
> +			o_parent = o_child;
> +			n_parent = n_mnt;
> +			n_mnt = clone_mnt(o_parent, o_parent->mnt.mnt_root, flag);
> +			if (IS_ERR(n_mnt))
>   				goto out;
>   			lock_mount_hash();
> -			list_add_tail(&q->mnt_list, &res->mnt_list);
> -			attach_mnt(q, parent, p->mnt_mp, false);
> +			list_add_tail(&n_mnt->mnt_list, &res->mnt_list);
> +			attach_mnt(n_mnt, n_parent, o_parent->mnt_mp, false);
>   			unlock_mount_hash();
>   		}
>   	}
> @@ -2028,7 +2027,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
>   		umount_tree(res, UMOUNT_SYNC);
>   		unlock_mount_hash();
>   	}
> -	return q;
> +	return n_mnt;
>   }
>   
>   /* Caller should check returned pointer for errors */

[PATCH v2] Employ `copy mount tree from src to dst` concept in copy_tree

[Jemmy]

Variable names in copy_tree (e.g., p, q, r, s) are opaque;
renaming them to be more descriptive
would aim to make the code easier to understand.

V2 Changes:
mnt 	-> src_root (root of the tree to copy)
r 		-> src_child (direct child of the root being cloning)
p 		-> src_parent (parent of src_mnt)
s 		-> src_mnt (current mount being copying)
parent	-> dst_parent (parent of dst_child)
q 		-> dst_mnt (freshly cloned mount)

Signed-off-by: Jemmy <jemmywong512@gmail.com>
---
 fs/namespace.c | 59 ++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5a51315c6678..38048b9b6c6c 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1966,69 +1966,72 @@ static bool mnt_ns_loop(struct dentry *dentry)
 	return current->nsproxy->mnt_ns->seq >= mnt_ns->seq;
 }
 
-struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
+struct mount *copy_tree(struct mount *src_root, struct dentry *dentry,
 					int flag)
 {
-	struct mount *res, *p, *q, *r, *parent;
+	struct mount *res, *src_parent, *src_child, *src_mnt,
+		*dst_parent, *dst_mnt;
 
-	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
+	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(src_root))
 		return ERR_PTR(-EINVAL);
 
 	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
 		return ERR_PTR(-EINVAL);
 
-	res = q = clone_mnt(mnt, dentry, flag);
-	if (IS_ERR(q))
-		return q;
+	res = dst_mnt = clone_mnt(src_root, dentry, flag);
+	if (IS_ERR(dst_mnt))
+		return dst_mnt;
 
-	q->mnt_mountpoint = mnt->mnt_mountpoint;
+	src_parent = src_root;
+	dst_mnt->mnt_mountpoint = src_root->mnt_mountpoint;
 
-	p = mnt;
-	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
-		struct mount *s;
-		if (!is_subdir(r->mnt_mountpoint, dentry))
+	list_for_each_entry(src_child, &src_root->mnt_mounts, mnt_child) {
+		if (!is_subdir(src_child->mnt_mountpoint, dentry))
 			continue;
 
-		for (s = r; s; s = next_mnt(s, r)) {
+		for (src_mnt = src_child; src_mnt;
+			src_mnt = next_mnt(src_mnt, src_child)) {
 			if (!(flag & CL_COPY_UNBINDABLE) &&
-			    IS_MNT_UNBINDABLE(s)) {
-				if (s->mnt.mnt_flags & MNT_LOCKED) {
+				IS_MNT_UNBINDABLE(src_mnt)) {
+				if (src_mnt->mnt.mnt_flags & MNT_LOCKED) {
 					/* Both unbindable and locked. */
-					q = ERR_PTR(-EPERM);
+					dst_mnt = ERR_PTR(-EPERM);
 					goto out;
 				} else {
-					s = skip_mnt_tree(s);
+					src_mnt = skip_mnt_tree(src_mnt);
 					continue;
 				}
 			}
 			if (!(flag & CL_COPY_MNT_NS_FILE) &&
-			    is_mnt_ns_file(s->mnt.mnt_root)) {
-				s = skip_mnt_tree(s);
+				is_mnt_ns_file(src_mnt->mnt.mnt_root)) {
+				src_mnt = skip_mnt_tree(src_mnt);
 				continue;
 			}
-			while (p != s->mnt_parent) {
-				p = p->mnt_parent;
-				q = q->mnt_parent;
+			while (src_parent != src_mnt->mnt_parent) {
+				src_parent = src_parent->mnt_parent;
+				dst_mnt = dst_mnt->mnt_parent;
 			}
-			p = s;
-			parent = q;
-			q = clone_mnt(p, p->mnt.mnt_root, flag);
-			if (IS_ERR(q))
+
+			src_parent = src_mnt;
+			dst_parent = dst_mnt;
+			dst_mnt = clone_mnt(src_mnt, src_mnt->mnt.mnt_root, flag);
+			if (IS_ERR(dst_mnt))
 				goto out;
 			lock_mount_hash();
-			list_add_tail(&q->mnt_list, &res->mnt_list);
-			attach_mnt(q, parent, p->mnt_mp, false);
+			list_add_tail(&dst_mnt->mnt_list, &res->mnt_list);
+			attach_mnt(dst_mnt, dst_parent, src_parent->mnt_mp, false);
 			unlock_mount_hash();
 		}
 	}
 	return res;
+
 out:
 	if (res) {
 		lock_mount_hash();
 		umount_tree(res, UMOUNT_SYNC);
 		unlock_mount_hash();
 	}
-	return q;
+	return dst_mnt;
 }
 
 /* Caller should check returned pointer for errors */
-- 
2.34.1

[PATCH v3] Improve readability of copy_tree

[Jemmy]

by employing `copy mount tree from src to dst` concept.
This involves renaming the opaque variables (e.g., p, q, r, s)
to be more descriptive, aiming to make the code easier to understand.

Changes:
mnt     -> src_root (root of the tree to copy)
r       -> src_child (direct child of the root being cloning)
p       -> src_parent (parent of src_mnt)
s       -> src_mnt (current mount being copying)
parent  -> dst_parent (parent of dst_child)
q       -> dst_mnt (freshly cloned mount)

Signed-off-by: Jemmy <jemmywong512@gmail.com>
---
 fs/namespace.c | 59 ++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5a51315c6678..0dd43633607b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1966,69 +1966,72 @@ static bool mnt_ns_loop(struct dentry *dentry)
 	return current->nsproxy->mnt_ns->seq >= mnt_ns->seq;
 }
 
-struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
+struct mount *copy_tree(struct mount *src_root, struct dentry *dentry,
 					int flag)
 {
-	struct mount *res, *p, *q, *r, *parent;
+	struct mount *res, *src_parent, *src_child, *src_mnt,
+		*dst_parent, *dst_mnt;
 
-	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
+	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(src_root))
 		return ERR_PTR(-EINVAL);
 
 	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
 		return ERR_PTR(-EINVAL);
 
-	res = q = clone_mnt(mnt, dentry, flag);
-	if (IS_ERR(q))
-		return q;
+	res = dst_mnt = clone_mnt(src_root, dentry, flag);
+	if (IS_ERR(dst_mnt))
+		return dst_mnt;
 
-	q->mnt_mountpoint = mnt->mnt_mountpoint;
+	src_parent = src_root;
+	dst_mnt->mnt_mountpoint = src_root->mnt_mountpoint;
 
-	p = mnt;
-	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
-		struct mount *s;
-		if (!is_subdir(r->mnt_mountpoint, dentry))
+	list_for_each_entry(src_child, &src_root->mnt_mounts, mnt_child) {
+		if (!is_subdir(src_child->mnt_mountpoint, dentry))
 			continue;
 
-		for (s = r; s; s = next_mnt(s, r)) {
+		for (src_mnt = src_child; src_mnt;
+			src_mnt = next_mnt(src_mnt, src_child)) {
 			if (!(flag & CL_COPY_UNBINDABLE) &&
-			    IS_MNT_UNBINDABLE(s)) {
-				if (s->mnt.mnt_flags & MNT_LOCKED) {
+			    IS_MNT_UNBINDABLE(src_mnt)) {
+				if (src_mnt->mnt.mnt_flags & MNT_LOCKED) {
 					/* Both unbindable and locked. */
-					q = ERR_PTR(-EPERM);
+					dst_mnt = ERR_PTR(-EPERM);
 					goto out;
 				} else {
-					s = skip_mnt_tree(s);
+					src_mnt = skip_mnt_tree(src_mnt);
 					continue;
 				}
 			}
 			if (!(flag & CL_COPY_MNT_NS_FILE) &&
-			    is_mnt_ns_file(s->mnt.mnt_root)) {
-				s = skip_mnt_tree(s);
+			    is_mnt_ns_file(src_mnt->mnt.mnt_root)) {
+				src_mnt = skip_mnt_tree(src_mnt);
 				continue;
 			}
-			while (p != s->mnt_parent) {
-				p = p->mnt_parent;
-				q = q->mnt_parent;
+			while (src_parent != src_mnt->mnt_parent) {
+				src_parent = src_parent->mnt_parent;
+				dst_mnt = dst_mnt->mnt_parent;
 			}
-			p = s;
-			parent = q;
-			q = clone_mnt(p, p->mnt.mnt_root, flag);
-			if (IS_ERR(q))
+
+			src_parent = src_mnt;
+			dst_parent = dst_mnt;
+			dst_mnt = clone_mnt(src_mnt, src_mnt->mnt.mnt_root, flag);
+			if (IS_ERR(dst_mnt))
 				goto out;
 			lock_mount_hash();
-			list_add_tail(&q->mnt_list, &res->mnt_list);
-			attach_mnt(q, parent, p->mnt_mp, false);
+			list_add_tail(&dst_mnt->mnt_list, &res->mnt_list);
+			attach_mnt(dst_mnt, dst_parent, src_parent->mnt_mp, false);
 			unlock_mount_hash();
 		}
 	}
 	return res;
+
 out:
 	if (res) {
 		lock_mount_hash();
 		umount_tree(res, UMOUNT_SYNC);
 		unlock_mount_hash();
 	}
-	return q;
+	return dst_mnt;
 }
 
 /* Caller should check returned pointer for errors */
-- 
2.34.1

Re: [PATCH v3] Improve readability of copy_tree

[Jan Kara]

On Thu 06-06-24 17:02:54, Jemmy wrote:
> by employing `copy mount tree from src to dst` concept.
> This involves renaming the opaque variables (e.g., p, q, r, s)
> to be more descriptive, aiming to make the code easier to understand.
> 
> Changes:
> mnt     -> src_root (root of the tree to copy)
> r       -> src_child (direct child of the root being cloning)

I'd call this src_root_child to distinguish it from a child of
"src_parent". Otherwise these names work for me.

> p       -> src_parent (parent of src_mnt)
> s       -> src_mnt (current mount being copying)
> parent  -> dst_parent (parent of dst_child)
> q       -> dst_mnt (freshly cloned mount)
> 
> Signed-off-by: Jemmy <jemmywong512@gmail.com>

								Honza

> ---
>  fs/namespace.c | 59 ++++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5a51315c6678..0dd43633607b 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1966,69 +1966,72 @@ static bool mnt_ns_loop(struct dentry *dentry)
>  	return current->nsproxy->mnt_ns->seq >= mnt_ns->seq;
>  }
>  
> -struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
> +struct mount *copy_tree(struct mount *src_root, struct dentry *dentry,
>  					int flag)
>  {
> -	struct mount *res, *p, *q, *r, *parent;
> +	struct mount *res, *src_parent, *src_child, *src_mnt,
> +		*dst_parent, *dst_mnt;
>  
> -	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
> +	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(src_root))
>  		return ERR_PTR(-EINVAL);
>  
>  	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
>  		return ERR_PTR(-EINVAL);
>  
> -	res = q = clone_mnt(mnt, dentry, flag);
> -	if (IS_ERR(q))
> -		return q;
> +	res = dst_mnt = clone_mnt(src_root, dentry, flag);
> +	if (IS_ERR(dst_mnt))
> +		return dst_mnt;
>  
> -	q->mnt_mountpoint = mnt->mnt_mountpoint;
> +	src_parent = src_root;
> +	dst_mnt->mnt_mountpoint = src_root->mnt_mountpoint;
>  
> -	p = mnt;
> -	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
> -		struct mount *s;
> -		if (!is_subdir(r->mnt_mountpoint, dentry))
> +	list_for_each_entry(src_child, &src_root->mnt_mounts, mnt_child) {
> +		if (!is_subdir(src_child->mnt_mountpoint, dentry))
>  			continue;
>  
> -		for (s = r; s; s = next_mnt(s, r)) {
> +		for (src_mnt = src_child; src_mnt;
> +			src_mnt = next_mnt(src_mnt, src_child)) {
>  			if (!(flag & CL_COPY_UNBINDABLE) &&
> -			    IS_MNT_UNBINDABLE(s)) {
> -				if (s->mnt.mnt_flags & MNT_LOCKED) {
> +			    IS_MNT_UNBINDABLE(src_mnt)) {
> +				if (src_mnt->mnt.mnt_flags & MNT_LOCKED) {
>  					/* Both unbindable and locked. */
> -					q = ERR_PTR(-EPERM);
> +					dst_mnt = ERR_PTR(-EPERM);
>  					goto out;
>  				} else {
> -					s = skip_mnt_tree(s);
> +					src_mnt = skip_mnt_tree(src_mnt);
>  					continue;
>  				}
>  			}
>  			if (!(flag & CL_COPY_MNT_NS_FILE) &&
> -			    is_mnt_ns_file(s->mnt.mnt_root)) {
> -				s = skip_mnt_tree(s);
> +			    is_mnt_ns_file(src_mnt->mnt.mnt_root)) {
> +				src_mnt = skip_mnt_tree(src_mnt);
>  				continue;
>  			}
> -			while (p != s->mnt_parent) {
> -				p = p->mnt_parent;
> -				q = q->mnt_parent;
> +			while (src_parent != src_mnt->mnt_parent) {
> +				src_parent = src_parent->mnt_parent;
> +				dst_mnt = dst_mnt->mnt_parent;
>  			}
> -			p = s;
> -			parent = q;
> -			q = clone_mnt(p, p->mnt.mnt_root, flag);
> -			if (IS_ERR(q))
> +
> +			src_parent = src_mnt;
> +			dst_parent = dst_mnt;
> +			dst_mnt = clone_mnt(src_mnt, src_mnt->mnt.mnt_root, flag);
> +			if (IS_ERR(dst_mnt))
>  				goto out;
>  			lock_mount_hash();
> -			list_add_tail(&q->mnt_list, &res->mnt_list);
> -			attach_mnt(q, parent, p->mnt_mp, false);
> +			list_add_tail(&dst_mnt->mnt_list, &res->mnt_list);
> +			attach_mnt(dst_mnt, dst_parent, src_parent->mnt_mp, false);
>  			unlock_mount_hash();
>  		}
>  	}
>  	return res;
> +
>  out:
>  	if (res) {
>  		lock_mount_hash();
>  		umount_tree(res, UMOUNT_SYNC);
>  		unlock_mount_hash();
>  	}
> -	return q;
> +	return dst_mnt;
>  }
>  
>  /* Caller should check returned pointer for errors */
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4] Improve readability of copy_tree

[Jemmy]

by employing `copy mount tree from src to dst` concept.
This involves renaming the opaque variables (e.g., p, q, r, s)
to be more descriptive, aiming to make the code easier to understand.

Changes:
mnt     -> src_root (root of the tree to copy)
r       -> src_root_child (direct child of the root being cloning)
p       -> src_parent (parent of src_mnt)
s       -> src_mnt (current mount being copying)
parent  -> dst_parent (parent of dst_child)
q       -> dst_mnt (freshly cloned mount)

Signed-off-by: Jemmy <jemmywong512@gmail.com>
---
 fs/namespace.c | 59 ++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5a51315c6678..b0202e37515e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1966,69 +1966,72 @@ static bool mnt_ns_loop(struct dentry *dentry)
 	return current->nsproxy->mnt_ns->seq >= mnt_ns->seq;
 }
 
-struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
+struct mount *copy_tree(struct mount *src_root, struct dentry *dentry,
 					int flag)
 {
-	struct mount *res, *p, *q, *r, *parent;
+	struct mount *res, *src_parent, *src_root_child, *src_mnt,
+		*dst_parent, *dst_mnt;
 
-	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
+	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(src_root))
 		return ERR_PTR(-EINVAL);
 
 	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
 		return ERR_PTR(-EINVAL);
 
-	res = q = clone_mnt(mnt, dentry, flag);
-	if (IS_ERR(q))
-		return q;
+	res = dst_mnt = clone_mnt(src_root, dentry, flag);
+	if (IS_ERR(dst_mnt))
+		return dst_mnt;
 
-	q->mnt_mountpoint = mnt->mnt_mountpoint;
+	src_parent = src_root;
+	dst_mnt->mnt_mountpoint = src_root->mnt_mountpoint;
 
-	p = mnt;
-	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
-		struct mount *s;
-		if (!is_subdir(r->mnt_mountpoint, dentry))
+	list_for_each_entry(src_root_child, &src_root->mnt_mounts, mnt_child) {
+		if (!is_subdir(src_root_child->mnt_mountpoint, dentry))
 			continue;
 
-		for (s = r; s; s = next_mnt(s, r)) {
+		for (src_mnt = src_root_child; src_mnt;
+		    src_mnt = next_mnt(src_mnt, src_root_child)) {
 			if (!(flag & CL_COPY_UNBINDABLE) &&
-			    IS_MNT_UNBINDABLE(s)) {
-				if (s->mnt.mnt_flags & MNT_LOCKED) {
+			    IS_MNT_UNBINDABLE(src_mnt)) {
+				if (src_mnt->mnt.mnt_flags & MNT_LOCKED) {
 					/* Both unbindable and locked. */
-					q = ERR_PTR(-EPERM);
+					dst_mnt = ERR_PTR(-EPERM);
 					goto out;
 				} else {
-					s = skip_mnt_tree(s);
+					src_mnt = skip_mnt_tree(src_mnt);
 					continue;
 				}
 			}
 			if (!(flag & CL_COPY_MNT_NS_FILE) &&
-			    is_mnt_ns_file(s->mnt.mnt_root)) {
-				s = skip_mnt_tree(s);
+			    is_mnt_ns_file(src_mnt->mnt.mnt_root)) {
+				src_mnt = skip_mnt_tree(src_mnt);
 				continue;
 			}
-			while (p != s->mnt_parent) {
-				p = p->mnt_parent;
-				q = q->mnt_parent;
+			while (src_parent != src_mnt->mnt_parent) {
+				src_parent = src_parent->mnt_parent;
+				dst_mnt = dst_mnt->mnt_parent;
 			}
-			p = s;
-			parent = q;
-			q = clone_mnt(p, p->mnt.mnt_root, flag);
-			if (IS_ERR(q))
+
+			src_parent = src_mnt;
+			dst_parent = dst_mnt;
+			dst_mnt = clone_mnt(src_mnt, src_mnt->mnt.mnt_root, flag);
+			if (IS_ERR(dst_mnt))
 				goto out;
 			lock_mount_hash();
-			list_add_tail(&q->mnt_list, &res->mnt_list);
-			attach_mnt(q, parent, p->mnt_mp, false);
+			list_add_tail(&dst_mnt->mnt_list, &res->mnt_list);
+			attach_mnt(dst_mnt, dst_parent, src_parent->mnt_mp, false);
 			unlock_mount_hash();
 		}
 	}
 	return res;
+
 out:
 	if (res) {
 		lock_mount_hash();
 		umount_tree(res, UMOUNT_SYNC);
 		unlock_mount_hash();
 	}
-	return q;
+	return dst_mnt;
 }
 
 /* Caller should check returned pointer for errors */
-- 
2.34.1

Re: [PATCH v4] Improve readability of copy_tree

[Christian Brauner]

On Fri, 07 Jun 2024 01:39:12 +0800, Jemmy wrote:
> by employing `copy mount tree from src to dst` concept.
> This involves renaming the opaque variables (e.g., p, q, r, s)
> to be more descriptive, aiming to make the code easier to understand.
> 
> Changes:
> mnt     -> src_root (root of the tree to copy)
> r       -> src_root_child (direct child of the root being cloning)
> p       -> src_parent (parent of src_mnt)
> s       -> src_mnt (current mount being copying)
> parent  -> dst_parent (parent of dst_child)
> q       -> dst_mnt (freshly cloned mount)
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] Improve readability of copy_tree
      https://git.kernel.org/vfs/vfs/c/5692e7579306

Re: [PATCH v4] Improve readability of copy_tree

[Jan Kara]

On Fri 07-06-24 01:39:12, Jemmy wrote:
> by employing `copy mount tree from src to dst` concept.
> This involves renaming the opaque variables (e.g., p, q, r, s)
> to be more descriptive, aiming to make the code easier to understand.
> 
> Changes:
> mnt     -> src_root (root of the tree to copy)
> r       -> src_root_child (direct child of the root being cloning)
> p       -> src_parent (parent of src_mnt)
> s       -> src_mnt (current mount being copying)
> parent  -> dst_parent (parent of dst_child)
> q       -> dst_mnt (freshly cloned mount)
> 
> Signed-off-by: Jemmy <jemmywong512@gmail.com>

Thanks! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/namespace.c | 59 ++++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5a51315c6678..b0202e37515e 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1966,69 +1966,72 @@ static bool mnt_ns_loop(struct dentry *dentry)
>  	return current->nsproxy->mnt_ns->seq >= mnt_ns->seq;
>  }
>  
> -struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
> +struct mount *copy_tree(struct mount *src_root, struct dentry *dentry,
>  					int flag)
>  {
> -	struct mount *res, *p, *q, *r, *parent;
> +	struct mount *res, *src_parent, *src_root_child, *src_mnt,
> +		*dst_parent, *dst_mnt;
>  
> -	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt))
> +	if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(src_root))
>  		return ERR_PTR(-EINVAL);
>  
>  	if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry))
>  		return ERR_PTR(-EINVAL);
>  
> -	res = q = clone_mnt(mnt, dentry, flag);
> -	if (IS_ERR(q))
> -		return q;
> +	res = dst_mnt = clone_mnt(src_root, dentry, flag);
> +	if (IS_ERR(dst_mnt))
> +		return dst_mnt;
>  
> -	q->mnt_mountpoint = mnt->mnt_mountpoint;
> +	src_parent = src_root;
> +	dst_mnt->mnt_mountpoint = src_root->mnt_mountpoint;
>  
> -	p = mnt;
> -	list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) {
> -		struct mount *s;
> -		if (!is_subdir(r->mnt_mountpoint, dentry))
> +	list_for_each_entry(src_root_child, &src_root->mnt_mounts, mnt_child) {
> +		if (!is_subdir(src_root_child->mnt_mountpoint, dentry))
>  			continue;
>  
> -		for (s = r; s; s = next_mnt(s, r)) {
> +		for (src_mnt = src_root_child; src_mnt;
> +		    src_mnt = next_mnt(src_mnt, src_root_child)) {
>  			if (!(flag & CL_COPY_UNBINDABLE) &&
> -			    IS_MNT_UNBINDABLE(s)) {
> -				if (s->mnt.mnt_flags & MNT_LOCKED) {
> +			    IS_MNT_UNBINDABLE(src_mnt)) {
> +				if (src_mnt->mnt.mnt_flags & MNT_LOCKED) {
>  					/* Both unbindable and locked. */
> -					q = ERR_PTR(-EPERM);
> +					dst_mnt = ERR_PTR(-EPERM);
>  					goto out;
>  				} else {
> -					s = skip_mnt_tree(s);
> +					src_mnt = skip_mnt_tree(src_mnt);
>  					continue;
>  				}
>  			}
>  			if (!(flag & CL_COPY_MNT_NS_FILE) &&
> -			    is_mnt_ns_file(s->mnt.mnt_root)) {
> -				s = skip_mnt_tree(s);
> +			    is_mnt_ns_file(src_mnt->mnt.mnt_root)) {
> +				src_mnt = skip_mnt_tree(src_mnt);
>  				continue;
>  			}
> -			while (p != s->mnt_parent) {
> -				p = p->mnt_parent;
> -				q = q->mnt_parent;
> +			while (src_parent != src_mnt->mnt_parent) {
> +				src_parent = src_parent->mnt_parent;
> +				dst_mnt = dst_mnt->mnt_parent;
>  			}
> -			p = s;
> -			parent = q;
> -			q = clone_mnt(p, p->mnt.mnt_root, flag);
> -			if (IS_ERR(q))
> +
> +			src_parent = src_mnt;
> +			dst_parent = dst_mnt;
> +			dst_mnt = clone_mnt(src_mnt, src_mnt->mnt.mnt_root, flag);
> +			if (IS_ERR(dst_mnt))
>  				goto out;
>  			lock_mount_hash();
> -			list_add_tail(&q->mnt_list, &res->mnt_list);
> -			attach_mnt(q, parent, p->mnt_mp, false);
> +			list_add_tail(&dst_mnt->mnt_list, &res->mnt_list);
> +			attach_mnt(dst_mnt, dst_parent, src_parent->mnt_mp, false);
>  			unlock_mount_hash();
>  		}
>  	}
>  	return res;
> +
>  out:
>  	if (res) {
>  		lock_mount_hash();
>  		umount_tree(res, UMOUNT_SYNC);
>  		unlock_mount_hash();
>  	}
> -	return q;
> +	return dst_mnt;
>  }
>  
>  /* Caller should check returned pointer for errors */
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR