Possible bug & suggested fix for zfs_is_mounted()

[jmlayman]

Hi - I believe I have found (and fixed) a bug with the zfs_is_mounted() function. What I am trying to do is unmount a volume via zfs_unmount() and then at a later time call zfs_is_mounted() on the same volume, expecting it to return false, and then mounting it with zfs_mount(). What I am seeing is that if the volume was not mounted to begin with (in other words, zfs_unmount was never called) the above sequence of events works fine. However, if the volume was mounted before running my program, zfs_unmount() works correctly but the call to zfs_is_mounted() still thinks the volume is mounted (returns true) and so my volume never gets mounted. I have been able to isolate this to a simple test program (see below). I am using zfs 0.6.1 on Ubuntu 12.04 (3.2.0-38-generic).

I debugged the issue & this is what I found. When zfs_unmount() is called, the volume does indeed get removed from /etc/mtab. Then when zfs_is_mounted() is called at a later time, the libzfs_mnttab_find() function in libzfs_dataset.c is invoked to iterate over the existing /etc/mtab entries. At this time that function issues a rewind() call which I think was intended to re-read the contents of /etc/mtab. However, all rewind() really does is reset the file pointer & so the contents of the original /etc/mtab (the one containing the volume entry) is simply re-processed, the volume is found, and the function returns 0 instead of ENOENT. I should mention my test program has caching disabled but I have tried enabling it & get the same results. My fix was to replace the rewind() call in libzfs_mnttab_find() with an freopen instead:

hdl->libzfs_mnttab = freopen(MNTTAB, "r", hdl->libzfs_mnttab);

This forces the contents of /etc/mtab to be re-read & so the entry for the volume that was removed earlier by zfs_unmount() is truly gone and the function correctly returns ENOENT.

The underlying problem seems to be that the file pointed to by hdl->libzfs_mnttab (/etc/mtab) has been modified by the OS (probably when the umount() system call was invoked) and so the libzfs_mnttab handle stored in hdl is pointing to stale data. To confirm this, I also tried issuing a call to libzfs_init() in between calls to unmountVolume/mountVolume in my test program and this works, presumably because libzfs_init() reads /etc/mtab every time. This also explains why calls to "zfs unmount" followed by "zfs mount" always work, as each invocation of zfs reads /etc/mtab.

There may be a better solution than an freopen(), but that is what I tried & it seems to work. There is also a question of what to do if that call fails. Yes, it should never happen, but we've all been there before!

Let me know if you need more information or if perhaps I am using the zfs calls incorrectly. I thought disabling the cache was the solution, but there could be other configuration settings I missed. Using my test program I can reproduce this 100% of the time on two different machines. If you run my test program on a mounted volume, you will see it unmount the volume, wait 3 seconds, and then fail to mount the volume because zfs_is_mounted() returns true. If you run it a second time, you will see the volume skip the zfs_unmount() call (because it is already unmounted) and then call zfs_mount() because zfs_is_mounted() returns false. This time, the volume will be mounted. So basically every other run of the program works, and the outcome is predictable based on whether or not the volume in question (vol1 for me) is mounted.

#include <string>
#include <cstdio>
#include <cerrno>
#include <cstring>
#include <libzfs.h>

void mountVolume(const std::string& volume);
void unmountVolume(const std::string& volume);

libzfs_handle_t *g_zfs;

int main(int argc, char *argv[]) {

    if ((g_zfs = libzfs_init()) == NULL) {
        return 1;
    }

    libzfs_print_on_error(g_zfs, B_TRUE);
    libzfs_mnttab_cache(g_zfs, B_FALSE);

    unmountVolume("vol1");
    sleep(3);
    mountVolume("vol1");

    libzfs_fini(g_zfs);

    return 0;
}

