Improve dbuf_read() error reporting

Previous code reported non-ZIO errors only via return value, but
not via parent ZIO.  It could cause NULL-dereference panics due
to dmu_buf_hold_array_by_dnode() ignoring the return value,
relying solely on parent ZIO status.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ameer Hamza <ahamza@ixsystems.com>
Reported by:	Ameer Hamza <ahamza@ixsystems.com>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes #16042

diff --git a/module/zfs/dbuf.c b/module/zfs/dbuf.c
index 4e190c131e1dde2f9977573071d597f11b9c1b28..0ab143bd089f7e3723af891c0c93bba3bd1bd703 100644 (file)
--- a/module/zfs/dbuf.c
+++ b/module/zfs/dbuf.c
@@ -1557,17 +1557,14 @@ dbuf_read_verify_dnode_crypt(dmu_buf_impl_t *db, uint32_t flags)
  * returning.
  */
 static int
-dbuf_read_impl(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags,
+dbuf_read_impl(dmu_buf_impl_t *db, dnode_t *dn, zio_t *zio, uint32_t flags,
     db_lock_type_t dblt, const void *tag)
 {
-       dnode_t *dn;
        zbookmark_phys_t zb;
        uint32_t aflags = ARC_FLAG_NOWAIT;
        int err, zio_flags;
        blkptr_t bp, *bpp;
 
-       DB_DNODE_ENTER(db);
-       dn = DB_DNODE(db);
        ASSERT(!zfs_refcount_is_zero(&db->db_holds));
        ASSERT(MUTEX_HELD(&db->db_mtx));
        ASSERT(db->db_state == DB_UNCACHED || db->db_state == DB_NOFILL);
@@ -1643,8 +1640,6 @@ dbuf_read_impl(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags,
        if (err != 0)
                goto early_unlock;
 
-       DB_DNODE_EXIT(db);
-
        db->db_state = DB_READ;
        DTRACE_SET_STATE(db, "read issued");
        mutex_exit(&db->db_mtx);
@@ -1669,12 +1664,11 @@ dbuf_read_impl(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags,
         * parent's rwlock, which would be a lock ordering violation.
         */
        dmu_buf_unlock_parent(db, dblt, tag);
-       (void) arc_read(zio, db->db_objset->os_spa, bpp,
+       return (arc_read(zio, db->db_objset->os_spa, bpp,
            dbuf_read_done, db, ZIO_PRIORITY_SYNC_READ, zio_flags,
-           &aflags, &zb);
-       return (err);
+           &aflags, &zb));
+
 early_unlock:
-       DB_DNODE_EXIT(db);
        mutex_exit(&db->db_mtx);
        dmu_buf_unlock_parent(db, dblt, tag);
        return (err);
@@ -1759,7 +1753,7 @@ dbuf_fix_old_data(dmu_buf_impl_t *db, uint64_t txg)
 }
 
 int
-dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
+dbuf_read(dmu_buf_impl_t *db, zio_t *pio, uint32_t flags)
 {
        int err = 0;
        boolean_t prefetch;
@@ -1775,7 +1769,7 @@ dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
        dn = DB_DNODE(db);
 
        prefetch = db->db_level == 0 && db->db_blkid != DMU_BONUS_BLKID &&
-           (flags & DB_RF_NOPREFETCH) == 0 && dn != NULL;
+           (flags & DB_RF_NOPREFETCH) == 0;
 
        mutex_enter(&db->db_mtx);
        if (flags & DB_RF_PARTIAL_FIRST)
@@ -1822,13 +1816,13 @@ dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
 
                db_lock_type_t dblt = dmu_buf_lock_parent(db, RW_READER, FTAG);
 
-               if (zio == NULL && (db->db_state == DB_NOFILL ||
+               if (pio == NULL && (db->db_state == DB_NOFILL ||
                    (db->db_blkptr != NULL && !BP_IS_HOLE(db->db_blkptr)))) {
                        spa_t *spa = dn->dn_objset->os_spa;
-                       zio = zio_root(spa, NULL, NULL, ZIO_FLAG_CANFAIL);
+                       pio = zio_root(spa, NULL, NULL, ZIO_FLAG_CANFAIL);
                        need_wait = B_TRUE;
                }
-               err = dbuf_read_impl(db, zio, flags, dblt, FTAG);
+               err = dbuf_read_impl(db, dn, pio, flags, dblt, FTAG);
                /*
                 * dbuf_read_impl has dropped db_mtx and our parent's rwlock
                 * for us
@@ -1849,9 +1843,10 @@ dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
                 */
                if (need_wait) {
                        if (err == 0)
-                               err = zio_wait(zio);
+                               err = zio_wait(pio);
                        else
-                               VERIFY0(zio_wait(zio));
+                               (void) zio_wait(pio);
+                       pio = NULL;
                }
        } else {
                /*
@@ -1878,7 +1873,7 @@ dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
                                ASSERT(db->db_state == DB_READ ||
                                    (flags & DB_RF_HAVESTRUCT) == 0);
                                DTRACE_PROBE2(blocked__read, dmu_buf_impl_t *,
-                                   db, zio_t *, zio);
+                                   db, zio_t *, pio);
                                cv_wait(&db->db_changed, &db->db_mtx);
                        }
                        if (db->db_state == DB_UNCACHED)
@@ -1887,6 +1882,13 @@ dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
                }
        }
 
+       if (pio && err != 0) {
+               zio_t *zio = zio_null(pio, pio->io_spa, NULL, NULL, NULL,
+                   ZIO_FLAG_CANFAIL);
+               zio->io_error = err;
+               zio_nowait(zio);
+       }
+
        return (err);
 }