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 send/recv patches #4742

Closed
wants to merge 11 commits into from
Closed

Conversation

behlendorf
Copy link
Contributor

No description provided.

@behlendorf
Copy link
Contributor Author

@kernelOfTruth can you please review and test. This is a rebased version of #4396 with all the functional/rsend tests enabled and passing with a couple exceptions. rsend_008_pos and rsend_009_pos are disabled, they also failing the automated OpenZFS testing not an issue with this patch.

@ahrens rsend_020_pos hits the ASSERT in the drr->drr_type != DRR_BEGIN case in module/zfs/libzfs_sendrecv.c:dump_record() However, when commenting out the ASSERT the test appears to pass. Do you know if this is a known issue? Does the OpenZFS automated testing run with all the ASSERTs enabled?

@ahrens
Copy link
Member

ahrens commented Jun 8, 2016

rsend_020_pos hits the ASSERT in the drr->drr_type != DRR_BEGIN case in module/zfs/libzfs_sendrecv.c:dump_record() However, when commenting out the ASSERT the test appears to pass. Do you know if this is a known issue?

@behlendorf Assuming you are talking about:

        ASSERT(ZIO_CHECKSUM_IS_ZERO(&drr->drr_u.
            drr_checksum.drr_checksum));

I have not seen that fail. This could indicate a problem with zeroing out the record to begin with, or that the drr_checksum overlaps with fields in the other union members (which is what this assert is trying to check).

Does the OpenZFS automated testing run with all the ASSERTs enabled?

Yes.

@behlendorf
Copy link
Contributor Author

@ahrens OK thanks, I'll run it down.

@behlendorf behlendorf changed the title OpenZFS 2605 want to resume interrupted zfs send OpenZFS zfs send/recv patches Jun 9, 2016
@behlendorf behlendorf force-pushed the illumos-2605 branch 2 times, most recently from 64e0567 to 3675433 Compare June 15, 2016 22:41
@behlendorf
Copy link
Contributor Author

behlendorf commented Jun 15, 2016

I've added commit 30ffc01 to this PR which addresses the last remaining issue. 30ffc01 adds a new ZFS_IOC_RECV_NEW ioctl, similar to ZFS_IOC_SEND_NEW, which is used for a resumable zfs receive. This way the zfs_cmd_t structure doesn't need to be modified as was done in the original patch which breaks user/kernel compatibility. We can not change these legacy interfaces.

@ahrens can you comment on if I should submit a version of this change upstream. You've already made the change to zfs_cmd_t so I wouldn't include that part. But it also adds a libzfs core function called lzc_receive_one() which zfs_receive_one() has been updated to use in addition to the new non-legacy ZFS_IOC_RECV_NEW ioctl. I could submit a patch which includes just those portions.

@ahrens
Copy link
Member

ahrens commented Jun 16, 2016

@behlendorf It looks like the difference between lzc_receive_with_header in illumos and lzc_receive_one in this patch is the addition of the following arguments:

int cleanup_fd, uint64_t *read_bytes,
 +    uint64_t *errflags, uint64_t *action_handle, nvlist_t **errors

Is there a description of the new functionality of these new arguments? Assuming it's generally useful, I'd be happy to have that upstream. Also tagging @pcd1193182 for comment.

@behlendorf
Copy link
Contributor Author

@ahrens great, I'll definitely make another pass to improve the documentation for this function. None of these additional arguments add new functionality they solely provide access to the four existing return values of ZFS_IOC_RECV. This way zfs_receive_one() can properly use libzfs_core and get out of the business of constructing its own zfs_cmd_t.

  • "read_bytes" -> number of bytes read
    
  • "error_flags" -> zprop_errflags_t
    
  • "action_handle" -> handle for this guid/ds mapping
    
  • "errors" -> error for each unapplied received property (nvlist)
    

Let me polish the patch a little bit more and I'll submit a version to OpenZFS.

