zfs_rename: restructure to have cleaner fallbacks

This is in preparation for RENAME_EXCHANGE and RENAME_WHITEOUT support
for ZoL, but the changes here allow for far nicer fallbacks than the
previous implementation (the source and target are re-linked in case of
the final link failing).

In addition, a small cleanup was done for the "target exists but is a
different type" codepath so that it's more understandable.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Closes #12209
Closes #14070

include/os/linux/zfs/sys/zfs_dir.h

@@ -52,6 +52,7 @@ extern "C" {
 extern int zfs_dirent_lock(zfs_dirlock_t **, znode_t *, char *, znode_t **,
     int, int *, pathname_t *);
 extern void zfs_dirent_unlock(zfs_dirlock_t *);
+extern int zfs_drop_nlink(znode_t *, dmu_tx_t *, boolean_t *);
 extern int zfs_link_create(zfs_dirlock_t *, znode_t *, dmu_tx_t *, int);
 extern int zfs_link_destroy(zfs_dirlock_t *, znode_t *, dmu_tx_t *, int,
     boolean_t *);

module/os/linux/zfs/zfs_dir.c

@@ -926,6 +926,74 @@ zfs_dropname(zfs_dirlock_t *dl, znode_t *zp, znode_t *dzp, dmu_tx_t *tx,
 	return (error);
 }
 
+static int
+zfs_drop_nlink_locked(znode_t *zp, dmu_tx_t *tx, boolean_t *unlinkedp)
+{
+	zfsvfs_t	*zfsvfs = ZTOZSB(zp);
+	int		zp_is_dir = S_ISDIR(ZTOI(zp)->i_mode);
+	boolean_t	unlinked = B_FALSE;
+	sa_bulk_attr_t	bulk[3];
+	uint64_t	mtime[2], ctime[2];
+	uint64_t	links;
+	int		count = 0;
+	int		error;
+
+	if (zp_is_dir && !zfs_dirempty(zp))
+		return (SET_ERROR(ENOTEMPTY));
+
+	if (ZTOI(zp)->i_nlink <= zp_is_dir) {
+		zfs_panic_recover("zfs: link count on %lu is %u, "
+		    "should be at least %u", zp->z_id,
+		    (int)ZTOI(zp)->i_nlink, zp_is_dir + 1);
+		set_nlink(ZTOI(zp), zp_is_dir + 1);
+	}
+	drop_nlink(ZTOI(zp));
+	if (ZTOI(zp)->i_nlink == zp_is_dir) {
+		zp->z_unlinked = B_TRUE;
+		clear_nlink(ZTOI(zp));
+		unlinked = B_TRUE;
+	} else {
+		SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zfsvfs),
+		    NULL, &ctime, sizeof (ctime));
+		SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_FLAGS(zfsvfs),
+		    NULL, &zp->z_pflags, sizeof (zp->z_pflags));
+		zfs_tstamp_update_setup(zp, STATE_CHANGED, mtime,
+		    ctime);
+	}
+	links = ZTOI(zp)->i_nlink;
+	SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_LINKS(zfsvfs),
+	    NULL, &links, sizeof (links));
+	error = sa_bulk_update(zp->z_sa_hdl, bulk, count, tx);
+	ASSERT3U(error, ==, 0);
+
+	if (unlinkedp != NULL)
+		*unlinkedp = unlinked;
+	else if (unlinked)
+		zfs_unlinked_add(zp, tx);
+
+	return (0);
+}
+
+/*
+ * Forcefully drop an nlink reference from (zp) and mark it for deletion if it
+ * was the last link. This *must* only be done to znodes which have already
+ * been zfs_link_destroy()'d with ZRENAMING. This is explicitly only used in
+ * the error path of zfs_rename(), where we have to correct the nlink count if
+ * we failed to link the target as well as failing to re-link the original
+ * znodes.
+ */
+int
+zfs_drop_nlink(znode_t *zp, dmu_tx_t *tx, boolean_t *unlinkedp)
+{
+	int error;
+
+	mutex_enter(&zp->z_lock);
+	error = zfs_drop_nlink_locked(zp, tx, unlinkedp);
+	mutex_exit(&zp->z_lock);
+
+	return (error);
+}
+
 /*
  * Unlink zp from dl, and mark zp for deletion if this was the last link. Can
  * fail if zp is a mount point (EBUSY) or a non-empty directory (ENOTEMPTY).
@@ -966,31 +1034,8 @@ zfs_link_destroy(zfs_dirlock_t *dl, znode_t *zp, dmu_tx_t *tx, int flag,
 			return (error);
 		}
 
-		if (ZTOI(zp)->i_nlink <= zp_is_dir) {
+		/* The only error is !zfs_dirempty() and we checked earlier. */
-			zfs_panic_recover("zfs: link count on %lu is %u, "
+		ASSERT3U(zfs_drop_nlink_locked(zp, tx, &unlinked), ==, 0);
-			    "should be at least %u", zp->z_id,
-			    (int)ZTOI(zp)->i_nlink, zp_is_dir + 1);
-			set_nlink(ZTOI(zp), zp_is_dir + 1);
-		}
-		drop_nlink(ZTOI(zp));
-		if (ZTOI(zp)->i_nlink == zp_is_dir) {
-			zp->z_unlinked = B_TRUE;
-			clear_nlink(ZTOI(zp));
-			unlinked = B_TRUE;
-		} else {
-			SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zfsvfs),
-			    NULL, &ctime, sizeof (ctime));
-			SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_FLAGS(zfsvfs),
-			    NULL, &zp->z_pflags, sizeof (zp->z_pflags));
-			zfs_tstamp_update_setup(zp, STATE_CHANGED, mtime,
-			    ctime);
-		}
-		links = ZTOI(zp)->i_nlink;
-		SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_LINKS(zfsvfs),
-		    NULL, &links, sizeof (links));
-		error = sa_bulk_update(zp->z_sa_hdl, bulk, count, tx);
-		count = 0;
-		ASSERT(error == 0);
 		mutex_exit(&zp->z_lock);
 	} else {
 		error = zfs_dropname(dl, zp, dzp, tx, flag);

module/os/linux/zfs/zfs_vnops_os.c

@@ -2876,17 +2876,13 @@ top:
 		/*
 		 * Source and target must be the same type.
 		 */
-		if (S_ISDIR(ZTOI(szp)->i_mode)) {
+		boolean_t s_is_dir = S_ISDIR(ZTOI(szp)->i_mode) != 0;
-			if (!S_ISDIR(ZTOI(tzp)->i_mode)) {
+		boolean_t t_is_dir = S_ISDIR(ZTOI(tzp)->i_mode) != 0;
-				error = SET_ERROR(ENOTDIR);
+
+		if (s_is_dir != t_is_dir) {
+			error = SET_ERROR(s_is_dir ? ENOTDIR : EISDIR);
 			goto out;
 		}
-		} else {
-			if (S_ISDIR(ZTOI(tzp)->i_mode)) {
-				error = SET_ERROR(EISDIR);
-				goto out;
-			}
-		}
 		/*
 		 * POSIX dictates that when the source and target
 		 * entries refer to the same file object, rename
@@ -2941,12 +2937,9 @@ top:
 		return (error);
 	}
 
-	if (tzp)	/* Attempt to remove the existing target */
+	/*
-		error = zfs_link_destroy(tdl, tzp, tx, zflg, NULL);
+	 * Unlink the source.
-
+	 */
-	if (error == 0) {
-		error = zfs_link_create(tdl, szp, tx, ZRENAMING);
-		if (error == 0) {
 	szp->z_pflags |= ZFS_AV_MODIFIED;
 	if (tdzp->z_pflags & ZFS_PROJINHERIT)
 		szp->z_pflags |= ZFS_PROJINHERIT;
@@ -2956,36 +2949,37 @@ top:
 	ASSERT0(error);
 
 	error = zfs_link_destroy(sdl, szp, tx, ZRENAMING, NULL);
-			if (error == 0) {
+	if (error)
+		goto commit;
+
+	/*
+	 * Unlink the target.
+	 */
+	if (tzp) {
+		error = zfs_link_destroy(tdl, tzp, tx, zflg, NULL);
+		if (error)
+			goto commit_link_szp;
+	}
+
+	/*
+	 * Create a new link at the target.
+	 */
+	error = zfs_link_create(tdl, szp, tx, ZRENAMING);
+	if (error) {
+		/*
+		 * If we have removed the existing target, a subsequent call to
+		 * zfs_link_create() to add back the same entry, but with a new
+		 * dnode (szp), should not fail.
+		 */
+		ASSERT3P(tzp, ==, NULL);
+		goto commit_link_tzp;
+	}
+
 	zfs_log_rename(zilog, tx, TX_RENAME |
 	    (flags & FIGNORECASE ? TX_CI : 0), sdzp,
 	    sdl->dl_name, tdzp, tdl->dl_name, szp);
-			} else {
-				/*
-				 * At this point, we have successfully created
-				 * the target name, but have failed to remove
-				 * the source name.  Since the create was done
-				 * with the ZRENAMING flag, there are
-				 * complications; for one, the link count is
-				 * wrong.  The easiest way to deal with this
-				 * is to remove the newly created target, and
-				 * return the original error.  This must
-				 * succeed; fortunately, it is very unlikely to
-				 * fail, since we just created it.
-				 */
-				VERIFY3U(zfs_link_destroy(tdl, szp, tx,
-				    ZRENAMING, NULL), ==, 0);
-			}
-		} else {
-			/*
-			 * If we had removed the existing target, subsequent
-			 * call to zfs_link_create() to add back the same entry
-			 * but, the new dnode (szp) should not fail.
-			 */
-			ASSERT(tzp == NULL);
-		}
-	}
 
+commit:
 	dmu_tx_commit(tx);
 out:
 	if (zl != NULL)
@@ -3013,6 +3007,29 @@ out:
 
 	zfs_exit(zfsvfs, FTAG);
 	return (error);
+
+	/*
+	 * Clean-up path for broken link state.
+	 *
+	 * At this point we are in a (very) bad state, so we need to do our
+	 * best to correct the state. In particular, the nlink of szp is wrong
+	 * because we were destroying and creating links with ZRENAMING.
+	 *
+	 * link_create()s are allowed to fail (though they shouldn't because we
+	 * only just unlinked them and are putting the entries back during
+	 * clean-up). But if they fail, we can just forcefully drop the nlink
+	 * value to (at the very least) avoid broken nlink values -- though in
+	 * the case of non-empty directories we will have to panic.
+	 */
+commit_link_tzp:
+	if (tzp) {
+		if (zfs_link_create(tdl, tzp, tx, ZRENAMING))
+			VERIFY3U(zfs_drop_nlink(tzp, tx, NULL), ==, 0);
+	}
+commit_link_szp:
+	if (zfs_link_create(sdl, szp, tx, ZRENAMING))
+		VERIFY3U(zfs_drop_nlink(szp, tx, NULL), ==, 0);
+	goto commit;
 }
 
 /*