[PATCH] tests: add btrfs mount tests

[PATCH] tests: add btrfs mount tests

[Stanislav Brabec]

btrfs needs a special support in mount. Add a testcase for btrfs specific
problems.

Coverage:
352740e8: bind mounts pointing to btrfs subvolume
2cd28fc8: mounting default subvolume
d2f82678: use of "auto"
618a8814: use of "subvolid"

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
---
 tests/expected/mount/fstab-btrfs |  1 +
 tests/ts/mount/fstab-btrfs       | 99 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)
 create mode 100644 tests/expected/mount/fstab-btrfs
 create mode 100755 tests/ts/mount/fstab-btrfs

diff --git a/tests/expected/mount/fstab-btrfs b/tests/expected/mount/fstab-btrfs
new file mode 100644
index 0000000..3582111
--- /dev/null
+++ b/tests/expected/mount/fstab-btrfs
@@ -0,0 +1 @@
+Success
diff --git a/tests/ts/mount/fstab-btrfs b/tests/ts/mount/fstab-btrfs
new file mode 100755
index 0000000..1c1d0fd
--- /dev/null
+++ b/tests/ts/mount/fstab-btrfs
@@ -0,0 +1,99 @@
+#!/bin/bash
+
+#
+# Copyright (C) 2016 Stanislav Brabec <sbrabec@suse.cz>
+#
+# This file is part of util-linux.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This file is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+TS_TOPDIR="${0%/*}/../.."
+TS_DESC="btrfs (fstab)"
+
+. $TS_TOPDIR/functions.sh
+ts_init "$*"
+
+ts_check_test_command "$TS_CMD_MOUNT"
+ts_check_test_command "$TS_CMD_UMOUNT"
+
+ts_skip_nonroot
+ts_check_losetup
+ts_check_prog "mkfs.btrfs"
+ts_check_prog "btrfs"
+
+ts_device_init 42
+DEVICE=$TS_LODEV
+[ -d "$TS_MOUNTPOINT-create" ] || mkdir -p "$TS_MOUNTPOINT-create"
+[ -d "$TS_MOUNTPOINT-default" ] || mkdir -p "$TS_MOUNTPOINT-default"
+[ -d "$TS_MOUNTPOINT-subvol" ] || mkdir -p "$TS_MOUNTPOINT-subvol"
+[ -d "$TS_MOUNTPOINT-subvolid" ] || mkdir -p "$TS_MOUNTPOINT-subvolid"
+[ -d "$TS_MOUNTPOINT-bind" ] || mkdir -p "$TS_MOUNTPOINT-bind"
+mkfs.btrfs -f -d single -m single $DEVICE &> /dev/null || ts_die "Cannot make btrfs on $DEVICE"
+
+$TS_CMD_MOUNT -o loop "$DEVICE" "$TS_MOUNTPOINT-create"
+pushd . >/dev/null
+cd "$TS_MOUNTPOINT-create"
+mkdir -p d0/dd0/ddd0
+cd ./d0/dd0/ddd0
+touch file{1..5}
+btrfs subvol create s1 >/dev/null
+cd ./s1
+touch file{1..5}
+mkdir -p d1/dd1/ddd1
+cd ./d1/dd1/ddd1
+btrfs subvol create s2 >/dev/null
+DEFAULT_SUBVOLID=$(btrfs inspect rootid s2)
+btrfs subvol set-default $DEFAULT_SUBVOLID . >/dev/null
+NON_DEFAULT_SUBVOLID=$(btrfs subvol list "$TS_MOUNTPOINT-create" | while read dummy id rest ; do if test $id = $DEFAULT_SUBVOLID ; then continue ; fi ; echo $id ; done)
+cd ../../../..
+mkdir -p d2/dd2/ddd2
+cd ./d2/dd2/ddd2
+btrfs subvol create s3 >/dev/null
+popd >/dev/null
+NON_DEFAULT_SUBVOL=d0/dd0/ddd0/d2/dd2/ddd2/s3
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT-create"
+
+# Tests with fs == btrfs
+# mounting default subvolume, deep in the structure, without entry in fstab
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-default" "btrfs" ""
+# mounting default subvolume, deep in the structure
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvol" "btrfs" "subvol=$NON_DEFAULT_SUBVOL"
+# mounting non-default subvolume
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvolid" "btrfs" "subvolid=$NON_DEFAULT_SUBVOLID"
+
+# test bind mount pointing to subvolume root
+ts_fstab_add "$TS_MOUNTPOINT-subvol" "$TS_MOUNTPOINT-bind" "auto" "bind"
+
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+
+ts_fstab_clean
+
+# Tests with fs == auto
+# mounting default subvolume, deep in the structure, without entry in fstab
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-default" "auto" ""
+# mounting default subvolume, deep in the structure
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvol" "auto" "subvol=$NON_DEFAULT_SUBVOL"
+# mounting non-default subvolume
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvolid" "auto" "subvolid=$NON_DEFAULT_SUBVOLID"
+
+# test bind mount pointing to subvolume sub-directory
+mkdir -p "$TS_MOUNTPOINT-subvol/bind-mnt"
+ts_fstab_add "$TS_MOUNTPOINT-subvol/bind-mnt" "$TS_MOUNTPOINT-bind" "auto" "bind"
+
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+
+ts_fstab_clean
+
+ts_log "Success"
+ts_finalize
+
-- 
2.7.0

-- 
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] tests: add btrfs mount tests (fails!)

