zdb: better handling for corrupt block pointers #17166

asomers · 2025-03-21T20:57:43Z

When dumping indirect blocks, attempt to print corrupt block pointers rather than abort the program.

Sponsored by: ConnectWise
Signed-off-by: Alan Somers asomers@gmail.com

Motivation and Context

When trying to print a file's indirect blocks with "zfs -vvbbbb", if that file contains corrupt block pointers zdb will fail assertions and crash.

Description

Remove three assertions, replacing them with warnings printed to stderr. The rest of zdb does not require those block pointers to be intact.

How Has This Been Tested?

Tested with a dataset that had become corrupted, similarly to #17077 .

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)
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.

When dumping indirect blocks, attempt to print corrupt block pointers rather than abort the program. Sponsored by: ConnectWise Signed-off-by: Alan Somers <asomers@gmail.com>

amotin

I agree that assertions are not good for the data we read from disks and that might get corrupted, but I am not sure just complaining to stderr is a good solution either. Error messages printed to stderr might be difficult to correlate to the output printed to stdout. Also since we use zdb as a pool verification tool in some CI tests, I am not sure spamming stderr without returning actual error status would be noticed.

asomers · 2025-03-24T16:55:46Z

I agree that assertions are not good for the data we read from disks and that might get corrupted, but I am not sure just complaining to stderr is a good solution either. Error messages printed to stderr might be difficult to correlate to the output printed to stdout. Also since we use zdb as a pool verification tool in some CI tests, I am not sure spamming stderr without returning actual error status would be noticed.

So what would you prefer? I could print the messages to stdout instead, and eventually exit with status 1.

amotin · 2025-03-24T17:00:52Z

Exit code would definitely be good. How to better print it I am not sure though. I haven't looked close on that part of the code. But I already saw zdb using stderr for non-error messages, making it useless as error indicator. And as I have noted their correlation with stdout was a pain. So I'd say if we shoudl print the pointer, lets integrate printing of invalid values there also.

Signed-off-by: Alan Somers <asomers@gmail.com>

asomers · 2025-04-07T23:21:44Z

The latest commit prints all of the errors to stdout instead of stderr. It wasn't possible to print the L1 block's fill error on the same line as the block pointer itself, at least not without buffering potentially massive amounts of text in RAM, so it's printed afterwards. The output looks like this:

       a90000000   L1  DVA[0]=<5:4088dc7a0000:15000> DVA[1]=<2:3a8fade2f000:15000> [L1 ZFS plain file] fletcher4 lz4 unencrypted LE contiguous unique double size=20000L/c000P birth=7970383L/7970383P fill=16305515943967419494 cksum=000010c4104a06a3:01b38f9c940ef690:543c183d6a120c26:3f99c20b19c3eb3e
...
       a93940000    L0 DVA[0]=<14483840:7928e65e665e1a00:e200000> DVA[1]=<0:148154a00000000:1c39e00> DVA[2]=<8394880:0:1000000> [L0 unallocated] inherit inherit unencrypted BE gang unique triple size=200L/200P birth=1158310211095814315L/4409647458288664576P fill=6480302737822677281 cksum=0a4c2450925129cd:0124e955ddeccd13:0d3c3894ff64c1dd:c5d48c69c7b34245(ERROR: Block pointer type (0) does not match dnode type (19))
...
       a93a60000    L0 HOLE [L0 unallocated] size=200L birth=12694941463093612928L(ERROR: Block pointer type (0) does not match dnode type (19))
...
       a90000000: ERROR: Block pointer fill (16305515943967419494) does not match calculated value (4160680085501909892)
       a94000000   L1  DVA[0]=<0:3de2940b4000:15000> DVA[1]=<4:4174597d5000:15000> [L1 ZFS plain file] fletcher4 lz4 unencrypted LE contiguous unique double size=20000L/c000P birth=7974332L/7974332P fill=897 cksum=0000108206fb0d15:019223eac4ced4e5:0d3002f5ea989522:7045956156f0242b

amotin

It is quite verbose. I wonder what happen with some code that may try to parse the output (I think we have some in ZTS), and without error status returned still it might be difficult to handle properly.

asomers · 2025-04-09T16:36:40Z

It is quite verbose. I wonder what happen with some code that may try to parse the output (I think we have some in ZTS), and without error status returned still it might be difficult to handle properly.

I could change the exit status to 1 if any corruption is found. However, there is precedent for not doing that. zdb will already report corruption and yet exit 0 in the following conditions:

unmatched FREE(s)
Livelist ALLOC ... from TXG ... FREED at TXG ...
DOUBLE ALLOC
DOUBLE FREE
Found block that crosses metaslab boundary
Found livelist blocks marked as allocated for indirect vdevs
while trying to open subobj id
path not found, possibly leaked
potentially leaked objects detected

So for consistency's sake, I think I should leave the exit status unchanged.

allanjude · 2025-04-22T17:23:25Z

cmd/zdb/zdb.c

