Fix dryrun and intra-pool resumable 'zfs send' #6623

loli10K · 2017-09-09T20:48:03Z

Description

This PR fixes a couple of issue with resumable zfs send.

zfs send -n -t <token> should dump its output to stdout, not stderr, for consistency with other dry-run commands.
zfs send -t <token> for an incremental send should be able to resume successfully when sending to the same pool: a subtle issue in zfs_iter_children() doesn't currently allow this.
Because resuming from a token requires "guid" -> "dataset" mapping (guid_to_name()), we have to walk the whole hierarchy to find the right snapshots to send.
When resuming an incremental send both source and destination live in the same pool and have the same guid: this is where zfs_iter_children() gets confused and picks up the wrong snapshot, so we end up trying to send an incremental "destination@snap1 -> source@snap2" stream instead of "source@snap1 -> source@snap2": this fails with an "Invalid cross-device link" (EXDEV) error.

EDIT: just to reiterate, the second bug looks like this

root@linux:~# zfs send -vt $TOKEN > /dev/null
resume token contents:
nvlist version: 0
	fromguid = 0x46f626fea92a89af
	object = 0x1
	offset = 0x0
	bytes = 0x0
	toguid = 0x7b604c01bd7168bd
	toname = testpool/fs@snap2
send from testpool/fs/newfs@snap1 to testpool/fs@snap2
TIME        SENT   SNAPSHOT
warning: cannot send 'testpool/fs@snap2': Invalid cross-device link

We need to know which snapshot to send from by its guid (fromguid = 0x46f626fea92a89af). Both testpool/fs@snap1 and testpool/fs/newfs@snap1 are good candidates:

root@linux:~# zfs get guid
NAME                     PROPERTY  VALUE  SOURCE
testpool                 guid      3700393278039286775  -
testpool/fs              guid      10649419692702751658  -
testpool/fs@snap1        guid      5113317302127462831  -
testpool/fs@snap2        guid      8890189234786363581  -
testpool/fs/newfs        guid      16613946719253299331  -
testpool/fs/newfs@snap1  guid      5113317302127462831  -
root@linux:~# 
root@linux:~# printf '%d\n' 0x46f626fea92a89af
5113317302127462831

zfs_iter_children(testpool/fs) currently walks the dataset hierarchy in this order (children first, then snapshots)

testpool/fs
testpool/fs/newfs
testpool/fs/newfs@snap1
testpool/fs@snap1
testpool/fs@snap2

when it should be doing (snapshot first, then children)

testpool/fs
testpool/fs@snap1
testpool/fs@snap2
testpool/fs/newfs
testpool/fs/newfs@snap1

to find the correct snapshot (testpool/fs@snap1).

NOTE: i've yet to add a new test case to the ZTS but don't know where the best place would be ("rsend" or "zfs_send"?). rsend_019_pos would probably be a good choice but it's currently disabled.

EDIT: waiting for #6632 to get merged so rsend_019_pos is back in business.

Motivation and Context

Fix #6618
Fix #6619

How Has This Been Tested?

# 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
#
dd if=/dev/urandom of=/$POOLNAME/data.bin bs=1M count=10
zfs snapshot $POOLNAME@snap1
zfs send $POOLNAME@snap1 | zfs recv -s $POOLNAME/mirror
#
dd if=/dev/urandom of=/$POOLNAME/data.bin bs=1M count=10
zfs snapshot $POOLNAME@snap2
zfs send -i $POOLNAME@snap1 $POOLNAME@snap2 | dd bs=1M count=1 | zfs recv -s $POOLNAME/mirror
#
TOKEN=$(zfs get -H -o value receive_resume_token $POOLNAME/mirror)
zfs send -nP -t $TOKEN > /dev/null
zfs send -t $TOKEN > /dev/null

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.

Because resuming from a token requires "guid" -> "snapshot" mapping we have to walk the whole dataset hierarchy to find the right snapshot to send; when both source and destination exists, for an incremental resumable stream, libzfs gets confused and picks up the wrong snapshot to send from: this results in attempting to send "destination@snap1 -> source@snap2" instead of "source@snap1 -> source@snap2" which fails with a "Invalid cross-device link" error (EXDEV). Fix this by adjusting the logic behind dataset traversal in zfs_iter_children() to pick the right snapshot to send from. Additionally update dry-run 'zfs send -t' to print its output to stderr: this is consistent with other dry-run commands. Signed-off-by: loli10K <ezomori.nozomu@gmail.com>

codecov · 2017-10-01T06:12:51Z

Codecov Report

Merging #6623 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6623      +/-   ##
==========================================
+ Coverage   74.07%   74.07%   +<.01%     
==========================================
  Files         295      295              
  Lines       93883    93884       +1     
==========================================
+ Hits        69543    69544       +1     
  Misses      24340    24340

Flag	Coverage Δ
#kernel	`73.9% <ø> (-0.16%)`	⬇️
#user	`63.25% <100%> (-0.16%)`	⬇️

