[PATCH 0/2] btrfs: allow findmnt(8) to find which subvol of btrfs is

[PATCH 0/2] btrfs: allow findmnt(8) to find which subvol of btrfs is

[Qu Wenruo]

Before this patchset, btrfs has 2 different routine to handle subvolume mount:
1) "subvol=" routine
When subvol name is given by "subvol=" mount option, btrfs will mount the fs
and then use mount_subtree() to mount the subvolume.
This routine is good since vfs layer is aware of which subtree is mounted.
So findmnt(8) can show the source subvol of a btrfs mount.

2) "subvolid=" and default subvolume routine
When "subvolid=" or mounting default subvolume, btrfs will call
get_default_root() to find subvolid or default subvolume.
The problem is that, all the subvol searching are done internally,
vfs is unaware of the return dentry is just a subtree.
So findmnt(8) can't show the source subvol of a btrfs mount.

This patchset will merge all this routine into 1) "subvol=" routine
and make findmnt(8) able to show source subvol of a btrfs mount.

After the patchset, btrfs will:
1) mount fs root under all situation first.
If user gives "subvolid=5" mount option, then the mount is done.
Or mount continues.

2) If no subvolume id or name is given, do default subvolume search.

3) If subvolume id is given(or searched by 2), do backref walk.
Search backref in tree root to generate the subvolume path.

4) call mount_subtree()
Whatever the subvolume path is generated by backref walk or just given by
"subvol=" mount option, just call mount_subtree() to finish the mount.

Qu Wenruo (2):
  btrfs: Call mount_subtree() even 'subvolid=' mount option is given.
  btrfs: Merge default subvolume mount codes into btrfs_mount_subvol().

 fs/btrfs/super.c | 324 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 250 insertions(+), 74 deletions(-)

-- 
2.0.1

[PATCH 1/2] btrfs: Call mount_subtree() even 'subvolid=' mount option is given.

[Qu Wenruo]

btrfs uses differnet routine to handle 'subvolid=' and 'subvol=' mount
option.
Given 'subvol=' mount option, btrfs will mount btrfs first and then call
mount_subtree() to mount a subtree of btrfs, making vfs handle the path
searching.
This is good since vfs layer know extactly that a subtree mount is done
and findmnt(8) knows which subtree is mounted.

However when using 'subvolid=' mount option, btrfs will do all the
internal subvolume objectid searching and checking, making VFS unaware
about which subtree is mounted, as result, findmnt(8) can't showing any
useful subtree mount info for end users.

This patch will use the root backref to reverse search the subvolume
path for a given subvolid, making findmnt(8) works again.

Reported-by: Stefan G.Weichinger <lists@xunil.at>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/super.c | 231 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 201 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 8e16bca..23363a2 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -58,6 +58,7 @@
 #include "dev-replace.h"
 #include "free-space-cache.h"
 #include "backref.h"
+#include "locking.h"
 #include "tests/btrfs-tests.h"
 
 #define CREATE_TRACE_POINTS
@@ -1117,28 +1118,81 @@ static inline int is_subvolume_inode(struct inode *inode)
 	return 0;
 }
 
+static int u64_to_strlen(u64 number)
+{
+	int ret = 0;
+
+	/* Zero is the special one, return 1 directly */
+	if (!number)
+		return 1;
+
+	while (number) {
+		ret++;
+		number /= 10;
+	}
+	return ret;
+}
+
+#define CLEAR_SUBVOL		1
+#define CLEAR_SUBVOLID		2
 /*
- * This will strip out the subvol=%s argument for an argument string and add
- * subvolid=0 to make sure we get the actual tree root for path walking to the
- * subvol we want.
+ * This will strip out the subvol=%s or subvolid=%s argument for an argumen
+ * string and add subvolid=0 to make sure we get the actual tree root for path
+ * walking to the subvol we want.
  */
-static char *setup_root_args(char *args)
+static char *setup_root_args(char *args, int flags, u64 subvol_objectid)
 {
-	unsigned len = strlen(args) + 2 + 1;
-	char *src, *dst, *buf;
+	unsigned len;
+	char *src = NULL, *dst, *buf, *comma;
+	char *subvol_string = "subvolid=";
+	int option_len = 0;
+
+	if (!args) {
+		/* Case 1, not args, all default mounting
+		 * just return 'subvolid=<FS_ROOT>' */
+		len = strlen(subvol_string) +
+		      u64_to_strlen(subvol_objectid) + 1;
+		dst = kmalloc(len, GFP_NOFS);
+		if (!dst)
+			return NULL;
+		sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
+		return dst;
+	}
 
-	/*
-	 * We need the same args as before, but with this substitution:
-	 * s!subvol=[^,]+!subvolid=0!
-	 *
-	 * Since the replacement string is up to 2 bytes longer than the
-	 * original, allocate strlen(args) + 2 + 1 bytes.
-	 */
+	switch (flags) {
+	case CLEAR_SUBVOL:
+		src = strstr(args, "subvol=");
+		break;
+	case CLEAR_SUBVOLID:
+		src = strstr(args, "subvolid=");
+		break;
+	}
 
-	src = strstr(args, "subvol=");
-	/* This shouldn't happen, but just in case.. */
-	if (!src)
-		return NULL;
+	if (!src) {
+		/* Case 2, some args, default subvolume mounting
+		 * just append ',subvolid=<FS_ROOT>' */
+
+		/* 1 for ending '\0', 1 for leading ',' */
+		len = strlen(args) + strlen(subvol_string) +
+		      u64_to_strlen(subvol_objectid) + 2;
+		dst = kmalloc(len, GFP_NOFS);
+		if (!dst)
+			return NULL;
+		strcpy(dst, args);
+		sprintf(dst + strlen(args), ",%s%llu", subvol_string,
+			subvol_objectid);
+		return dst;
+	}
+
+	/* Case 3, subvolid=/subvol=  mount
+	 * repalce the 'subvolid/subvol' options to 'subvolid=<FS_ROOT>' */
+	comma = strchr(src, ',');
+	if (comma)
+		option_len = comma - src;
+	else
+		option_len = strlen(src);
+	len = strlen(args) - option_len  + strlen(subvol_string) +
+	      u64_to_strlen(subvol_objectid) + 1;
 
 	buf = dst = kmalloc(len, GFP_NOFS);
 	if (!buf)
@@ -1154,28 +1208,126 @@ static char *setup_root_args(char *args)
 		dst += strlen(args);
 	}
 
