Skip to content

Commit

Permalink
Fix race in dnode_check_slots_free()
Browse files Browse the repository at this point in the history
Currently, dnode_check_slots_free() works by checking dn->dn_type
in the dnode to determine if the dnode is reclaimable. However,
there is a small window of time between dnode_free_sync() in the
first call to dsl_dataset_sync() and when the useraccounting code
is run when the type is set DMU_OT_NONE, but the dnode is not yet
evictable. This patch adds a check for whether dn_dirty_link is
active to determine if we are in this state, avoiding the crash.

This patch also corrects several instances when dn_dirty_link was
treated as a list_node_t when it is technically a multilist_node_t.

Fixes: openzfs#7147

Signed-off-by: Tom Caputi <[email protected]>
  • Loading branch information
Tom Caputi committed Apr 5, 2018
1 parent 5152a74 commit ad6d51f
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 12 deletions.
2 changes: 1 addition & 1 deletion module/zfs/dmu.c
Original file line number Diff line number Diff line change
Expand Up @@ -2284,7 +2284,7 @@ dmu_offset_next(objset_t *os, uint64_t object, boolean_t hole, uint64_t *off)
* Check if dnode is dirty
*/
for (i = 0; i < TXG_SIZE; i++) {
if (list_link_active(&dn->dn_dirty_link[i])) {
if (multilist_link_active(&dn->dn_dirty_link[i])) {
clean = B_FALSE;
break;
}
Expand Down
56 changes: 45 additions & 11 deletions module/zfs/dnode.c
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ dnode_cons(void *arg, void *unused, int kmflag)
bzero(&dn->dn_next_maxblkid[0], sizeof (dn->dn_next_maxblkid));

for (i = 0; i < TXG_SIZE; i++) {
list_link_init(&dn->dn_dirty_link[i]);
multilist_link_init(&dn->dn_dirty_link[i]);
dn->dn_free_ranges[i] = NULL;
list_create(&dn->dn_dirty_records[i],
sizeof (dbuf_dirty_record_t),
Expand Down Expand Up @@ -188,7 +188,7 @@ dnode_dest(void *arg, void *unused)
ASSERT(!list_link_active(&dn->dn_link));

for (i = 0; i < TXG_SIZE; i++) {
ASSERT(!list_link_active(&dn->dn_dirty_link[i]));
ASSERT(!multilist_link_active(&dn->dn_dirty_link[i]));
ASSERT3P(dn->dn_free_ranges[i], ==, NULL);
list_destroy(&dn->dn_dirty_records[i]);
ASSERT0(dn->dn_next_nblkptr[i]);
Expand Down Expand Up @@ -614,7 +614,7 @@ dnode_allocate(dnode_t *dn, dmu_object_type_t ot, int blocksize, int ibs,
ASSERT0(dn->dn_rm_spillblk[i]);
ASSERT0(dn->dn_next_blksz[i]);
ASSERT0(dn->dn_next_maxblkid[i]);
ASSERT(!list_link_active(&dn->dn_dirty_link[i]));
ASSERT(!multilist_link_active(&dn->dn_dirty_link[i]));
ASSERT3P(list_head(&dn->dn_dirty_records[i]), ==, NULL);
ASSERT3P(dn->dn_free_ranges[i], ==, NULL);
}
Expand Down Expand Up @@ -1081,26 +1081,60 @@ dnode_set_slots(dnode_children_t *children, int idx, int slots, void *ptr)
}
}

static boolean_t
dnode_check_evictable(dnode_t *dn)
{
dmu_object_type_t type;

mutex_enter(&dn->dn_mtx);
type = dn->dn_type;
mutex_exit(&dn->dn_mtx);

if (type != DMU_OT_NONE)
return (B_FALSE);

/*
* There is a short time after dnode_free_sync() but before
* user accounting happens where the dnode cannot be evicted.
* We check for this by determining if the dnode has any
* active links into os_synced_dnodes.
*/
for (int i = 0; i < TXG_SIZE; i++) {
boolean_t link_active;
multilist_sublist_t *mls;
multilist_t *synced_list = &dn->dn_objset->os_synced_dnodes[i];

mls = multilist_sublist_lock_obj(synced_list, dn);
link_active = multilist_link_active(&dn->dn_dirty_link[i]);
multilist_sublist_unlock(mls);

if (link_active)
return (B_FALSE);
}

return (B_TRUE);
}

static boolean_t
dnode_check_slots_free(dnode_children_t *children, int idx, int slots)
{
ASSERT3S(idx + slots, <=, DNODES_PER_BLOCK);

/*
* If all dnode slots are either already free or
* evictable return B_TRUE.
*/
for (int i = idx; i < idx + slots; i++) {
dnode_handle_t *dnh = &children->dnc_children[i];
dnode_t *dn = dnh->dnh_dnode;

if (dn == DN_SLOT_FREE) {
continue;
} else if (DN_SLOT_IS_PTR(dn)) {
mutex_enter(&dn->dn_mtx);
dmu_object_type_t type = dn->dn_type;
mutex_exit(&dn->dn_mtx);

if (type != DMU_OT_NONE)
if (!dnode_check_evictable(dn))
return (B_FALSE);

continue;
else
continue;
} else {
return (B_FALSE);
}
Expand Down Expand Up @@ -1633,7 +1667,7 @@ dnode_setdirty(dnode_t *dn, dmu_tx_t *tx)
/*
* If we are already marked dirty, we're done.
*/
if (list_link_active(&dn->dn_dirty_link[txg & TXG_MASK])) {
if (multilist_link_active(&dn->dn_dirty_link[txg & TXG_MASK])) {
multilist_sublist_unlock(mls);
return;
}
Expand Down

0 comments on commit ad6d51f

Please sign in to comment.