Fix zil replay panic when TX_REMOVE followed by TX_CREATE

If TX_REMOVE is followed by TX_CREATE on the same object id, we need to
make sure the object removal is completely finished before creation. The
current implementation relies on dnode_hold_impl with
DNODE_MUST_BE_ALLOCATED returning ENOENT. While this check seems to work
fine before, in current version it does not guarantee the object removal
is completed.

We fix this by checking if DNODE_MUST_BE_FREE returns successful
instead. Also add test and remove dead code in dnode_hold_impl.

Closes #7151
Closes #8910
Closes #9123
Signed-off-by: Chunwei Chen <david.chen@nutanix.com>

module/zfs/dnode.c

@@ -1518,16 +1518,6 @@ dnode_hold_impl(objset_t *os, uint64_t object, int flag, int slots,
 		return (SET_ERROR(EINVAL));
 	}
 
-	if (dn->dn_free_txg) {
-		DNODE_STAT_BUMP(dnode_hold_free_txg);
-		type = dn->dn_type;
-		mutex_exit(&dn->dn_mtx);
-		dnode_slots_rele(dnc, idx, slots);
-		dbuf_rele(db, FTAG);
-		return (SET_ERROR((flag & DNODE_MUST_BE_ALLOCATED) ?
-		    ENOENT : EEXIST));
-	}
-
 	if (zfs_refcount_add(&dn->dn_holds, tag) == 1)
 		dbuf_add_ref(db, dnh);
 

module/zfs/zfs_replay.c

@@ -270,6 +270,19 @@ zfs_replay_swap_attrs(lr_attr_t *lrattr)
 	    (lrattr->lr_attr_masksize - 1)), 3 * sizeof (uint64_t));
 }
 
+static int
+check_foid_available(zfsvfs_t *zfsvfs, uint64_t foid)
+{
+	int err;
+	dnode_t *dn;
+
+	err = dnode_hold_impl(zfsvfs->z_os, foid,
+	    DNODE_MUST_BE_FREE, 1, FTAG, &dn);
+	if (err == 0)
+		dnode_rele(dn, FTAG);
+	return (err);
+}
+
 /*
  * Replay file create with optional ACL, xvattr information as well
  * as option FUID information.
@@ -337,8 +350,8 @@ zfs_replay_create_acl(void *arg1, void *arg2, boolean_t byteswap)
 	xva.xva_vattr.va_nblocks = lr->lr_gen;
 	xva.xva_vattr.va_fsid = dnodesize;
 
-	error = dmu_object_info(zfsvfs->z_os, lr->lr_foid, NULL);
-	if (error != ENOENT)
+	error = check_foid_available(zfsvfs, lr->lr_foid);
+	if (error)
 		goto bail;
 
 	if (lr->lr_common.lrc_txtype & TX_CI)
@@ -473,8 +486,8 @@ zfs_replay_create(void *arg1, void *arg2, boolean_t byteswap)
 	xva.xva_vattr.va_nblocks = lr->lr_gen;
 	xva.xva_vattr.va_fsid = dnodesize;
 
-	error = dmu_object_info(zfsvfs->z_os, objid, NULL);
-	if (error != ENOENT)
+	error = check_foid_available(zfsvfs, lr->lr_foid);
+	if (error)
 		goto out;
 
 	if (lr->lr_common.lrc_txtype & TX_CI)

tests/zfs-tests/tests/functional/slog/slog_replay_fs.ksh

@@ -74,6 +74,17 @@ log_must zpool create $TESTPOOL $VDEV log mirror $LDEV
 log_must zfs set compression=on $TESTPOOL
 log_must zfs create $TESTPOOL/$TESTFS
 
+# Prep for the test of TX_REMOVE followed by TX_CREATE
+log_must mkdir /$TESTPOOL/$TESTFS/dir0
+log_must eval 'for i in $(seq 1000); do touch /$TESTPOOL/$TESTFS/dir0/file.$i; done'
+
+#
+# Reimport to reset dnode allocation pointer.
+# This is to make sure we will have TX_REMOVE and TX_CREATE on same id
+#
+log_must zpool export $TESTPOOL
+log_must zpool import -f -d $VDIR $TESTPOOL
+
 #
 # This dd command works around an issue where ZIL records aren't created
 # after freezing the pool unless a ZIL header already exists. Create a file
@@ -91,6 +102,10 @@ log_must zpool freeze $TESTPOOL
 # 3. Run various user commands that create files, directories and ACLs
 #
 
+# TX_REMOVE followed by TX_CREATE
+log_must eval 'rm -f /$TESTPOOL/$TESTFS/dir0/*'
+log_must eval 'for i in $(seq 1000); do touch /$TESTPOOL/$TESTFS/dir0/file.$i; done'
+
 # TX_CREATE
 log_must touch /$TESTPOOL/$TESTFS/a
 
@@ -193,6 +208,9 @@ log_must zpool import -f -d $VDIR $TESTPOOL
 log_note "Verify current block usage:"
 log_must zdb -bcv $TESTPOOL
 
+log_note "Verify number of files"
+log_must test "$(ls /$TESTPOOL/$TESTFS/dir0 | wc -l)" -eq 1000
+
 log_note "Verify copy of xattrs:"
 log_must attr -l /$TESTPOOL/$TESTFS/xattr.dir
 log_must attr -l /$TESTPOOL/$TESTFS/xattr.file