Skip to content

Commit

Permalink
zfs_rename: restructure to have cleaner fallbacks
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Aleksa Sarai <[email protected]>
Closes openzfs#12209
Closes openzfs#14070
  • Loading branch information
@cyphar @shodanshok
cyphar authored and shodanshok committed Nov 3, 2022
1 parent feb7d79 commit 91611d2
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 76 deletions.
1 change: 1 addition & 0 deletions include/os/linux/zfs/sys/zfs_dir.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *);
Expand Down
95 changes: 70 additions & 25 deletions module/os/linux/zfs/zfs_dir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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) {
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);
count = 0;
ASSERT(error == 0);
/* The only error is !zfs_dirempty() and we checked earlier. */
ASSERT3U(zfs_drop_nlink_locked(zp, tx, &unlinked), ==, 0);
mutex_exit(&zp->z_lock);
} else {
error = zfs_dropname(dl, zp, dzp, tx, flag);
Expand Down
119 changes: 68 additions & 51 deletions module/os/linux/zfs/zfs_vnops_os.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2876,16 +2876,12 @@ zfs_rename(znode_t *sdzp, char *snm, znode_t *tdzp, char *tnm,
/*
* Source and target must be the same type.
*/
if (S_ISDIR(ZTOI(szp)->i_mode)) {
if (!S_ISDIR(ZTOI(tzp)->i_mode)) {
error = SET_ERROR(ENOTDIR);
goto out;
}
} else {
if (S_ISDIR(ZTOI(tzp)->i_mode)) {
error = SET_ERROR(EISDIR);
goto out;
}
boolean_t s_is_dir = S_ISDIR(ZTOI(szp)->i_mode) != 0;
boolean_t t_is_dir = S_ISDIR(ZTOI(tzp)->i_mode) != 0;

if (s_is_dir != t_is_dir) {
error = SET_ERROR(s_is_dir ? ENOTDIR : EISDIR);
goto out;
}
/*
* POSIX dictates that when the source and target
Expand Down Expand Up @@ -2941,51 +2937,49 @@ zfs_rename(znode_t *sdzp, char *snm, znode_t *tdzp, char *tnm,
return (error);
}

if (tzp) /* Attempt to remove the existing target */
error = zfs_link_destroy(tdl, tzp, tx, zflg, NULL);
/*
* Unlink the source.
*/
szp->z_pflags |= ZFS_AV_MODIFIED;
if (tdzp->z_pflags & ZFS_PROJINHERIT)
szp->z_pflags |= ZFS_PROJINHERIT;

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;

error = sa_update(szp->z_sa_hdl, SA_ZPL_FLAGS(zfsvfs),
(void *)&szp->z_pflags, sizeof (uint64_t), tx);
ASSERT0(error);
error = sa_update(szp->z_sa_hdl, SA_ZPL_FLAGS(zfsvfs),
(void *)&szp->z_pflags, sizeof (uint64_t), tx);
ASSERT0(error);

error = zfs_link_destroy(sdl, szp, tx, ZRENAMING, NULL);
if (error == 0) {
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);
}
error = zfs_link_destroy(sdl, szp, tx, ZRENAMING, NULL);
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);

commit:
dmu_tx_commit(tx);
out:
if (zl != NULL)
Expand Down Expand Up @@ -3013,6 +3007,29 @@ zfs_rename(znode_t *sdzp, char *snm, znode_t *tdzp, char *tnm,

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;
}

/*
Expand Down

0 comments on commit 91611d2

Please sign in to comment.