Impacted Files	Coverage Δ
lib/libzfs/libzfs_iter.c	`91.62% <100%> (ø)`	⬆️
lib/libzfs/libzfs_sendrecv.c	`76.5% <100%> (+0.06%)`	⬆️
cmd/zed/agents/zfs_retire.c	`46.58% <0%> (-6.22%)`	⬇️
cmd/zed/agents/zfs_mod.c	`61.75% <0%> (-3.86%)`	⬇️
module/zfs/zrlock.c	`82.81% <0%> (-1.57%)`	⬇️
module/zcommon/zfs_uio.c	`91.86% <0%> (-1.17%)`	⬇️
module/icp/algs/edonr/edonr.c	`43.45% <0%> (-0.94%)`	⬇️
module/zfs/dmu_tx.c	`80.87% <0%> (-0.8%)`	⬇️
module/zfs/dsl_pool.c	`93.51% <0%> (-0.75%)`	⬇️
cmd/zed/agents/fmd_api.c	`28.46% <0%> (-0.72%)`	⬇️
... and 36 more

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 01ff0d7...f28bcb2. Read the comment docs.

behlendorf

Nice fix!

behlendorf · 2017-10-03T20:45:49Z

@pcd1193182 could you please review this fix. I would expect OpenZFS to suffer from this same problem.

Because resuming from a token requires "guid" -> "snapshot" mapping we have to walk the whole dataset hierarchy to find the right snapshot to send; when both source and destination exists, for an incremental resumable stream, libzfs gets confused and picks up the wrong snapshot to send from: this results in attempting to send "destination@snap1 -> source@snap2" instead of "source@snap1 -> source@snap2" which fails with a "Invalid cross-device link" error (EXDEV). Fix this by adjusting the logic behind dataset traversal in zfs_iter_children() to pick the right snapshot to send from. Additionally update dry-run 'zfs send -t' to print its output to stderr: this is consistent with other dry-run commands. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#6618 Closes openzfs#6619 Closes openzfs#6623

Because resuming from a token requires "guid" -> "snapshot" mapping we have to walk the whole dataset hierarchy to find the right snapshot to send; when both source and destination exists, for an incremental resumable stream, libzfs gets confused and picks up the wrong snapshot to send from: this results in attempting to send "destination@snap1 -> source@snap2" instead of "source@snap1 -> source@snap2" which fails with a "Invalid cross-device link" error (EXDEV). Fix this by adjusting the logic behind dataset traversal in zfs_iter_children() to pick the right snapshot to send from. Additionally update dry-run 'zfs send -t' to print its output to stderr: this is consistent with other dry-run commands. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes #6618 Closes #6619 Closes #6623

Because resuming from a token requires "guid" -> "snapshot" mapping we have to walk the whole dataset hierarchy to find the right snapshot to send; when both source and destination exists, for an incremental resumable stream, libzfs gets confused and picks up the wrong snapshot to send from: this results in attempting to send "destination@snap1 -> source@snap2" instead of "source@snap1 -> source@snap2" which fails with a "Invalid cross-device link" error (EXDEV). Fix this by adjusting the logic behind dataset traversal in zfs_iter_children() to pick the right snapshot to send from. Additionally update dry-run 'zfs send -t' to print its output to stderr: this is consistent with other dry-run commands. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#6618 Closes openzfs#6619 Closes openzfs#6623

loli10K added the Status: Work in Progress Not yet ready for general review label Sep 9, 2017

loli10K mentioned this pull request Sep 19, 2017

Fix some ZFS Test Suite issues #6632

Merged

13 tasks

loli10K force-pushed the issue-6618 branch from fd5d9f8 to 676caa1 Compare September 26, 2017 18:05

loli10K removed the Status: Work in Progress Not yet ready for general review label Sep 27, 2017

loli10K force-pushed the issue-6618 branch from 676caa1 to f28bcb2 Compare October 1, 2017 06:12

behlendorf approved these changes Oct 3, 2017

View reviewed changes

gmelikov approved these changes Oct 4, 2017

View reviewed changes

behlendorf merged commit aee1dd4 into openzfs:master Oct 10, 2017

behlendorf added this to PR Needed in 0.7.3 Oct 10, 2017

loli10K deleted the issue-6618 branch October 15, 2017 06:22

tonyhutter moved this from PR Needed to Merged to 0.7.3 in 0.7.3 Oct 16, 2017

loli10K mentioned this pull request Dec 30, 2017

8940 Sending an intra-pool resumable send stream may result in EXDEV openzfs/openzfs#510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dryrun and intra-pool resumable 'zfs send' #6623

Fix dryrun and intra-pool resumable 'zfs send' #6623

loli10K commented Sep 9, 2017 •

edited

codecov bot commented Oct 1, 2017 •

edited

behlendorf left a comment

behlendorf commented Oct 3, 2017

Fix dryrun and intra-pool resumable 'zfs send' #6623

Fix dryrun and intra-pool resumable 'zfs send' #6623

Conversation

loli10K commented Sep 9, 2017 • edited

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Oct 1, 2017 • edited

Codecov Report

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf commented Oct 3, 2017

loli10K commented Sep 9, 2017 •

edited

codecov bot commented Oct 1, 2017 •

edited