[Stanislav Brabec]

Stanislav Brabec wrote:
> Coverage:

I just found a bug in my testcase:
To prevent race bind-mnt has to be created in time of fs creation.

  btrfs subvol create s3 >/dev/null
+mkdir -p s3/bind-mnt
  popd >/dev/null

But, what is worse, I found a problem in previous patches.

Things are even more complicated than I thought, and patches still not 
cover all cases.

Up to now, I have been thinking that device + subvolid is unique 
identifier in the mountinfo. But it is not true, at least not when bind 
mounts take its role.

Using bind mounts that point to a sub-directory of a subvolume of btrfs 
results in two lines in mountinfo that have the same subvolid but 
different subvol.

Second "mount -a" then fails badly. Not only that it mounts the bind 
mount second time, it also mistakenly mounts another volume over a 
previously mounted volume.

dev/loop0 on 
/home/sbrabec/VCS/util-linux/tests/output/mount/fstab-btrfs-mnt-subvol 
type btrfs 
(rw,relatime,space_cache,subvolid=258,subvol=/d0/dd0/ddd0/d2/dd2/ddd2/s3/bind-mnt)
=> totally wrong

We are back on the beginning.

I have to re-evaluate all patches I sent up to now.

TODO:

Check whether it is possible to directly mount sub-directory of 
sub-volume that is not a subvolume per self.

If yes:
- We will need to introduce full path evaluation of btrfs sub-volume, 
and always compare it with mountinfo.

If not:
- We will have to search the whole mountinfo for a particular subvolid 
and and find a shortest subvol string.

David, could you bring some light into it?

-- 
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] tests: add btrfs mount tests (fails!)

[Stanislav Brabec]

Stanislav Brabec wrote:
> (rw,relatime,space_cache,subvolid=258,subvol=/d0/dd0/ddd0/d2/dd2/ddd2/s3/bind-mnt)

After a discussion with David, we came to the conclusion that the
problem is caused by bug in the mountinfo provided by kernel:

It reports invalid subvol=/d0/dd0/ddd0/d2/dd2/ddd2/s3/bind-mnt, which
breaks both the logic inside new patches, and the convention that it is
possible to create a mount command from the options reported by
mountinfo.

After a further research, we found, that kernel is currently unfixable
in a correct way without refactoring of btrfs or changes in vfs.

It implies that fstab support for btrfs is not fully fixable. But it is
possible to write a heuristic, that can work in most cases, and in
other cases it will behave at least reasonably (i. e. not mount again,
not mount incorrect directories etc.)


Here is the case that CANNOT be fixed correctly:

Suppose you have following fstab:

/dev/sda1                 /           btrfs subvol=.snapshots/1/snapshot 0 0
/dev/sda1                 /.snapshots btrfs subvol=.snapshots            0 0

Now suppose additional line:
/var                      /mnt/var    none  bind                         0 0

And following line:
.snapshots/1/snapshot/var /mnt/var    none  bind                         0 0

However both lines mount the same part of btrfs hierarchy, files inside
will have different inode numbers for each.

mountinfo provided by the current btrfs is not sufficient to
discriminate between them, and what is even worse, in second case it
reports incorrect subvolid.


As we cannot expect correct fix for that in next say few years,
util-linux has to live with lies, incorrect subvol, unmountable
subvolid.


Conclusion:

Don't suppose unique relation between subvol and subvolid. Always go
through the whole mountinfo, and find a shortest string corresponding
to the particular subvolid. It is the correct one.

In case of bind mounts, don't trust subvolid and subvol at all. The
only real information is the source. btrfs guesses the rest, often
correctly, sometimes incorrectly. subvol is just a copy of this path
(possibly unusable in real mount command), subvolid is the id of the
nearest parent node, not the real subvolid of the volume the bind mount
points to. If device and source matches, the best guess heuristic says,
that the bind mount is probably already mounted. (It could be false,
but we cannot detect it.)


-- 
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] tests: add btrfs mount tests (fails!)

[Stanislav Brabec]

Stanislav Brabec wrote:

> However both lines mount the same part of btrfs hierarchy, files inside
> will have different inode numbers for each.

Fix:

inode number will be the same, independently on subvolume actually used, 
and the reported subvolid.

But the mount reference count differs depending on the bind mount source.

In the first case, I can run
umount /.snapshots

in the second, I will get EBUSY.

=> The "mount -a" problem can be reasonably fixed, but depending on the 
algorithm, we could encounter unfixable problem during "umount -a".

-- 
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

another mount -a problem, not related to btrfs

[Stanislav Brabec]

Dne 3.2.2016 v 18:00 Stanislav Brabec napsal(a):
> Stanislav Brabec wrote:
>> (rw,relatime,space_cache,subvolid=258,subvol=/d0/dd0/ddd0/d2/dd2/ddd2/s3/bind-mnt)
>>
>
> After a discussion with David, we came to the conclusion that the
> problem is caused by bug in the mountinfo provided by kernel:
>
While working on a fix, I found a new bug, not related to btrfs:

truncate -s10M source.img
mkdir -p /source/subdir
mkdir -p /dest
/sbin/mkfs.ext4 source.img
losetup /dev/loop0 $PWD/source.img
echo "/dev/loop0 /source ext4 defaults 0 0" >>/etc/fstab
mount -a
mkdir -p /source/subdir
mount -o bind /source /dest
echo "/source/subdir /dest auto bind 0 0" >>/etc/fstab
mount -a