-	strcpy(dst, "subvolid=0");
-	dst += strlen("subvolid=0");
+	len = sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
+	dst += len;
 
 	/*
 	 * If there is a "," after the original subvol=... string,
 	 * copy that suffix into our buffer.  Otherwise, we're done.
 	 */
-	src = strchr(src, ',');
-	if (src)
-		strcpy(dst, src);
+	if (comma)
+		strcpy(dst, comma);
 
 	return buf;
 }
 
-static struct dentry *mount_subvol(const char *subvol_name, int flags,
-				   const char *device_name, char *data)
+static char *str_append_head(char *dest, char *src)
+{
+	memmove(dest + strlen(src), dest, strlen(dest) + 1);
+	memcpy(dest, src, strlen(src));
+	return dest;
+}
+
+/* Find the path for given subvol_objectid.
+ * Caller needs to readlock the root tree and kzalloc PATH_MAX for
+ * subvol_name and namebuf */
+static char *find_subvol_by_id(struct btrfs_root *root, u64 subvol_objectid)
+{
+	struct btrfs_key key;
+	struct btrfs_key found_key;
+	struct btrfs_root_ref *ref;
+	struct btrfs_path *path;
+	char *namebuf = NULL;
+	char *new_buf = NULL;
+	char *subvol_ret = NULL;
+	int ret = 0;
+	u16 namelen = 0;
+
+	path = btrfs_alloc_path();
+	/* Alloc 1 byte for later strlen() calls */
+	subvol_ret = kzalloc(1, GFP_NOFS);
+	if (!path || !subvol_ret) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	key.objectid = subvol_objectid;
+	key.type = BTRFS_ROOT_BACKREF_KEY;
+	key.offset = 0;
+	/* We don't need to lock the tree_root,
+	 * if when we do the backref walking, some one deleted/moved
+	 * the subvol, we just return -ENOENT or let mount_subtree
+	 * return -ENOENT and no disaster will happen.
+	 * User should not modify subvolume when trying to mount it */
+	while (key.objectid != BTRFS_FS_TREE_OBJECTID) {
+		ret = btrfs_search_slot_for_read(root, &key, path, 1, 1);
+		if (ret < 0)
+			goto out;
+		if (ret) {
+			ret = -ENOENT;
+			goto out;
+		}
+		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
+				      path->slots[0]);
+		if (found_key.objectid != key.objectid ||
+		    found_key.type != BTRFS_ROOT_BACKREF_KEY) {
+			ret = -ENOENT;
+			goto out;
+		}
+		key.objectid = found_key.offset;
+		ref = btrfs_item_ptr(path->nodes[0], path->slots[0],
+				     struct btrfs_root_ref);
+		namelen = btrfs_root_ref_name_len(path->nodes[0], ref);
+		/* One for ending '\0' One for '/' */
+		new_buf = krealloc(namebuf, namelen + 2, GFP_NOFS);
+		if (!new_buf) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		namebuf = new_buf;
+		read_extent_buffer(path->nodes[0], namebuf,
+				   (unsigned long)(ref + 1), namelen);
+		btrfs_release_path(path);
+		*(namebuf + namelen) = '/';
+		*(namebuf + namelen + 1) = '\0';
+
+		new_buf = krealloc(subvol_ret, strlen(subvol_ret) + namelen + 2,
+				   GFP_NOFS);
+		if (!new_buf) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		subvol_ret = new_buf;
+		str_append_head(subvol_ret, namebuf);
+	}
+out:
+	kfree(namebuf);
+	btrfs_free_path(path);
+	if (ret < 0) {
+		kfree(subvol_ret);
+		return ERR_PTR(ret);
+	} else
+		return subvol_ret;
+
+}
+
+static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
+				   int flags, const char *device_name,
+				   char *data)
 {
 	struct dentry *root;
 	struct vfsmount *mnt;
+	struct btrfs_root *tree_root = NULL;
 	char *newargs;
+	char *subvol_ret = NULL;
+	int ret = 0;
 
-	newargs = setup_root_args(data);
+	if (subvol_name)
+		newargs = setup_root_args(data, CLEAR_SUBVOL,
+					  BTRFS_FS_TREE_OBJECTID);
+	else
+		newargs = setup_root_args(data, CLEAR_SUBVOLID,
+					  BTRFS_FS_TREE_OBJECTID);
 	if (!newargs)
 		return ERR_PTR(-ENOMEM);
 	mnt = vfs_kern_mount(&btrfs_fs_type, flags, device_name,
@@ -1207,19 +1359,36 @@ static struct dentry *mount_subvol(const char *subvol_name, int flags,
 
 	if (IS_ERR(mnt))
 		return ERR_CAST(mnt);
-
+	tree_root = btrfs_sb(mnt->mnt_sb)->tree_root;
+
+	if (!subvol_name) {
+		subvol_ret = find_subvol_by_id(tree_root, subvol_objectid);
+		if (IS_ERR(subvol_ret)) {
+			ret = PTR_ERR(subvol_ret);
+			subvol_ret = NULL;
+			mntput(mnt);
+			goto out;
+		}
+		subvol_name = subvol_ret;
+	}
 	root = mount_subtree(mnt, subvol_name);
 
 	if (!IS_ERR(root) && !is_subvolume_inode(root->d_inode)) {
 		struct super_block *s = root->d_sb;
 		dput(root);
-		root = ERR_PTR(-EINVAL);
+		ret = -EINVAL;
 		deactivate_locked_super(s);
 		printk(KERN_ERR "BTRFS: '%s' is not a valid subvolume\n",
 				subvol_name);
 	}
 
-	return root;
+out:
+	kfree(subvol_ret);
+	if (ret < 0)
+		return ERR_PTR(ret);
+	else
+		return root;
+
 }
 
 /*
@@ -1252,8 +1421,10 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 		return ERR_PTR(error);
 	}
 
-	if (subvol_name) {
-		root = mount_subvol(subvol_name, flags, device_name, data);
+	if (subvol_name || (subvol_objectid != 0 &&
+			    subvol_objectid != BTRFS_FS_TREE_OBJECTID)) {
+		root = mount_subvol(subvol_name, subvol_objectid, flags,
+				    device_name, data);
 		kfree(subvol_name);
 		return root;
 	}
-- 
2.0.1

[PATCH 2/2] btrfs: Merge default subvolume mount codes into btrfs_mount_subvol().

[Qu Wenruo]

Old btrfs codes do the default subvolume search in get_default_root(),
which makes default subvolume mount will not info vfs that it's a
subtree mount.

Now since btrfs_mount_subvol() will mount all btrfs subvolume throught
mount_subtree() vfs API, merge the default subvolume searching codes
into it, and now btrfs handles all 3 types of subvolume mount in one
place.('subvol=' 'subvolid=' and default subvolume mount).

This is done by reuse the old dir_id->root_objectid search routine and
then reuse the subvolume_object mount routine in previous patch.

Reported-by: Stefan G.Weichinger <lists@xunil.at>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/super.c | 101 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 48 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 23363a2..7bed7a1 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -842,14 +842,10 @@ out:
 	return error;
 }
 
-static struct dentry *get_default_root(struct super_block *sb,
-				       u64 subvol_objectid)
+static struct dentry *get_default_root(struct super_block *sb)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
-	struct btrfs_root *root = fs_info->tree_root;
 	struct btrfs_root *new_root;
-	struct btrfs_dir_item *di;
-	struct btrfs_path *path;
 	struct btrfs_key location;
 	struct inode *inode;
 	struct dentry *dentry;
@@ -860,51 +856,15 @@ static struct dentry *get_default_root(struct super_block *sb,
 	 * We have a specific subvol we want to mount, just setup location and
 	 * go look up the root.
 	 */
-	if (subvol_objectid) {
-		location.objectid = subvol_objectid;
-		location.type = BTRFS_ROOT_ITEM_KEY;
-		location.offset = (u64)-1;
-		goto find_root;
-	}
-
-	path = btrfs_alloc_path();
-	if (!path)
-		return ERR_PTR(-ENOMEM);
-	path->leave_spinning = 1;
-
-	/*
-	 * Find the "default" dir item which points to the root item that we
-	 * will mount by default if we haven't been given a specific subvolume
-	 * to mount.
-	 */
-	dir_id = btrfs_super_root_dir(fs_info->super_copy);
-	di = btrfs_lookup_dir_item(NULL, root, path, dir_id, "default", 7, 0);
-	if (IS_ERR(di)) {
-		btrfs_free_path(path);
-		return ERR_CAST(di);
-	}
-	if (!di) {
-		/*
-		 * Ok the default dir item isn't there.  This is weird since
-		 * it's always been there, but don't freak out, just try and
-		 * mount to root most subvolume.
-		 */
-		btrfs_free_path(path);
-		dir_id = BTRFS_FIRST_FREE_OBJECTID;
-		new_root = fs_info->fs_root;
-		goto setup_root;
-	}
-
-	btrfs_dir_item_key_to_cpu(path->nodes[0], di, &location);
-	btrfs_free_path(path);
+	location.objectid = BTRFS_FS_TREE_OBJECTID;
+	location.type = BTRFS_ROOT_ITEM_KEY;
+	location.offset = (u64)-1;
 
-find_root:
 	new_root = btrfs_read_fs_root_no_name(fs_info, &location);
 	if (IS_ERR(new_root))
 		return ERR_CAST(new_root);
 
 	dir_id = btrfs_root_dirid(&new_root->root_item);
-setup_root:
 	location.objectid = dir_id;
 	location.type = BTRFS_INODE_ITEM_KEY;
 	location.offset = 0;
@@ -1137,7 +1097,7 @@ static int u64_to_strlen(u64 number)
 #define CLEAR_SUBVOLID		2
 /*
  * This will strip out the subvol=%s or subvolid=%s argument for an argumen
- * string and add subvolid=0 to make sure we get the actual tree root for path
+ * string and add subvolid=5 to make sure we get the actual tree root for path
  * walking to the subvol we want.
  */
 static char *setup_root_args(char *args, int flags, u64 subvol_objectid)
@@ -1198,6 +1158,14 @@ static char *setup_root_args(char *args, int flags, u64 subvol_objectid)
 	if (!buf)
 		return NULL;
 
+	if (!src) {
+		strcpy(dst, args);
+		strcpy(dst, ",");
+		strcpy(dst, subvol_string);
+		return dst;
+	}
+
+
 	/*
 	 * If the subvol= arg is not at the start of the string,
 	 * copy whatever precedes it into buf.
@@ -1311,6 +1279,37 @@ out:
 
 }
 
+static int find_default_subvol_objectid(struct btrfs_root *root,
+					u64 *subvol_objectid)
+{
+	struct btrfs_super_block *super = root->fs_info->super_copy;
+	struct btrfs_dir_item *di;
+	struct btrfs_key key;
+	struct btrfs_path *path;
+	u64 dir_id;
+	int ret = 0;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+	path->leave_spinning = 1;
+	dir_id = btrfs_super_root_dir(super);
+	di = btrfs_lookup_dir_item(NULL, root, path, dir_id, "defaults", 7, 0);
+	if (IS_ERR(di)) {
+		ret = PTR_ERR(di);
+		goto out;
+	}
+	if (!di) {
+		*subvol_objectid = BTRFS_FS_TREE_OBJECTID;
+		goto out;
+	}
+	btrfs_dir_item_key_to_cpu(path->nodes[0], di, &key);
+	*subvol_objectid = key.objectid;
+out:
+	btrfs_free_path(path);
+	return ret;
+}
+
 static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
 				   int flags, const char *device_name,
 				   char *data)
@@ -1359,8 +1358,15 @@ static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
 
 	if (IS_ERR(mnt))
 		return ERR_CAST(mnt);
+
 	tree_root = btrfs_sb(mnt->mnt_sb)->tree_root;
 
+	if (!subvol_name && !subvol_objectid) {
+		/* default subvolume mount */
+		ret = find_default_subvol_objectid(tree_root, &subvol_objectid);
+		if (ret < 0)
+			goto out;
+	}
 	if (!subvol_name) {
 		subvol_ret = find_subvol_by_id(tree_root, subvol_objectid);
 		if (IS_ERR(subvol_ret)) {
@@ -1421,8 +1427,7 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 		return ERR_PTR(error);
 	}
 
-	if (subvol_name || (subvol_objectid != 0 &&
-			    subvol_objectid != BTRFS_FS_TREE_OBJECTID)) {
+	if (subvol_objectid != BTRFS_FS_TREE_OBJECTID) {
 		root = mount_subvol(subvol_name, subvol_objectid, flags,
 				    device_name, data);
 		kfree(subvol_name);
@@ -1483,7 +1488,7 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 					 flags & MS_SILENT ? 1 : 0);
 	}
 
-	root = !error ? get_default_root(s, subvol_objectid) : ERR_PTR(error);
+	root = !error ? get_default_root(s) : ERR_PTR(error);
 	if (IS_ERR(root))
 		deactivate_locked_super(s);
 
-- 
2.0.1

Re: [PATCH 1/2] btrfs: Call mount_subtree() even 'subvolid=' mount option is given.

[Chandan Rajendra]

On Wednesday 16 Jul 2014 12:07:10 Qu Wenruo wrote:
> +/* Find the path for given subvol_objectid.
> + * Caller needs to readlock the root tree and kzalloc PATH_MAX for
> + * subvol_name and namebuf */
> +static char *find_subvol_by_id(struct btrfs_root *root, u64 subvol_objectid)
> +{
> +	struct btrfs_key key;
> +	struct btrfs_key found_key;
> +	struct btrfs_root_ref *ref;
> +	struct btrfs_path *path;
> +	char *namebuf = NULL;
> +	char *new_buf = NULL;
> +	char *subvol_ret = NULL;
> +	int ret = 0;
> +	u16 namelen = 0;
> +
> +	path = btrfs_alloc_path();
> +	/* Alloc 1 byte for later strlen() calls */
> +	subvol_ret = kzalloc(1, GFP_NOFS);
> +	if (!path || !subvol_ret) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	key.objectid = subvol_objectid;
> +	key.type = BTRFS_ROOT_BACKREF_KEY;
> +	key.offset = 0;
> +	/* We don't need to lock the tree_root,
> +	 * if when we do the backref walking, some one deleted/moved
> +	 * the subvol, we just return -ENOENT or let mount_subtree
> +	 * return -ENOENT and no disaster will happen.
> +	 * User should not modify subvolume when trying to mount it */
> +	while (key.objectid != BTRFS_FS_TREE_OBJECTID) {
> +		ret = btrfs_search_slot_for_read(root, &key, path, 1, 1);
> +		if (ret < 0)
> +			goto out;
> +		if (ret) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
> +				      path->slots[0]);
> +		if (found_key.objectid != key.objectid ||
> +		    found_key.type != BTRFS_ROOT_BACKREF_KEY) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +		key.objectid = found_key.offset;
> +		ref = btrfs_item_ptr(path->nodes[0], path->slots[0],
> +				     struct btrfs_root_ref);
> +		namelen = btrfs_root_ref_name_len(path->nodes[0], ref);
> +		/* One for ending '\0' One for '/' */
> +		new_buf = krealloc(namebuf, namelen + 2, GFP_NOFS);
> +		if (!new_buf) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		namebuf = new_buf;
> +		read_extent_buffer(path->nodes[0], namebuf,
> +				   (unsigned long)(ref + 1), namelen);
> +		btrfs_release_path(path);
> +		*(namebuf + namelen) = '/';
> +		*(namebuf + namelen + 1) = '\0';
> +
> +		new_buf = krealloc(subvol_ret, strlen(subvol_ret) + namelen + 2,
> +				   GFP_NOFS);
> +		if (!new_buf) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		subvol_ret = new_buf;
> +		str_append_head(subvol_ret, namebuf);
> +	}
> +out:
> +	kfree(namebuf);
> +	btrfs_free_path(path);
> +	if (ret < 0) {
> +		kfree(subvol_ret);
> +		return ERR_PTR(ret);
> +	} else
> +		return subvol_ret;
> +
> +}

Hello Qu Wenruo,

If the subvolume being mounted exists in a sub-directory of the parent's
subvolume, then the find_subvol_by_id() fails to contruct the correct
path to the subvolume. Hence mount would fail.

For e.g.

[root@guest0 ~]# btrfs subvolume list /mnt/btrfs/
ID 257 gen 7 top level 5 path dir1/sub1
[root@guest0 ~]# umount /mnt/btrfs
[root@guest0 ~]# mount -o subvolid=257 /dev/loop0 /mnt/btrfs/
mount: mount(2) failed: No such file or directory

-- 
chandan

Re: [PATCH 1/2] btrfs: Call mount_subtree() even 'subvolid=' mount option is given.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs: Call mount_subtree() even 'subvolid=' 
mount option is given.
From: Chandan Rajendra <chandan@linux.vnet.ibm.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年07月18日 14:25
> On Wednesday 16 Jul 2014 12:07:10 Qu Wenruo wrote:
>> +/* Find the path for given subvol_objectid.
>> + * Caller needs to readlock the root tree and kzalloc PATH_MAX for
>> + * subvol_name and namebuf */
>> +static char *find_subvol_by_id(struct btrfs_root *root, u64 subvol_objectid)
>> +{
>> +	struct btrfs_key key;
>> +	struct btrfs_key found_key;
>> +	struct btrfs_root_ref *ref;
>> +	struct btrfs_path *path;
>> +	char *namebuf = NULL;
>> +	char *new_buf = NULL;
>> +	char *subvol_ret = NULL;
>> +	int ret = 0;
>> +	u16 namelen = 0;
>> +
>> +	path = btrfs_alloc_path();
>> +	/* Alloc 1 byte for later strlen() calls */
>> +	subvol_ret = kzalloc(1, GFP_NOFS);
>> +	if (!path || !subvol_ret) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	key.objectid = subvol_objectid;
>> +	key.type = BTRFS_ROOT_BACKREF_KEY;
>> +	key.offset = 0;
>> +	/* We don't need to lock the tree_root,
>> +	 * if when we do the backref walking, some one deleted/moved
>> +	 * the subvol, we just return -ENOENT or let mount_subtree
>> +	 * return -ENOENT and no disaster will happen.
>> +	 * User should not modify subvolume when trying to mount it */
>> +	while (key.objectid != BTRFS_FS_TREE_OBJECTID) {
>> +		ret = btrfs_search_slot_for_read(root, &key, path, 1, 1);
>> +		if (ret < 0)
>> +			goto out;
>> +		if (ret) {
>> +			ret = -ENOENT;
>> +			goto out;
>> +		}
>> +		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
>> +				      path->slots[0]);
>> +		if (found_key.objectid != key.objectid ||
>> +		    found_key.type != BTRFS_ROOT_BACKREF_KEY) {
>> +			ret = -ENOENT;
>> +			goto out;
>> +		}
>> +		key.objectid = found_key.offset;
>> +		ref = btrfs_item_ptr(path->nodes[0], path->slots[0],
>> +				     struct btrfs_root_ref);
>> +		namelen = btrfs_root_ref_name_len(path->nodes[0], ref);
>> +		/* One for ending '\0' One for '/' */
>> +		new_buf = krealloc(namebuf, namelen + 2, GFP_NOFS);
>> +		if (!new_buf) {
>> +			ret = -ENOMEM;
>> +			goto out;
>> +		}
>> +		namebuf = new_buf;
>> +		read_extent_buffer(path->nodes[0], namebuf,
>> +				   (unsigned long)(ref + 1), namelen);
>> +		btrfs_release_path(path);
>> +		*(namebuf + namelen) = '/';
>> +		*(namebuf + namelen + 1) = '\0';
>> +
>> +		new_buf = krealloc(subvol_ret, strlen(subvol_ret) + namelen + 2,
>> +				   GFP_NOFS);
>> +		if (!new_buf) {
>> +			ret = -ENOMEM;
>> +			goto out;
>> +		}
>> +		subvol_ret = new_buf;
>> +		str_append_head(subvol_ret, namebuf);
>> +	}
>> +out:
>> +	kfree(namebuf);
>> +	btrfs_free_path(path);
>> +	if (ret < 0) {
>> +		kfree(subvol_ret);
>> +		return ERR_PTR(ret);
>> +	} else
>> +		return subvol_ret;
>> +
>> +}
> Hello Qu Wenruo,
>
> If the subvolume being mounted exists in a sub-directory of the parent's
> subvolume, then the find_subvol_by_id() fails to contruct the correct
> path to the subvolume. Hence mount would fail.
>
> For e.g.
>
> [root@guest0 ~]# btrfs subvolume list /mnt/btrfs/
> ID 257 gen 7 top level 5 path dir1/sub1
> [root@guest0 ~]# umount /mnt/btrfs
> [root@guest0 ~]# mount -o subvolid=257 /dev/loop0 /mnt/btrfs/
> mount: mount(2) failed: No such file or directory
>
Oh, I forgot such situation, thanks for the test.
I'll update the patch to V2 to deal the problem.

