Concurrent modifications to "/etc/dfs/sharetab" does not work

[prakashsurya]

System information

Type	Version/Name
Distribution Name	Ubuntu
Distribution Version	Ubuntu 18.04
Linux Kernel	4.15.0-34-generic
Architecture	x86_64
ZFS Version	v0.8.0-rc1
SPL Version	v0.8.0-rc1

Describe the problem you're observing

When multiple processes are concurrently modifying the /etc/dfs/sharetab file (e.g. zfs set sharenfs=on ..., zfs create -o sharenfs=on ..., etc.) the contents of /etc/dfs/sharetab can become "corrupt"; i.e. not all shared filesystems will be listed in that file.

Describe how to reproduce the problem

This can be reproduced with the following script:

#!/bin/bash
touch /etc/dfs/sharetab
echo -n "exportfs -- "; exportfs | wc -l
echo -n "sharetab -- "; cat /etc/dfs/sharetab | wc -l
for i in `seq 1 100`; do
        (sleep 1; zfs create -o sharenfs=on tank/fs-$i) &
done
wait
echo -n "exportfs -- "; exportfs | wc -l
echo -n "sharetab -- "; cat /etc/dfs/sharetab | wc -l

When I run this script, I see the following:

$ sudo ./reproducer.sh
exportfs -- 0
sharetab -- 0
exportfs -- 100
sharetab -- 1

Here's some more information after running that reproducer:

$ sudo exportfs | head
/tank/fs-71     <world>
/tank/fs-42     <world>
/tank/fs-61     <world>
/tank/fs-48     <world>
/tank/fs-16     <world>
/tank/fs-40     <world>
/tank/fs-52     <world>
/tank/fs-80     <world>
/tank/fs-90     <world>
/tank/fs-57     <world>

$ sudo cat /etc/dfs/sharetab | head
/tank/fs-71     -       nfs     rw,crossmnt

$ sudo zfs list -r tank | head
NAME          USED  AVAIL  REFER  MOUNTPOINT
tank         3.80M  21.8G    30K  /tank
tank/fs-1      24K  21.8G    24K  /tank/fs-1
tank/fs-10     24K  21.8G    24K  /tank/fs-10
tank/fs-100    24K  21.8G    24K  /tank/fs-100
tank/fs-11     24K  21.8G    24K  /tank/fs-11
tank/fs-12     24K  21.8G    24K  /tank/fs-12
tank/fs-13     24K  21.8G    24K  /tank/fs-13
tank/fs-14     24K  21.8G    24K  /tank/fs-14
tank/fs-15     24K  21.8G    24K  /tank/fs-15

[prakashsurya]

We have a change that we've been using to explore a potential fix for this issue here: prakashsurya@f593311

With this in place, the /etc/dfs/sharetab file remains consistent with the contents of exportfs and the number of shared filesystems that are created, but I've seen cases where zfs set sharenfs=off or zfs destroy will fail. So far, I'm not sure if these failures are a result of our work-in-progress change that I linked to, or due to a tangential problem in the code.

To be clear, here's roughly what I'm doing to test the change I linked to above:

1. Concurrently create 100 filesystems with zfs create
   • After, verify exportfs and sharetab have 0 lines
2. Concurrently set sharenfs=on on these 100 filesystems with zfs set sharenfs=on
   • After, verify exportfs and sharetab have 100 lines
3. Concurrently set sharenfs=off on these 100 filesystems with zfs set sharenfs=off
   • After, verify exportfs and sharetab have 0 lines
4. Concurrently destroy these 100 filesystems with zfs destroy
   • After, verify exportfs and sharetab have 0 lines (redundant due to step 3)

[behlendorf]

@prakashsurya have you considered using libmount for this. These days most (maybe all) distributions rely on it to perform these updates safely.

http://ftp.ntu.edu.tw/linux/utils/util-linux/v2.22/libmount-docs/index.html

One of the major advantages of this approach is that it makes these updates safe even when they're running concurrently with other system tools (i.e. exportfs, mount). Right now on Linux we're updating /etc/dfs/sharetab which is effectively private to ZFS since on Linux the correct file path is /var/lib/nfs/etab. By switching to libmount, which didn't exist when this code was originally written, it should be safe to rely on /var/lib/nfs/etab since it can now lock it.

/*
 * nfs_check_exportfs() checks that the exportfs command runs
 * and also maintains a temporary copy of the output from
 * exportfs -v.
 * To update this temporary copy simply call this function again.
 *
 * TODO : Use /var/lib/nfs/etab instead of our private copy.
 *        But must implement locking to prevent concurrent access.
 *

It'd be great to see those tests added to the ZTS as part of this.

[prakashsurya]

Thanks for the pointer!

I didn't know about libmount, so I'll try and give that a look, and see when I can make some time to move the code over to it. I had already thought lots of this code would need to get rewritten based on what I've looked at so far (that comment included), so it's good to know that there's a proper library that we can now use.

[behlendorf]

@alek-p may have already started on the libmount conversion but I'm not sure how far along that work is.

[alek-p]

Bringing in libmount was something we've looked into but I haven't actually started any integration work.