Block cloning conditionally destroy ARC buffer

dmu_buf_will_clone() calls arc_buf_destroy() if there is an assosciated
ARC buffer with the dbuf. However, this can only be done conditionally.
If the preivous dirty record's dr_data is pointed at db_dbf then
destroying it can lead to NULL pointer deference when syncing out the
previous dirty record.

This updates dmu_buf_fill_clone() to only call arc_buf_destroy() if the
previous dirty records dr_data is not pointing to db_buf. The block
clone wil still set the dbuf's db_buf and db_data to NULL, but this will
not cause any issues as any previous dirty record dr_data will still be
pointing at the ARC buffer.

A test case block_cloning_overwrite was added that was provided by Ameer
Hamza that would previously fail without this patch.

Signed-off-by: Brian Atkinson <[email protected]>

Author: bwatkinson

module/zfs/dbuf.c

@@ -2705,6 +2705,9 @@ void
 dmu_buf_will_clone(dmu_buf_t *db_fake, dmu_tx_t *tx)
 {
 	dmu_buf_impl_t *db = (dmu_buf_impl_t *)db_fake;
+	ASSERT0(db->db_level);
+	ASSERT(db->db_blkid != DMU_BONUS_BLKID);
+	ASSERT(db->db.db_object != DMU_META_DNODE_OBJECT);
 
 	/*
 	 * Block cloning: We are going to clone into this block, so undirty
@@ -2716,11 +2719,22 @@ dmu_buf_will_clone(dmu_buf_t *db_fake, dmu_tx_t *tx)
 	VERIFY(!dbuf_undirty(db, tx));
 	ASSERT0P(dbuf_find_dirty_eq(db, tx->tx_txg));
 	if (db->db_buf != NULL) {
-		arc_buf_destroy(db->db_buf, db);
+		/*
+		 * If there is an associated ARC buffer with this dbuf we can
+		 * only destroy it if the previous dirty record does not
+		 * reference it.
+		 */
+		dbuf_dirty_record_t *dr = list_head(&db->db_dirty_records);
+		if (dr == NULL || dr->dt.dl.dr_data != db->db_buf)
+			arc_buf_destroy(db->db_buf, db);
+
 		db->db_buf = NULL;
 		dbuf_clear_data(db);
 	}
 
+	ASSERT3P(db->db_buf, ==, NULL);
+	ASSERT3P(db->db.db_data, ==, NULL);
+
 	db->db_state = DB_NOFILL;
 	DTRACE_SET_STATE(db, "allocating NOFILL buffer for clone");
 

tests/runfiles/common.run

@@ -80,8 +80,9 @@ tests = ['block_cloning_clone_mmap_cached',
     'block_cloning_copyfilerange_cross_dataset',
     'block_cloning_cross_enc_dataset',
     'block_cloning_copyfilerange_fallback_same_txg',
-    'block_cloning_replay', 'block_cloning_replay_encrypted',
-    'block_cloning_lwb_buffer_overflow', 'block_cloning_clone_mmap_write']
+    'block_cloning_overwrites', 'block_cloning_replay',
+    'block_cloning_replay_encrypted', 'block_cloning_lwb_buffer_overflow',
+    'block_cloning_clone_mmap_write']
 tags = ['functional', 'block_cloning']
 
 [tests/functional/bootfs]

tests/test-runner/bin/zts-report.py.in

@@ -324,6 +324,8 @@ elif sys.platform.startswith('linux'):
             ['SKIP', cfr_cross_reason],
         'block_cloning/block_cloning_disabled_copyfilerange':
             ['SKIP', cfr_reason],
+        'block_cloning/block_cloning_overwrite':
+            ['SKIP', cfr_reason],
         'block_cloning/block_cloning_lwb_buffer_overflow':
             ['SKIP', cfr_reason],
         'block_cloning/block_cloning_replay':

tests/zfs-tests/tests/Makefile.am

@@ -475,6 +475,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/block_cloning/block_cloning_ficlonerange.ksh \
 	functional/block_cloning/block_cloning_ficlonerange_partial.ksh \
 	functional/block_cloning/block_cloning_cross_enc_dataset.ksh \
+	functional/block_cloning/block_cloning_overwrites.ksh \
 	functional/block_cloning/block_cloning_replay.ksh \
 	functional/block_cloning/block_cloning_replay_encrypted.ksh \
 	functional/block_cloning/block_cloning_lwb_buffer_overflow.ksh \

tests/zfs-tests/tests/functional/block_cloning/block_cloning_overwrites.ksh

@@ -0,0 +1,76 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or https://opensource.org/licenses/CDDL-1.0.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2024 by Triad National Security, LLC.
+#
+
+#
+# DESCRIPTION:
+#	Verify that overwriting a file that is being block cloned works
+#
+# STRATEGY:
+#	1. Create a file in a dataset
+#	2. Continously block clone the file to a second file in a different
+#	   dataset and sync the zpool after cloning
+#	3. Continously overwrite the second file
+#  
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/block_cloning/block_cloning.kshlib
+
+verify_runnable "global"
+
+if is_linux && [[ $(linux_version) -lt $(linux_version "4.5") ]]; then
+  log_unsupported "copy_file_range not available before Linux 4.5"
+fi
+
+claim="Overwriting a block cloned file does not cause issues."
+
+log_assert $claim
+
+function cleanup
+{
+	datasetexists $TESTPOOL && destroy_pool $TESTPOOL
+}
+
+log_onexit cleanup
+
+log_must zpool create -o feature@block_cloning=enabled $TESTPOOL $DISKS
+log_must zfs create -o sync=always $TESTPOOL/$TESTFS1
+log_must zfs create -o sync=always $TESTPOOL/$TESTFS2
+
+log_must dd if=/dev/urandom of=/$TESTPOOL/$TESTFS1/file1 bs=1M count=128
+log_must sync_pool $TESTPOOL
+runtime=$(($(date +%s) + 180)) # 3 mins
+
+while [[ $(date +%s) -lt $runtime ]]; do
+	log_must clonefile -f /$TESTPOOL/$TESTFS1/file1 \
+	    /$TESTPOOL/$TESTFS2/file2 0 0 $((128 * 1024 * 1024))
+	log_must sync_pool $TESTPOOL
+done &
+while [[ $(date +%s) -lt $runtime ]]; do
+	log_must dd if=/dev/urandom of=/$TESTPOOL/$TESTFS2/file2 bs=1M count=128
+done &
+wait
+
+log_pass $claim