Make TX abort after assign safer

[amotin]

It is not right, but there are few examples when TX is aborted after being assigned in case of error. To handle it better on production systems add extra cleanup steps.

While here, replace couple dmu_tx_abort() calls in simple cases.

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)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• 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]

Thinking about it again, I've decided that it is wrong to call dmu_tx_abort() on already assigned transactions, since if any modifications are already done as part of the TXG, abort won't undo them. I've fixed couple cases where it was done, but there are at least few more I've spotted in zfs_setattr*() that require closer look, and where this PR should help.

[robn]

Thinking about it again, I've decided that it is wrong to call dmu_tx_abort() on already assigned transactions, since if any modifications are already done as part of the TXG, abort won't undo them.

Why does the tx_txg == 0 assert at the top of dmu_tx_abort() not trip in that case?

[robn]

Oh heh, you've already gone there. Ok I'll read the diff first 😅

[robn]

The important parts appear to be trading the aborts for commits.

I was going to approve this anyway and say that I guess it doesn't hurt, but I don't love hiding bugs. But then I saw you'd already said "on production systems", which explains why the asserts wouldn't trip.

Still, it might be useful to put something in dbgmsg or similar to at least have a record of something, but I suppose it depends on the nature of your production debugging - no point having something there that no one will ever read.