@behlendorf behlendorf changed the title OpenZFS zfs send/recv patches OpenZFS send/recv patches Jun 16, 2016
.ad
.sp .6
.RS 4n
Force a rollback of the file system to the most recent snapshot before performing the receive operation. If receiving an incremental replication stream (for example, one generated by \fBzfs send -R -[iI]\fR), destroy snapshots and file systems that do not exist on the sending side.
Forces the stream to be received as a clone of the given snapshot.
Copy link
Contributor

@pcd1193182 pcd1193182 Jun 16, 2016

Choose a reason for hiding this comment

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

Seems a bit odd that this is the only section in the man page that wraps to 80 characters. Is that deliberate?

Copy link
Contributor Author

@behlendorf behlendorf Jun 16, 2016

Choose a reason for hiding this comment

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

Will fix.

@behlendorf behlendorf self-assigned this Jun 20, 2016
@behlendorf behlendorf force-pushed the illumos-2605 branch 8 times, most recently from 7118d33 to 13397d7 Compare June 24, 2016 22:24
@dpquigl
Copy link
Contributor

dpquigl commented Jun 27, 2016

What can we do to get this PR merged. It is the base for being able to port over compressed send/recv so we can continue on our porting of ADB.

@behlendorf
Copy link
Contributor Author

@dpquigl I had hoped to merge this on Friday but ran in to one last issue. The resumable send/recv tests fail when the new AVX2 optimized fletcher4 checksums are enabled. I was able to reliable reproduce this issue using the rsend_019_pos test case but haven't yet had the time to fully investigate. @dpquigl if either you or @jxiong have time and can get the to root cause this is ready to me merged.

ahrens and others added 2 commits June 28, 2016 13:47
2605 want to resume interrupted zfs send
Reviewed by: George Wilson <[email protected]>
Reviewed by: Paul Dagnelie <[email protected]>
Reviewed by: Richard Elling <[email protected]>
Reviewed by: Xin Li <[email protected]>
Reviewed by: Arne Jansen <[email protected]>
Approved by: Dan McDonald <[email protected]>
Ported-by: kernelOfTruth <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/2605
OpenZFS-commit: openzfs/openzfs@9c3fd12

6980 6902 causes zfs send to break due to 32-bit/64-bit struct mismatch
Reviewed by: Paul Dagnelie <[email protected]>
Reviewed by: George Wilson <[email protected]>
Approved by: Robert Mustacchi <[email protected]>
Ported by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/6980
OpenZFS-commit: openzfs/openzfs@ea4a67f

Porting notes:
- All rsend and snapshop tests enabled and updated for Linux.
- Fix misuse of input argument in traverse_visitbp().
- Fix ISO C90 warnings and errors.
- Fix gcc 'missing braces around initializer' in
  'struct send_thread_arg to_arg =' warning.
- Replace 4 argument fletcher_4_native() with 3 argument version,
  this change was made in OpenZFS 4185 which has not been ported.
- Part of the sections for 'zfs receive' and 'zfs send' was
  rewritten and reordered to approximate upstream.
- Fix mktree xattr creation, 'user.' prefix required.
- Minor fixes to newly enabled test cases
- Long holds for volumes allowed during receive for minor registration.
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: Paul Dagnelie <[email protected]>
Approved by: Robert Mustacchi <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/6051
OpenZFS-commit: openzfs/openzfs@620f322
pcd1193182 and others added 8 commits June 28, 2016 13:47
Authored by: Paul Dagnelie <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: Prakash Surya <[email protected]>
Reviewed by: Richard Elling <[email protected]>
Approved by: Dan McDonald <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/6394
OpenZFS-commit: openzfs/openzfs@68ecb2e
…EERECORDS

