arc: Drop an incorrect assert

[rincebrain]

Motivation and Context

#9897, #12020

Description

Dropped an assert that was, in fact, merely often true when correct operation was happening.

How Has This Been Tested?

I had two reliable reproducers on my Debian buster systems - both of them, without this change, would trip this assert in minutes; with this change, they never did in over an hour of load each, and no obvious bad behavior occurred either. (The two reproducers, just in case this regresses, were "mild/moderate IO from inside a chroot on a zpool over NFS from a sparc64 client (rebuilding mandb, in particular, was quite good at it)" and "disk IO on a file on a zpool using kvm and virtio-blk/virtio-scsi storage". (Some of these constraints may be unnecessarily specific, but that is what I used.))

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.

[ahrens]

The assertion is based on the understanding that anonymous HDR's do not have DVA's, which I think is one of the defining characteristic of anonymous HDR's. During correct operation, how do we come to have an anonymous HDR with a DVA?

[behlendorf]

I believe we can come to have an anonymous buffer here with a non-empty header in the event that it was overridden after being sync'd out. If you look a few lines down in arc_release() you'll see there's a comment which explicitly calls out this case. I was hoping either you or George could take a look and confirm this is the case, this #9897 (comment) pointed out the original illumos/illumos-gate@dcbf3bd commit ( 6950 ARC should cache compressed data ) which @grwilson authored and you committed to illumos.

[ahrens]

@rincebrain I'm still not understanding how we get into this situation. @grwilson and I have been looking at the code trying to see what could be happening here. We noticed some interesting code paths that are taken when using dedup. When you reproduce the problem, are you using dedup (or have you ever used it on this storage pool)?

(Specifically: for dedup'ed-away writes, if the matching block is already in the ARC, then arc_write_done() seems to set b_dva but not call arc_access() (because exists != NULL), and therefore not change b_state out of anon. However, this would seem to happen very commonly (if using dedup), so it may be that something else normally mitigates this unexpected state.)

[rincebrain]

@rincebrain I'm still not understanding how we get into this situation. @grwilson and I have been looking at the code trying to see what could be happening here. We noticed some interesting code paths that are taken when using dedup. When you reproduce the problem, are you using dedup (or have you ever used it on this storage pool)?

(Specifically: for dedup'ed-away writes, if the matching block is already in the ARC, then arc_write_done() seems to set b_dva but not call arc_access() (because exists != NULL), and therefore not change b_state out of anon. However, this would seem to happen very commonly (if using dedup), so it may be that something else normally mitigates this unexpected state.)

No DDT entries are present on any pool on my original system, according to zpool status -D - I would be astonished if I had ever enabled dedup on it, but I have a lot of auto snapshotting, so zpool history can't tell me anything useful.

On the testbed I was playing with this on, no, never dedup enabled, ever.

[mmaybee]

@grwilson and @ahrens, can we come to a consensus on this proposed change? As @behlendorf has pointed out, there is an explicit clear of the header just below this ASSERT with a comment indicating that the DVA could be non-null in some circumstances (BTW, both code and ASSERT came from George in the same change set). It seems like either the code or the ASSERT have to be in error here.

[blastwave]

Merely a follow up that I have seen this also on FreeBSD 14.0 CURRENT ( amd64/x86_64 ) and the fix is trivial. Just comment out line 6538 from arc.c and life goes on just fine. Please bear in mind that we only ever see this panic when running a full debug kernel with witness options etc etc etc. Even more rare and strange is that I can trigger the panic repeatedly when I run QEMU on the FreeBSD target system.

[grwilson]

I was able to go back through our internal repos to find the origin of the assertion and after looking at various commits, it is clear to me that the assertion was added in error. The code change proposed here is correct.