Got:
/dev/loop0 on /source type ext4 (rw,relatime,data=ordered)
/dev/loop0 on /dest type ext4 (rw,relatime,data=ordered)
/dev/loop0 on /dest type ext4 (rw,relatime,data=ordered)
/dev/loop0 on /source type ext4 (rw,relatime,data=ordered)
and empty /source

It apparently should not happen.

-- 
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] tests: add btrfs mount tests (fails!)

[Karel Zak]

On Tue, Feb 02, 2016 at 09:14:48PM +0100, Stanislav Brabec wrote:
> Stanislav Brabec wrote:
> >Coverage:
> 
> I just found a bug in my testcase:
> To prevent race bind-mnt has to be created in time of fs creation.
> 
>  btrfs subvol create s3 >/dev/null
> +mkdir -p s3/bind-mnt

So, this is enough to fix the testcase, right? I'd like to merge it.

    Karel

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

Re: [PATCH] tests: add btrfs mount tests (fails!)

[Stanislav Brabec]

Karel Zak wrote:
> On Tue, Feb 02, 2016 at 09:14:48PM +0100, Stanislav Brabec wrote:
>> Stanislav Brabec wrote:
>> +mkdir -p s3/bind-mnt
>
> So, this is enough to fix the testcase, right? I'd like to merge it.
>
>      Karel
>
Well, not.


There are missing umount commands in the middle of the test, which 
trigger following problem:

truncate -s10M source.img
mkdir -p /source/subdir
mkdir -p /dest
/sbin/mkfs.ext4 source.img
losetup /dev/loop0 $PWD/source.img
mount -text4 /dev/loop0 /source
mkdir -p /source/subdir
mount -o bind /source /dest
mount -o bind /source/subdir /dest

Mounting a bind mount over a root of existing bind mount causes mounting 
over the bind mount source. I expected mounting over the target 
directory I specified.

I am not sure, whether it is correct behavior, but for sure it is 
surprising.


Additionally, I am working on a reproducer of an invalid "mount -a" 
behavior triggered by kernel bug reporting false subvol. Now I have only 
a reproducer outside testsuite:

#/btrfs_test.img /btrfs_mnt_root auto loop,subvol=/ 0 0
#/btrfs_test.img /btrfs_mnt auto loop,subvolid=257 0 0
#/btrfs_mnt_root/d0/dd0/ddd0/s1/bind-mnt /btrfs_bind auto bind 0 0

Round 1:
Uncomment 1st and 3rd line and call mount -a.

Round 2:
Uncomment 2nd line and call mount -a.

Result:
Only two mounts exist.

I am working on a subtest that will trigger it inside the test suite.

I already have a possible fix for it, but I want to confirm that it is 
correct before sending it upstream.

-- 
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] tests: add btrfs mount tests (fails!)

[Stanislav Brabec]

Stanislav Brabec wrote:
> I am working on a subtest that will trigger it inside the test suite.
>
> I already have a possible fix for it, but I want to confirm that it is
> correct before sending it upstream.
>
I just have a reproducer of the new problem.

The newly found problem is not btrfs speficic:

echo "/ext4.img /ext4_mnt1 auto loop 0 0" >>/etc/fstab
mount -a
echo "/ext4.img /ext4_mnt2 auto loop 0 0" >>/etc/fstab
mount -a
mount
umount /ext4_mnt1 /ext4_mnt2
sed -i /ext4.img/d /etc/fstab

Result:
Nothing was mounted into /ext4_mnt2

(This is the problem with loop files in fstab, which I mentioned in my 
past mails, but was not able to reproduce.)


It also means, that I still don't have a reproducer that will 
demonstrate mount confused by an incorrect subvol provided by kernel.

-- 
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] tests: add btrfs mount tests

[Stanislav Brabec]

btrfs needs a special support in mount. Add a testcase for btrfs specific
problems.

Coverage:
352740e8: bind mounts pointing to btrfs
2cd28fc8: mounting default subvolume
d2f82678: use of "auto"
618a8814: use of "subvolid"

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
---
 tests/expected/mount/fstab-btrfs       |   1 +
 tests/expected/mount/fstab-btrfs-auto  |   1 +
 tests/expected/mount/fstab-btrfs-btrfs |   1 +
 tests/ts/mount/fstab-btrfs             | 128 +++++++++++++++++++++++++++++++++
 4 files changed, 131 insertions(+)
 create mode 100644 tests/expected/mount/fstab-btrfs
 create mode 100644 tests/expected/mount/fstab-btrfs-auto
 create mode 100644 tests/expected/mount/fstab-btrfs-btrfs
 create mode 100755 tests/ts/mount/fstab-btrfs

