Always wait for txg sync when umounting dataset

Currently, when unmounting a filesystem, ZFS will only wait for
a txg sync if the dataset is dirty and not readonly. However, this
can be problematic in cases where a dataset is remounted readonly
immediately before being unmounted, which often happens when the
system is being shut down. Since encrypted datasets require that
all I/O is completed before the dataset is disowned, this issue
causes problems when write I/Os leak into the txgs after the
dataset is disowned, which can happen when sync=disabled.

While looking into fixes for this issue, it was discovered that
dsl_dataset_is_dirty() does not return B_TRUE when the dataset has
been removed from the txg dirty datasets list, but has not actually
been processed yet. Furthermore, the implementation is comletely
different from dmu_objset_is_dirty(), adding to the confusion.
Rather than relying on this function, this patch forces the umount
code path (and the remount readonly code path) to always perform a
txg sync on read-write datasets and removes the function altogether.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Tom Caputi <tcaputi@datto.com>
Closes #7753
Closes #7795

diff --git a/include/sys/dsl_dataset.h b/include/sys/dsl_dataset.h
index abd178e300f24c9fb41d64d4cc57c16b779c853e..dbe4cb706a1f214852e34dcba3ef895c7e8066ef 100644 (file)
--- a/include/sys/dsl_dataset.h
+++ b/include/sys/dsl_dataset.h
@@ -395,7 +395,6 @@ int dsl_dataset_space_written(dsl_dataset_t *oldsnap, dsl_dataset_t *new,
     uint64_t *usedp, uint64_t *compp, uint64_t *uncompp);
 int dsl_dataset_space_wouldfree(dsl_dataset_t *firstsnap, dsl_dataset_t *last,
     uint64_t *usedp, uint64_t *compp, uint64_t *uncompp);
-boolean_t dsl_dataset_is_dirty(dsl_dataset_t *ds);
 
 int dsl_dsobj_to_dsname(char *pname, uint64_t obj, char *buf);
 

diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c
index bb9b4a1c78cba16aeb1677019694c02b816de576..b6e3b9a5c7f3ee6bdae37d79905e6598a0571e1d 100644 (file)
--- a/module/zfs/dsl_dataset.c
+++ b/module/zfs/dsl_dataset.c
@@ -794,15 +794,6 @@ dsl_dataset_rele_flags(dsl_dataset_t *ds, ds_hold_flags_t flags, void *tag)
            (flags & DS_HOLD_FLAG_DECRYPT)) {
                (void) spa_keystore_remove_mapping(ds->ds_dir->dd_pool->dp_spa,
                    ds->ds_object, ds);
-
-               /*
-                * Encrypted datasets require that users only release their
-                * decrypting reference after the dirty data has actually
-                * been written out. This ensures that the mapping exists
-                * when it is needed to write out dirty data.
-                */
-               ASSERT(dmu_buf_user_refcount(ds->ds_dbuf) != 0 ||
-                   !dsl_dataset_is_dirty(ds));
        }
 
        dmu_buf_rele(ds->ds_dbuf, tag);
@@ -1168,17 +1159,6 @@ dsl_dataset_dirty(dsl_dataset_t *ds, dmu_tx_t *tx)
        }
 }
 
