[PATCH?] fix mount -a on btrfs bind mount

[PATCH?] fix mount -a on btrfs bind mount

[Stanislav Brabec]

If I have a fstab that contains:

UUID=1f976144-6c23-4622-bf6c-cd9821acc49d /var/log             btrfs      subvol=@/var/log      0 0
/var/log                /local/nfsv4/log                none    bind,private

When calling "mount -a" multiple times, it mounts the bind
mount again and again.

Here are related mountinfo entries with my kernel:
44 34 0:30 /@/var/log /var/log rw,relatime shared:27 - btrfs /dev/vda2 rw,space_cache,subvolid=269,subvol=/@/var/log
95 34 0:30 /@/var/log /local/nfsv4/log rw,relatime - btrfs /dev/vda2 rw,space_cache,subvolid=269,subvol=/@/var/log

Debugging this case, I found that the btrfs specific code in
mnt_table_get_fs_root() returns bad value:

after remove_mountpoint_from_path() root is "/"
mnt_table_find_target(tb, "/var/log", MNT_ITER_BACKWARD)
returns src_root set to "/@/var/log".
startswith() does not match and the snprintf() attempts to
construct the final root value by concatenating of both
strings:

FS root result: /@/var/log/

It looks strange, and it does not work:

Then it returns to mnt_table_is_fs_mounted(), and the iterator
while (mnt_table_next_fs(tb, &itr, &fs) == 0)
could never get a match.
In section starting with if (root)
and mnt_fs_get_root(fs) returns "/@/var/log"
This is compared with "/@/var/log/" above, and match is never
got.

Here is a possible easy fix. If it is correct, the code should
be simplified to remove string copying there.

I am not sure whether it has any side effect, but it helps in
my case.

Note that 41510d26ddd9a4c83f8589dded539e2985535dcf added a
patch to "else if" part of this if, that is also a bit
suspicious: btrfs path should not contain "//" instead of
"/{subvolume}/", at least not in a correct use.

---
 libmount/src/tab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 0df8d49..44894e9 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -1252,7 +1252,7 @@ struct libmnt_fs *mnt_table_get_fs_root(struct libmnt_table *tb,
 
 			if (!tmp)
 				goto err;
-			snprintf(tmp, sz, "%s%s", src_root, root);
+			snprintf(tmp, sz, "%s", src_root);
 			free(root);
 			root = tmp;
 		}
-- 
2.6.3

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

[PATCH?] fix mount -a on btrfs bind mount

[Stanislav Brabec]

If I have a fstab that contains:

UUID=1f976144-6c23-4622-bf6c-cd9821acc49d /var/log             btrfs 
   subvol=@/var/log      0 0
/var/log                /local/nfsv4/log                none    bind,private

When calling "mount -a" multiple times, it mounts the bind
mount again and again.

Here are related mountinfo entries with my kernel:
44 34 0:30 /@/var/log /var/log rw,relatime shared:27 - btrfs /dev/vda2 
rw,space_cache,subvolid=269,subvol=/@/var/log
95 34 0:30 /@/var/log /local/nfsv4/log rw,relatime - btrfs /dev/vda2 
rw,space_cache,subvolid=269,subvol=/@/var/log

Debugging this case, I found that the btrfs specific code in
mnt_table_get_fs_root() returns bad value:

after remove_mountpoint_from_path() root is "/"
mnt_table_find_target(tb, "/var/log", MNT_ITER_BACKWARD)
returns src_root set to "/@/var/log".
startswith() does not match and the snprintf() attempts to
construct the final root value by concatenating of both
strings:

FS root result: /@/var/log/

It looks strange, and it does not work:

Then it returns to mnt_table_is_fs_mounted(), and the iterator
while (mnt_table_next_fs(tb, &itr, &fs) == 0)
could never get a match.
In section starting with if (root)
and mnt_fs_get_root(fs) returns "/@/var/log"
This is compared with "/@/var/log/" above, and match is never
got.

Here is a possible easy fix. If it is correct, the code should
be simplified to remove string copying there.

I am not sure whether it has any side effect, but it helps in
my case.

Note that 41510d26ddd9a4c83f8589dded539e2985535dcf added a
patch to "else if" part of this if, that is also a bit
suspicious: btrfs path should not contain "//" instead of
"/{subvolume}/", at least not in a correct use.

---
  libmount/src/tab.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 0df8d49..44894e9 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -1252,7 +1252,7 @@ struct libmnt_fs *mnt_table_get_fs_root(struct 
libmnt_table *tb,

  			if (!tmp)
  				goto err;
-			snprintf(tmp, sz, "%s%s", src_root, root);
+			snprintf(tmp, sz, "%s", src_root);
  			free(root);
  			root = tmp;
  		}
