Be more careful with locking db.db_mtx

[asomers]

Lock db->db_mtx in some places that access db->db_data. But don't lock it in free_children, even though it does access db->db_data, because that leads to a recurse-on-non-recursive panic.

Lock db->db_rwlock in some places that access db->db.db_data's contents.

Closes #16626
Sponsored by: ConnectWise

Motivation and Context

Fixes occasional in-memory corruption which is usually manifested as a panic with a message like "blkptr XXX has invalid XXX" or "blkptr XXX has no valid DVAs". I suspect that some on-disk corruption bugs have been caused by this same root cause, too.

Description

Always lock dmu_buf_impl_t.db_mtx in places that access the value of dmu_buf_impl_t.db->db_data. And always lockdmu_buf_impl_t.db_rwlock in places that access the contents of dmu_buf_impl_t.db->db_rwlock.

Note that free_children still violates these rules. It can't easily be fixed without causing other problems. A proper fix is left for the future.

How Has This Been Tested?

I cannot reproduce the bug on command, so I had to rely on statistics to validate the patch.

• Since the beginning of 2025, servers running the vulnerable workload on FreeBSD 14.1 without this patch have crashed with a probability of 0.34% per server per day. The distribution of crashes fits a Poisson distribution, suggesting that each crash is random and independent. That is, a server that's already crashed once is no more likely to crash in the future than one which hasn't crashed yet.
• Servers running the vulnerable workload on FreeBSD 14.2 with this patch have accumulated a total of 1301 days of uptime with no crashes. So I conclude with 98.8% confidence that the 14.2 upgrade combined with the patch is effective.
• Servers running the vulnerable workload on FreeBSD 14.2 without the patch are too few to draw conclusions about. But I don't see any related changes in the diff between 14.1 and 14.2. So I think that the patch is responsible for the cessation of crashes, not the upgrade.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[alek-p]

I've already reviewed this internally, and as the PR description states, we've had a good experience running with this patch for the last couple of months

[amotin]

