macOS: changes to zfsdev_get_state

macOS: use our minor with zfsdev_state_init() [squash]

macOS: avoid cycling through minors

Author: lundman

module/os/macos/zfs/zfs_ioctl_os.c

@@ -91,40 +91,67 @@ zfsdev_get_dev(void)
 	return ((dev_t)tsd_get(zfsdev_private_tsd));
 }
 
-/* Not sure what these are supposed to be - upstream assumes they can be set */
+/* We can't set ->private method, so this function does nothing */
 void
 zfsdev_private_set_state(void *priv, zfsdev_state_t *zs)
 {
+	zfsdev_state_t **actual_zs = (zfsdev_state_t **)priv;
+	if (actual_zs != NULL)
+		*actual_zs = zs;
 }
 
+/* Loop all zs looking for matching dev_t */
 zfsdev_state_t *
 zfsdev_private_get_state(void *priv)
 {
+	dev_t dev = (dev_t)priv;
+	zfsdev_state_t *zs;
+	mutex_enter(&zfsdev_state_lock);
+	zs = zfsdev_get_state(dev, ZST_ALL);
+	mutex_exit(&zfsdev_state_lock);
+	return (zs);
 }
 
 static int
 zfsdev_open(dev_t dev, int flags, int devtype, struct proc *p)
 {
 	int error;
+	zfsdev_state_t *actual_zs = NULL;
 
 	mutex_enter(&zfsdev_state_lock);
-	if (zfsdev_get_state(minor(dev), ZST_ALL)) {
-		mutex_exit(&zfsdev_state_lock);
-		return (0);
-	}
-	error = zfsdev_state_init((void *)dev);
-	mutex_exit(&zfsdev_state_lock);
 
-	return (-error);
+	/*
+	 * Check if the minor already exists, something that zfsdev_state_init()
+	 * does internally, but it doesn't know of the minor we are to use.
+	 * This should never happen, so use ASSERT()
+	 */
+	ASSERT3P(zfsdev_get_state(minor(dev), ZST_ALL), ==, NULL);
+
+	error = zfsdev_state_init((void *)&actual_zs);
+	/*
+	 * We are given the minor to use, so we set it here. We can't use
+	 * zfsdev_private_set_state() as it is called before zfsdev_state_init()
+	 * sets the minor. Also, since zfsdev_state_init() doesn't return zs
+	 * nor the minor they pick, we ab/use "priv" to return it to us.
+	 * Maybe we should change zfsdev_state_init() instead of this dance,
+	 * either to take 'minor' to use, or, to return zs.
+	 */
+	if (error == 0 && actual_zs != NULL)
+		actual_zs->zs_minor = minor(dev);
+	mutex_exit(&zfsdev_state_lock);
+	return (error);
 }
 
 static int
 zfsdev_release(dev_t dev, int flags, int devtype, struct proc *p)
 {
-	mutex_enter(&zfsdev_state_lock);
-	zfsdev_state_destroy((void *)dev);
-	mutex_exit(&zfsdev_state_lock);
+	/* zfsdev_state_destroy() doesn't check for NULL, so pre-lookup here */
+	void *priv;
 
+	priv = (void *)(uintptr_t)minor(dev);
+	zfsdev_state_t *zs = zfsdev_private_get_state(priv);
+	if (zs != NULL)
+		zfsdev_state_destroy(priv);
 	return (0);
 }
 
@@ -240,9 +267,7 @@ static int
 zfs_ioc_osx_proxy_remove(const char *unused, nvlist_t *innvl,
     nvlist_t *outnvl)
 {
-	int error;
 	char *osname = NULL;
-	char value[MAXPATHLEN * 2];
 
 	if (nvlist_lookup_string(innvl,
 	    ZPOOL_CONFIG_POOL_NAME, &osname) != 0)
@@ -304,6 +329,33 @@ static struct cdevsw zfs_cdevsw = {
 	.d_type		= D_DISK
 };
 
+/*
+ * This is an identical copy of zfsdev_minor_alloc() except we check if
+ * 'last_minor + 0' is available instead of 'last_minor + 1'. The latter
+ * will cycle through minors unnecessarily, when it 'often' is available
+ * again. Which gives us unattractive things like;
+ * crw-rw-rw-  1 root  wheel   34, 0x0000213A May 31 14:42 /dev/zfs
+ */
+static minor_t
+zfsdev_minor_alloc_os(void)
+{
+	static minor_t last_minor = 0;
+	minor_t m;
+
+	ASSERT(MUTEX_HELD(&zfsdev_state_lock));
+
+	for (m = last_minor; m != last_minor; m++) {
+		if (m > ZFSDEV_MAX_MINOR)
+			m = 1;
+		if (zfsdev_get_state(m, ZST_ALL) == NULL) {
+			last_minor = m;
+			return (m);
+		}
+	}
+
+	return (0);
+}
+
 /* Callback to create a unique minor for each open */
 static int
 zfs_devfs_clone(__unused dev_t dev, int action)
@@ -312,7 +364,7 @@ zfs_devfs_clone(__unused dev_t dev, int action)
 
 	if (action == DEVFS_CLONE_ALLOC) {
 		mutex_enter(&zfsdev_state_lock);
-		minorx = zfsdev_minor_alloc();
+		minorx = zfsdev_minor_alloc_os();
 		mutex_exit(&zfsdev_state_lock);
 		return (minorx);
 	}