Skip to content

dnode: fix how we track and check dirtyness #17658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

dnode: fix how we track and check dirtyness #17658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0d28f44
dnode: add dn_dirtycnt, count of number of txgs this dnode is dirty on
robn Aug 19, 2025
c3e90e9
dnode_is_dirty: reimplement in terms of dn_dirtycnt
robn Aug 19, 2025
927bae1
dnode: remove dn_dirty_txg and DNODE_IS_DIRTY
robn Aug 19, 2025
3e5cb7b
dnode: remove dn_dirtyctx_firstset
robn Aug 19, 2025
9a744de
dnode: remove dn_dirtyctx and dnode_dirtycontext
robn Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions include/sys/dmu_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,12 +168,10 @@ extern "C" {
* dn_allocated_txg
* dn_free_txg
* dn_assigned_txg
* dn_dirty_txg
* dn_dirtycnt
* dd_assigned_tx
* dn_notxholds
* dn_nodnholds
* dn_dirtyctx
* dn_dirtyctx_firstset
* (dn_phys copy fields?)
* (dn_phys contents?)
* held from:
Expand Down
14 changes: 1 addition & 13 deletions include/sys/dnode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,6 @@ struct dmu_buf_impl;
struct objset;
struct zio;

enum dnode_dirtycontext {
DN_UNDIRTIED,
DN_DIRTY_OPEN,
DN_DIRTY_SYNC
};

/* Is dn_used in bytes? if not, it's in multiples of SPA_MINBLOCKSIZE */
#define DNODE_FLAG_USED_BYTES (1 << 0)
#define DNODE_FLAG_USERUSED_ACCOUNTED (1 << 1)
Expand Down Expand Up @@ -340,11 +334,9 @@ struct dnode {
uint64_t dn_allocated_txg;
uint64_t dn_free_txg;
uint64_t dn_assigned_txg;
uint64_t dn_dirty_txg; /* txg dnode was last dirtied */
uint8_t dn_dirtycnt;
kcondvar_t dn_notxholds;
kcondvar_t dn_nodnholds;
enum dnode_dirtycontext dn_dirtyctx;
const void *dn_dirtyctx_firstset; /* dbg: contents meaningless */

/* protected by own devices */
zfs_refcount_t dn_tx_holds;
Expand Down Expand Up @@ -440,7 +432,6 @@ void dnode_rele_and_unlock(dnode_t *dn, const void *tag, boolean_t evicting);
int dnode_try_claim(objset_t *os, uint64_t object, int slots);
boolean_t dnode_is_dirty(dnode_t *dn);
void dnode_setdirty(dnode_t *dn, dmu_tx_t *tx);
void dnode_set_dirtyctx(dnode_t *dn, dmu_tx_t *tx, const void *tag);
void dnode_sync(dnode_t *dn, dmu_tx_t *tx);
void dnode_allocate(dnode_t *dn, dmu_object_type_t ot, int blocksize, int ibs,
dmu_object_type_t bonustype, int bonuslen, int dn_slots, dmu_tx_t *tx);
Expand Down Expand Up @@ -468,9 +459,6 @@ void dnode_free_interior_slots(dnode_t *dn);

void dnode_set_storage_type(dnode_t *dn, dmu_object_type_t type);

#define DNODE_IS_DIRTY(_dn) \
((_dn)->dn_dirty_txg >= spa_syncing_txg((_dn)->dn_objset->os_spa))

#define DNODE_LEVEL_IS_CACHEABLE(_dn, _level) \
((_dn)->dn_objset->os_primary_cache == ZFS_CACHE_ALL || \
(((_level) > 0 || DMU_OT_IS_METADATA((_dn)->dn_type)) && \
Expand Down
21 changes: 0 additions & 21 deletions module/zfs/dbuf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2270,14 +2270,6 @@ dbuf_dirty(dmu_buf_impl_t *db, dmu_tx_t *tx)
if (dn->dn_objset->os_dsl_dataset != NULL)
rrw_exit(&dn->dn_objset->os_dsl_dataset->ds_bp_rwlock, FTAG);
#endif
/*
* We make this assert for private objects as well, but after we
* check if we're already dirty. They are allowed to re-dirty
* in syncing context.
*/
ASSERT(dn->dn_object == DMU_META_DNODE_OBJECT ||
dn->dn_dirtyctx == DN_UNDIRTIED || dn->dn_dirtyctx ==
(dmu_tx_is_syncing(tx) ? DN_DIRTY_SYNC : DN_DIRTY_OPEN));

mutex_enter(&db->db_mtx);
/*
Expand All @@ -2289,12 +2281,6 @@ dbuf_dirty(dmu_buf_impl_t *db, dmu_tx_t *tx)
db->db_state == DB_CACHED || db->db_state == DB_FILL ||
db->db_state == DB_NOFILL);

mutex_enter(&dn->dn_mtx);
dnode_set_dirtyctx(dn, tx, db);
if (tx->tx_txg > dn->dn_dirty_txg)
dn->dn_dirty_txg = tx->tx_txg;
mutex_exit(&dn->dn_mtx);

if (db->db_blkid == DMU_SPILL_BLKID)
dn->dn_have_spill = B_TRUE;

Expand All @@ -2313,13 +2299,6 @@ dbuf_dirty(dmu_buf_impl_t *db, dmu_tx_t *tx)
return (dr_next);
}

/*
* Only valid if not already dirty.
*/
ASSERT(dn->dn_object == 0 ||
dn->dn_dirtyctx == DN_UNDIRTIED || dn->dn_dirtyctx ==
(dmu_tx_is_syncing(tx) ? DN_DIRTY_SYNC : DN_DIRTY_OPEN));

ASSERT3U(dn->dn_nlevels, >, db->db_level);

/*
Expand Down
6 changes: 6 additions & 0 deletions module/zfs/dmu_objset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2037,6 +2037,8 @@ userquota_updates_task(void *arg)
dn->dn_id_flags |= DN_ID_CHKED_BONUS;
}
dn->dn_id_flags &= ~(DN_ID_NEW_EXIST);
ASSERT3U(dn->dn_dirtycnt, >, 0);
dn->dn_dirtycnt--;
mutex_exit(&dn->dn_mtx);

multilist_sublist_remove(list, dn);
Expand Down Expand Up @@ -2070,6 +2072,10 @@ dnode_rele_task(void *arg)

dnode_t *dn;
while ((dn = multilist_sublist_head(list)) != NULL) {
mutex_enter(&dn->dn_mtx);
ASSERT3U(dn->dn_dirtycnt, >, 0);
dn->dn_dirtycnt--;
mutex_exit(&dn->dn_mtx);
multilist_sublist_remove(list, dn);
dnode_rele(dn, &os->os_synced_dnodes);
}
Expand Down
103 changes: 32 additions & 71 deletions module/zfs/dnode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,7 @@ dnode_cons(void *arg, void *unused, int kmflag)
dn->dn_allocated_txg = 0;
dn->dn_free_txg = 0;
dn->dn_assigned_txg = 0;
dn->dn_dirty_txg = 0;
dn->dn_dirtyctx = 0;
dn->dn_dirtyctx_firstset = NULL;
dn->dn_dirtycnt = 0;
dn->dn_bonus = NULL;
dn->dn_have_spill = B_FALSE;
dn->dn_zio = NULL;
Expand Down Expand Up @@ -229,9 +227,7 @@ dnode_dest(void *arg, void *unused)
ASSERT0(dn->dn_allocated_txg);
ASSERT0(dn->dn_free_txg);
ASSERT0(dn->dn_assigned_txg);
ASSERT0(dn->dn_dirty_txg);
ASSERT0(dn->dn_dirtyctx);
ASSERT0P(dn->dn_dirtyctx_firstset);
ASSERT0(dn->dn_dirtycnt);
ASSERT0P(dn->dn_bonus);
ASSERT(!dn->dn_have_spill);
ASSERT0P(dn->dn_zio);
Expand Down Expand Up @@ -692,10 +688,8 @@ dnode_destroy(dnode_t *dn)
dn->dn_allocated_txg = 0;
dn->dn_free_txg = 0;
dn->dn_assigned_txg = 0;
dn->dn_dirty_txg = 0;
dn->dn_dirtycnt = 0;

dn->dn_dirtyctx = 0;
dn->dn_dirtyctx_firstset = NULL;
if (dn->dn_bonus != NULL) {
mutex_enter(&dn->dn_bonus->db_mtx);
dbuf_destroy(dn->dn_bonus);
Expand Down Expand Up @@ -800,11 +794,9 @@ dnode_allocate(dnode_t *dn, dmu_object_type_t ot, int blocksize, int ibs,
dn->dn_bonuslen = bonuslen;
dn->dn_checksum = ZIO_CHECKSUM_INHERIT;
dn->dn_compress = ZIO_COMPRESS_INHERIT;
dn->dn_dirtyctx = 0;

dn->dn_free_txg = 0;
dn->dn_dirtyctx_firstset = NULL;
dn->dn_dirty_txg = 0;
dn->dn_dirtycnt = 0;

dn->dn_allocated_txg = tx->tx_txg;
dn->dn_id_flags = 0;
Expand Down Expand Up @@ -955,9 +947,7 @@ dnode_move_impl(dnode_t *odn, dnode_t *ndn)
ndn->dn_allocated_txg = odn->dn_allocated_txg;
ndn->dn_free_txg = odn->dn_free_txg;
ndn->dn_assigned_txg = odn->dn_assigned_txg;
ndn->dn_dirty_txg = odn->dn_dirty_txg;
ndn->dn_dirtyctx = odn->dn_dirtyctx;
ndn->dn_dirtyctx_firstset = odn->dn_dirtyctx_firstset;
ndn->dn_dirtycnt = odn->dn_dirtycnt;
ASSERT0(zfs_refcount_count(&odn->dn_tx_holds));
zfs_refcount_transfer(&ndn->dn_holds, &odn->dn_holds);
ASSERT(avl_is_empty(&ndn->dn_dbufs));
Expand Down Expand Up @@ -1020,9 +1010,7 @@ dnode_move_impl(dnode_t *odn, dnode_t *ndn)
odn->dn_allocated_txg = 0;
odn->dn_free_txg = 0;
odn->dn_assigned_txg = 0;
odn->dn_dirty_txg = 0;
odn->dn_dirtyctx = 0;
odn->dn_dirtyctx_firstset = NULL;
odn->dn_dirtycnt = 0;
odn->dn_have_spill = B_FALSE;
odn->dn_zio = NULL;
odn->dn_oldused = 0;
Expand Down Expand Up @@ -1273,8 +1261,8 @@ dnode_check_slots_free(dnode_children_t *children, int idx, int slots)
} else if (DN_SLOT_IS_PTR(dn)) {
mutex_enter(&dn->dn_mtx);
boolean_t can_free = (dn->dn_type == DMU_OT_NONE &&
zfs_refcount_is_zero(&dn->dn_holds) &&
!DNODE_IS_DIRTY(dn));
dn->dn_dirtycnt == 0 &&
zfs_refcount_is_zero(&dn->dn_holds));
mutex_exit(&dn->dn_mtx);

if (!can_free)
Expand Down Expand Up @@ -1757,17 +1745,23 @@ dnode_hold(objset_t *os, uint64_t object, const void *tag, dnode_t **dnp)
* reference on the dnode. Returns FALSE if unable to add a
* new reference.
*/
static boolean_t
dnode_add_ref_locked(dnode_t *dn, const void *tag)
{
ASSERT(MUTEX_HELD(&dn->dn_mtx));
if (zfs_refcount_is_zero(&dn->dn_holds))
return (FALSE);
VERIFY(1 < zfs_refcount_add(&dn->dn_holds, tag));
return (TRUE);
}

boolean_t
dnode_add_ref(dnode_t *dn, const void *tag)
{
mutex_enter(&dn->dn_mtx);
if (zfs_refcount_is_zero(&dn->dn_holds)) {
mutex_exit(&dn->dn_mtx);
return (FALSE);
}
VERIFY(1 < zfs_refcount_add(&dn->dn_holds, tag));
boolean_t r = dnode_add_ref_locked(dn, tag);
mutex_exit(&dn->dn_mtx);
return (TRUE);
return (r);
}

void
Expand Down Expand Up @@ -1830,31 +1824,20 @@ dnode_try_claim(objset_t *os, uint64_t object, int slots)
}

/*
* Checks if the dnode itself is dirty, or is carrying any uncommitted records.
* It is important to check both conditions, as some operations (eg appending
* to a file) can dirty both as a single logical unit, but they are not synced
* out atomically, so checking one and not the other can result in an object
* appearing to be clean mid-way through a commit.
* Test if the dnode is dirty, or carrying uncommitted records.
*
* Do not change this lightly! If you get it wrong, dmu_offset_next() can
* detect a hole where there is really data, leading to silent corruption.
* dn_dirtycnt is the number of txgs this dnode is dirty on. It's incremented
* in dnode_setdirty() the first time the dnode is dirtied on a txg, and
* decremented in either dnode_rele_task() or userquota_updates_task() when the
* txg is synced out.
*/
boolean_t
dnode_is_dirty(dnode_t *dn)
{
mutex_enter(&dn->dn_mtx);

for (int i = 0; i < TXG_SIZE; i++) {
if (multilist_link_active(&dn->dn_dirty_link[i]) ||
!list_is_empty(&dn->dn_dirty_records[i])) {
mutex_exit(&dn->dn_mtx);
return (B_TRUE);
}
}

boolean_t dirty = (dn->dn_dirtycnt != 0);
mutex_exit(&dn->dn_mtx);

return (B_FALSE);
return (dirty);
}

void
Expand Down Expand Up @@ -1916,7 +1899,11 @@ dnode_setdirty(dnode_t *dn, dmu_tx_t *tx)
* dnode will hang around after we finish processing its
* children.
*/
VERIFY(dnode_add_ref(dn, (void *)(uintptr_t)tx->tx_txg));
mutex_enter(&dn->dn_mtx);
VERIFY(dnode_add_ref_locked(dn, (void *)(uintptr_t)tx->tx_txg));
dn->dn_dirtycnt++;
ASSERT3U(dn->dn_dirtycnt, <=, 3);
mutex_exit(&dn->dn_mtx);

(void) dbuf_dirty(dn->dn_dbuf, tx);

Expand Down Expand Up @@ -2221,32 +2208,6 @@ dnode_dirty_l1range(dnode_t *dn, uint64_t start_blkid, uint64_t end_blkid,
mutex_exit(&dn->dn_dbufs_mtx);
}

void
dnode_set_dirtyctx(dnode_t *dn, dmu_tx_t *tx, const void *tag)
{
/*
* Don't set dirtyctx to SYNC if we're just modifying this as we
* initialize the objset.
*/
if (dn->dn_dirtyctx == DN_UNDIRTIED) {
dsl_dataset_t *ds = dn->dn_objset->os_dsl_dataset;

if (ds != NULL) {
rrw_enter(&ds->ds_bp_rwlock, RW_READER, tag);
}
if (!BP_IS_HOLE(dn->dn_objset->os_rootbp)) {
if (dmu_tx_is_syncing(tx))
dn->dn_dirtyctx = DN_DIRTY_SYNC;
else
dn->dn_dirtyctx = DN_DIRTY_OPEN;
dn->dn_dirtyctx_firstset = tag;
}
if (ds != NULL) {
rrw_exit(&ds->ds_bp_rwlock, tag);
}
}
}

static void
dnode_partial_zero(dnode_t *dn, uint64_t off, uint64_t blkoff, uint64_t len,
dmu_tx_t *tx)
Expand Down
Loading