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.

Sign up for GitHub

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

Jump to bottom

Enforce "-F" flag on resuming recv of full/newfs on existing dataset #13857

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Enforce "-F" flag on resuming recv of full/newfs on existing dataset #13857

merged 1 commit into from
Sep 27, 2022

Conversation

jsai20
Copy link
Contributor

@jsai20 jsai20 commented Sep 9, 2022 •
edited
Loading

When receiving full/newfs on existing dataset, then it should be done with "-F" flag. Its enforced for initial receive in checks done in zfs_receive_one function of libzfs. Similarly, on resuming full/newfs recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created dataset and it's marked INCONSISTENT. But when receiving on existing dataset, recv is first done on %recv and its marked INCONSISTENT. Existing dataset is not marked INCONSISTENT. Resume of full/newfs receive with dataset not INCONSISTENT indicates that its resuming newfs on existing dataset. So, enforce "-F" flag in this case.

Signed-off-by: Jitendra Patidar jitendra.patidar@nutanix.com
Closes #13856

Motivation and Context

Description

How Has This Been Tested?

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:

@jsai20
Copy link
Contributor Author

jsai20 commented Sep 9, 2022

Validation with this patch:

Validate “resume” of new/full fs on existing dataset without “-F” flag fails with error and no assert/panic seen.

$ zfs get receive_resume_token testpool/dstfs
NAME PROPERTY VALUE SOURCE
testpool/dstfs receive_resume_token 1-1460efcfae-148-789c636064000310a500c4ec50360710e72765a526973030a8409460caa7a515a79630c001489e0d493ea9b224b51848efc8c2aebf243fbd3433858121a460e9e21b96ce5c0648f29c60f9bcc4dc54209d5a5c52909f9fa30f62a4153b14e725161842cde36640b83f393fb7a028b5b8383f1bbb7dc945259920f3746e482683e435a0f6c0e4538a8ad27212d38b61fe1160e060e4c1e6efe292ca02a0394c08af33000076ff297d -
$ sudo zfs send -t 1-1460efcfae-148-789c636064000310a500c4ec50360710e72765a526973030a8409460caa7a515a79630c001489e0d493ea9b224b51848efc8c2aebf243fbd3433858121a460e9e21b96ce5c0648f29c60f9bcc4dc54209d5a5c52909f9fa30f62a4153b14e725161842cde36640b83f393fb7a028b5b8383f1bbb7dc945259920f3746e482683e435a0f6c0e4538a8ad27212d38b61fe1160e060e4c1e6efe292ca02a0394c08af33000076ff297d |sudo zfs recv -sv testpool/dstfs
cannot receive resume stream: destination 'testpool/dstfs' exists
must specify -F to overwrite it
$

Validate with "-F" it succeedes.

$ sudo zfs send -t 1-1460efcfae-148-789c636064000310a500c4ec50360710e72765a526973030a8409460caa7a515a79630c001489e0d493ea9b224b51848efc8c2aebf243fbd3433858121a460e9e21b96ce5c0648f29c60f9bcc4dc54209d5a5c52909f9fa30f62a4153b14e725161842cde36640b83f393fb7a028b5b8383f1bbb7dc945259920f3746e482683e435a0f6c0e4538a8ad27212d38b61fe1160e060e4c1e6efe292ca02a0394c08af33000076ff297d |sudo zfs recv -svF testpool/dstfs
receiving full stream of testpool/testfs@snap1 into testpool/dstfs@snap1
received 34.2K stream in 0.10 seconds (349K/sec)
$

Validate that “resume” of new/full fs with no existing dataset, doesn’t need “-F” flag.

$ sudo zfs destroy -r testpool/dstfs

$ ls -l stream.full
-rw-rw-r--. 1 jiten jiten 30720 Sep 8 07:57 stream.full
$ cat stream.full |sudo zfs recv -sv testpool/dstfs
receiving full stream of testpool/testfs@snap1 into testpool/dstfs@snap1
cannot receive new filesystem stream: checksum mismatch or incomplete stream.
Partially received snapshot is saved.
A resuming stream can be generated on the sending system by running:
zfs send -t 1-1460efcfae-148-789c636064000310a500c4ec50360710e72765a526973030a8409460caa7a515a79630c001489e0d493ea9b224b51848efc8c2aebf243fbd3433858121a460e9e21b96ce5c0648f29c60f9bcc4dc54209d5a5c52909f9fa30f62a4153b14e725161842cde36640b83f393fb7a028b5b8383f1bbb7dc945259920f3746e482683e435a0f6c0e4538a8ad27212d38b61fe1160e060e4c1e6efe292ca02a0394c08af33000076ff297d
$