diff --git a/tests/expected/mount/fstab-btrfs b/tests/expected/mount/fstab-btrfs
new file mode 100644
index 0000000..3582111
--- /dev/null
+++ b/tests/expected/mount/fstab-btrfs
@@ -0,0 +1 @@
+Success
diff --git a/tests/expected/mount/fstab-btrfs-auto b/tests/expected/mount/fstab-btrfs-auto
new file mode 100644
index 0000000..3582111
--- /dev/null
+++ b/tests/expected/mount/fstab-btrfs-auto
@@ -0,0 +1 @@
+Success
diff --git a/tests/expected/mount/fstab-btrfs-btrfs b/tests/expected/mount/fstab-btrfs-btrfs
new file mode 100644
index 0000000..3582111
--- /dev/null
+++ b/tests/expected/mount/fstab-btrfs-btrfs
@@ -0,0 +1 @@
+Success
diff --git a/tests/ts/mount/fstab-btrfs b/tests/ts/mount/fstab-btrfs
new file mode 100755
index 0000000..0b11fd6
--- /dev/null
+++ b/tests/ts/mount/fstab-btrfs
@@ -0,0 +1,128 @@
+#!/bin/bash
+
+#
+# Copyright (C) 2016 Stanislav Brabec <sbrabec@suse.cz>
+#
+# This file is part of util-linux.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This file is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+TS_TOPDIR="${0%/*}/../.."
+TS_DESC="btrfs (fstab)"
+
+. $TS_TOPDIR/functions.sh
+ts_init "$*"
+
+ts_check_test_command "$TS_CMD_MOUNT"
+ts_check_test_command "$TS_CMD_UMOUNT"
+
+ts_skip_nonroot
+ts_check_losetup
+ts_check_prog "mkfs.btrfs"
+ts_check_prog "btrfs"
+
+TS_MOUNTPOINT_ANY="$TS_MOUNTPOINT"
+TS_MOUNTPOINT_CREATE="$TS_MOUNTPOINT-create"
+TS_MOUNTPOINT_DEFAULT="$TS_MOUNTPOINT-default"
+TS_MOUNTPOINT_SUBVOL="$TS_MOUNTPOINT-subvol"
+TS_MOUNTPOINT_SUBVOLID="$TS_MOUNTPOINT-subvolid"
+TS_MOUNTPOINT_BIND="$TS_MOUNTPOINT-bind"
+
+ts_device_init 42
+DEVICE=$TS_LODEV
+[ -d "$TS_MOUNTPOINT_CREATE" ] || mkdir -p "$TS_MOUNTPOINT_CREATE"
+[ -d "$TS_MOUNTPOINT_DEFAULT" ] || mkdir -p "$TS_MOUNTPOINT_DEFAULT"
+[ -d "$TS_MOUNTPOINT_SUBVOL" ] || mkdir -p "$TS_MOUNTPOINT_SUBVOL"
+[ -d "$TS_MOUNTPOINT_SUBVOLID" ] || mkdir -p "$TS_MOUNTPOINT_SUBVOLID"
+[ -d "$TS_MOUNTPOINT_BIND" ] || mkdir -p "$TS_MOUNTPOINT_BIND"
+mkfs.btrfs -f -d single -m single $DEVICE &> /dev/null || ts_die "Cannot make btrfs on $DEVICE"
+
+$TS_CMD_MOUNT -o loop "$DEVICE" "$TS_MOUNTPOINT_CREATE"
+pushd . >/dev/null
+cd "$TS_MOUNTPOINT_CREATE"
+mkdir -p d0/dd0/ddd0
+cd ./d0/dd0/ddd0
+touch file{1..5}
+btrfs subvol create s1 >/dev/null
+cd ./s1
+touch file{1..5}
+mkdir bind-point
+mkdir -p d1/dd1/ddd1
+cd ./d1/dd1/ddd1
+btrfs subvol create s2 >/dev/null
+DEFAULT_SUBVOLID=$(btrfs inspect rootid s2)
+btrfs subvol set-default $DEFAULT_SUBVOLID . >/dev/null
+NON_DEFAULT_SUBVOLID=$(btrfs subvol list "$TS_MOUNTPOINT_CREATE" | while read dummy id rest ; do if test $id = $DEFAULT_SUBVOLID ; then continue ; fi ; echo $id ; done)
+cd ../../../..
+mkdir -p d2/dd2/ddd2
+cd ./d2/dd2/ddd2
+btrfs subvol create s3 >/dev/null
+mkdir -p s3/bind-mnt
+popd >/dev/null
+NON_DEFAULT_SUBVOL=d0/dd0/ddd0/d2/dd2/ddd2/s3
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_CREATE"
+
+
+ts_init_subtest "btrfs"
+# Tests with fs == btrfs
+# mounting default subvolume, deep in the structure, without entry in fstab
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT_DEFAULT" "btrfs" ""
+# mounting default subvolume, deep in the structure
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT_SUBVOL" "btrfs" "subvol=$NON_DEFAULT_SUBVOL"
+# mounting non-default subvolume
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT_SUBVOLID" "btrfs" "subvolid=$NON_DEFAULT_SUBVOLID"
+# test bind mount pointing to subvolume root
+ts_fstab_add "$TS_MOUNTPOINT_SUBVOLID" "$TS_MOUNTPOINT_BIND" "auto" "bind"
+
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_BIND" 2>&1 >> $TS_OUTPUT
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_DEFAULT" 2>&1 >> $TS_OUTPUT
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_SUBVOL" 2>&1 >> $TS_OUTPUT
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_SUBVOLID" 2>&1 >> $TS_OUTPUT
+
+# check that everything was unmounted
+$TS_CMD_MOUNT | grep "$TS_MOUNTPOINT_ANY" 2>&1 >> $TS_OUTPUT
+
+ts_fstab_clean
+ts_log "Success"
+ts_finalize_subtest
+
+ts_init_subtest "auto"
+# Tests with fs == auto
+# mounting default subvolume, deep in the structure, without entry in fstab
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT_DEFAULT" "auto" ""
+# mounting default subvolume, deep in the structure
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT_SUBVOL" "auto" "subvol=$NON_DEFAULT_SUBVOL"
+# mounting non-default subvolume
+ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT_SUBVOLID" "auto" "subvolid=$NON_DEFAULT_SUBVOLID"
+# test bind mount pointing to subvolume sub-directory
+ts_fstab_add "$TS_MOUNTPOINT_SUBVOL/bind-mnt" "$TS_MOUNTPOINT_BIND" "auto" "bind"
+
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
+
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_BIND" 2>&1 >> $TS_OUTPUT
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_DEFAULT" 2>&1 >> $TS_OUTPUT
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_SUBVOL" 2>&1 >> $TS_OUTPUT
+$TS_CMD_UMOUNT "$TS_MOUNTPOINT_SUBVOLID" 2>&1 >> $TS_OUTPUT
+
+# check that everything was unmounted
+$TS_CMD_MOUNT | grep "$TS_MOUNTPOINT_ANY" 2>&1 >> $TS_OUTPUT
+
+ts_fstab_clean
+ts_log "Success"
+ts_finalize_subtest
+
+
+ts_finalize
+
-- 
2.7.0

