OpenZFS 7252 - create long versions of ZFS send / receive options #5951
Conversation
|
@bunder2015, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @rlaager and @ahrens to be potential reviewers. |
There was a problem hiding this comment.
@bunder2015 Thank you for submitting this!
Commit messages for OpenZFS patch ports should have a subject line that starts with OpenZFS <issue #> - . Also, you need a - in your Ported-by: line. You can refer to the OpenZFS Patch Port section of the contributing guidelines for an example.
Edit: Looks like you corrected your Ported-by: as I was typing the above. :)
|
Okay I think I got it right this time... but something seems off with that new formatting checker...
It's there... but what is also odd, is it's saying that twice. |
bunder2015
changed the title
|
@bunder2015 I noticed that. I'm looking into it. Thanks for the heads up. |
|
@bunder2015 The issues you were seeing should be corrected once #5952 lands. Sorry for the trouble! |
|
Looks like I missed some illumos-isms in the test scripts. I'll try to get these cleaned up. |
| known=${feature[$flag]} | ||
| [[ -z $known ]] && log_fail "Unknown feature: $flag" | ||
|
|
||
| derived=$($ECHO "$flags & ${feature[$flag]} = X" | mdb | $SED 's/ //g') |
There was a problem hiding this comment.
Will need a replacement for mdb, I tried using ksh directly but it doesn't provide the intended output.
| log_must $ZFS create -o compress=lz4 $recvfs | ||
| typeset dir=$(get_prop mountpoint $sendfs) | ||
| # Don't use write_compressible: we want compressible but undedupable data here. | ||
| log_must $CP /kernel/genunix $dir/file |
There was a problem hiding this comment.
Any recommendations for a replacement file to use that's available our Linux buildbots?
There was a problem hiding this comment.
I've been trying to think of a file that would be on every buildbot and that has a reasonable size, would something like awk, grep, mv, ls, tar, ps be large enough for this? Most of these are over 100kb in size. If we need bigger, ksh and git are around 2mb in size. (genunix is 3mb)
There was a problem hiding this comment.
mkfile may be useful to make a file in the size you want.
There was a problem hiding this comment.
Do you know if mkfile puts any content in the file, or if its just an empty file? I imagine that would have the same problem as write_compressible.
There was a problem hiding this comment.
Hmm, it may make a file full of 0s. Another option may be to use dd and with your input stream being /dev/urandom or something similar.
There was a problem hiding this comment.
Unfortunately, urandom doesn't compress.
Compressed 4194304 bytes into 4194323 bytes ==> 100.00%
There was a problem hiding this comment.
You can use write_file which will write a single non-zero compressible pattern.
There was a problem hiding this comment.
Fun fact, if you pass R to file_write's -d option, it will write psudorandom data:
./tests/zfs-tests/cmd/file_write/file_write -o overwrite -f randomdata.dat -d R
| # For lz4, this method works for blocks up to 16k, but not larger | ||
| [[ $recsize -eq $((32 * 1024)) ]] && break | ||
|
|
||
| log_must mkholes -h 0:$((recsize - 8)) -d $((recsize - 8)):8 \ |
There was a problem hiding this comment.
Looks like we're also missing mkholes.
There was a problem hiding this comment.
mkholes is part of the test suite. see usr/src/test/zfs-tests/cmd/mkholes on illumos
There was a problem hiding this comment.
Found it, thanks!
| fl.l_whence = SEEK_SET; | ||
| fl.l_start = off; | ||
| fl.l_len = len; | ||
| if (fcntl(fd, F_FREESP, &fl) != 0) { |
There was a problem hiding this comment.
Will need to find a Linux alternative for F_FREESP. Would FALLOC_FL_PUNCH_HOLE be sufficient?
There was a problem hiding this comment.
fallocate with the FALLOC_FL_PUNCH_HOLE mode might work.
There was a problem hiding this comment.
tests/zfs-tests/cmd/randfree_file/randfree_file.c has an example of using fallocate.
|
@bunder2015 #5952 landed in master. If you rebase, the STYLE builder should begin to pass its style checking. |
|
Thanks. Looks like we're down to 3 problems in the test scripts, fio is saying it needs --size, something to do with loop devices on some zpool creates, and a missing include to get within_percent to work. I'm not 100% on the loop devices but I think I got the other two working. |
|
@bunder2015 my expectation is that #5903 will get merged some time this week. At which point you won't need to make any of the |
|
@dinatale2 might want to check #5952 again, now it says I didn't sign off for a port. 😆 |
|
@bunder2015 It looks like you merged master into your branch. So the buildbot is testing the commit with the message What you want to do is roll back to your previous commit (the one that has a commit message of |
|
Thanks, I think I got that sorted out. Looked over the last set of logs and found a few things...
Missing loop/vdev stuff should be fixed. Need to look at fio again, doesn't look like its putting its files in the right places. I'm gonna set up a personal buildbot VM in the morning to keep the noise down. |
|
@bunder2015 hole punching on Linux wasn't supported until the 2.6.38 kernel. My suggestion would be to update the test case to verify that FALLOC_FL_KEEP_SIZE works and if not skip the test case with |
Shouldn't we call
instead of
to punch holes? It seems that's what
It also seems our 649 zpl_fallocate_common(struct inode *ip, int mode, loff_t offset, loff_t len)
650 {
651 int error = -EOPNOTSUPP;
652
653 #if defined(FALLOC_FL_PUNCH_HOLE) && defined(FALLOC_FL_KEEP_SIZE)
654 cred_t *cr = CRED();
655 flock64_t bf;
656 loff_t olen;
657 fstrans_cookie_t cookie;
658
659 if (mode != (FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
660 return (error);
661 and the VFS too, http://lxr.free-electrons.com/source/fs/open.c?v=4.6#L244: 244 /* Punch hole must have keep size set */
245 if ((mode & FALLOC_FL_PUNCH_HOLE) &&
246 !(mode & FALLOC_FL_KEEP_SIZE))
247 return -EOPNOTSUPP; |
|
@loli10K @bunder2015 The man page also confirms the need to have |
|
I'll take another look, thanks for the tips. Been busy since Monday, I'll try to get things updated tonight or tomorrow. |
|
Quick update, rebased on master, fixed fio, fixed file_write, and I think I fixed mkholes, although it got killed by the buildbot last night. Still need to update the embedded_blocks test to check for fallocate support. |
|
|
||
| buf = (char *)umem_alloc(readlen, UMEM_NOFAIL); | ||
| vbuf = (char *)umem_zalloc(readlen, UMEM_NOFAIL); | ||
| while (bytes_read < len) { |
There was a problem hiding this comment.
fallocate(fd, FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE, off, len) on an empty file won't extend it from off of len bytes because of FALLOC_FL_KEEP_SIZE: bytes_read will always be 0 and thus < len, this will cause an infinite loop here and is the reason the BB is hitting the 10m limit on send-c_embedded_blocks.
behlendorf
changed the title
module/zfs/lz4.c
Outdated
| @@ -87,7 +87,7 @@ lz4_decompress_zfs(void *s_start, void *d_start, size_t s_len, | |||
|
|
|||
| /* | |||
| * Returns 0 on success (decompression function returned non-negative) | |||
| * and non-zero on failure (decompression function returned negative. | |||
| * and non-zero on failure (decompression function returned negative.) | |||
There was a problem hiding this comment.
nit: the . should be after the parenthesis, not before.
There was a problem hiding this comment.
Oops, thanks.
PS: I got my buildbot running, so I'm hoping to have some of the last few bugs ironed out soon.
| shift | ||
|
|
||
| [[ -f $file ]] || log_fail "Couldn't find file: $file" | ||
| typeset flags=$(cat $file | zstreamdump | awk '/features =/ {print $3}') |
There was a problem hiding this comment.
I assume you disabled "send-cpL_varied_recsize" because you're getting "arithmetic syntax error" from ksh.
This seems to happen when we have multiple BEGIN records and the flags variable becomes a multiline string:
root@debian-8-zfs:~# zfs create $POOLNAME/fs
root@debian-8-zfs:~# zfs create $POOLNAME/fs/sub1
root@debian-8-zfs:~# zfs create $POOLNAME/fs/sub2
root@debian-8-zfs:~# zfs snap -r $POOLNAME/fs@snap1
root@debian-8-zfs:~# zfs send -cpLRDe $POOLNAME/fs@snap1 > $TMPDIR/zstream.dat
root@debian-8-zfs:~# #
root@debian-8-zfs:~# cat $TMPDIR/zstream.dat | zstreamdump | awk '/features =/ {print $3}'
7
430007
430007
430007
root@debian-8-zfs:~# ksh
# flags='7
> 430007
> 430007
> 430007'
#
# printf "%x" $((0x$flags & 0x430007))
ksh: 0x7
430007
430007
430007 & 0x430007: arithmetic syntax error
#
mdb seems to consider only the ending line:
[root@52-54-00-d3-7a-f3 ~]# echo -e "0\n0\n4 & 4 = X" | mdb
4
[root@52-54-00-d3-7a-f3 ~]# echo -e "0\n4\n0 & 4 = X" | mdb
0
[root@52-54-00-d3-7a-f3 ~]# echo -e "4\n0\n0 & 4 = X" | mdb
0
[root@52-54-00-d3-7a-f3 ~]#
We should be able to "fix" the issue like this:
typeset flags=$(cat $file | zstreamdump | awk '/features =/ {features = $3} END {print features}')There was a problem hiding this comment.
It was only intended to be temporary while I ironed out other test issues, but I'll give that a try, thanks.
|
Ouch. send-cpL_varied_recsize gave me this during a zfs receive...
|
|
@bunder2015 that seems to be unrelated (well, at least not caused by) the code in this PR, i can reproduce the same issue with the very first commit that introduced compressed zfs send/receive 2aa3438. This also happens on Illumos. More info from This needs more debugging. Let's keep |
|
The failures hear are likely related to #6003 which @don-brady just opened a PR for. |
|
I can still hit the same VERIFY running 5800a5b: Reproducer here: https://gist.github.com/loli10K/5dc583110136d1d4138706555ad1f8a5. |
|
@loli10K @bunder2015 thanks for the reproducer! The issue is caused by an incorrect check in |
|
@behlendorf nice, this seems to fix it from my limited testing. I thought this was an issue on the receiving side, the code seems to put a lot of trust (maybe too much) on the input data. |
|
@loli10K yes it does, the recv side could certainly benefit from some additional sanity checks and hardening. |
|
@bunder2015 feel free to remove the |
|
I tried running it through the testbots, but without those 2 patches they all failed. |
...nd / 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: bunder2015 <[email protected]> Porting over some remaining changes from OpenZFS commit 5602294 OpenZFS-issue: https://www.illumos.org/issues/7628 OpenZFS-commit: openzfs/openzfs@5602294
Resolve slight differences in the do_dump function between Linux and OpenZFS. Signed-off-by: Brian Behlendorf <[email protected]>
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]> OpenZFS-issue: https://www.illumos.org/issues/7252 OpenZFS-commit: openzfs/openzfs@5602294f Porting Notes: most of 7252 was already picked up during ABD work. This commit represents the gap from the final commit to openzfs. Signed-off-by: Don Brady <[email protected]>
|
Replaced by #6067. @bunder2015 I hope you don't mind but I've taken your work and @don-brady's from #6003 and used them as the basis for #6067. This updated PR is passing all the new tests after a few minor modifications to the test scripts. It would be great if you could review it. |
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: bunder2015 [email protected]
Porting over some remaining changes from OpenZFS commit 5602294
OpenZFS-issue: https://www.illumos.org/issues/7628
OpenZFS-commit: openzfs/openzfs@5602294
Motivation and Context
Thought this would be an easy first attempt at porting, found it on the tracking page.
How Has This Been Tested?
Please be kind to me buildbots and reviewers. 😃
edit: I'll try to get a personal build box VM going this week, sorry for the buildbot noise.
Types of changes
Checklist:
Signed-off-by.