dbuf: separate refcount calls for dbuf and dbuf_user #16191
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
If I boot the affected system with this patch, I get the panic below. Though, this is the result of patching FreeBSD's in-tree copy of OpenZFS, which might be different enough from |
In 92dc4ad I updated the dbuf_cache accounting to track the size of userdata associated with dbufs. This adds the size of the dbuf+userdata together in a single call to zfs_refcount_add_many(), but sometime removes them in separate calls to zfs_refcount_remove_many(), if dbuf and userdata are evicted separately. What I didn't realise is that when refcount tracking is on, zfs_refcount_add_many() and zfs_refcount_remove_many() are expected to be paired, with their second & third args (count & holder) the same on both sides. Splitting the remove part into two calls means the counts don't match up, tripping a panic. This commit fixes that, by always adding and removing the dbuf and userdata counts separately. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reported-by: Mark Johnston <[email protected]> Signed-off-by: Rob Norris <[email protected]>
robn
force-pushed
the
dbuf-userdata-refcount-separate
branch
from
f8f576e to
9341a9e
Compare
|
Ugh, that's my mistake - overexcited copypasta. Updated. Re in-tree, it'll be the same in main, but not in 14; specifically, after freebsd/freebsd-src@a2b560cc. It wouldn't apply cleanly without that, so I reckon you're good. |
The new version appears to fix the problem for me, thanks! As expected, the ARC leak is still there. |
behlendorf
approved these changes
May 13, 2024
amotin
approved these changes
May 15, 2024
lundman
pushed a commit
to openzfsonwindows/openzfs
that referenced
this pull request
Sep 4, 2024
In 92dc4ad I updated the dbuf_cache accounting to track the size of userdata associated with dbufs. This adds the size of the dbuf+userdata together in a single call to zfs_refcount_add_many(), but sometime removes them in separate calls to zfs_refcount_remove_many(), if dbuf and userdata are evicted separately. What I didn't realise is that when refcount tracking is on, zfs_refcount_add_many() and zfs_refcount_remove_many() are expected to be paired, with their second & third args (count & holder) the same on both sides. Splitting the remove part into two calls means the counts don't match up, tripping a panic. This commit fixes that, by always adding and removing the dbuf and userdata counts separately. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reported-by: Mark Johnston <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16191
0mp
pushed a commit
to 0mp/zfs
that referenced
this pull request
Nov 28, 2024
In 92dc4ad I updated the dbuf_cache accounting to track the size of userdata associated with dbufs. This adds the size of the dbuf+userdata together in a single call to zfs_refcount_add_many(), but sometime removes them in separate calls to zfs_refcount_remove_many(), if dbuf and userdata are evicted separately. What I didn't realise is that when refcount tracking is on, zfs_refcount_add_many() and zfs_refcount_remove_many() are expected to be paired, with their second & third args (count & holder) the same on both sides. Splitting the remove part into two calls means the counts don't match up, tripping a panic. This commit fixes that, by always adding and removing the dbuf and userdata counts separately. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reported-by: Mark Johnston <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16191 (cherry picked from commit e675852)
0mp
pushed a commit
to 0mp/zfs
that referenced
this pull request
Dec 3, 2024
In 92dc4ad I updated the dbuf_cache accounting to track the size of userdata associated with dbufs. This adds the size of the dbuf+userdata together in a single call to zfs_refcount_add_many(), but sometime removes them in separate calls to zfs_refcount_remove_many(), if dbuf and userdata are evicted separately. What I didn't realise is that when refcount tracking is on, zfs_refcount_add_many() and zfs_refcount_remove_many() are expected to be paired, with their second & third args (count & holder) the same on both sides. Splitting the remove part into two calls means the counts don't match up, tripping a panic. This commit fixes that, by always adding and removing the dbuf and userdata counts separately. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reported-by: Mark Johnston <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16191 (cherry picked from commit e675852)
brong
pushed a commit
to fastmailops/zfs
that referenced
this pull request
Dec 19, 2024
In 92dc4ad I updated the dbuf_cache accounting to track the size of userdata associated with dbufs. This adds the size of the dbuf+userdata together in a single call to zfs_refcount_add_many(), but sometime removes them in separate calls to zfs_refcount_remove_many(), if dbuf and userdata are evicted separately. What I didn't realise is that when refcount tracking is on, zfs_refcount_add_many() and zfs_refcount_remove_many() are expected to be paired, with their second & third args (count & holder) the same on both sides. Splitting the remove part into two calls means the counts don't match up, tripping a panic. This commit fixes that, by always adding and removing the dbuf and userdata counts separately. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reported-by: Mark Johnston <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16191 (cherry picked from commit e675852)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Investigation on #15802 found this. This is probably not the fix for that, but it is in the way of hunting it down.
Description
In 92dc4ad I updated the dbuf_cache accounting to track the size of userdata associated with dbufs. This adds the size of the dbuf+userdata together in a single call to zfs_refcount_add_many(), but sometime removes them in separate calls to zfs_refcount_remove_many(), if dbuf and userdata are evicted separately.
What I didn't realise is that when refcount tracking is on, zfs_refcount_add_many() and zfs_refcount_remove_many() are expected to be paired, with their second & third args (count & holder) the same on both sides. Splitting the remove part into two calls means the counts don't match up, tripping a panic.
This commit fixes that, by always adding and removing the dbuf and userdata counts separately.
How Has This Been Tested?
Just light sanity checking Test suite run with debug enabled is in progress.
Types of changes
Checklist:
Signed-off-by.