Authored by: Andrew Stormont <[email protected]>
Reviewed by: Anil Vijarnia <[email protected]>
Reviewed by: Kim Shrier <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Approved by: Dan McDonald <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/6536
OpenZFS-commit: openzfs/openzfs@880094b
Authored by: Eli Rosenthal <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: Dan Kimmel <[email protected]>
Reviewed by: Paul Dagnelie <[email protected]>
Reviewed by: Dan McDonald <[email protected]>
Approved by: Robert Mustacchi <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/6738
OpenZFS-commit: openzfs/openzfs@c20404ff
…eeds refquota

Authored by: Dan McDonald <[email protected]>
Reviewed by: John Kennedy <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Approved by: Gordon Ross <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/4986
OpenZFS-commit: openzfs/openzfs@5878fad
Authored by: Dan McDonald <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: Yuri Pankov <[email protected]>
Reviewed by: Toomas Soome <[email protected]>
Approved by: Gordon Ross <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/6562
OpenZFS-commit: openzfs/openzfs@5f7a8e6
Adds ZFS_IOC_RECV_NEW for resumable streams and preserves the legacy
ZFS_IOC_RECV user/kernel interface.  The new interface supports all
stream options but is currently only used for resumable streams.
This way updated user space utilities will interoperate with older
kernel modules.

ZFS_IOC_RECV_NEW is modeled after the existing ZFS_IOC_SEND_NEW
handler.  Non-Linux OpenZFS platforms have opted to change the
legacy interface in an incompatible fashion instead of adding a
new ioctl.

Signed-off-by: Brian Behlendorf <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: Prakash Surya <[email protected]>
Reviewed by: Igor Kozhukhov <[email protected]>
Approved by: Dan McDonald <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/6314
OpenZFS-commit: openzfs/openzfs@d6160ee
…g name

Reviewed by: Prakash Surya <[email protected]>
Reviewed by: Dan Kimmel <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: Yuri Pankov <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

Calling dsl_dataset_name on a dataset with a 256 byte buffer is asking
for trouble. We should check every dataset on import, using a 1024 byte
buffer and checking each time to see if the dataset's new name is longer
than 256 bytes.

OpenZFS-issue: https://www.illumos.org/issues/6876
OpenZFS-commit: openzfs/openzfs@ca8674e
@behlendorf
Copy link
Contributor Author

behlendorf commented Jun 28, 2016

@ironMann @jxiong can you please review commit 832fe89 from the stack. The fletcher 4 failures I was observing with the patch stack was caused by the updated send/recv code calling fletcher_4_native() with a buffer which was not a multiple of 8 bytes. Rather than attempt to handle this in any of the optimized implementation I've added a check to fallback to the scalar routines for this specific rare case.

The fletcher_4_native() and fletcher_4_byteswap() functions may only
safely use the vectorized implementations when the buffer is 128-bit
aligned.  This is because both the AVX2 and SSE implementations process
four 32-bit words per iterations.  Fallback to the scalar implementation
which only processes a single 32-bit word for unaligned buffers.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#4330
@behlendorf
Copy link
Contributor Author

Merged as:

5c27b29 Merge branch 'illumos-2605'
0dab2e8 Vectorized fletcher_4 must be 128-bit aligned
d1d19c7 OpenZFS 6876 - Stack corruption after importing a pool with a too-long n
eca7b76 OpenZFS 6314 - buffer overflow in dsl_dataset_name
43e52ed Implement zfs_ioc_recv_new() for OpenZFS 2605
8c62a0d OpenZFS 6562 - Refquota on receive doesn't account for overage
671c935 OpenZFS 4986 - receiving replication stream fails if any snapshot exceed
f8866f8 OpenZFS 6738 - zfs send stream padding needs documentation
b607405 OpenZFS 6536 - zfs send: want a way to disable setting of DRR_FLAG_FREER
e6d3a84 OpenZFS 6393 - zfs receive a full send as a clone
fd41e93 OpenZFS 6051 - lzc_receive: allow the caller to read the begin record
47dfff3 OpenZFS 2605, 6980, 6902

@behlendorf behlendorf deleted the illumos-2605 branch May 18, 2018 18:38
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.

7 participants