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

Fix zfs incremental send remove '-o' properties #7478

Merged
merged 1 commit into from
May 1, 2018

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented Apr 26, 2018

Description

This change should correctly identify the top-level dataset in zfs_receive_one(). This was accidentally introduced in bee7e4f: the test case did not correctly verify this situation because it uses adjacent snapshots, basically testing zfs send -i instead of zfs send -I: this commit adds an additional intermediary snapshot to the test script.

Motivation and Context

When receiving an incremental send stream with intermediary snapshots zfs_receive_one() does not correctly identify the top-level dataset: consequently we restore said snapshots as if they were children
datasets in the hierarchy, forcing inheritance of any property received with zfs send -o and effectively removing any locally set value.

How Has This Been Tested?

Reproducer:

# misc functions
function is_linux() {
   if [[ "$(uname)" == "Linux" ]]; then
      return 0
   else
      return 1
   fi
}
# setup
POOLNAME='testpool'
if is_linux; then
   TMPDIR='/var/tmp'
   mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
   zpool destroy $POOLNAME
   rm -f $TMPDIR/zpool_$POOLNAME.dat
   fallocate -l 128m $TMPDIR/zpool_$POOLNAME.dat
   zpool create $POOLNAME $TMPDIR/zpool_$POOLNAME.dat
else
   TMPDIR='/tmp'
   zpool destroy $POOLNAME
   rm -f $TMPDIR/zpool_$POOLNAME.dat
   mkfile 128m $TMPDIR/zpool_$POOLNAME.dat
   zpool create $POOLNAME $TMPDIR/zpool_$POOLNAME.dat
fi
#
zfs create -p testpool/fs/subfs
zfs snap -r testpool/fs@1
zfs snap -r testpool/fs@2
zfs snap -r testpool/fs@3
zfs snap -r testpool/fs@4
zfs send -R testpool/fs@1 | zfs recv testpool/recv
zfs send -RI testpool/fs@1 testpool/fs@2 | zfs recv -F -o ns:prop=val testpool/recv
zfs get ns:prop -o all -r testpool/recv
zfs set ns:prop=newval testpool/recv
zfs get ns:prop -o all -r testpool/recv
zfs send -RI testpool/fs@2 testpool/fs@4 | zfs recv -F -o ns:prop=val testpool/recv
zfs get ns:prop -o all -r testpool/recv

Output with current master:

root@linux:/usr/src/zfs# zfs send -RI testpool/fs@1 testpool/fs@2 | zfs recv -F -o ns:prop=val testpool/recv
root@linux:/usr/src/zfs# zfs get ns:prop -o all -r testpool/recv
NAME                   PROPERTY  VALUE    RECEIVED  SOURCE
testpool/recv          ns:prop   val      -         local
testpool/recv@1        ns:prop   val      -         inherited from testpool/recv
testpool/recv@2        ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs    ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@1  ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@2  ns:prop   val      -         inherited from testpool/recv
root@linux:/usr/src/zfs# zfs set ns:prop=newval testpool/recv
root@linux:/usr/src/zfs# zfs get ns:prop -o all -r testpool/recv
NAME                   PROPERTY  VALUE    RECEIVED  SOURCE
testpool/recv          ns:prop   newval   -         local
testpool/recv@1        ns:prop   newval   -         inherited from testpool/recv
testpool/recv@2        ns:prop   newval   -         inherited from testpool/recv
testpool/recv/subfs    ns:prop   newval   -         inherited from testpool/recv
testpool/recv/subfs@1  ns:prop   newval   -         inherited from testpool/recv
testpool/recv/subfs@2  ns:prop   newval   -         inherited from testpool/recv
root@linux:/usr/src/zfs# zfs send -RI testpool/fs@2 testpool/fs@4 | zfs recv -F -o ns:prop=val testpool/recv
root@linux:/usr/src/zfs# zfs get ns:prop -o all -r testpool/recv
NAME                   PROPERTY  VALUE    RECEIVED  SOURCE
testpool/recv          ns:prop   -        -         -
testpool/recv@1        ns:prop   -        -         -
testpool/recv@2        ns:prop   -        -         -
testpool/recv@3        ns:prop   -        -         -
testpool/recv@4        ns:prop   -        -         -
testpool/recv/subfs    ns:prop   -        -         -
testpool/recv/subfs@1  ns:prop   -        -         -
testpool/recv/subfs@2  ns:prop   -        -         -
testpool/recv/subfs@3  ns:prop   -        -         -
testpool/recv/subfs@4  ns:prop   -        -         -
root@linux:/usr/src/zfs# 

With patch applied:

