Re: [PATCH v3] Improve readability of copy_tree

[Jan Kara <jack@suse.cz>]

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