-- 
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] tests: add btrfs mount tests

[Karel Zak]

On Thu, Feb 11, 2016 at 07:07:22PM +0100, Stanislav Brabec wrote:
>  tests/expected/mount/fstab-btrfs       |   1 +
>  tests/expected/mount/fstab-btrfs-auto  |   1 +
>  tests/expected/mount/fstab-btrfs-btrfs |   1 +
>  tests/ts/mount/fstab-btrfs             | 128 +++++++++++++++++++++++++++++++++
>  4 files changed, 131 insertions(+)
>  create mode 100644 tests/expected/mount/fstab-btrfs
>  create mode 100644 tests/expected/mount/fstab-btrfs-auto
>  create mode 100644 tests/expected/mount/fstab-btrfs-btrfs
>  create mode 100755 tests/ts/mount/fstab-btrfs

 Applied, thanks!
 
 (Seems you're familiar with our test suite now, let's hope we will
 see more tests from you;-)
    
    Karel

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

Re: [PATCH] tests: add btrfs mount tests (fails!)

[Karel Zak]

On Thu, Feb 11, 2016 at 05:34:27PM +0100, Stanislav Brabec wrote:
> Stanislav Brabec wrote:
> >I am working on a subtest that will trigger it inside the test suite.
> >
> >I already have a possible fix for it, but I want to confirm that it is
> >correct before sending it upstream.
> >
> I just have a reproducer of the new problem.
> 
> The newly found problem is not btrfs speficic:
> 
> echo "/ext4.img /ext4_mnt1 auto loop 0 0" >>/etc/fstab
> mount -a
> echo "/ext4.img /ext4_mnt2 auto loop 0 0" >>/etc/fstab
> mount -a
> mount
> umount /ext4_mnt1 /ext4_mnt2
> sed -i /ext4.img/d /etc/fstab
> 
> Result:
> Nothing was mounted into /ext4_mnt2
> 
> (This is the problem with loop files in fstab, which I mentioned in my past
> mails, but was not able to reproduce.)

Fixed. There was "break" rather than "continue" in the mnt_table_is_fs_mounted().

    Karel

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

Re: [PATCH] tests: add btrfs mount tests (fails!)

[Stanislav Brabec]

On Feb 11 2016 at 14:47 Stanislav Brabec wrote:
> There are missing umount commands in the middle of the test, which
> trigger following problem:
>
> truncate -s10M source.img
> mkdir -p /source/subdir
> mkdir -p /dest
> /sbin/mkfs.ext4 source.img
> losetup /dev/loop0 $PWD/source.img
> mount -text4 /dev/loop0 /source
> mkdir -p /source/subdir
> mount -o bind /source /dest
> mount -o bind /source/subdir /dest
>
> Mounting a bind mount over a root of existing bind mount causes mounting
> over the bind mount source. I expected mounting over the target
> directory I specified.
>
> I am not sure, whether it is correct behavior, but for sure it is
> surprising.

We discussed it with David and considered that it is an unexpected but
correct behavior, that is a consequence of systemd introducing new
default behavior for mounts. All bind mounts are shared by default,
which causes propagation of a mount inside a target (including the
target root) into a source.

> Additionally, I am working on a reproducer of an invalid "mount -a"
> behavior triggered by kernel bug reporting false subvol.

> I already have a possible fix for it, but I want to confirm that it is
> correct before sending it upstream.

I finally found a reproducer.

It is a negative reproducer, i. e. fstab that should cause error, but
it does not. I am still not sure, whether positive reproducer exists. I
will send a fix and testcase in a separate thread.

After that, all discovered btrfs mount issues will be finally fixed.

There is another known problem with non intuitive output of lsblk with
btrfs, but it is above the scope of this thread.

-- 
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] tests: add btrfs mount tests (fails!)

[Stanislav Brabec]

On Feb 19, 2016 at 16:02 Stanislav Brabec wrote:
> On Feb 11 2016 at 14:47 Stanislav Brabec wrote:
>> There are missing umount commands in the middle of the test, which
>> trigger following problem:
>>
>> Additionally, I am working on a reproducer of an invalid "mount -a"
>> behavior triggered by kernel bug reporting false subvol.
> 
>> I already have a possible fix for it, but I want to confirm that it is
>> correct before sending it upstream.
> 
> I finally found a reproducer.
> 
Well, now I have a real reproducer that is not triggered by another bug.

My findings:

