Refactor dbuf_read() for safer decryption

[amotin]

In dbuf_read_verify_dnode_crypt():

• We don't need original dbuf locked there. Instead take a lock on a dnode dbuf, that is actually manipulated.
• Block decryption for a dnode dbuf if it is currently being written. ARC hash lock does not protect anonymous buffers, so arc_untransform() is unsafe when used on buffers being written, that may happen in case of encrypted dnode buffers, since they are not copied by dbuf_dirty()/dbuf_hold_copy().

In dbuf_read():

• If the buffer is in flight, recheck its compression/encryption status after it is cached, since it may need arc_untransform().

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.

[amotin]

@rincebrain Once this hopefully pass CI, I'd appreciate a test on your cursed system.

[rincebrain]

You may be amused to know I already had noticed this new PR and had it building.

I'll let you know. :)

[rincebrain]

Well, CI is gonna blow up with:

module/zfs/dbuf.c: In function ‘dbuf_read_bonus’:
module/zfs/dbuf.c:1420:59: error: unused parameter ‘flags’ [-Werror=unused-parameter]
 1420 | dbuf_read_bonus(dmu_buf_impl_t *db, dnode_t *dn, uint32_t flags)
      |                                                  ~~~~

[amotin]

Well, CI is gonna blow up with:

@rincebrain Already fixed.

[rincebrain]

It hasn't crashed horribly in 60 runs so far, when usually it does within 2-3, but I will temper that with the fact that I also used to find adding too many debug prints (or too expensive code changes) would jitter the race enough that it rarely happened even if the changes could not have fixed it.

So I'll keep it running in a loop for a day or two and see if it goes bang, but tentatively, it does seem to be an improvement.

[amotin]

I've leaked locks in DB_RF_NEVERWAIT case. Another try.

[rincebrain]

I have, incidentally, been running this in a loop since, and not burped once.

I'm not sure I'm confident enough in the ARC code involved to mark it as Reviewed-by and have it mean much, but it also looks reasonable to me.

[behlendorf]

Thanks for digging in to this. None of this code is easy to reason about but the refactoring here and additional checks do make sense. Coupled with @rincebrain's testing and a clean CI run I think this is good to go.