Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix zfs_rename to update symlinks under /dev (Fixes #408) #523

Closed
wants to merge 1 commit into from

Conversation

schakrava
Copy link
Contributor

When a zvol consisting of snapshots is renamed, the symlinks under
/dev for snapshots are not renamed at all. Exporting and reimporting
the zpool updates the link properly but that is not desirable to
do. We need to update udev in zfs_rename to update these symlinks as
part of zfs rename.

I've update the zfs_rename function to remove symlinks prior to
renaming the volume and create new links upon success or restore old
links upon failure. Also, I've moved parentname and zhrp variable
asignments up towards the beginning as they are needed for both
recursive and volume rename cases. I've also added supporting
functions zfs_create_link_snapvol and zfs_remove_link_snapvol to be
used as callbacks for zfs_iter_snapshots

When a zvol consisting of snapshots is renamed, the symlinks under
/dev for snapshots are not renamed at all. Exporting and reimporting
the zpool updates the link properly but that is not desirable to
do. We need to update udev in zfs_rename to update these symlinks as
part of zfs rename.

I've update the zfs_rename function to remove symlinks prior to
renaming the volume and create new links upon success or restore old
links upon failure. Also, I've moved parentname and zhrp variable
asignments up towards the beginning as they are needed for both
recursive and volume rename cases. I've also added supporting
functions zfs_create_link_snapvol and zfs_remove_link_snapvol to be
used as callbacks for zfs_iter_snapshots
@behlendorf
Copy link
Contributor

Thanks for working on this. After reviewing your proposed patch I have one big outstanding question. Does this bug exist on OpenSolaris as well? After reviewing all of the relevant code I don't see how the OpenSolaris version handles this case either so it seems like it must. If you have access to an OpenSolaris box it would be interesting to test.

Assuming that the Solaris system doesn't handle this case either and we're both not just over looking the existing machinery let me suggest an alternate fix. The reason this works for the volume itself and not the snapshot can be found in zfs_ioc_rename(). Notice the zvol_remove_minor(); zvol_create_minor() block. This has the effect of unregistering and registering a new miror with sysfs which will result in udev automatically updating the symlinks. It should be pretty straight forward to extend this same basic mechanism to cover snapshots of the volume.

Here's some preliminary non-functional code to outline what I have in mind. You'll need to fix up the dataset names and such but it should do what you want by iterating over the snapshots allowing you to remove and recreate them.

diff --git a/include/sys/zvol.h b/include/sys/zvol.h
index 815b186..df3fbde 100644
--- a/include/sys/zvol.h
+++ b/include/sys/zvol.h
@@ -41,6 +41,7 @@ extern int zvol_create_minor(const char *);
 extern int zvol_create_minors(const char *);
 extern int zvol_remove_minor(const char *);
 extern void zvol_remove_minors(const char *);
+extern int zvol_rename_minor(char *, char *);
 extern int zvol_set_volsize(const char *, uint64_t);
 extern int zvol_set_volblocksize(const char *, uint64_t);
 
diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index c8ff035..a47eaf9 100644
--- a/module/zfs/zfs_ioctl.c
+++ b/module/zfs/zfs_ioctl.c
@@ -3300,10 +3300,8 @@ zfs_ioc_rename(zfs_cmd_t *zc)
        }
 
        err = dmu_objset_rename(zc->zc_name, zc->zc_value, recursive);
-       if ((err == 0) && (zc->zc_objset_type == DMU_OST_ZVOL)) {
-               (void) zvol_remove_minor(zc->zc_name);
-               (void) zvol_create_minor(zc->zc_value);
-       }
+       if ((err == 0) && (zc->zc_objset_type == DMU_OST_ZVOL))
+               (void) zvol_rename_minor(zc->zc_name, zc->zc_value);
 
        return (err);
 }
diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c
index a25fbbe..b2a233e 100644
--- a/module/zfs/zvol.c
+++ b/module/zfs/zvol.c
@@ -768,6 +768,34 @@ zvol_remove(zvol_state_t *zv_remove)
 }
 
 static int
+zvol_rename_cb(const char *dsname, void *arg)
+{
+       char *new_name = arg;
+       int error = 0;
+
+       printk("dsname = %s, new_name = %s\n", dsname, new_name);
+
+       (void) zvol_remove_minor(dsname);
+       (void) zvol_create_minor(new_name);
+
+       return (error);
+}
+
+/*
+ * Rename a zvol removing as creating new minors as needed.  This
+ * is required to ensure sysfs is notified and udev triggered.
+ */
+int
+zvol_rename_minor(char *old_name, char *new_name)
+{
+       int error = 0;
+
+       error = dmu_objset_find(old_name, zvol_rename_cb,
+           new_name, DS_FIND_SNAPSHOTS);
+
+       return (error);
+}
+
+static int
 zvol_first_open(zvol_state_t *zv)
 {
        objset_t *os;

@behlendorf
Copy link
Contributor

Fixed by commit 0e20a31 which was based on this patch originally.

@behlendorf behlendorf closed this Nov 20, 2012
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 21, 2018
In b4ad50a, we abandoned memalloc_noio_save in favor of spl_fstrans_mark
because earlier kernel with it doesn't turn off __GFP_FS. However, for newer
kernel, we would prefer PF_MEMALLOC_NOIO because it would work for allocation
in kernel which we cannot control otherwise. So in this patch, we turn on both
PF_FSTRANS and PF_MEMALLOC_NOIO in spl_fstrans_mark.

Signed-off-by: Chunwei Chen <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#523
pcd1193182 pushed a commit to pcd1193182/zfs that referenced this pull request Sep 26, 2023
When starting the agent and discovering zettacaches, if there's an old
zettacache (with an older on-disk format) present, the agent crashes.
We should instead ignore this old cache.

Bonus cleanup: document recommended relationship between tunables.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ZVOL ZFS Volumes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants