Fix cloning into already dirty dbufs.

Undirty the dbuf and destroy its buffer when cloning into it.

Coverity ID: CID-1535375
Reported-by: Richard Yao
Reported-by: Benjamin Coddington
Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Pawel Jakub Dawidek <pawel@dawidek.net>
Closes #14655

diff --git a/include/sys/dbuf.h b/include/sys/dbuf.h
index a06316362e57394fa265a0671afd331984b35b7d..fb26a83b184456684c74eadaef020f6b7ec4be0e 100644 (file)
--- a/include/sys/dbuf.h
+++ b/include/sys/dbuf.h
@@ -382,6 +382,7 @@ void dbuf_assign_arcbuf(dmu_buf_impl_t *db, arc_buf_t *buf, dmu_tx_t *tx);
 dbuf_dirty_record_t *dbuf_dirty(dmu_buf_impl_t *db, dmu_tx_t *tx);
 dbuf_dirty_record_t *dbuf_dirty_lightweight(dnode_t *dn, uint64_t blkid,
     dmu_tx_t *tx);
+boolean_t dbuf_undirty(dmu_buf_impl_t *db, dmu_tx_t *tx);
 arc_buf_t *dbuf_loan_arcbuf(dmu_buf_impl_t *db);
 void dmu_buf_write_embedded(dmu_buf_t *dbuf, void *data,
     bp_embedded_type_t etype, enum zio_compress comp,

diff --git a/module/zfs/dbuf.c b/module/zfs/dbuf.c
index 80ea1bfe41971cccb040e828abf0306e2f4f507a..617c850296b44854f6fb28c98c243c135eaaeb0d 100644 (file)
--- a/module/zfs/dbuf.c
+++ b/module/zfs/dbuf.c
@@ -175,7 +175,6 @@ struct {
                continue;                                               \
 }
 
-static boolean_t dbuf_undirty(dmu_buf_impl_t *db, dmu_tx_t *tx);
 static void dbuf_write(dbuf_dirty_record_t *dr, arc_buf_t *data, dmu_tx_t *tx);
 static void dbuf_sync_leaf_verify_bonus_dnode(dbuf_dirty_record_t *dr);
 static int dbuf_read_verify_dnode_crypt(dmu_buf_impl_t *db, uint32_t flags);
@@ -2518,7 +2517,7 @@ dbuf_undirty_bonus(dbuf_dirty_record_t *dr)
  * Undirty a buffer in the transaction group referenced by the given
  * transaction.  Return whether this evicted the dbuf.
  */
-static boolean_t
+boolean_t
 dbuf_undirty(dmu_buf_impl_t *db, dmu_tx_t *tx)
 {
        uint64_t txg = tx->tx_txg;

diff --git a/module/zfs/dmu.c b/module/zfs/dmu.c
index e6bade11c8593710599ef7211fb05155ffad9582..23b6667524cc2a8ed3e1b30eda897c100c66a8f5 100644 (file)
--- a/module/zfs/dmu.c
+++ b/module/zfs/dmu.c
@@ -2190,7 +2190,8 @@ dmu_read_l0_bps(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
        for (int i = 0; i < numbufs; i++) {
                dbuf = dbp[i];
                db = (dmu_buf_impl_t *)dbuf;
-               bp = db->db_blkptr;
+
+               mutex_enter(&db->db_mtx);
 
                /*
                 * If the block is not on the disk yet, it has no BP assigned.
@@ -2212,10 +2213,16 @@ dmu_read_l0_bps(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
                                 * The block was modified in the same
                                 * transaction group.
                                 */
+                               mutex_exit(&db->db_mtx);
                                error = SET_ERROR(EAGAIN);
                                goto out;
                        }
+               } else {
+                       bp = db->db_blkptr;
                }
+
+               mutex_exit(&db->db_mtx);
+
                if (bp == NULL) {
                        /*
                         * The block was created in this transaction group,
@@ -2273,19 +2280,23 @@ dmu_brt_clone(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
                ASSERT(db->db_blkid != DMU_BONUS_BLKID);
                ASSERT(BP_IS_HOLE(bp) || dbuf->db_size == BP_GET_LSIZE(bp));
 
-               if (db->db_state == DB_UNCACHED) {
-                       /*
-                        * XXX-PJD: If the dbuf is already cached, calling
-                        * dmu_buf_will_not_fill() will panic on assertion
-                        * (db->db_buf == NULL) in dbuf_clear_data(),
-                        * which is called from dbuf_noread() in DB_NOFILL
-                        * case. I'm not 100% sure this is the right thing
-                        * to do, but it seems to work.
-                        */
-                       dmu_buf_will_not_fill(dbuf, tx);
+               mutex_enter(&db->db_mtx);
+
+               VERIFY(!dbuf_undirty(db, tx));
+               ASSERT(list_head(&db->db_dirty_records) == NULL);
+               if (db->db_buf != NULL) {
+                       arc_buf_destroy(db->db_buf, db);
+                       db->db_buf = NULL;
                }
 
+               mutex_exit(&db->db_mtx);
+
+               dmu_buf_will_not_fill(dbuf, tx);
+
+               mutex_enter(&db->db_mtx);
+
                dr = list_head(&db->db_dirty_records);
+               VERIFY(dr != NULL);
                ASSERT3U(dr->dr_txg, ==, tx->tx_txg);
                dl = &dr->dt.dl;
                dl->dr_overridden_by = *bp;
@@ -2301,6 +2312,8 @@ dmu_brt_clone(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
                            BP_PHYSICAL_BIRTH(bp);
                }
 
+               mutex_exit(&db->db_mtx);
+
                /*
                 * When data in embedded into BP there is no need to create
                 * BRT entry as there is no data block. Just copy the BP as