Fix reads for blocks freed after being cloned

PR #18421 fixed a case when reads for blocks cloned after being
freed could return zeroes.  But it created an opposite problem,
when reads for blocks freed after being cloned could return non-
zero content from the cloning.

This patch fixes the problem by creating a more specialized
form of dnode_block_freed(), taking into account the TXG when
the cloning has happened and checking frees only in TXGs after.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes #18421
Closes #18724

include/sys/dnode.h

@@ -449,6 +449,8 @@ void dnode_diduse_space(dnode_t *dn, int64_t space);
 void dnode_new_blkid(dnode_t *dn, uint64_t blkid, dmu_tx_t *tx,
     boolean_t have_read, boolean_t force);
 uint64_t dnode_block_freed(dnode_t *dn, uint64_t blkid);
+uint64_t dnode_block_freed_after(dnode_t *dn, uint64_t blkid,
+    uint64_t override_txg);
 void dnode_init(void);
 void dnode_fini(void);
 int dnode_next_offset(dnode_t *dn, int flags, uint64_t *off,

module/zfs/dbuf.c

@@ -1476,17 +1476,25 @@ dbuf_read_hole(dmu_buf_impl_t *db, dnode_t *dn, blkptr_t *bp)
 
 	int is_hole = bp == NULL || BP_IS_HOLE(bp);
 	/*
-	 * For level 0 blocks only, if the above check fails:
-	 * Recheck BP_IS_HOLE() after dnode_block_freed() in case dnode_sync()
-	 * processes the delete record and clears the bp while we are waiting
-	 * for the dn_mtx (resulting in a "no" from block_freed).
+	 * For level 0 blocks only, if the above check didn't find a hole,
+	 * consult dnode_block_freed() to check for pending frees.
 	 *
-	 * If bp != db->db_blkptr, it means that it was overridden (by a block
-	 * clone or direct I/O write). We cannot rely on dnode_block_freed as
-	 * the range can be freed in an earlier TXG but overridden in later.
+	 * If the block has been overridden by a block clone or direct I/O
+	 * write, we can't use dnode_block_freed() directly because it would
+	 * find frees from TXGs before the override, which should not make
+	 * the block appear freed.  Instead, check only free ranges from
+	 * TXGs after the override.
 	 */
-	if (!is_hole && db->db_level == 0 && bp == db->db_blkptr)
-		is_hole = dnode_block_freed(dn, db->db_blkid) || BP_IS_HOLE(bp);
+	if (!is_hole && db->db_level == 0) {
+		dbuf_dirty_record_t *dr = list_head(&db->db_dirty_records);
+		if (dr != NULL &&
+		    (dr->dt.dl.dr_brtwrite || dr->dt.dl.dr_diowrite)) {
+			is_hole = dnode_block_freed_after(dn,
+			    db->db_blkid, dr->dr_txg);
+		} else {
+			is_hole = dnode_block_freed(dn, db->db_blkid);
+		}
+	}
 
 	if (is_hole) {
 		db_data = dbuf_alloc_arcbuf(db);

module/zfs/dnode.c

@@ -2475,6 +2475,37 @@ dnode_block_freed(dnode_t *dn, uint64_t blkid)
 	return (i < TXG_SIZE);
 }
 
+/*
+ * Check if a level-0 block was freed in a TXG after override_txg.
+ *
+ * When a block has been overridden (e.g., by block clone or direct I/O),
+ * we can't use dnode_block_freed() because it checks all active TXGs.
+ * A free from a TXG *before* the override should not make the block appear
+ * freed.
+ */
+uint64_t
+dnode_block_freed_after(dnode_t *dn, uint64_t blkid, uint64_t override_txg)
+{
+	ASSERT(blkid != DMU_BONUS_BLKID);
+	ASSERT(blkid != DMU_SPILL_BLKID);
+
+	if (dn->dn_free_txg)
+		return (TRUE);
+
+	mutex_enter(&dn->dn_mtx);
+	uint64_t open = spa_open_txg(dmu_objset_spa(dn->dn_objset));
+	for (uint64_t txg = override_txg + 1; txg <= open; txg++) {
+		int i = txg & TXG_MASK;
+		if (dn->dn_free_ranges[i] != NULL &&
+		    zfs_range_tree_contains(dn->dn_free_ranges[i], blkid, 1)) {
+			mutex_exit(&dn->dn_mtx);
+			return (TRUE);
+		}
+	}
+	mutex_exit(&dn->dn_mtx);
+	return (FALSE);
+}
+
 /* call from syncing context when we actually write/free space for this dnode */
 void
 dnode_diduse_space(dnode_t *dn, int64_t delta)