void mountVolume(const std::string& volume) {

  zfs_handle_t *zfs = zfs_open(g_zfs, volume.c_str(), ZFS_TYPE_FILESYSTEM);
  fprintf(stderr, "zfs_open: %s\n", strerror(errno));
  if (zfs) {
    // We have a zfs handle - see if the volume is mounted.
    if (!zfs_is_mounted(zfs, NULL)) {
      // No, the volume isn't mounted - mount it.
      fprintf(stderr, "zfs_is_mounted: volume %s is not mounted - will mount it\n", zfs_get_name(zfs));
      if (zfs_mount(zfs, NULL, 0)) {
        fprintf(stderr, "zfs_mount: %s\n", strerror(errno));
      } else {
        fprintf(stderr, "zfs_mount: %s successfully mounted\n", zfs_get_name(zfs));
      }
    } else {
      fprintf(stderr, "zfs_is_mounted: %s is already mounted - nothing to do\n", zfs_get_name(zfs));
    }
    zfs_close(zfs);
    fprintf(stderr, "zfs_close: %s\n", strerror(errno));
  }
}

void unmountVolume(const std::string& volume) {

  zfs_handle_t *zfs = zfs_open(g_zfs, volume.c_str(), ZFS_TYPE_FILESYSTEM);
  fprintf(stderr, "zfs_open: %s\n", strerror(errno));
  if (zfs) {
    // We have a zfs handle - see if the volume is mounted.
    if (zfs_is_mounted(zfs, NULL)) {
      // Yes, the volume is mounted - unmount it.
      fprintf(stderr, "zfs_is_mounted: volume %s is mounted - will unmount\n", zfs_get_name(zfs));
      if (zfs_unmount(zfs, NULL, 0)) {
        fprintf(stderr, "zfs_unmount: %s\n", strerror(errno));
      } else {
        fprintf(stderr, "zfs_unmount: %s successfully unmounted\n", zfs_get_name(zfs));
      }
    } else {
      fprintf(stderr, "zfs_is_mounted: %s is not mounted - nothing to do\n", zfs_get_name(zfs));
    }
    zfs_close(zfs);
    fprintf(stderr, "zfs_close: %s\n", strerror(errno));
  }
}

My gcc version & compilation line:

gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)

g++ -g -I/home/devel/zfs-0.6.1/include -I/home/devel/zfs-0.6.1/lib/libspl/include -DHAVE_IOCTL_IN_SYS_IOCTL_H test.cpp -L/home/devel/zfs-0.6.1/lib/libzfs/.libs -lzfs

[behlendorf]

@jmlayman Nice job running this one to ground.

As I'm sure you noticed we've disabled the libzfs mount cache under Linux. The idea was this would ensure that we always picked up the latest mtab changes. However, the system mount utilities must update the mtab file by writing a new one, removing the old one, and moving the new version in to place. We keep getting stale data from the old version because we have an open file handle for it. The mount utilities on other platforms likely rewrite the file in place.

I think your fix is entirely reasonable but we should handle the error case. Can you open a pull request which fixes the use of rewind() in libzfs_mnttab_find() and libzfs_mnttab_update()?

Thanks for taking the time to look at this!

[jmlayman]

Thanks. I just submitted a pull request with the fix. Let me know if
you have any questions or need me to make further edits.

On 08/09/13 19:24, Brian Behlendorf wrote:

@jmlayman https://github.com/jmlayman Nice job running this one to
ground.

As I'm sure you noticed we've disabled the libzfs mount cache under
Linux. The idea was this would ensure that we always picked up the
latest mtab changes. However, the system mount utilities must update
the mtab file by writing a new one, removing the old one, and moving
the new version in to place. We keep getting stale data from the old
version because we have an open file handle for it. The mount
utilities on other platforms likely rewrite the file in place.

I think your fix is entirely reasonable but we should handle the error
case. Can you open a pull request which fixes the use of rewind() in
libzfs_mnttab_find() and libzfs_mnttab_update()?

Thanks for taking the time to look at this!

—
Reply to this email directly or view it on GitHub
#1611 (comment).

[FransUrbo]

Will this affect #1484?

[behlendorf]

@FransUrbo Yes, it's certainly related, although this shouldn't make it any worse.