Thanks,
Qu

Re: [PATCH 1/2] btrfs: Call mount_subtree() even 'subvolid=' mount option is given.

[David Sterba]

On Wed, Jul 16, 2014 at 12:07:10PM +0800, Qu Wenruo wrote:
> btrfs uses differnet routine to handle 'subvolid=' and 'subvol=' mount
> option.
> Given 'subvol=' mount option, btrfs will mount btrfs first and then call
> mount_subtree() to mount a subtree of btrfs, making vfs handle the path
> searching.
> This is good since vfs layer know extactly that a subtree mount is done
> and findmnt(8) knows which subtree is mounted.
> 
> However when using 'subvolid=' mount option, btrfs will do all the
> internal subvolume objectid searching and checking, making VFS unaware
> about which subtree is mounted, as result, findmnt(8) can't showing any
> useful subtree mount info for end users.
> 
> This patch will use the root backref to reverse search the subvolume
> path for a given subvolid, making findmnt(8) works again.
> 
> Reported-by: Stefan G.Weichinger <lists@xunil.at>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Ack for unifying the way subvol= and subvolid= are handled, but I don't
like some aspects of the implementation.

The kmalloc/krealloc makes it really complicated and is not imho
necessary. The mount options length is limited to PAGE_SIZE in the vfs
code. Do the same here, allocate a page, filter the options, do the
necessary processing and just check for overflows.