$ sudo zfs send -t 1-1460efcfae-148-789c636064000310a500c4ec50360710e72765a526973030a8409460caa7a515a79630c001489e0d493ea9b224b51848efc8c2aebf243fbd3433858121a460e9e21b96ce5c0648f29c60f9bcc4dc54209d5a5c52909f9fa30f62a4153b14e725161842cde36640b83f393fb7a028b5b8383f1bbb7dc945259920f3746e482683e435a0f6c0e4538a8ad27212d38b61fe1160e060e4c1e6efe292ca02a0394c08af33000076ff297d |sudo zfs recv -sv testpool/dstfs
receiving full stream of testpool/testfs@snap1 into testpool/dstfs@snap1
received 34.2K stream in 0.07 seconds (476K/sec)
$

behlendorf
behlendorf requested changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks for investigating this, your analysis looks right to me. Just a few comments.

Please update and add your nice test case from #13856 to the ZTS for this PR.

lib/libzfs/libzfs_sendrecv.c Show resolved Hide resolved
@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing Component: Send/Recv "zfs send/recv" feature labels Sep 15, 2022
@behlendorf
Copy link
Contributor

@jsai20 if you're already working in this area, you may also be interested in 13598.

@jsai20
Copy link
Contributor Author

jsai20 commented Sep 20, 2022

Thanks for investigating this, your analysis looks right to me. Just a few comments.

Please update and add your nice test case from #13856 to the ZTS for this PR.
Ok

@jsai20
Copy link
Contributor Author

jsai20 commented Sep 20, 2022

@jsai20 if you're already working in this area, you may also be interested in 13598.
I would check, but not sure, if I understand it well.

@jsai20 jsai20 force-pushed the jsai20/resume-fullrecv-fix branch 2 times, most recently from 04c5d11 to e1baad8 Compare September 20, 2022 05:31
@jsai20
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
6e4eb21 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
@jsai20 jsai20 force-pushed the jsai20/resume-fullrecv-fix branch from e1baad8 to 6e4eb21 Compare September 20, 2022 06:31
@behlendorf behlendorf requested a review from tuxoko September 26, 2022 23:58
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 27, 2022
@behlendorf behlendorf merged commit 3ed9d68 into openzfs:master Sep 27, 2022
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 28, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
78f754d 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
505e96f 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
032950e 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
ea37f5c 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
aee0a73 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
f1505ca 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
6d55b1b 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
d17bd61 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
214d757 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@jsai20 @andrewc12
Enforce "-F" flag on resuming recv of full/newfs on existing dataset
1277d28 
When receiving full/newfs on existing dataset, then it should be done
with "-F" flag. Its enforced for initial receive in checks done in
zfs_receive_one function of libzfs. Similarly, on resuming full/newfs
recv on existing dataset, it should be done with "-F" flag.

When dataset doesn't exist, then full/new recv is done on newly created
dataset and it's marked INCONSISTENT. But when receiving on existing
dataset, recv is first done on %recv and its marked INCONSISTENT.
Existing dataset is not marked INCONSISTENT. Resume of full/newfs
receive with dataset not INCONSISTENT indicates that its resuming newfs
on existing dataset. So, enforce "-F" flag in this case.

Also return an error from dmu_recv_resume_begin_check() in zfs kernel,
when its resuming full/newfs recv without force.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Chunwei Chen <david.chen@nutanix.com>
Signed-off-by: Jitendra Patidar <jitendra.patidar@nutanix.com>
Closes openzfs#13856
Closes openzfs#13857
@jsai20 jsai20 deleted the jsai20/resume-fullrecv-fix branch August 1, 2024 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@tuxoko tuxoko tuxoko approved these changes

Assignees
No one assigned
Labels
Component: Send/Recv "zfs send/recv" feature Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resuming receive of full/newfs on existing dataset doesn't enforce "-F" flag and effectively hits assert/panic
3 participants
@jsai20 @behlendorf @tuxoko