fix a subtle bug in dnode_next_offset() with txg > 0 #11200
Conversation
|
I reproduced the test failure (zdb crash). |
|
@ahrens , @behlendorf , I think that the latest test failures for this PR actually uncover a pre-existing bug in The failures are an assertion that I analyzed the problematic scenario and I see that it happens, for instance, in this call chain: |
|
Further looking through the code, I see two things. First, yes, the fill count is the number of dnodes. That can be seen in Second, it looks like the call chain I mentioned is the only call chain where both minimum level is zero and the search is for a hole. I see that I think that it could have been reasonable to change the meaning of dnode fill from a number of dnodes to a number of slots. But I haven't even tried checking where else that fill is used. I am sure that there is something that depends on the current meaning. So, for time being, I am just going to relax the assertion and to implement backward compatible behavior. |
|
The latest panic-s on all Linux flavors (except for Ubuntu 20.04 x86_64 which looks unrelated) look like a potential problem in dnode_sync_free_range_impl(). An example of the stack trace: |
|
The FreeBSD head ztest crash also looks like a race. Stack: |
|
@behlendorf , @ahrens , from your experience, how would you interpret the test results? The checkstyle failure is because I didn't add a sign-off to the latest commit in this PR. Two FreeBSD failures are both for the same test, cli_root/zfs_receive/receive-o-x_props_override. The CentOS Stream 8 failure is suspicious, though. |
|
@avg-I these are all known unrelated failures. The FreeBSD There's a good chance if you add your signed-off-by and rebase this on the latest master you'll be able to get a clean CI run. |
Only stop the search when the upward search cannot find a matching block pointer in the requested direction from the current offset. That definitely means that the search is exhausted. But if the downward search cannot find a suitable block at the requested level, then cycle back to the upward search for farther offsets. This also accounts for a special case of a search for an unallocated dnode that's been introduced with the large dnode support. That search is a level-0 search in a meta-dnode. It can fail without trying a subsequent L0 block if the current L0 block is full and at least one dnode occupies more than one slot. So, from the fill count perspective the block appears to have some free space, but it actually does not have any. Also, do not ascend needlessly beyond the maximum level that covers all possible offsets. Going above that level is bound to be fruitless. This change reworks a change to dnode_next_offset_level made in commit 031d7c2. After this change the span can never be too large for a shift. The described situation can happen with a (meta-)dnode like this: Object lvl iblk dblk dsize dnsize lsize %full type 0 6 128K 16K 575K 512 608K 3.78 DMU dnode That is, dn_datablkshift = 14, dn_indblkshift = 17, dn_nlevels = 6 and 64-bit offsets. Level 5 already covers all possible disk offsets. In addition, explicit handle too far offsets when working at the top level. For lower level we try hold a dbuf at the correct level and offset. If the offset is too far then the hold will fail and we will handle that accordingly. But at the top level we never checked that the requested offset is within the range that can be covered by the dnode's block pointers. Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
3a6eca8 to
7cb437a
Compare
|
@behlendorf , thank you for the advice! |
There was a problem hiding this comment.
@avg-I your analysis of this bug looks right to me as does the proposed fix. But given how subtle this whole search function is it'd be nice if we could really verify it's right but adding some kind of correctness test for dnode_next_offset(). It seems like it's be fairly straight forward to test the txg == 0 case with something like a SEEK_HOLE test case (which is something). But you're right, exercising the txg != 0 case is a lot trickier.
| if ((flags & DNODE_FIND_BACKWARDS) != 0) { | ||
| int epbs, span; | ||
|
|
||
| epbs = dn->dn_phys->dn_indblkshift - SPA_BLKPTRSHIFT; |
There was a problem hiding this comment.
nit: epbs is already declared and set correctly at the top of the function.
| int epbs, span; | ||
|
|
||
| epbs = dn->dn_phys->dn_indblkshift - SPA_BLKPTRSHIFT; | ||
| span = (lvl - 1) * epbs + dn->dn_datablkshift; |
There was a problem hiding this comment.
| span = (lvl - 1) * epbs + dn->dn_datablkshift; | |
| int span = (lvl - 1) * epbs + dn->dn_datablkshift; |
Only stop the search when the upward search cannot find a matching
block pointer in the requested direction from the current offset.
That definitely means that the search is exhausted.
But if teh downward search cannot find a suitable block at the
requested level, then cycle back to the upward search for farther
offsets.
This should fix #11196
Signed-off-by: Andriy Gapon agapon@panzura.com