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

OpenZFS 7252 - compressed zfs send / receive #6067

Closed
wants to merge 1 commit into from

Conversation

behlendorf
Copy link
Contributor

OpenZFS 7252 - compressed zfs send / receive
OpenZFS 7628 - create long versions of ZFS send / receive options

Authored by: Dan Kimmel [email protected]
Reviewed by: George Wilson [email protected]
Reviewed by: John Kennedy [email protected]
Reviewed by: Matthew Ahrens [email protected]
Reviewed by: Paul Dagnelie [email protected]
Reviewed by: Pavel Zakharov [email protected]
Reviewed by: Sebastien Roy [email protected]
Reviewed by: David Quigley [email protected]
Reviewed by: Thomas Caputi [email protected]
Approved by: Dan McDonald [email protected]
Ported-by: Don Brady [email protected]
Ported-by: bunder2015 [email protected]
Ported-by: Brian Behlendorf [email protected]

OpenZFS-issue: https://www.illumos.org/issues/7252
OpenZFS-commit: openzfs/openzfs@5602294

Porting Notes:

  • Most of 7252 was already picked up during ABD work. This
    commit represents the gap from the final commit to openzfs.
  • Fixed split_large_blocks check in do_dump()
  • An alternate version of the write_compressible() function was
    implemented for Linux which does not depend on fio. The behavior
    of fio differs significantly based on the exact version.
  • mkholes was replaced with truncate for Linux.

OpenZFS 7252 - compressed zfs send / receive
OpenZFS 7628 - create long versions of ZFS send / receive options

Authored by: Dan Kimmel <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: John Kennedy <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: Paul Dagnelie <[email protected]>
Reviewed by: Pavel Zakharov <[email protected]>
Reviewed by: Sebastien Roy <[email protected]>
Reviewed by: David Quigley <[email protected]>
Reviewed by: Thomas Caputi <[email protected]>
Approved by: Dan McDonald <[email protected]>
Ported-by: Don Brady <[email protected]>
Ported-by: bunder2015 <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/7252
OpenZFS-commit: openzfs/openzfs@5602294

Porting Notes:
- Most of 7252 was already picked up during ABD work.  This
  commit represents the gap from the final commit to openzfs.
- Fixed split_large_blocks check in do_dump()
- An alternate version of the write_compressible() function was
  implemented for Linux which does not depend on fio.  The behavior
  of fio differs significantly based on the exact version.
- mkholes was replaced with truncate for Linux.
@mention-bot
Copy link

@behlendorf, thanks for your PR! By analyzing the history of the files in this pull request, we identified @grwilson, @ahrens and @skiselkov to be potential reviewers.

[[ -d $dir ]] || log_fail "No directory: $dir"

# Under Linux fio is not currently used since its behavior can
# differ significant accross versions. This includes missing
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "significantly across", "and" on next line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

Copy link
Contributor

@dpquigl dpquigl left a comment

Choose a reason for hiding this comment

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

Previously reviewed the kernel code when it was in a PR by Don. The test code looks to do perform the testing steps properly. Linux specific changes seem sane. Its unfortunate FIO doesn't behave properly.

@bunder2015
Copy link
Contributor

Tests good on my VM. :shipit:

Out of curiosity, how did you get send-c_verify_contents and send-c_volume working? The tests look unchanged from my last attempts.

@behlendorf
Copy link
Contributor Author

@bunder2015 this is the issue @dpquigl alluded to and you observed when porting this.

The fio(1) options used for OpenZFS simply don't produce the same behavior under Linux. At least with fio-2.2.8, I didn't try other versions. Specifically, the --buffer_compress_* options result in a sparse file rather than a 66% compressible file. Providing a simpler write_compressible() function for Linux resolved the failing test cases.

Copy link
Contributor

@loli10K loli10K left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2478,7 +2504,7 @@ arc_loan_inuse_buf(arc_buf_t *buf, void *tag)
(void) refcount_add(&hdr->b_l1hdr.b_refcnt, arc_onloan_tag);
(void) refcount_remove(&hdr->b_l1hdr.b_refcnt, tag);

atomic_add_64(&arc_loaned_bytes, -arc_buf_size(buf));
arc_loaned_bytes_update(arc_buf_size(buf));
Copy link
Contributor

Choose a reason for hiding this comment

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

We are dropping a minus ("-") sign from the old code here: looking at the Illumos commit i think it's correct but since i'm not really familiar with the ARC code i'm asking anyway just to be extra sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good observation. Yes, the existing code was wrong here and we need to drop the minus sign. The arc_loan_buf* and arc_return_buf functions should increment and decrement this value accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants