Skip to content

Commit

Permalink
Fix destroy snapshots and race on zfs diff
Browse files Browse the repository at this point in the history 
Fixes "dataset not found" error on zfs destory <snapshot> see openzfs#173.
Fixes race in dsl_dataset_user_release_tmp() when the temp snapshot
from zfs diff dataset@snap command is used see openzfs#481.
  • Loading branch information
@b333z
b333z committed Feb 6, 2012
1 parent 04f06e0 commit 2d69db2
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion module/zfs/zfs_ctldir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,14 @@ zfsctl_unmount_snapshot(zfs_sb_t *zsb, char *name, int flags)
else
zfsctl_sep_free(sep);
} else {
error = ENOENT;
/*
* This was not recorded in z_ctldir_snaps but may have been
* mounted manually so may need to add check and unmount if
* this being called prior to a destroy.
* Returning ENOENT here causes race in dsl_dataset_user_release_tmp() and
* prevents destroy of snapshots so return 0.
*/
error = 0;
}

mutex_exit(&zsb->z_ctldir_lock);
Expand Down

4 comments on commit 2d69db2

@behlendorf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for running down the root cause on this. I think a slightly cleaner fix would be just to check for ENOENT as a safe return code from zfs_unmount_snap() in dsl_dataset_user_release_one(). This was the ENOENT behavior is preserved for all other callers which might need to know why an unmount failed. I'll roll this change in to an updated snapshot patch.

diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c
index f4455e7..0214b7c 100644
--- a/module/zfs/dsl_dataset.c
+++ b/module/zfs/dsl_dataset.c
@@ -3846,7 +3846,7 @@ dsl_dataset_user_release_one(const char *dsname, void *ar
                name = kmem_asprintf("%s@%s", dsname, ha->snapname);
                error = zfs_unmount_snap(name, NULL);
                strfree(name);
-               if (error) {
+               if (error && (error != ENOENT)) {
                        dsl_dataset_rele(ds, dtag);
                        return (error);

@b333z
Copy link
Owner Author

@b333z b333z commented on 2d69db2 Feb 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries sounds good, I did start with that change but I pushed it down into zfs_umount_snap because the change above fixes the temp snapshot destroy in the diff, but think deleting a snapshot using zfs destroy and getting dataset not found comes from a different caller that also not setup to deal with ENOENT from zfs_unmount_snap, so we might need to identify that caller as well.

Do you know the path zfs destroy would take? I put a print_backtrace in the stap script I was using in the zfs_unmount_snap but it didn't seem to make sense and then I ran out of time and went on hol's...

@b333z
Copy link
Owner Author

@b333z b333z commented on 2d69db2 Feb 8, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@behlendorf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zfs destroy path is short, just zfs_ioc_destroy->zfs_unmount_snap(). But you raise an interesting point about the other various error cases. It got me wondering why this isn't an issue on OpenSolaris. It turns out it's because their dounmount() hook already takes a reference to the mounted vfs namespace so ENOENT is impossible. The Linux version I reworked just takes a path. So we should really do is rework zfs_unmount_snap() to provide the same behavior and just treat ENOENT as success like Solaris.

Please sign in to comment.