Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZFS send should use spill block prefetched from send_reader_thread #16701

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tuxoko
Copy link
Contributor

@tuxoko tuxoko commented Oct 29, 2024

Motivation and Context

Currently, even though send_reader_thread prefetches spill block, do_dump() will not use it and issues its own blocking arc_read. This may cause significant performance degradation when sending datasets with lots of spill blocks.

For unmodified spill blocks, we also create send_range struct for them in send_reader_thread and issue prefetches for them. We piggyback them on the dnode send_range instead of enqueueing them so we don't break send_range_after check.

On one of our setup, we see at least 5x difference during affected areas.

Description

How Has This Been Tested?

Manual send/recv on dataset with spill block.
Manual raw send/recv on encrypted dataset with spill block.

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:

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Oct 29, 2024
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to make sense.

@ahrens do think it was just missed from #10067, or there is something behind it?

@tuxoko
Copy link
Contributor Author

tuxoko commented Oct 31, 2024

Not sure why tests are showing failure even though the test results seems mostly fine.
Rebase and rerun the tests.

@amotin
Copy link
Member

amotin commented Oct 31, 2024

Crashed on assertion:

  [ 5544.971043] VERIFY(dscp->dsc_dso->dso_dryrun || srdp->abuf != NULL || srdp->abd != NULL) failed
  [ 5544.974720] PANIC at dmu_send.c:980:do_dump()

@tuxoko
Copy link
Contributor Author

tuxoko commented Oct 31, 2024

Crashed on assertion:

  [ 5544.971043] VERIFY(dscp->dsc_dso->dso_dryrun || srdp->abuf != NULL || srdp->abd != NULL) failed
  [ 5544.974720] PANIC at dmu_send.c:980:do_dump()

Hmm...
This doesn't show up in our internal CI for some reason.

Edit: Ok, I think it's because we turn off zfs_send_unmodified_spill_blocks

Currently, even though send_reader_thread prefetches spill block,
do_dump() will not use it and issues its own blocking arc_read. This
causes significant performance degradation when sending datasets with
lots of spill blocks.

For unmodified spill blocks, we also create send_range struct for them
in send_reader_thread and issue prefetches for them. We piggyback them
on the dnode send_range instead of enqueueing them so we don't break
send_range_after check.

Signed-off-by: Chunwei Chen <[email protected]>
@tuxoko
Copy link
Contributor Author

tuxoko commented Nov 1, 2024

Updated to prefetch also unmodified spill block in send_reader_thread.

@tuxoko tuxoko requested a review from amotin November 1, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants