zfs `mount -a` race

[c0d3z3r0]

System information

Type	Version/Name
Distribution Name	Debian
Distribution Version	testing
Linux Kernel	4.20.5 and 4.20.10
Architecture	x64
ZFS/SPL Version	master / 0.8.0-rc3-g0a1086319

Describe the problem you're observing

Expected behaviour of zfs mount -a is that it detects dependencies and waits for these dependencies to be available before doing any mounts below. For example dpool/data/test depends on dpool/data, when dpool/data is mounted on /var/data and dpool/data/test gets mounted on /var/data/test AFTER /var/data is available to the system.

This is what happens with zfs mount -a:

root@debian:[~]# zfs mount
rpool/ROOT                      /
root@debian:[~]# ls /var/data/
root@debian:[~]# zfs mount -a
root@debian:[~]# zfs mount
rpool/ROOT                      /
dpool/data                       /var/data
dpool/data/test                /var/data/test
# ZFS says mount was ok; but it is not!
root@debian:[~]# ls /var/data/
testfile_dpool_data
# test is missing!
# Unmount everything again
root@debian:[~]# zfs umount dpool/data
umount: /var/data/test: not mounted.
cannot unmount '/var/data/test': umount failed
root@debian:[~]# zfs umount dpool/data/test
umount: /var/data/test: not mounted.
cannot unmount '/var/data/test': umount failed
# Uhm. What!? Check linux mount
root@debian:[~]# mount | grep 'var/data'
dpool/data on /var/data type zfs (rw,relatime,xattr,noacl)
dpool/data/test on /var/data/test type zfs (rw,relatime,xattr,noacl)
# Ok, do a manual unmount
root@debian:[~]# umount /var/data/test
umount: /var/data/test: no mount point specified.
# Hmm, ok, that's caused by the (unwanted) overlayed mount. Unmount it first.
root@debian:[~]# umount /var/data     
root@debian:[~]# ls /var/data
test
root@debian:[~]# zfs mount
rpool/ROOT                      /
dpool/data/test                 /var/data/test
# Ok, this is correct. Then let's unmount dpool/data/test
root@debian:[~]# zfs umount dpool/data/test
root@debian:[~]# zfs mount
rpool/ROOT                      /
root@debian:[~]# ls /var/data/
test
root@debian:[~]# ls /var/data/test/
root@debian:[~]#

Doing it manually:

root@debian:[~]# zfs mount
rpool/ROOT                      /
root@debian:[~]# ls /var/data/
root@debian:[~]# zfs mount dpool/data
root@debian:[~]# ls /var/data/
testfile_dpool_data test
root@debian:[~]# ls /var/data/test
root@debian:[~]# zfs mount dpool/data/test
root@debian:[~]# ls /var/data/test
testfile_dpool_data_test
root@debian:[~]# zfs unmount /var/data/test; zfs unmount /var/data
root@debian:[~]# ls /var/data/
root@debian:[~]# 

The problem here IMHO is that zfs does not wait for the dependency mount (dpool/data) to be really available and mistakenly does a overlayed mount when the sub-zfs (dpool/data/test) becomes available first.

Update:

When setting ZFS_SERIAL_MOUNT=1, everything works fine. So the problem is not a missing wait/sleep/whatever but it's the parallization of dependent mounts (zfs_foreach_mountpoint).

Update 2: See #8450 (comment)

[c0d3z3r0]

@kpande I don't want to do an overlay mount. In my example I want to mount dpool/data to /var/data and dpool/data/test to /var/data/test but here dpool/data gets mounted after dpool/data/test which results in an (unwanted) overlay mount

[c0d3z3r0]

The solution would be to detect those dependencies in zfs_foreach_mountpoint and put them in the same thread

[c0d3z3r0]

yes, it can not be an "unwanted" overlay mount, as by default, ZFS doesn't mount in non-empty directories.

Yes, it should not mount on non-empty directories but it does. This is a race condition caused by parallelization.
I have added some debug output. Here we see that both empty-dir checks happen at the same time, way before anything is mounted at all.

514825 Create mountpoint: /var/data/test
 514826 Check empty: /var/data
 514910 Check empty: /var/data/test
 514916 Do mount: /var/data/test
 515100 Do mount: /var/data
 515645 mtab: /var/data/test
 515748 mtab: /var/data

By delaying the test thread a bit the problem is gone.
Debug output:

67366 mtab: /var/data
 67405 Check empty: /var/data
 67408 Do mount: /var/data
 67776 Check empty: /var/data/test
 67807 Do mount: /var/data/test
 74833 mtab: /var/data/test

As I said, there must be some tasklist prepared where dependent mountpoints have to be in the same one.

why not just set canmount=off on dpool/data and leave canmount=on for dpool/data/test? I assume you don't actually want to store data in dpool/data, just use it to organise filesystems?

Well, I'm currently setting up that system from scratch... I am not sure how the final directory structure will be but, yes, maybe I simply should not do it like this.... However, the race condition should be fixed anyways.

[c0d3z3r0]

The comment for zfs_mount_task says

 * Thread pool function to mount one file system. On completion, it finds and
 * schedules its children to be mounted. This depends on the sorting done in
 * zfs_foreach_mountpoint(). Note that the degenerate case (**chain of entries
 * each descending from the previous**) will have **no parallelism since we always
 * have to wait for the parent to finish mounting** before we can schedule
 * its children.

This already implements what I meant with a tasklist...

zfs_foreach_mountpoint:
  for i in non_descendant_idx:
    zfs_dispatch_mount

zfs_dispatch_mount:
  zfs_mount_task

zfs_mount_task:
  for i in non_descendant_idx:
    mnt_func
    zfs_dispatch_mount

zfs_mount_task dispatches the mounting of a mountpoint's children, so it needs to wait until the mountpoint is populated as described above. This seems to be missing...
I am not really sure how to check if the mount is done already so I added an usleep(10*1000) (10ms) after the mnt_func() call for testing. This fixes the issue...

Then I tried this but it did not solve the issue since zfs_is_mounted always returns true there... hmmmm...

diff --git a/lib/libzfs/libzfs_mount.c b/lib/libzfs/libzfs_mount.c
index 649c232aa..42632f02e 100644
--- a/lib/libzfs/libzfs_mount.c
+++ b/lib/libzfs/libzfs_mount.c
@@ -1431,9 +1431,23 @@ zfs_mount_task(void *arg)
        verify(zfs_prop_get(handles[idx], ZFS_PROP_MOUNTPOINT, mountpoint,
            sizeof (mountpoint), NULL, NULL, 0, B_FALSE) == 0);
 
+       printf("mount task %s\n", mountpoint);
+
        if (mp->mnt_func(handles[idx], mp->mnt_data) != 0)
                return;
 
+       //const char * name = zfs_get_name(handles[idx]);
+       //printf("1 hdl: %s, mp: %s, mnt: %d\n", name, mountpoint, zfs_is_mounted(handles[idx], NULL));
+
+       if (zfs_is_mountable(handles[idx], mountpoint, sizeof (mountpoint), NULL)) {
+               //printf("%s mountable\n", name);
+               while (!zfs_is_mounted(handles[idx], NULL))
+                       usleep(1000);
+       }
+
+       //printf("2 hdl: %s, mp: %s, mnt: %d\n", name, mountpoint, zfs_is_mounted(handles[idx], NULL));
+
+
        /*
         * We dispatch tasks to mount filesystems with mountpoints underneath
         * this one. We do this by dispatching the next filesystem with a