-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
zfs: add option for forcible unmounting dataset while receiving snapshot. #9904
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9904 +/- ##
==========================================
+ Coverage 79.56% 79.61% +0.05%
==========================================
Files 384 384
Lines 121788 121793 +5
==========================================
+ Hits 96900 96968 +68
+ Misses 24888 24825 -63
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed By: Allan Jude allanjude@freebsd.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR. I'd suggest we add a test case for this, you've already almost written in the PR comments. The only slightly tricky bit is that we should expect the test to fail on Linux and pass on FreeBSD. See the inline comment about this.
@behlendorf That's very interesting. |
That's right. I thought we had updated the man page to mention this, but looking at it now that's clearly not the case. It would be great if you could update it in this PR. |
Ok will do, thanks! |
a8b7f6e
to
54a60ec
Compare
force unmount on Linux is useful only for NFS, as i understand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a couple minor nits.
# STRATEGY: | ||
# 1. Create snapshot of file system | ||
# 2. Make a zfs filesystem mountpoint busy | ||
# 3. Recife filesystem with force flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Recife -> Receive
|
||
for snap in $init_snap $rst_snap; do | ||
snapexists $snap && \ | ||
log_must zfs destroy -f $snap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use destroy_snapshot
here.
done | ||
|
||
datasetexists $rst_root && \ | ||
log_must zfs destroy -Rf $rst_root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And destroy_dataset
here.
tests/zfs-tests/tests/functional/cli_root/zfs_receive/zfs_receive_016_pos.ksh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, zfs_recieve tests passed on FreeBSD with the commits cherry-picked on top of ZoF.
for file in $full_bkup | ||
do | ||
[[ -e $file ]] && \ | ||
log_must rm -f $file | ||
done | ||
|
||
[[ -d $TESTDIR1 ]] && \ | ||
log_must rm -rf $TESTDIR1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm -rf $full_bkup $TESTDIR1
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hym I'm not sure. The purpose is not to know what exactly went wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If something fails in cleanup we still want to finish cleaning up. There will be error messages in the logs to know what went wrong.
The purpose of cleanup is to put the system back the way it was when the test started. If there is something that should be tested that is relevant to the test, it can happen in the test rather than in cleanup. Cleanup should make a best effort to clean up as much as possible. It is potentially being invoked halfway through the test because something already went wrong.
…hot. Currently when the dataset is in use we can't receive snapshot. zfs send test/1@asd | zfs recv -FM test/2 cannot unmount '/test/2': Device busy The same goes for the Linux version: oshogbo@u-wing:/test$ sudo sudo zfs send test/1@b | sudo zfs recv -F test/2 umount: /test/2: target is busy. cannot unmount '/test/2': umount failed oshogbo@u-wing:/test$ uname -a Linux u-wing 4.18.0-25-generic openzfs#26-Ubuntu SMP Mon Jun 24 09:32:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux This commits add option 'M' which forcible unmounting the dataset. Thanks to to that we can enforce receiving snapshot in single step. Discussed with: pjd Reviewed by: AllanJude (FreeBSD version) FreeBSD review: https://reviews.freebsd.org/D22306 Signed-off-by: Mariusz Zaborski <oshogbo@vexillium.org>
As pointed out by behlendorf, Linux does not support forcible unmount. Document this behavior. Signed-off-by: Mariusz Zaborski <oshogbo@vexillium.org>
Signed-off-by: Mariusz Zaborski <oshogbo@vexillium.org>
Codecov Report
@@ Coverage Diff @@
## master #9904 +/- ##
========================================
- Coverage 79% 79% -<1%
========================================
Files 385 385
Lines 122389 122394 +5
========================================
- Hits 97266 97168 -98
- Misses 25123 25226 +103
Continue to review full report at Codecov.
|
Sorry about the delay. This looks ready to go. The only minor concern raised is the cleanup in the test case which could result in files being left around on error. Which is unlikely at best, @oshogbo if you're happy with the current version of the PR please post as much and I'll get this merged. |
I'm happy to go. I fixed (I think) everything you mention. I'm not good in the zfs test framework so I'm not sure if something else still there. |
Sounds good. I've gone ahead and merged this PR. If we encounter any issues with the new tests we can tackle them as a followup PR, though I don't expect any. |
Currently when the dataset is in use we can't receive snapshots. zfs send test/1@asd | zfs recv -FM test/2 cannot unmount '/test/2': Device busy This commits add option 'M' which attempts to forcibly unmount the dataset. Thanks to this we can enforce receiving snapshots in a single step. Note that this functionality is not supported on Linux because the VFS will prevent active mounted filesystems from being unmounted, even with the force option. This is the intended VFS behavior. Test cases were added to verify the expected behavior based on the platform. Discussed-with: Pawel Jakub Dawidek <pjd@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Allan Jude <allanjude@freebsd.org> External-issue: https://reviews.freebsd.org/D22306 Closes openzfs#9904
Motivation and Context
Currently on FreeBSD when the dataset is in use we can't receive snapshot.
The same goes for the Linux version:
Description
This commits add option 'M' which forcible unmounting the dataset.
Thanks to to that we can enforce receiving snapshot in single step.
Discussed with: pjd
Reviowed by: AllanJude (FreeBSD version), bcr (man page)
FreeBSD review: https://reviews.freebsd.org/D22306
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by
.