-- 
2.6.3

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

[PATCH] fix mount -a on btrfs bind mount

[Stanislav Brabec]

Do not prepend src_root to root for bind mounts on btrfs.

root has to be literally the same as src_root, which is already the path
prefixed by /{subvolume}. Otherwise it will not match in
mnt_fs_streq_target(fs, xtgt) inside mnt_table_is_fs_mounted() and
repeated "mount -a" will mount it again and again.

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
---
 libmount/src/tab.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 0df8d49..eeb9c57 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -1247,12 +1247,9 @@ struct libmnt_fs *mnt_table_get_fs_root(struct libmnt_table *tb,
 		 */
 		src_root = mnt_fs_get_root(src_fs);
 		if (src_root && !startswith(root, src_root)) {
-			size_t sz = strlen(root) + strlen(src_root) + 1;
-			char *tmp = malloc(sz);
-
+			char *tmp = strdup (src_root);
 			if (!tmp)
 				goto err;
-			snprintf(tmp, sz, "%s%s", src_root, root);
 			free(root);
 			root = tmp;
 		}
-- 
2.6.3

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] fix mount -a on btrfs bind mount

[Karel Zak]

On Fri, Nov 27, 2015 at 06:15:41PM +0100, Stanislav Brabec wrote:
> root has to be literally the same as src_root, which is already the path
> prefixed by /{subvolume}. Otherwise it will not match in
> mnt_fs_streq_target(fs, xtgt) inside mnt_table_is_fs_mounted() and
> repeated "mount -a" will mount it again and again.

yes

>  		if (src_root && !startswith(root, src_root)) {
> -			size_t sz = strlen(root) + strlen(src_root) + 1;
> -			char *tmp = malloc(sz);
> -
> +			char *tmp = strdup (src_root);
>  			if (!tmp)
>  				goto err;
> -			snprintf(tmp, sz, "%s%s", src_root, root);
>  			free(root);
>  			root = tmp;
>  		}

but this code is there for another use case. The problem is when the
fstab_fs source is *sub-directory* on btrfs subvolume (or another bind
mount). For example imagine:

  /dev/sdc        /mnt/test       btrfs   subvol=/anydir
  /mnt/test/foo   /mnt/test2      auto    bind

then FS root for /mnt/test2 is /anydir/foo on /dev/sdc. The problem is
that the code does not recognize your use-case with "/" (no sub-directory 
on source FS).

My bugfix:

 https://github.com/karelzak/util-linux/commit/352740e88e2c9cb180fe845ce210b1c7b5ad88c7

try it :-)

Anyway, we need more regression tests for these crazy use-cases.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] fix mount -a on btrfs bind mount

[Stanislav Brabec]

On Tue, Dec 2, 2015 at 01:51PM +0100, Karel Zak wrote:
> My bugfix:
>
>   https://github.com/karelzak/util-linux/commit/352740e88e2c9cb180fe845ce210b1c7b5ad88c7
>
> try it :-)
>
If also fixed my problem.

Thanks.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] fix mount -a on btrfs bind mount

[David Sterba]

On Wed, Dec 02, 2015 at 01:51:17PM +0100, Karel Zak wrote:
> On Fri, Nov 27, 2015 at 06:15:41PM +0100, Stanislav Brabec wrote:
> > root has to be literally the same as src_root, which is already the path
> > prefixed by /{subvolume}. Otherwise it will not match in
> > mnt_fs_streq_target(fs, xtgt) inside mnt_table_is_fs_mounted() and
> > repeated "mount -a" will mount it again and again.
> 
> yes
> 
> >  		if (src_root && !startswith(root, src_root)) {
> > -			size_t sz = strlen(root) + strlen(src_root) + 1;
> > -			char *tmp = malloc(sz);
> > -
> > +			char *tmp = strdup (src_root);
> >  			if (!tmp)
> >  				goto err;
> > -			snprintf(tmp, sz, "%s%s", src_root, root);
> >  			free(root);
> >  			root = tmp;
> >  		}
> 
> but this code is there for another use case. The problem is when the
> fstab_fs source is *sub-directory* on btrfs subvolume (or another bind
> mount). For example imagine:
> 
>   /dev/sdc        /mnt/test       btrfs   subvol=/anydir

Only subvolumes are accepted as parameter for 'subvol=', otherwise bind
mount needs to be used.