Open
Conversation
Contributor
Author
|
retest |
chriskirby1
force-pushed
the
clk/truncate_breakup
branch
from
497372d to
809dcde
Compare
Contributor
Author
|
The test failures are either hung task timeouts on debug kernels or the try_drain_data_freed commit budget issue that's fixed by another PR. The hung task complaints are gone from non-debug kernels. This change also keeps what could be background cleanup work from blocking access to an entire inode shard, so I'm keeping the PR open. |
Contributor
|
I would put an empty newline before any s-o-b lines, for clarity. Having them right after the last sentence of the commit message makes them look munged. |
Fix an issue where the final freeing of extents for unlinked files could trigger spurious hung task timeout warnings. Instead of holding the scoutfs inode lock for the entire duration of extent freeing, release and reacquire the lock every time the transaction sequence number changes. This is only used in the evict and orphan inode cleanup paths; truncate and release continue to free during a single lock hold. Remove the hung_task_timeout_secs workaround from the large-fragmented-free test script. Signed-off-by: Chris Kirby <ckirby@versity.com>
chriskirby1
force-pushed
the
clk/truncate_breakup
branch
from
809dcde to
417261a
Compare
Contributor
|
I'm concerned that we still have not solved the actual issue here. I've hit the task hung timeout with Even if we are going to drastically allow hung tasks to take much longer, it's still a sign that something is locked semi permanently while the goal of this patch is to lock/unlock every second or so... right? |
Contributor
Author
|
I'm assuming this is still a debug kernel. When I looked at how many extents we were removing during each pass, the number fluctuated pretty wildly on a debug kernel. It was often taking 10+ seconds just to see the trans_seq change. Sometimes it was > 20s. And the worker thread wasn't doing a huge number of extent removals during that time- things just seemed to get very slow for a bit. Sometimes we would only remove 16 extents during that interval. Other times, it was tens of thousands. On normal el95, it was a lot more consistent. I see the trans_seq moving forward pretty much once a second (tracing is only reporting at 1s granularity). To me that seems like we're just seeing the effects of the debug kernel, but if you think it's more than that, I'll investigate. |
Collaborator
There can be more than one issue that's causing hung tasks. This can be a fine solution to its part while leaving more work to do. Tracing would show what lock they were waiting on and what's happening with it. I'd guess that the server commits are insanely slow, and they're in the path of forward progress on locks. Clients won't send invalidation responses to release write locks until they commit their dirty transaction. The entire point of the test is to create dense client-side allocation btrees full of fragmented extent items that then cause a block of IO on the server per item in the client's trees. A few hundred items per block is a crazy iop amplification factor for a commit that can have thousands of blocks. I'd try an experiment that limits the amount of work that try_drain_data_freed() will do before giving up and letting its caller send the commit response. |
Contributor
|
There can be more than one issue that's causing hung tasks.
I think that's what I should have written in the first round.
This commit itself looks in order to me. I suspect you're correct on
server commits being the bottleneck here.
|
zabbo
previously requested changes
Jun 12, 2025
Collaborator
zabbo
left a comment
zabbo
left a comment
There was a problem hiding this comment.
Hopefully the orphan inode group binning will mean we don't have to return to this, but if we do, I wanted to get my thoughts down while they were fresh.
zabbo
requested changes
Jun 13, 2025
Collaborator
zabbo
left a comment
zabbo
left a comment
There was a problem hiding this comment.
OK, this time I think I actually managed to click all the right buttons to provide review comments! As we've discussed, though, hopefully the orphan scanner inode sharding will mean we don't need to bother with this. We'll see.
| int scoutfs_data_truncate_items(struct super_block *sb, struct inode *inode, | ||
| u64 ino, u64 iblock, u64 last, bool offline, | ||
| struct scoutfs_lock *lock) | ||
| u64 ino, u64 *iblock, u64 last, bool offline, |
Collaborator
There was a problem hiding this comment.
I think this function has gotten to the point where it's no longer helpful shared code with some differences, but acctually two different things interleaved with conditionals. Instead of also letting it make partial progress, let's split it into two functions: this that truncates live inodes and requires the inode pointer, and a new _data_free_items that is specifically built to remove a transaction's worth of extents and then return. It wouldn't need any of the arguments -- it'd always start at 0 and remove whatever it finds.
| scoutfs_ino(inode), | ||
| sblock, U64_MAX, | ||
| false, lock); | ||
| &sblock, U64_MAX, |
Collaborator
There was a problem hiding this comment.
These changes make me nervous. This used to obviously always truncate (s,e) and (s, ~0). Now it probably truncates from (s, e) and (e, ~0), but we'd have to go diving into the implementation to make sure there isn't some accidental final increment before returning that means it actually truncates (e + 1, ~0), or whatever. Let's avoid this pattern as much as possible. (It's why truncate_extent_items() returns the block number instead of updating an arg. This lesson was learned over and over in the kernel with the *ppos arguments to write functions that sometimes updated write pointers, or not, and was a nightmare.)
| scoutfs_unlock(sb, lock, SCOUTFS_LOCK_WRITE); | ||
| scoutfs_unlock(sb, orph_lock, SCOUTFS_LOCK_WRITE_ONLY); | ||
|
|
||
| if (more) |
Collaborator
There was a problem hiding this comment.
Adding this loop without clearing more at the start makes me nervous. It certainly looks like it'll loop forever if any of the errors before more is cleared are returned (perhaps from shutdown or forced unmount returning blanket errors from net/io paths). I'd rather not bother with this additional loop and would instead make sure to do enough work per iteration that we don't mind returning to the caller and letting them use their existing retry mechanisms.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fix an issue where the final freeing of extents for unlinked files could trigger spurious hung task timeout warnings. Instead of holding the scoutfs inode lock for the entire duration of extent freeing, release and reacquire the lock every time the transaction sequence number changes.
This is only used in the evict and orphan inode cleanup paths; truncate and release continue to free during a single lock hold.
Remove the hung_task_timeout_secs workaround from the large-fragmented-free test script.