`list_link_active` panic in `dnode_is_dirty`

[robn]

I reproduced the crash first noticed here: #17625 (comment)

It took over 400 rounds of seekflood, so its tricky to hit.

[Mon Aug 18 20:22:33 2025] VERIFY3B(node->next == ((void *) 0x100 + (0xdead000000000000UL)), ==, node->prev == ((void *) 0x122 + (0xdead000000000000UL))) failed (0 == 1)
[Mon Aug 18 20:22:33 2025] PANIC at list.h:188:list_link_active()
[Mon Aug 18 20:22:33 2025] Showing stack for process 1895973
[Mon Aug 18 20:22:33 2025] CPU: 0 UID: 0 PID: 1895973 Comm: seekflood Tainted: P           OE       6.17.0-rc2 #1 PREEMPT(voluntary)
[Mon Aug 18 20:22:33 2025] Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[Mon Aug 18 20:22:33 2025] Hardware name: FreeBSD BHYVE/BHYVE, BIOS 14.0 10/17/2021
[Mon Aug 18 20:22:33 2025] Call Trace:
[Mon Aug 18 20:22:33 2025]  <TASK>
[Mon Aug 18 20:22:33 2025]  dump_stack_lvl+0x5d/0x80
[Mon Aug 18 20:22:33 2025]  spl_panic+0xf3/0x118 [spl]
[Mon Aug 18 20:22:33 2025]  ? dnode_hold_impl+0x8eb/0x1080 [zfs]
[Mon Aug 18 20:22:33 2025]  list_link_active+0x69/0x70 [zfs]
[Mon Aug 18 20:22:33 2025]  dnode_is_dirty+0x62/0x190 [zfs]
[Mon Aug 18 20:22:33 2025]  dmu_offset_next+0xc4/0x260 [zfs]
[Mon Aug 18 20:22:33 2025]  zfs_holey_common+0xa0/0x190 [zfs]
[Mon Aug 18 20:22:33 2025]  zfs_holey+0x51/0x80 [zfs]
[Mon Aug 18 20:22:33 2025]  zpl_llseek+0x89/0xd0 [zfs]
[Mon Aug 18 20:22:33 2025]  ksys_lseek+0x3f/0xb0
[Mon Aug 18 20:22:33 2025]  do_syscall_64+0x84/0x2f0
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? do_syscall_64+0xbc/0x2f0
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? do_syscall_64+0xbc/0x2f0
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? zpl_iter_write+0x134/0x160 [zfs]
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? vfs_write+0x25d/0x450
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? ksys_write+0x6b/0xe0
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? __task_pid_nr_ns+0xa0/0xb0
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? do_syscall_64+0xbc/0x2f0
[Mon Aug 18 20:22:33 2025]  ? __task_pid_nr_ns+0xa0/0xb0
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  ? do_syscall_64+0xbc/0x2f0
[Mon Aug 18 20:22:33 2025]  ? srso_alias_return_thunk+0x5/0xfbef5
[Mon Aug 18 20:22:33 2025]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[Mon Aug 18 20:22:33 2025] RIP: 0033:0x7fc65bb24637
[Mon Aug 18 20:22:33 2025] Code: 8b 05 c5 37 0e 00 64 c7 00 0d 00 00 00 eb b2 e8 7f 95 01 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 08 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 01 c3 48 8b 15 91 37 0e 00 f7 d8 6
4 89 02 48
[Mon Aug 18 20:22:33 2025] RSP: 002b:00007fffcbd45058 EFLAGS: 00000246 ORIG_RAX: 0000000000000008
[Mon Aug 18 20:22:33 2025] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007fc65bb24637
[Mon Aug 18 20:22:33 2025] RDX: 0000000000000003 RSI: 0000000000000000 RDI: 0000000000000003
[Mon Aug 18 20:22:33 2025] RBP: 00007fffcbd45100 R08: 0000000000000000 R09: 0000000000000000
[Mon Aug 18 20:22:33 2025] R10: 0000000000000180 R11: 0000000000000246 R12: 00000000000001e6
[Mon Aug 18 20:22:33 2025] R13: 0000000000000004 R14: 0000000000000003 R15: 0000000000000000
[Mon Aug 18 20:22:33 2025]  </TASK>

I don't have the brain left today to try and understand it fully and patch it. Here's what I've learned if someone wants to get to it before I do.

The crash in question:

static inline int
list_link_active(list_node_t *node)
{
	EQUIV(node->next == LIST_POISON1, node->prev == LIST_POISON2);
	return (node->next != LIST_POISON1);
}

So its just testing that both pointers are poisoned, or neither are.

Calling function:

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);
		}
	}

	mutex_exit(&dn->dn_mtx);

	return (B_FALSE);
}

multilist_link_active() is thin wrapping around list_link_active():

int
multilist_link_active(multilist_node_t *link)
{
	return (list_link_active(link));
}

In crash debugger:

sdb> find_task 1895973 | frame 6 dn | member dn_dirty_link
(multilist_node_t [4]){
        {
                .next = (struct list_head *)0xdead000000000100,
                .prev = (struct list_head *)0xdead000000000122,
        },
        {
                .next = (struct list_head *)0xdead000000000100,
                .prev = (struct list_head *)0xdead000000000122,
        },
        {
                .next = (struct list_head *)0xdead000000000100,
                .prev = (struct list_head *)0xdead000000000122,
        },
        {
                .next = (struct list_head *)0xdead000000000100,
                .prev = (struct list_head *)0xdead000000000122,
        },
}

