[v2] Btrfs: deal with all 'subvol=xxx' options once
diff mbox

Message ID 1416903611-29812-1-git-send-email-wangshilong1991@gmail.com
State New, archived
Headers show

Commit Message

Wang Shilong Nov. 25, 2014, 8:20 a.m. UTC
Steps to reproduce:
 # mkfs.btrfs -f /dev/sdb
 # mount -t btrfs /dev/sdb /mnt
 # btrfs sub create /mnt/dir
 # mount -t btrfs /dev/sdb /mnt -o subvol=dir,subvol=dir

It fails with:
 mount: mount(2) failed: No such file or directory

Btrfs deal with subvolume mounting in a recursive way,
to avoid looping, it will stripe out 'subvol=xxxx' string,
then next loop will stop.Problem here is it only deal one
string once, if users specify mount option multiple times.
It will loop several times which is not good, and above
reproducing steps will also return confusing results.

Fix this problem by striping out all 'subvol=xxx' options,
only last is valid.

Signed-off-by: Wang Shilong <wangshilong1991@gmail.com>
---
v1->v2: error handling and comment update
---
 fs/btrfs/super.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Comments

David Sterba Nov. 25, 2014, 5:36 p.m. UTC | #1
On Tue, Nov 25, 2014 at 04:20:11PM +0800, Wang Shilong wrote:
> Steps to reproduce:
>  # mkfs.btrfs -f /dev/sdb
>  # mount -t btrfs /dev/sdb /mnt
>  # btrfs sub create /mnt/dir
>  # mount -t btrfs /dev/sdb /mnt -o subvol=dir,subvol=dir
> 
> It fails with:
>  mount: mount(2) failed: No such file or directory

The bug is real, but I don't like the fix. The mount path is hard to
read already, and I'm afraid your fix adds another unobvious step to the
whole processing.

setup_root_args replaces subvol= with subvolid=0 once. I suggest to
replace all occurences of subvol= here and not rely on the recursive
behaviour of the mount callbacks.

The (buggy) way how it works now is that the first occurence of subvol
will get parsed and passed as

newroot = vfs_kern_mount(",subvol=second,...,subvolid=0")

and this will call back again to btrfs_mount and will try to mount the
subvol 'second' but now relative to 'newroot'.

Try this:

# mkfs.btrfs -f /dev/sdb
# mount -t btrfs /dev/sdb /mnt
# btrfs sub create /mnt/dir
# btrfs sub create /mnt/dir/dir2
# mount -t btrfs /dev/sdb /mnt -o subvol=dir,subvol=dir2

mount succeeds and the mounted subvolume is dir2.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wang Shilong Nov. 26, 2014, 1:36 a.m. UTC | #2
> 
> On Tue, Nov 25, 2014 at 04:20:11PM +0800, Wang Shilong wrote:
>> Steps to reproduce:
>> # mkfs.btrfs -f /dev/sdb
>> # mount -t btrfs /dev/sdb /mnt
>> # btrfs sub create /mnt/dir
>> # mount -t btrfs /dev/sdb /mnt -o subvol=dir,subvol=dir
>> 
>> It fails with:
>> mount: mount(2) failed: No such file or directory
> 
> The bug is real, but I don't like the fix. The mount path is hard to
> read already, and I'm afraid your fix adds another unobvious step to the
> whole processing.
> 
> setup_root_args replaces subvol= with subvolid=0 once. I suggest to
> replace all occurences of subvol= here and not rely on the recursive
> behaviour of the mount callbacks.

ok, if you like this way, i will do it.


> 
> The (buggy) way how it works now is that the first occurence of subvol
> will get parsed and passed as
> 
> newroot = vfs_kern_mount(",subvol=second,...,subvolid=0")
> 
> and this will call back again to btrfs_mount and will try to mount the
> subvol 'second' but now relative to 'newroot'.
> 
> Try this:
> 
> # mkfs.btrfs -f /dev/sdb
> # mount -t btrfs /dev/sdb /mnt
> # btrfs sub create /mnt/dir
> # btrfs sub create /mnt/dir/dir2
> # mount -t btrfs /dev/sdb /mnt -o subvol=dir,subvol=dir2
> 
> mount succeeds and the mounted subvolume is dir2.

Best Regards,
Wang Shilong

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 54bd91e..42f3176 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1115,7 +1115,7 @@  static inline int is_subvolume_inode(struct inode *inode)
  * 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)
 {
 	unsigned len = strlen(args) + 2 + 1;
 	char *src, *dst, *buf;
@@ -1129,13 +1129,12 @@  static char *setup_root_args(char *args)
 	 */
 
 	src = strstr(args, "subvol=");
-	/* This shouldn't happen, but just in case.. */
 	if (!src)
 		return NULL;
 
 	buf = dst = kmalloc(len, GFP_NOFS);
 	if (!buf)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	/*
 	 * If the subvol= arg is not at the start of the string,
@@ -1161,6 +1160,27 @@  static char *setup_root_args(char *args)
 	return buf;
 }
 
+static char *setup_root_args(char *args)
+{
+	char *p, *new_args;
+
+	p = new_args = __setup_root_args(args);
+	/* in case users specify subvol=xxx option multiple times */
+	while (p) {
+		p = __setup_root_args(new_args);
+		if (!p)
+			break;
+		if (!IS_ERR(p)) {
+			kfree(new_args);
+			new_args = p;
+		} else {
+			kfree(new_args);
+			return NULL;
+		}
+	}
+	return new_args;
+}
+
 static struct dentry *mount_subvol(const char *subvol_name, int flags,
 				   const char *device_name, char *data)
 {