-			ASSERT3U(fill, ==, BP_GET_FILL(bp));
+		if (!err) {
+			if (fill != BP_GET_FILL(bp)) {
+				(void) printf("%16llx: Block pointer "


print to stderr instead?

That's what my first commit did. But I switched to stdout in response to @amotin's comments.

I think printing to stderr makes sense only if we exit immediately, so it is the last message before we return error status. But if we continue, then using different stream makes it difficult to understand what's actually wrong. Om the other side, if we print it to stdout, then it has to be formatted in some way nice to possible parsers or readers, unless we expect them to ignore all the output after getting an errors.

asomers · 2025-04-22T17:23:53Z

As discussed in the OpenZFS leadership meeting today, we agreed to change zdb to always exit non-zero if it detects any corruption. I'll create a distinct error code for that.

Signed-off-by: Alan Somers <asomers@gmail.com>

asomers · 2025-04-23T14:51:45Z

@amotin @allanjude with the latest commits, zdb will exit 3 if on-disk corruption was detected. It will also follow the usual convention of exiting 2 due to invalid CLI usage.

amotin

Just randomly looking around the handled cases I found couple more.

amotin · 2025-04-24T19:12:21Z

cmd/zdb/zdb.c

@@ -439,6 +442,7 @@ metaslab_spacemap_validation_cb(space_map_entry_t *sme, void *arg)
 			    (u_longlong_t)txg, (u_longlong_t)offset,
 			    (u_longlong_t)size, (u_longlong_t)mv->mv_vdid,
 			    (u_longlong_t)mv->mv_msid);
+			corruption_found = B_TRUE;


Just 13 lines up is also a case of double alloc.

amotin · 2025-04-24T19:13:39Z

cmd/zdb/zdb.c

@@ -542,6 +546,7 @@ mv_populate_livelist_allocs(metaslab_verify_t *mv, sublivelist_verify_t *sv)
 			    (u_longlong_t)DVA_GET_VDEV(&svb->svb_dva),
 			    (u_longlong_t)DVA_GET_OFFSET(&svb->svb_dva),
 			    (u_longlong_t)DVA_GET_ASIZE(&svb->svb_dva));
+			corruption_found = B_TRUE;


Similar error 17 lines up.

Signed-off-by: Alan Somers <asomers@gmail.com>

zdb: better handling for corrupt block pointers

c004cee

When dumping indirect blocks, attempt to print corrupt block pointers rather than abort the program. Sponsored by: ConnectWise Signed-off-by: Alan Somers <asomers@gmail.com>

asomers force-pushed the zdb-corrupt-bp branch from 6b058ec to c004cee Compare March 24, 2025 15:31

amotin reviewed Mar 24, 2025

View reviewed changes

fixup: print errors to stdout instead of stderr

22ceb77

Signed-off-by: Alan Somers <asomers@gmail.com>

amotin reviewed Apr 8, 2025

View reviewed changes

allanjude reviewed Apr 22, 2025

View reviewed changes

asomers added 2 commits April 23, 2025 08:46

Squash: always exit 3 if corruption was detected.

53c5e12

Signed-off-by: Alan Somers <asomers@gmail.com>

Squash: exit 2 for usage errors. Document exit status.

453306a

Signed-off-by: Alan Somers <asomers@gmail.com>

amotin reviewed Apr 24, 2025

View reviewed changes

Fixup: two more locations that indicate corruption.

0d2eb64

Signed-off-by: Alan Somers <asomers@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zdb: better handling for corrupt block pointers #17166

zdb: better handling for corrupt block pointers #17166

asomers commented Mar 21, 2025

amotin left a comment

asomers commented Mar 24, 2025

amotin commented Mar 24, 2025

asomers commented Apr 7, 2025

amotin left a comment

asomers commented Apr 9, 2025

allanjude Apr 22, 2025

asomers Apr 22, 2025

amotin Apr 22, 2025

asomers commented Apr 22, 2025

asomers commented Apr 23, 2025

amotin left a comment

amotin Apr 24, 2025

amotin Apr 24, 2025

zdb: better handling for corrupt block pointers #17166

Are you sure you want to change the base?

zdb: better handling for corrupt block pointers #17166

Conversation

asomers commented Mar 21, 2025

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

amotin left a comment

Choose a reason for hiding this comment

asomers commented Mar 24, 2025

amotin commented Mar 24, 2025

asomers commented Apr 7, 2025

amotin left a comment

Choose a reason for hiding this comment

asomers commented Apr 9, 2025

allanjude Apr 22, 2025

Choose a reason for hiding this comment

asomers Apr 22, 2025

Choose a reason for hiding this comment

amotin Apr 22, 2025

Choose a reason for hiding this comment

asomers commented Apr 22, 2025

asomers commented Apr 23, 2025

amotin left a comment

Choose a reason for hiding this comment

amotin Apr 24, 2025

Choose a reason for hiding this comment

amotin Apr 24, 2025

Choose a reason for hiding this comment