So most likely, we caught this in a transition from "active" to "inactive", that is, being removed from the list. That is, list_del() in the kernel, which is list_remove*() for us:

static inline void list_del(struct list_head *entry)
{
	__list_del_entry(entry);
	entry->next = LIST_POISON1;
	entry->prev = LIST_POISON2;
}

This is wrapped by multilist_sublist_remove() and variants:

multilist_sublist_remove(multilist_sublist_t *mls, void *obj)
{
	ASSERT(MUTEX_HELD(&mls->mls_lock));
	list_remove(&mls->mls_list, obj);
}

dn_dirty_link is the list linkage node for os_dirty_dnodes and os_synced_dnodes:

int
dmu_objset_open_impl(spa_t *spa, dsl_dataset_t *ds, blkptr_t *bp,
    objset_t **osp)
{
...
	for (i = 0; i < TXG_SIZE; i++) {
		multilist_create(&os->os_dirty_dnodes[i], sizeof (dnode_t),
		    offsetof(dnode_t, dn_dirty_link[i]),
		    dnode_multilist_index_func);

void
dmu_objset_sync(objset_t *os, zio_t *pio, dmu_tx_t *tx)
{
...
		multilist_create(&os->os_synced_dnodes, sizeof (dnode_t),
		    offsetof(dnode_t, dn_dirty_link[txgoff]),
		    dnode_multilist_index_func);

There appears to be three calls to multilist_sublist_remove() for one of these lists, in:

• dmu_objset_sync_dnodes()
• userquota_updates_task()
• dnode_rele_task()

None hold db_mtx while removing the dnode form the list, which dnode_is_dirty() is using to protect access to the node, so there's nothing stopping it observing the removal directly, tripping the assert and crashing.

The "fixes" here seem to be to either take the sublist lock while checking dirtiness, or cache the dirty state on the dnode. @rrevans had some good thoughts about this in #15615 (comment); now is probably the time to dust that off and have a go at it.

[rrevans]

@robn This matches my recollection and old notes.

I think storing a count of dirty txgs in the node e.g. dn_dirtycnt under dn_mtx should be preferred here despite the per dnode cost.

1. It cleanly and directly measures the thing we are interested in instead of inferring it from other state.
2. The two checks in dnode_is_dirty are still racy. There are no guarantees that the CPU calling dnode_is_dirty will not observe the write clearing dn_dirty_records after observing the first write clearing dn_dirty_link on another CPU, even on strongly-ordered architectures. This actually happening probably depends on some future clever CPU or compiler implementation, but it's still technically a racy pair of reads.
3. Iteration over the txg slots invites non-obvious races with txg_advance. Slot zero can start being re-used after it gets checked if the open txg wraps around at the wrong time on another CPU. I think it's safe here because dn_mtx and/or the file range lock protects all of the paths that dirty dnodes and thus the txg slots. That said it's still completely non-obvious this is even a concern in the first place, and the pattern should be avoided. The txg slot data logically requires holding the open txg and then accessing only that slot or being in sync context for that specific txg. Put differently, the DMU does not have an API that allows peeking at all txg state reliably. (My notes say versions prior to ec4f9b8 did not even hold dn_mtx.)

TL;DR- Counting txgs in which the dnode is dirty seems like the easy way out here.

[robn]

Hey, I'm glad to see you here! I've just been going back over the old PRs and your notes and its all there; I'd forgotten that this is pretty much exactly what we'd seen before. I even found my promise of getting back to it, which I'm not sure I can claim I did, even without setting a time.

Anyway, I've got a patch on the test rig right now that adds dn_dirtycnt exactly as you describe, and for (hopeful) avoidance of future doubt, removes dn_dirty_txg, dn_dirtyctx and dn_dirtyctx_firstset (!!). If it gets through ~1000 rounds of seekflood, it'll get a full ZTS run, and then it gets posted. It's a really simple solution that I like a lot; thank you for it!

I don't think the per-dnode cost is really much of an issue, or at least, not a new issue - we have to take the lock in all those touch points anyway.

Finally, I've had a (brief) look over your series that also checks for dirty buffers. Initial glance looks good. I'm not taking it for this because right now I just wanna fix the bug before my vacation next week, but if you feel like pushing on it again that'd be cool. If not, would you mind me picking it up sometime in the next couple months? Anything I should know?

[rrevans]

Finally, I've had a (brief) look over your series that also checks for dirty buffers. I'm not taking it for this because right now I just wanna fix the bug before my vacation next week

Thanks, and +1 for not getting into that whole thing to fix this panic.

but if you feel like pushing on it again that'd be cool. If not, would you mind me picking it up sometime in the next couple months?

Let me know what works for you? I'll pick this back up if you have time to look.

I'll leave it to you to judge the priority as I don't know how much the forced txg syncs matter to most users. It's largely a performance limit that I personally want to see removed.

That said, there are also real stability bugs in free_children and #11196 that seem worth fixing.

Anything I should know?

As I recall, the offset handling at boundary conditions is a fair chunk of the complication in dnode_next_offset, and I could be convinced to split that out of #16025 as a separate PR. The (then) existing code is not very tidy about the returned values for some cases. It needs a lot of staring to get the details in the loop right and make sure callers are compatible.

In the next patch, pending indirection growth is the most tricky bit, as documented here https://github.com/rrevans/zfs/blob/a765b1db54ca50ee6abfb29f03d37dffa7b6385c/module/zfs/dnode.c#L2602-L2632 since (if I recall) new level's physical blkptrs don't get allocated until dnode_increase_indirection in sync context. There'a lot of detail to look at there to get it right.