You can drop u64_to_strlen.

> +#define CLEAR_SUBVOL		1
> +#define CLEAR_SUBVOLID		2

Though they're internal and local to the file, please add BTRFS_ prefix
at least.

>  /*
> - * This will strip out the subvol=%s argument for an argument string and add
> - * subvolid=0 to make sure we get the actual tree root for path walking to the
> - * subvol we want.
> + * This will strip out the subvol=%s or subvolid=%s argument for an argumen
> + * string and add subvolid=0 to make sure we get the actual tree root for path
> + * walking to the subvol we want.
>   */
> -static char *setup_root_args(char *args)
> +static char *setup_root_args(char *args, int flags, u64 subvol_objectid)
>  {
> -	unsigned len = strlen(args) + 2 + 1;
> -	char *src, *dst, *buf;
> +	unsigned len;
> +	char *src = NULL, *dst, *buf, *comma;

Please use the recommended style and put each on a separate line. I'm
not sure if you'll need all of them for the implementation witouth the
kmallocs, the comment applies generally.

> +	char *subvol_string = "subvolid=";
> +	int option_len = 0;
> +
> +	if (!args) {
> +		/* Case 1, not args, all default mounting
> +		 * just return 'subvolid=<FS_ROOT>' */

Not the preferred style of comments.

> +		len = strlen(subvol_string) +
> +		      u64_to_strlen(subvol_objectid) + 1;
> +		dst = kmalloc(len, GFP_NOFS);
> +		if (!dst)
> +			return NULL;
> +		sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
> +		return dst;
> +	}
>  
> -	/*
> -	 * We need the same args as before, but with this substitution:
> -	 * s!subvol=[^,]+!subvolid=0!
> -	 *
> -	 * Since the replacement string is up to 2 bytes longer than the
> -	 * original, allocate strlen(args) + 2 + 1 bytes.
> -	 */
> +	switch (flags) {
> +	case CLEAR_SUBVOL:
> +		src = strstr(args, "subvol=");
> +		break;
> +	case CLEAR_SUBVOLID:
> +		src = strstr(args, "subvolid=");
> +		break;
> +	}
>  
> -	src = strstr(args, "subvol=");
> -	/* This shouldn't happen, but just in case.. */
> -	if (!src)
> -		return NULL;
> +	if (!src) {
> +		/* Case 2, some args, default subvolume mounting
> +		 * just append ',subvolid=<FS_ROOT>' */
> +
> +		/* 1 for ending '\0', 1 for leading ',' */
> +		len = strlen(args) + strlen(subvol_string) +
> +		      u64_to_strlen(subvol_objectid) + 2;
> +		dst = kmalloc(len, GFP_NOFS);
> +		if (!dst)
> +			return NULL;
> +		strcpy(dst, args);
> +		sprintf(dst + strlen(args), ",%s%llu", subvol_string,
> +			subvol_objectid);
> +		return dst;
> +	}
> +
> +	/* Case 3, subvolid=/subvol=  mount
> +	 * repalce the 'subvolid/subvol' options to 'subvolid=<FS_ROOT>' */
> +	comma = strchr(src, ',');
> +	if (comma)
> +		option_len = comma - src;
> +	else
> +		option_len = strlen(src);
> +	len = strlen(args) - option_len  + strlen(subvol_string) +
> +	      u64_to_strlen(subvol_objectid) + 1;
>  
>  	buf = dst = kmalloc(len, GFP_NOFS);
>  	if (!buf)
> @@ -1154,28 +1208,126 @@ static char *setup_root_args(char *args)
>  		dst += strlen(args);
>  	}
>  
> -	strcpy(dst, "subvolid=0");
> -	dst += strlen("subvolid=0");
> +	len = sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
> +	dst += len;
>  
>  	/*
>  	 * If there is a "," after the original subvol=... string,
>  	 * copy that suffix into our buffer.  Otherwise, we're done.
>  	 */
> -	src = strchr(src, ',');
> -	if (src)
> -		strcpy(dst, src);
> +	if (comma)
> +		strcpy(dst, comma);
>  
>  	return buf;
>  }
>  
> -static struct dentry *mount_subvol(const char *subvol_name, int flags,
> -				   const char *device_name, char *data)
> +static char *str_append_head(char *dest, char *src)