1) The way to reproduce is extremely obscure, because not only subvolid
has to match, but also target.

2) There is no way to fix it in an easy way. I suggested a new
algorithm searching for shortest subvol, but it would be vulnerable to
bad evaluation as well.

So I decided to keep the code as it is.

Way to reproduce:

To confuse the algorithm, there has to be a mounted something else into
the target than fstab specifies.

Step 1:
/dev/loop0 /btrfs_mnt_root btrfs subvol=/ 0 0
/btrfs_mnt_root/d0/dd0/ddd0/s1/d1/dd1/ddd1/s2/bind-mnt /btrfs_mnt_bind btrfs bind,private 0 0

It will create following mountinfo:
299 72 0:83 / /btrfs_mnt_root rw,relatime shared:211 - btrfs /dev/loop0 rw,space_cache,subvolid=5,subvol=/
312 72 0:83 /d0/dd0/ddd0/s1/d1/dd1/ddd1/s2/bind-mnt /btrfs_mnt_bind rw,relatime shared:211 - btrfs /dev/loop0 rw,space_cache,subvolid=258,subvol=/d0/dd0/ddd0/s1/d1/dd1/ddd1/s2/bind-mnt

Note that subvolid 258 does not correspond to / nor to
d0/dd0/ddd0/s1/d1/dd1/ddd1/s2/bind-mnt, but it corresponds to
d0/dd0/ddd0/s1/d1/dd1/ddd1/s2.

Now remove both lines from fstab and add:

/dev/loop0 /btrfs_mnt_bind btrfs defaults 0 0

Default subvolume is d0/dd0/ddd0/s1/d1/dd1/ddd1/s2, but the algorithm
(both the existing and the previously suggested one) falsely assumes
that it is /d0/dd0/ddd0/s1/d1/dd1/ddd1/s2/bind-mnt, because both
subvolid and target match.

In theory, the new fstab should cause mounting the default subvolume
over the existing one, but nothing happens.

Summary:

- The full fix would mean evaluation of the btrfs subvolume tree
  structure (several ioctl() with possible need of disk lookups) just
  to find, whether it is already mounted or no.

- The reproducer is really obscure, and nothing from a real life cannot
  trigger it.

- The full fix will be vulnerable to another unfixable problems (e. g.
  when somebody changes the default subvolume path while the device is
  mounted).

=> I propose to keep this part of code with a known bug.



Here is the the previously proposed (obsolete) fix:

---
 libmount/src/tab.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 102ed25..556d835 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -1224,6 +1224,18 @@ static char *remove_mountpoint_from_path(const char *path, const char *mnt)
 }
 
 #ifdef HAVE_BTRFS_SUPPORT