-boolean_t
-dsl_dataset_is_dirty(dsl_dataset_t *ds)
-{
-       for (int t = 0; t < TXG_SIZE; t++) {
-               if (txg_list_member(&ds->ds_dir->dd_pool->dp_dirty_datasets,
-                   ds, t))
-                       return (B_TRUE);
-       }
-       return (B_FALSE);
-}
-
 static int
 dsl_dataset_snapshot_reserve_space(dsl_dataset_t *ds, dmu_tx_t *tx)
 {

diff --git a/module/zfs/zfs_vfsops.c b/module/zfs/zfs_vfsops.c
index 488eaa4f2f45ec560ff92a037f2ad3dfd314ce87..8ae2ef929c83a597c3b997352b72510668c90e78 100644 (file)
--- a/module/zfs/zfs_vfsops.c
+++ b/module/zfs/zfs_vfsops.c
@@ -1745,10 +1745,10 @@ zfsvfs_teardown(zfsvfs_t *zfsvfs, boolean_t unmounting)
        zfs_unregister_callbacks(zfsvfs);
 
        /*
-        * Evict cached data
+        * Evict cached data. We must write out any dirty data before
+        * disowning the dataset.
         */
-       if (dsl_dataset_is_dirty(dmu_objset_ds(zfsvfs->z_os)) &&
-           !zfs_is_readonly(zfsvfs))
+       if (!zfs_is_readonly(zfsvfs))
                txg_wait_synced(dmu_objset_pool(zfsvfs->z_os), 0);
        dmu_objset_evict_dbufs(zfsvfs->z_os);
 
@@ -1970,6 +1970,9 @@ zfs_remount(struct super_block *sb, int *flags, zfs_mnt_t *zm)
        if (error)
                return (error);
 
+       if (!zfs_is_readonly(zfsvfs) && (*flags & MS_RDONLY))
+               txg_wait_synced(dmu_objset_pool(zfsvfs->z_os), 0);
+
        zfs_unregister_callbacks(zfsvfs);
        zfsvfs_vfs_free(zfsvfs->z_vfs);
 

diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c
index 19bc1b18ef23e75c323876fd7daf31e3cf3e5980..f7706f14312f62ab3e15c07ef6611f9c6ac098fc 100644 (file)
--- a/module/zfs/zvol.c
+++ b/module/zfs/zvol.c
@@ -1193,10 +1193,10 @@ zvol_shutdown_zv(zvol_state_t *zv)
        zv->zv_dn = NULL;
 
        /*
-        * Evict cached data
+        * Evict cached data. We must write out any dirty data before
+        * disowning the dataset.
         */
-       if (dsl_dataset_is_dirty(dmu_objset_ds(zv->zv_objset)) &&
-           !(zv->zv_flags & ZVOL_RDONLY))
+       if (!(zv->zv_flags & ZVOL_RDONLY))
                txg_wait_synced(dmu_objset_pool(zv->zv_objset), 0);
        (void) dmu_objset_evict_dbufs(zv->zv_objset);
 }
@@ -1489,8 +1489,7 @@ zvol_ioctl(struct block_device *bdev, fmode_t mode,
                invalidate_bdev(bdev);
                rw_enter(&zv->zv_suspend_lock, RW_READER);
 
-               if (dsl_dataset_is_dirty(dmu_objset_ds(zv->zv_objset)) &&
-                   !(zv->zv_flags & ZVOL_RDONLY))
+               if (!(zv->zv_flags & ZVOL_RDONLY))
                        txg_wait_synced(dmu_objset_pool(zv->zv_objset), 0);
 
                rw_exit(&zv->zv_suspend_lock);

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_remount.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_remount.ksh
index a83e2d117c8e6a2bf25584dc8e7bb73125a9e212..f7a0978352b552a457f6090cf465d0ba0e15cc50 100755 (executable)
--- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_remount.ksh
+++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_remount.ksh
@@ -36,8 +36,10 @@
 # 2. Verify we can (re)mount the dataset readonly/read-write
 # 3. Verify we can mount the snapshot and it's mounted readonly
 # 4. Verify we can't remount it read-write
-# 5. Re-import the pool readonly
-# 6. Verify we can't remount its filesystem read-write
+# 5. Verify we can remount a dataset readonly and unmount it with
+#    encryption=on and sync=disabled (issue #7753)
+# 6. Re-import the pool readonly
+# 7. Verify we can't remount its filesystem read-write
 #
 
 verify_runnable "both"
@@ -130,11 +132,21 @@ readonlyfs $MNTPSNAP
 checkmount $TESTSNAP 'ro'
 log_must umount $MNTPSNAP
 
-# 5. Re-import the pool readonly
+# 5. Verify we can remount a dataset readonly and unmount it with
+#    encryption=on and sync=disabled (issue #7753)
+log_must eval "echo 'password' | zfs create -o sync=disabled \
+    -o encryption=on -o keyformat=passphrase $TESTFS/crypt"
+CRYPT_MNTPFS="$(get_prop mountpoint $TESTFS/crypt)"
+log_must touch $CRYPT_MNTPFS/file.dat
+log_must mount -o remount,ro $TESTFS/crypt $CRYPT_MNTPFS
+log_must umount -f $CRYPT_MNTPFS
+zpool sync $TESTPOOL
+
+# 6. Re-import the pool readonly
 log_must zpool export $TESTPOOL
 log_must zpool import -o readonly=on $TESTPOOL
 
-# 6. Verify we can't remount its filesystem read-write
+# 7. Verify we can't remount its filesystem read-write
 readonlyfs $MNTPFS
 checkmount $TESTFS 'ro'
 log_mustnot mount -o remount,rw $MNTPFS