As I see, in most of cases (I've spotted only one different) when you are taking db_rwlock, you also take db_mtx. It makes no sense to me, unless the only few exceptions are enormously expensive or otherwise don't allow db_mtx to be taken. I feel like we need some better understanding of locking strategy. At least I do.

[snajpa]

FWIW, as we're discussing here, I even think - after all the staring at the code - that the locking itself is actually fine, it seems to be a result of optimizations exactly because things don't need to be overlocked if it's guaranteed to be OK via other logical dependencies.

I think I have actually nailed where the problem is, but @asomers says he can't try it :)

[asomers]

As I see, in most of cases (I've spotted only one different) when you are taking db_rwlock, you also take db_mtx. It makes no sense to me, unless the only few exceptions are enormously expensive or otherwise don't allow db_mtx to be taken. I feel like we need some better understanding of locking strategy. At least I do.

That's because of this comment from @pcd1193182: "So the subtlety here is that the value of the db.db_data and db_buf fields are, I believe, still protected by the db_mtx plus the db_holds refcount. The contents of the buffers are protected by the db_rwlock." So many places need both db_mtx and db_rwlock. Some need only the former. I don't know of any cases where code would only need the latter.

[snajpa]

I'm sorry, I mixed it up. This is definitely needed and then there's a bug with dbuf resize. Two different things.

[satmandu]

@asomers Are you still awaiting reviewers on this? I've been running with the changes from this PR without any issues for a while now. It would be nice to get in all the "prevents corruption" PRs before 2.4.0.

[clhedrick]

Does this apply to 2.2.8 also?

[amotin]

OK. I went through all this, and I believe most of locking is not neded -- se below. Only few I've left uncommented.

[asomers]

Though I see your comments, @amotin , I still struggle to understand the right thing to do, generally, because the locking requirements aren't well documented, nor are they enforced either by the compiler or at runtime. Here are the different descriptions I've seen:

From dbuf.h:

db.db_data, which is protected by db_mtx
...
[db_rwlock] Protects db_buf's contents if they contain an indirect block or data block of the meta-dnode

And here's what @pcd1193182 said in #17118 👍

The value of the `db.db_data` and `db_buf` fields
are protected by `db_mtx` plus the `db_holds` refcount.  The contents are
protected by `db_rwlock`.  `db_mtx` is also responsible for protecting some of
the other parts of the dbuf state.

And later

dbufs have different states,and when they are in these different states, they can only be accessed in
certain ways.

But I don't see any list of what the various states are, nor how to tell which state a dbuf is in.

@amotin added the following in that same discussion thread:

db_rwlock protect content of buffers that are parent (indirect or dnode) of
some other buffer, and we need to either write or read the block pointer of the
buffer, either directly or via de-referencing the pointer of db_blkptr pointing
inside it. All the parent buffers permanently referenced so can not be evicted,
and have only one copy, so their memory should never be reallocated, so db_mtx
protection is not required in this case.

And @amotin added some more detail in this PR:

• "If the db_dirtycnt below is zero (and it should be protected by db_mtx), then the buffer must be empty."
• "Indirects don't relocate."
• "meta-dnode dbufs are not relocatable"
• "db_rwlock didn't promise to protect [L0 blocks]"

I can't confidently make any changes here without a complete and accurate description of the locking comments. What I need are:

• Complete and accurate documentation in dbuf.h
• A way to enforce those requirements at runtime. Perhaps a macro that asserts that a db_buf is locked, or else doesn't need to be locked based on other data in the dmu_buf_impl, and can be called everywhere that db_buf is accessed. And a similar macro for db.db_data.

@amotin can you please help with that? At least with the first part?

[amotin]

@asomers Let me rephrase the key points:

• Indirects and L0 dnode dbufs are special in having only one data copy ever. They are always decompressed in memory, and if need do be decrypted (only bonus parts of dnode L0 can be encrypted, indirects are only signed), then it is done in place. It means they are never relocated in memory, so we don't need db_mtx to protect their db.db_data. And as long as we hold a reference on those dbufs, they can not be evicted and so change their state. This removes most of db_mtx acquisitions you've added.
• db_rwlock is designed to protect specifically indirects and L0 dnode blocks from torn writes when they are modified by sync context, but read by anything else. db_rwlock is not intended to protect any user data dbufs, modified only in open context. For those we have range locks, etc. This removes most of db_rwlock acquisitions you've added.

[IvanVolosyuk]

My humble opinion. I think it is reasonable request to:

• accurately document specifically what each lock is responsible for and in what states locking is required; enumerate possible states which require different approaches.
• add additional debug assertions to make it clear which code path have the lock already held.
• in places where locking is not needed due to single use - poison somehow the locks in debug mode to make any unexpected use crash
• in places where object is not reallocatable - add macro which makes it clear that locking is not needed and checks that the object is indeed not rellocatable.

It is good to have optimizations, but it is not healthy that there is very limit knowledge of the locking scheme in small group of people with poor documentation and inability to examine the code for correctness.

[amotin]

@asomers Despite my comments on many of the changes here, IIRC there were some that could be useful. Do you plan to clean this up, document, etc, or I'll have to take it over?

[asomers]

@asomers Despite my comments on many of the changes here, IIRC there were some that could be useful. Do you plan to clean this up, document, etc, or I'll have to take it over?

Yes. My approach is to create some assertion functions which check that either db_data is locked, or is in a state where it doesn't need to be. The WIP is here, but it isn't ready for review yet. Probably next week. https://github.com/asomers/zfs/tree/db_data_elide .

[asomers]

@amotin I've eliminated the lock acquisitions as you requested. Please review. Note that while I've run the ZFS test suite with this round of changes, I don't know whether they suffice to solve the original corruption bug. The only way to know that is to run the code in production. But I'd like your review before I try that, because it takes quite a bit of time and effort to get sufficient production time. Not to mention the risk of corrupting customer data again.

[behlendorf]

@asomers if you can rebase this on the latest commits in that master branch that should resolve most of the CI build failures. While you're at if please go ahead and squash the commits.

[asomers]

There are suddenly a lot of "Wrong value for OS variable!" failures. I think that virt-install on the CI server must've suddenly changed versions.

[amotin]

There are suddenly a lot of "Wrong value for OS variable!" failures.

I am not sure what it means, but when you last rebased?

[asomers]

There are suddenly a lot of "Wrong value for OS variable!" failures.

I am not sure what it means, but when you last rebased?

Not since September. I've avoided doing that, since it can make the review confusing. But I'll do it now.

[asomers]

@amotin I rebased the changes and fixed the two new panics that have appeared since the last rebase. It's easier now that #18131 is finished. The freebsd16-0c CI failure is not the result of this PR, and the checkstyle failure will resolve itself after I squash. Could you please review again?

[amotin]

Couple chunks below in dbuf_write_ready() you take db_rwlock on the dnode buffer. Though both cases are reads in sync context, and I would not expect them to race.

[amotin]

I am not sure why may we need locking here. level and blkid should be constants, I think.

[asomers]

db_state and db_dirtcnt certainly need to be protected by db_mtx. I could move the mutex_enter down until after the db_level check if you insist, though.

[amotin]

I haven't looked on a bigger picture here, but I'd say yeah, between later and never.

[amotin]

Couple small nits, but please review earlier comments still not marked resolved.

[amotin]

I wonder if dn_maxblkid update below we could move before the lock acquisition (or after the release?) to not think about the lock ordering? They seem unrelated.

[amotin]

We got this lock just 6 lines above and done nothing to it.