+/* All currently existing kernels reports incorrect values for bind
+ * mounts. Instead of subvol/subvolid of a subvolume used for creating of bind
+ * mount, it reports subvolid corresponding to nearest upper subvol node and
+ * subvol pointing to the current dir relative to btrfs root.
+ *
+ * The current kernel btrfs implementation has not access to proper VFS
+ * structures, so it is just guessing. Fixing this will need refactoring of
+ * kernel btrfs subvol implementation. It is not expected to happen anytime
+ * soon. Set this define to suppose that kernels are buggy, and don't blindly
+ * trust to the reported mountinfo values. */
+#define KERNEL_BTRFS_REPORTS_FALSE_SUBVOL 1
+
 static int get_btrfs_fs_root(struct libmnt_table *tb, struct libmnt_fs *fs, char **root)
 {
 	char *vol = NULL, *p;
@@ -1236,6 +1248,12 @@ static int get_btrfs_fs_root(struct libmnt_table *tb, struct libmnt_fs *fs, char
 		char *target;
 		struct libmnt_fs *f;
 		char subvolidstr[sizeof(stringify_value(UINT64_MAX))];
+#ifdef KERNEL_BTRFS_REPORTS_FALSE_SUBVOL
+		struct libmnt_fs *t;
+		struct libmnt_iter itr;
+		char *optval = NULL;
+		size_t optvalsz = 0, valsz, bestvalsz = PATH_MAX;
+#endif
 
 		DBG(BTRFS, ul_debug(" found subvolid=%s, checking", vol));
 
@@ -1248,9 +1266,24 @@ static int get_btrfs_fs_root(struct libmnt_table *tb, struct libmnt_fs *fs, char
 			goto err;
 
 		DBG(BTRFS, ul_debug(" tring target=%s subvolid=%s", target, subvolidstr));
+#ifndef KERNEL_BTRFS_REPORTS_FALSE_SUBVOL
 		f = mnt_table_find_target_with_option(tb, target,
 					"subvolid", subvolidstr,
 					MNT_ITER_BACKWARD);
+#else
+		f = NULL;
+		valsz = strlen(subvolidstr);
+		mnt_reset_iter(&itr, MNT_ITER_BACKWARD);
+		while (mnt_table_next_fs(tb, &itr, &t) == 0) {
+			if (mnt_fs_streq_target(t, target)
+			    && mnt_fs_get_option(t, "subvolid", &optval, &optvalsz) == 0
+			    && optvalsz == valsz
+			    && strncmp(optval, subvolidstr, optvalsz) == 0
+			    && optvalsz < bestvalsz)
+				f = t;
+		}
+#endif
+
 		if (!tb->cache)
 			free(target);
 		if (!f)
@@ -1271,6 +1304,12 @@ static int get_btrfs_fs_root(struct libmnt_table *tb, struct libmnt_fs *fs, char
 		char *target;
 		struct libmnt_fs *f;
 		char default_id_str[sizeof(stringify_value(UINT64_MAX))];
+#ifdef KERNEL_BTRFS_REPORTS_FALSE_SUBVOL
+		struct libmnt_fs *t;
+		struct libmnt_iter itr;
+		char *optval = NULL;
+		size_t optvalsz = 0, valsz, bestvalsz = PATH_MAX;
+#endif
 
 		DBG(BTRFS, ul_debug(" subvolid/subvol not found, checking default"));
 
@@ -1295,9 +1334,24 @@ static int get_btrfs_fs_root(struct libmnt_table *tb, struct libmnt_fs *fs, char
 		DBG(BTRFS, ul_debug(" tring target=%s default subvolid=%s",
 					target, default_id_str));
 
+#ifndef KERNEL_BTRFS_REPORTS_FALSE_SUBVOL
 		f = mnt_table_find_target_with_option(tb, target,
 					"subvolid", default_id_str,
 					MNT_ITER_BACKWARD);
+#else
+		f = NULL;
+		valsz = strlen(default_id_str);
+		mnt_reset_iter(&itr, MNT_ITER_BACKWARD);
+		while (mnt_table_next_fs(tb, &itr, &t) == 0) {
+			if (mnt_fs_streq_target(t, target)
+			    && mnt_fs_get_option(t, "subvolid", &optval, &optvalsz) == 0
+			    && optvalsz == valsz
+			    && strncmp(optval, default_id_str, optvalsz) == 0
+			    && optvalsz < bestvalsz)
+				f = t;
+		}
+#endif
+
 		if (!tb->cache)
 			free(target);
 		if (!f)
-- 
2.7.2

-- 
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] tests: add btrfs mount tests

[Ruediger Meier]

On Tuesday 02 February 2016, Stanislav Brabec wrote:
> btrfs needs a special support in mount. Add a testcase for btrfs
> specific problems.

IMO this test needs some more error handling, see below.

> Coverage:
> 352740e8: bind mounts pointing to btrfs subvolume
> 2cd28fc8: mounting default subvolume
> d2f82678: use of "auto"
> 618a8814: use of "subvolid"
>
> Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
> ---
>  tests/expected/mount/fstab-btrfs |  1 +
>  tests/ts/mount/fstab-btrfs       | 99
> ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100
> insertions(+)
>  create mode 100644 tests/expected/mount/fstab-btrfs
>  create mode 100755 tests/ts/mount/fstab-btrfs
>
> diff --git a/tests/expected/mount/fstab-btrfs
> b/tests/expected/mount/fstab-btrfs new file mode 100644
> index 0000000..3582111
> --- /dev/null
> +++ b/tests/expected/mount/fstab-btrfs
> @@ -0,0 +1 @@
> +Success
> diff --git a/tests/ts/mount/fstab-btrfs b/tests/ts/mount/fstab-btrfs
> new file mode 100755
> index 0000000..1c1d0fd
> --- /dev/null
> +++ b/tests/ts/mount/fstab-btrfs
> @@ -0,0 +1,99 @@
> +#!/bin/bash
> +
> +#
> +# Copyright (C) 2016 Stanislav Brabec <sbrabec@suse.cz>
> +#
> +# This file is part of util-linux.
> +#
> +# This file is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published
> by +# the Free Software Foundation; either version 2 of the License,
> or +# (at your option) any later version.
> +#
> +# This file is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +TS_TOPDIR="${0%/*}/../.."
> +TS_DESC="btrfs (fstab)"
> +
> +. $TS_TOPDIR/functions.sh
> +ts_init "$*"
> +
> +ts_check_test_command "$TS_CMD_MOUNT"
> +ts_check_test_command "$TS_CMD_UMOUNT"
> +
> +ts_skip_nonroot
> +ts_check_losetup
> +ts_check_prog "mkfs.btrfs"
> +ts_check_prog "btrfs"
> +
> +ts_device_init 42
> +DEVICE=$TS_LODEV
> +[ -d "$TS_MOUNTPOINT-create" ] || mkdir -p "$TS_MOUNTPOINT-create"
> +[ -d "$TS_MOUNTPOINT-default" ] || mkdir -p "$TS_MOUNTPOINT-default"
> +[ -d "$TS_MOUNTPOINT-subvol" ] || mkdir -p "$TS_MOUNTPOINT-subvol"
> +[ -d "$TS_MOUNTPOINT-subvolid" ] || mkdir -p
> "$TS_MOUNTPOINT-subvolid" +[ -d "$TS_MOUNTPOINT-bind" ] || mkdir -p
> "$TS_MOUNTPOINT-bind" +mkfs.btrfs -f -d single -m single $DEVICE &>
> /dev/null || ts_die "Cannot make btrfs on $DEVICE" +
> +$TS_CMD_MOUNT -o loop "$DEVICE" "$TS_MOUNTPOINT-create"
> +pushd . >/dev/null
> +cd "$TS_MOUNTPOINT-create"
> +mkdir -p d0/dd0/ddd0
> +cd ./d0/dd0/ddd0
> +touch file{1..5}
> +btrfs subvol create s1 >/dev/null
> +cd ./s1
> +touch file{1..5}
> +mkdir -p d1/dd1/ddd1
> +cd ./d1/dd1/ddd1
> +btrfs subvol create s2 >/dev/null

For example if you comment out above line (or if it would fail) then
all subtests below still succeed. This can't be right.

> +DEFAULT_SUBVOLID=$(btrfs inspect rootid s2)
> +btrfs subvol set-default $DEFAULT_SUBVOLID . >/dev/null
> +NON_DEFAULT_SUBVOLID=$(btrfs subvol list "$TS_MOUNTPOINT-create" |
> while read dummy id rest ; do if test $id = $DEFAULT_SUBVOLID ; then
> continue ; fi ; echo $id ; done) +cd ../../../..
> +mkdir -p d2/dd2/ddd2
> +cd ./d2/dd2/ddd2
> +btrfs subvol create s3 >/dev/null
> +popd >/dev/null
> +NON_DEFAULT_SUBVOL=d0/dd0/ddd0/d2/dd2/ddd2/s3
> +$TS_CMD_UMOUNT "$TS_MOUNTPOINT-create"
> +
> +# Tests with fs == btrfs
> +# mounting default subvolume, deep in the structure, without entry
> in fstab +ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-default" "btrfs" ""
> +# mounting default subvolume, deep in the structure
> +ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvol" "btrfs"
> "subvol=$NON_DEFAULT_SUBVOL" +# mounting non-default subvolume
> +ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvolid" "btrfs"
> "subvolid=$NON_DEFAULT_SUBVOLID" +
> +# test bind mount pointing to subvolume root
> +ts_fstab_add "$TS_MOUNTPOINT-subvol" "$TS_MOUNTPOINT-bind" "auto"
> "bind" +
> +$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
> +$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
> +
> +ts_fstab_clean
> +
> +# Tests with fs == auto
> +# mounting default subvolume, deep in the structure, without entry
> in fstab +ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-default" "auto" ""
> +# mounting default subvolume, deep in the structure
> +ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvol" "auto"
> "subvol=$NON_DEFAULT_SUBVOL" +# mounting non-default subvolume
> +ts_fstab_add "$DEVICE" "$TS_MOUNTPOINT-subvolid" "auto"
> "subvolid=$NON_DEFAULT_SUBVOLID" +
> +# test bind mount pointing to subvolume sub-directory
> +mkdir -p "$TS_MOUNTPOINT-subvol/bind-mnt"
> +ts_fstab_add "$TS_MOUNTPOINT-subvol/bind-mnt" "$TS_MOUNTPOINT-bind"
> "auto" "bind" +
> +$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
> +$TS_CMD_MOUNT -a 2>&1 >> $TS_OUTPUT
> +
> +ts_fstab_clean
> +
> +ts_log "Success"
> +ts_finalize
> +
> --
> 2.7.0

Re: [PATCH] tests: add btrfs mount tests

[Stanislav Brabec]

On Mar 14, 2016 at 02:20 Ruediger Meier wrote:
> On Tuesday 02 February 2016, Stanislav Brabec wrote:
> IMO this test needs some more error handling, see below.
>
>> +btrfs subvol create s2 >/dev/null
>
> For example if you comment out above line (or if it would fail) then
> all subtests below still succeed. This can't be right.

Well, it is a test suite for mount, not btrfs. I did not add any error 
checks in the code that creates the testing image.

Grepping through the whole test suite, I see that more than a half of 
all tests do not check for mkfs failure. Here we just have a multi-line 
mkfs.

It is possible to add a check for every command there, and fail the test 
if the command fails or returns something unexpected.

>> +NON_DEFAULT_SUBVOLID=$(btrfs subvol list "$TS_MOUNTPOINT-create" |

NON_DEFAULT_SUBVOLID will be empty.

>> "subvolid=$NON_DEFAULT_SUBVOLID" +

I am not sure how kernel will interpret subvolid=.

-- 
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] tests: add btrfs mount tests

[Ruediger Meier]

On Monday 14 March 2016, Stanislav Brabec wrote:
> On Mar 14, 2016 at 02:20 Ruediger Meier wrote:
> > On Tuesday 02 February 2016, Stanislav Brabec wrote:
> > IMO this test needs some more error handling, see below.
> >
> >> +btrfs subvol create s2 >/dev/null
> >
> > For example if you comment out above line (or if it would fail)
> > then all subtests below still succeed. This can't be right.
>
> Well, it is a test suite for mount, not btrfs. I did not add any
> error checks in the code that creates the testing image.

> Grepping through the whole test suite, I see that more than a half of
> all tests do not check for mkfs failure. Here we just have a
> multi-line mkfs.
>
> It is possible to add a check for every command there, and fail the
> test if the command fails or returns something unexpected.

You are right. I've added already some lines to skip outdated 
btrfs-tools. Nethertheless both subtests succeed even when they operate 
on an empty device without any filesystem or subvolume.

Ideally a test should be skipped if the system is broken. But if we 
don't have "skip rules" implemented yet then it should better fail than 
succeed. 

I know that we have a lot of such issues in our test suite. Just 
commented this one because it was recently added and I've noticed  it 
on existing systems.

> >> +NON_DEFAULT_SUBVOLID=$(btrfs subvol list "$TS_MOUNTPOINT-create"
> >> |
>
> NON_DEFAULT_SUBVOLID will be empty.
>
> >> "subvolid=$NON_DEFAULT_SUBVOLID" +
>
> I am not sure how kernel will interpret subvolid=.

Maybe just log the btrfs return codes to TS_OUTPUT. If we see failure in 
real life then we know that something has to be fixed, either the skip 
rules or the code to create subvols.

cu,
Rudi