I think this is called 'prepend' :)

> +{
> +	memmove(dest + strlen(src), dest, strlen(dest) + 1);
> +	memcpy(dest, src, strlen(src));

Yes, prepends src to dest inplace.

> +	return dest;
> +}
> +
> +/* Find the path for given subvol_objectid.
> + * Caller needs to readlock the root tree and kzalloc PATH_MAX for
> + * subvol_name and namebuf */
> +static char *find_subvol_by_id(struct btrfs_root *root, u64 subvol_objectid)
> +{
> +	struct btrfs_key key;
> +	struct btrfs_key found_key;
> +	struct btrfs_root_ref *ref;
> +	struct btrfs_path *path;
> +	char *namebuf = NULL;
> +	char *new_buf = NULL;
> +	char *subvol_ret = NULL;
> +	int ret = 0;
> +	u16 namelen = 0;
> +
> +	path = btrfs_alloc_path();
> +	/* Alloc 1 byte for later strlen() calls */
> +	subvol_ret = kzalloc(1, GFP_NOFS);
> +	if (!path || !subvol_ret) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	key.objectid = subvol_objectid;
> +	key.type = BTRFS_ROOT_BACKREF_KEY;
> +	key.offset = 0;
> +	/* We don't need to lock the tree_root,
> +	 * if when we do the backref walking, some one deleted/moved
> +	 * the subvol, we just return -ENOENT or let mount_subtree
> +	 * return -ENOENT and no disaster will happen.
> +	 * User should not modify subvolume when trying to mount it */
> +	while (key.objectid != BTRFS_FS_TREE_OBJECTID) {
> +		ret = btrfs_search_slot_for_read(root, &key, path, 1, 1);
> +		if (ret < 0)
> +			goto out;
> +		if (ret) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
> +				      path->slots[0]);
> +		if (found_key.objectid != key.objectid ||
> +		    found_key.type != BTRFS_ROOT_BACKREF_KEY) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +		key.objectid = found_key.offset;
> +		ref = btrfs_item_ptr(path->nodes[0], path->slots[0],
> +				     struct btrfs_root_ref);
> +		namelen = btrfs_root_ref_name_len(path->nodes[0], ref);
> +		/* One for ending '\0' One for '/' */
> +		new_buf = krealloc(namebuf, namelen + 2, GFP_NOFS);
> +		if (!new_buf) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		namebuf = new_buf;
> +		read_extent_buffer(path->nodes[0], namebuf,
> +				   (unsigned long)(ref + 1), namelen);
> +		btrfs_release_path(path);
> +		*(namebuf + namelen) = '/';
> +		*(namebuf + namelen + 1) = '\0';
> +
> +		new_buf = krealloc(subvol_ret, strlen(subvol_ret) + namelen + 2,
> +				   GFP_NOFS);
> +		if (!new_buf) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		subvol_ret = new_buf;
> +		str_append_head(subvol_ret, namebuf);
> +	}
> +out:
> +	kfree(namebuf);
> +	btrfs_free_path(path);
> +	if (ret < 0) {
> +		kfree(subvol_ret);
> +		return ERR_PTR(ret);
> +	} else
> +		return subvol_ret;
> +
> +}
> +
> +static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
> +				   int flags, const char *device_name,
> +				   char *data)
>  {
>  	struct dentry *root;
>  	struct vfsmount *mnt;
> +	struct btrfs_root *tree_root = NULL;
>  	char *newargs;
> +	char *subvol_ret = NULL;
> +	int ret = 0;
>  
> -	newargs = setup_root_args(data);
> +	if (subvol_name)
> +		newargs = setup_root_args(data, CLEAR_SUBVOL,
> +					  BTRFS_FS_TREE_OBJECTID);
> +	else
> +		newargs = setup_root_args(data, CLEAR_SUBVOLID,
> +					  BTRFS_FS_TREE_OBJECTID);

There's no other value passed to setup_root_args than
BTRFS_FS_TREE_OBJECTID? So you can put it directly to setup_root_args,
or I'm missing someting.

Re: [PATCH 1/2] btrfs: Call mount_subtree() even 'subvolid=' mount option is given.

[Qu Wenruo]

Thanks for your comment.

I'm very sorry that this patch takes your time to review, but later 
patch(show_path one) should replace this patch.
As mentioned in that thread, this patch is not completly working.
And in fact, show_path() patch is the v2 version of this patch, but due 
to change of patch name, I didn't add the v2
tag.

Thanks,
Qu

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs: Call mount_subtree() even 'subvolid=' 
mount option is given.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年07月24日 20:48
> On Wed, Jul 16, 2014 at 12:07:10PM +0800, Qu Wenruo wrote:
>> btrfs uses differnet routine to handle 'subvolid=' and 'subvol=' mount
>> option.
>> Given 'subvol=' mount option, btrfs will mount btrfs first and then call
>> mount_subtree() to mount a subtree of btrfs, making vfs handle the path
>> searching.
>> This is good since vfs layer know extactly that a subtree mount is done
>> and findmnt(8) knows which subtree is mounted.
>>
>> However when using 'subvolid=' mount option, btrfs will do all the
>> internal subvolume objectid searching and checking, making VFS unaware
>> about which subtree is mounted, as result, findmnt(8) can't showing any
>> useful subtree mount info for end users.
>>
>> This patch will use the root backref to reverse search the subvolume
>> path for a given subvolid, making findmnt(8) works again.
>>
>> Reported-by: Stefan G.Weichinger <lists@xunil.at>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Ack for unifying the way subvol= and subvolid= are handled, but I don't
> like some aspects of the implementation.
>
> The kmalloc/krealloc makes it really complicated and is not imho
> necessary. The mount options length is limited to PAGE_SIZE in the vfs
> code. Do the same here, allocate a page, filter the options, do the
> necessary processing and just check for overflows.
>
> You can drop u64_to_strlen.
>
>> +#define CLEAR_SUBVOL		1
>> +#define CLEAR_SUBVOLID		2
> Though they're internal and local to the file, please add BTRFS_ prefix
> at least.
>
>>   /*
>> - * This will strip out the subvol=%s argument for an argument string and add
>> - * subvolid=0 to make sure we get the actual tree root for path walking to the
>> - * subvol we want.
>> + * This will strip out the subvol=%s or subvolid=%s argument for an argumen
>> + * string and add subvolid=0 to make sure we get the actual tree root for path
>> + * walking to the subvol we want.
>>    */
>> -static char *setup_root_args(char *args)
>> +static char *setup_root_args(char *args, int flags, u64 subvol_objectid)
>>   {
>> -	unsigned len = strlen(args) + 2 + 1;
>> -	char *src, *dst, *buf;
>> +	unsigned len;
>> +	char *src = NULL, *dst, *buf, *comma;
> Please use the recommended style and put each on a separate line. I'm
> not sure if you'll need all of them for the implementation witouth the
> kmallocs, the comment applies generally.
>
>> +	char *subvol_string = "subvolid=";
>> +	int option_len = 0;
>> +
>> +	if (!args) {
>> +		/* Case 1, not args, all default mounting
>> +		 * just return 'subvolid=<FS_ROOT>' */
> Not the preferred style of comments.
>
>> +		len = strlen(subvol_string) +
>> +		      u64_to_strlen(subvol_objectid) + 1;
>> +		dst = kmalloc(len, GFP_NOFS);
>> +		if (!dst)
>> +			return NULL;
>> +		sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
>> +		return dst;
>> +	}
>>   
>> -	/*
>> -	 * We need the same args as before, but with this substitution:
>> -	 * s!subvol=[^,]+!subvolid=0!
>> -	 *
>> -	 * Since the replacement string is up to 2 bytes longer than the
>> -	 * original, allocate strlen(args) + 2 + 1 bytes.
>> -	 */
>> +	switch (flags) {
>> +	case CLEAR_SUBVOL:
>> +		src = strstr(args, "subvol=");
>> +		break;
>> +	case CLEAR_SUBVOLID:
>> +		src = strstr(args, "subvolid=");
>> +		break;
>> +	}
>>   
>> -	src = strstr(args, "subvol=");
>> -	/* This shouldn't happen, but just in case.. */
>> -	if (!src)
>> -		return NULL;
>> +	if (!src) {
>> +		/* Case 2, some args, default subvolume mounting
>> +		 * just append ',subvolid=<FS_ROOT>' */
>> +
>> +		/* 1 for ending '\0', 1 for leading ',' */
>> +		len = strlen(args) + strlen(subvol_string) +
>> +		      u64_to_strlen(subvol_objectid) + 2;
>> +		dst = kmalloc(len, GFP_NOFS);
>> +		if (!dst)
>> +			return NULL;
>> +		strcpy(dst, args);
>> +		sprintf(dst + strlen(args), ",%s%llu", subvol_string,
>> +			subvol_objectid);
>> +		return dst;
>> +	}
>> +
>> +	/* Case 3, subvolid=/subvol=  mount
>> +	 * repalce the 'subvolid/subvol' options to 'subvolid=<FS_ROOT>' */
>> +	comma = strchr(src, ',');
>> +	if (comma)
>> +		option_len = comma - src;
>> +	else
>> +		option_len = strlen(src);
>> +	len = strlen(args) - option_len  + strlen(subvol_string) +
>> +	      u64_to_strlen(subvol_objectid) + 1;
>>   
>>   	buf = dst = kmalloc(len, GFP_NOFS);
>>   	if (!buf)
>> @@ -1154,28 +1208,126 @@ static char *setup_root_args(char *args)
>>   		dst += strlen(args);
>>   	}
>>   
>> -	strcpy(dst, "subvolid=0");
>> -	dst += strlen("subvolid=0");
>> +	len = sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
>> +	dst += len;
>>   
>>   	/*
>>   	 * If there is a "," after the original subvol=... string,
>>   	 * copy that suffix into our buffer.  Otherwise, we're done.
>>   	 */
>> -	src = strchr(src, ',');
>> -	if (src)
>> -		strcpy(dst, src);
>> +	if (comma)
>> +		strcpy(dst, comma);
>>   
>>   	return buf;
>>   }
>>   
>> -static struct dentry *mount_subvol(const char *subvol_name, int flags,
>> -				   const char *device_name, char *data)
>> +static char *str_append_head(char *dest, char *src)
> I think this is called 'prepend' :)
>
>> +{
>> +	memmove(dest + strlen(src), dest, strlen(dest) + 1);
>> +	memcpy(dest, src, strlen(src));
> Yes, prepends src to dest inplace.
>
>> +	return dest;
>> +}
>> +
>> +/* Find the path for given subvol_objectid.
>> + * Caller needs to readlock the root tree and kzalloc PATH_MAX for
>> + * subvol_name and namebuf */
>> +static char *find_subvol_by_id(struct btrfs_root *root, u64 subvol_objectid)
>> +{
>> +	struct btrfs_key key;
>> +	struct btrfs_key found_key;
>> +	struct btrfs_root_ref *ref;
>> +	struct btrfs_path *path;
>> +	char *namebuf = NULL;
>> +	char *new_buf = NULL;
>> +	char *subvol_ret = NULL;
>> +	int ret = 0;
>> +	u16 namelen = 0;
>> +
>> +	path = btrfs_alloc_path();
>> +	/* Alloc 1 byte for later strlen() calls */
>> +	subvol_ret = kzalloc(1, GFP_NOFS);
>> +	if (!path || !subvol_ret) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	key.objectid = subvol_objectid;
>> +	key.type = BTRFS_ROOT_BACKREF_KEY;
>> +	key.offset = 0;
>> +	/* We don't need to lock the tree_root,
>> +	 * if when we do the backref walking, some one deleted/moved
>> +	 * the subvol, we just return -ENOENT or let mount_subtree
>> +	 * return -ENOENT and no disaster will happen.
>> +	 * User should not modify subvolume when trying to mount it */
>> +	while (key.objectid != BTRFS_FS_TREE_OBJECTID) {
>> +		ret = btrfs_search_slot_for_read(root, &key, path, 1, 1);
>> +		if (ret < 0)
>> +			goto out;
>> +		if (ret) {
>> +			ret = -ENOENT;
>> +			goto out;
>> +		}
>> +		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
>> +				      path->slots[0]);
>> +		if (found_key.objectid != key.objectid ||
>> +		    found_key.type != BTRFS_ROOT_BACKREF_KEY) {
>> +			ret = -ENOENT;
>> +			goto out;
>> +		}
>> +		key.objectid = found_key.offset;
>> +		ref = btrfs_item_ptr(path->nodes[0], path->slots[0],
>> +				     struct btrfs_root_ref);
>> +		namelen = btrfs_root_ref_name_len(path->nodes[0], ref);
>> +		/* One for ending '\0' One for '/' */
>> +		new_buf = krealloc(namebuf, namelen + 2, GFP_NOFS);
>> +		if (!new_buf) {
>> +			ret = -ENOMEM;
>> +			goto out;
>> +		}
>> +		namebuf = new_buf;
>> +		read_extent_buffer(path->nodes[0], namebuf,
>> +				   (unsigned long)(ref + 1), namelen);
>> +		btrfs_release_path(path);
>> +		*(namebuf + namelen) = '/';
>> +		*(namebuf + namelen + 1) = '\0';
>> +
>> +		new_buf = krealloc(subvol_ret, strlen(subvol_ret) + namelen + 2,
>> +				   GFP_NOFS);
>> +		if (!new_buf) {
>> +			ret = -ENOMEM;
>> +			goto out;
>> +		}
>> +		subvol_ret = new_buf;
>> +		str_append_head(subvol_ret, namebuf);
>> +	}
>> +out:
>> +	kfree(namebuf);
>> +	btrfs_free_path(path);
>> +	if (ret < 0) {
>> +		kfree(subvol_ret);
>> +		return ERR_PTR(ret);
>> +	} else
>> +		return subvol_ret;
>> +
>> +}
>> +
>> +static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
>> +				   int flags, const char *device_name,
>> +				   char *data)
>>   {
>>   	struct dentry *root;
>>   	struct vfsmount *mnt;
>> +	struct btrfs_root *tree_root = NULL;
>>   	char *newargs;
>> +	char *subvol_ret = NULL;
>> +	int ret = 0;
>>   
>> -	newargs = setup_root_args(data);
>> +	if (subvol_name)
>> +		newargs = setup_root_args(data, CLEAR_SUBVOL,
>> +					  BTRFS_FS_TREE_OBJECTID);
>> +	else
>> +		newargs = setup_root_args(data, CLEAR_SUBVOLID,
>> +					  BTRFS_FS_TREE_OBJECTID);
> There's no other value passed to setup_root_args than
> BTRFS_FS_TREE_OBJECTID? So you can put it directly to setup_root_args,
> or I'm missing someting.