root@linux:~# zfs send -RI testpool/fs@1 testpool/fs@2 | zfs recv -F -o ns:prop=val testpool/recv
root@linux:~# zfs get ns:prop -o all -r testpool/recv
NAME                   PROPERTY  VALUE    RECEIVED  SOURCE
testpool/recv          ns:prop   val      -         local
testpool/recv@1        ns:prop   val      -         inherited from testpool/recv
testpool/recv@2        ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs    ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@1  ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@2  ns:prop   val      -         inherited from testpool/recv
root@linux:~# zfs set ns:prop=newval testpool/recv
root@linux:~# zfs get ns:prop -o all -r testpool/recv
NAME                   PROPERTY  VALUE    RECEIVED  SOURCE
testpool/recv          ns:prop   newval   -         local
testpool/recv@1        ns:prop   newval   -         inherited from testpool/recv
testpool/recv@2        ns:prop   newval   -         inherited from testpool/recv
testpool/recv/subfs    ns:prop   newval   -         inherited from testpool/recv
testpool/recv/subfs@1  ns:prop   newval   -         inherited from testpool/recv
testpool/recv/subfs@2  ns:prop   newval   -         inherited from testpool/recv
root@linux:~# zfs send -RI testpool/fs@2 testpool/fs@4 | zfs recv -F -o ns:prop=val testpool/recv
root@linux:~# zfs get ns:prop -o all -r testpool/recv
NAME                   PROPERTY  VALUE    RECEIVED  SOURCE
testpool/recv          ns:prop   val      -         local
testpool/recv@1        ns:prop   val      -         inherited from testpool/recv
testpool/recv@2        ns:prop   val      -         inherited from testpool/recv
testpool/recv@3        ns:prop   val      -         inherited from testpool/recv
testpool/recv@4        ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs    ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@1  ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@2  ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@3  ns:prop   val      -         inherited from testpool/recv
testpool/recv/subfs@4  ns:prop   val      -         inherited from testpool/recv
root@linux:~# 

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

When receiving an incremental send stream with intermediary snapshots
zfs_receive_one() does not correctly identify the top-level dataset:
consequently we restore said snapshots as if they were children
datasets in the hierarchy, forcing inheritance of any property received
with 'zfs send -o' and effectively removing any locally set value.

The test case did not correctly verify this situation because it uses
adjacent snapshots, basically testing 'zfs send -i' instead of
'zfs send -I': this commit adds an additional intermediary snapshot to
the test script.

Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
@codecov
Copy link

codecov bot commented Apr 26, 2018

Codecov Report

Merging #7478 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7478      +/-   ##
==========================================
+ Coverage   76.58%   76.86%   +0.28%     
==========================================
  Files         336      336              
  Lines      107137   107137              
==========================================
+ Hits        82047    82356     +309     
+ Misses      25090    24781     -309
Flag Coverage Δ
#kernel 77.29% <ø> (+0.35%) ⬆️
#user 66.28% <100%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 089500e...671fd32. Read the comment docs.

@ahrens
Copy link
Member

ahrens commented Apr 27, 2018

@pcd1193182 may want to take a look at this.

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.

Nice find!

Copy link
Contributor

@pcd1193182 pcd1193182 left a comment

Choose a reason for hiding this comment

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

Good catch!

@behlendorf behlendorf merged commit 3cbe89b into openzfs:master May 1, 2018
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 15, 2018
When receiving an incremental send stream with intermediary snapshots
zfs_receive_one() does not correctly identify the top-level dataset:
consequently we restore said snapshots as if they were children
datasets in the hierarchy, forcing inheritance of any property received
with 'zfs send -o' and effectively removing any locally set value.

The test case did not correctly verify this situation because it uses
adjacent snapshots, basically testing 'zfs send -i' instead of
'zfs send -I': this commit adds an additional intermediary snapshot to
the test script.

Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#7478
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 23, 2018
When receiving an incremental send stream with intermediary snapshots
zfs_receive_one() does not correctly identify the top-level dataset:
consequently we restore said snapshots as if they were children
datasets in the hierarchy, forcing inheritance of any property received
with 'zfs send -o' and effectively removing any locally set value.

The test case did not correctly verify this situation because it uses
adjacent snapshots, basically testing 'zfs send -i' instead of
'zfs send -I': this commit adds an additional intermediary snapshot to
the test script.

Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#7478
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 27, 2018
When receiving an incremental send stream with intermediary snapshots
zfs_receive_one() does not correctly identify the top-level dataset:
consequently we restore said snapshots as if they were children
datasets in the hierarchy, forcing inheritance of any property received
with 'zfs send -o' and effectively removing any locally set value.

The test case did not correctly verify this situation because it uses
adjacent snapshots, basically testing 'zfs send -i' instead of
'zfs send -I': this commit adds an additional intermediary snapshot to
the test script.

Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#7478
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 5, 2018
When receiving an incremental send stream with intermediary snapshots
zfs_receive_one() does not correctly identify the top-level dataset:
consequently we restore said snapshots as if they were children
datasets in the hierarchy, forcing inheritance of any property received
with 'zfs send -o' and effectively removing any locally set value.

The test case did not correctly verify this situation because it uses
adjacent snapshots, basically testing 'zfs send -i' instead of
'zfs send -I': this commit adds an additional intermediary snapshot to
the test script.

Reviewed-by: Paul Dagnelie <pcd@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#7478
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.

4 participants