Relax (ref)reservation constraints on ZVOLs #6610

loli10K · 2017-09-06T20:40:41Z

Description

This change allow refreservation to be set larger than the current ZVOL size: this should be safe as we normally set refreservation > volsize at ZVOL creation time when we account for metadata.

Motivation and Context

Fix #2468

How Has This Been Tested?

root@linux:/usr/src/zfs# # setup
root@linux:/usr/src/zfs# POOLNAME='testpool'
root@linux:/usr/src/zfs# if is_linux; then
>    TMPDIR='/dev/shm'
>    mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
>    zpool destroy $POOLNAME
>    fallocate -l 128m $TMPDIR/zpool.dat
>    zpool create $POOLNAME $TMPDIR/zpool.dat
> else
>    TMPDIR='/tmp'
>    zpool destroy $POOLNAME
>    mkfile 128m $TMPDIR/zpool.dat
>    zpool create $POOLNAME $TMPDIR/zpool.dat
> fi
cannot open 'testpool': no such pool
root@linux:/usr/src/zfs# #
root@linux:/usr/src/zfs# zfs create $POOLNAME/fs
root@linux:/usr/src/zfs# zfs create -V 10M $POOLNAME/zvol
root@linux:/usr/src/zfs# value=`zfs get -p -o value -H refreservation $POOLNAME/zvol`
root@linux:/usr/src/zfs# zfs set refreservation=none $POOLNAME/zvol
root@linux:/usr/src/zfs# zfs set refreservation=$value $POOLNAME/zvol

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.

richardelling

KISS

richardelling · 2017-09-06T21:59:06Z

lib/libzfs/libzfs_dataset.c

@@ -1459,7 +1459,6 @@ zfs_valid_proplist(libzfs_handle_t *hdl, zfs_type_t type, nvlist_t *nvl,

 			switch (prop) {
 			case ZFS_PROP_RESERVATION:


This whole case should go, reservations make even less sense than refreservations here.

richardelling · 2017-09-06T22:01:23Z

tests/runfiles/linux.run

@@ -151,7 +151,8 @@ tests = ['zfs_rename_001_pos', 'zfs_rename_002_pos', 'zfs_rename_003_pos',
    'zfs_rename_to_encrypted']

 [tests/functional/cli_root/zfs_reservation]
-tests = ['zfs_reservation_001_pos', 'zfs_reservation_002_pos']
+tests = ['zfs_reservation_001_pos', 'zfs_reservation_002_pos',


No need to add a new test file.
In refreserv_005_pos.ksh remove the tests for "# Verify it is affected by volsize" and if you want to add a bigger than test, do it there.

Is refreserv_005_pos.ksh even working properly? I don't understand why it tries to zfs set refreservation=- testpool/testfs/vol, does "-" have a different meaning on Illumos? On ZoL it will always fail with the following error: cannot set property for 'testpool/testfs/vol': bad numeric value '-'

source:

log_must zfs create -V 10M $vol # Verify the parent filesystem does not affect volume log_must zfs set quota=25M $fs log_must zfs set refreservation=10M $vol avail=$(get_prop mountpoint $vol) log_mustnot zfs set refreservation=$avail $vol

log:

ASSERTION: Volume (ref)reservation is not limited by volsize SUCCESS: zfs create -V 10M testpool/testfs/vol SUCCESS: zfs set quota=25M testpool/testfs SUCCESS: zfs set refreservation=10M testpool/testfs/vol SUCCESS: zfs set refreservation=- testpool/testfs/vol exited 1

great question! '-' is the response for "not set" and mountpoint is not appropriate for size or volumes. Methinks this should be more like:

avail=$(get_prop available $vol)

thus avail is the total space available in the pool, which we'll presume is always > the quota on the file system. In reality we just need to test if the refreservation is > the quota, which we set previously. A lazy way would be:

log_must zfs set quota=25M $fs log_must zfs set refreservation=26M $vol

but this test is kinda silly, too and knowing the code, this test should only fail if we don't have the available space (which quota doesn't need to know)

methinks a simple test to set refreservation less than and greater than the initial estimate thusly:

initial=$(zfs get -p -H -o value refreservation $vol) log_must zfs set refreservation=$((initial + 1)) $vol log_must zfs set refreservation=$((initial - 1)) $vol

NB, get_prop returns the pretty value, not easy to do math against

behlendorf

Looks good. I was going to suggest updating the zfs.8 man page, but after reading the relevant sections I see that they make no mention of this specific case. Which is fine.

@richardelling could you please review and updated patch again and go ahead approve it if you're happy with it.

behlendorf · 2017-09-11T19:27:42Z

tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_remount.ksh

@@ -44,7 +44,7 @@ verify_runnable "both"

 function cleanup
 {
-	log_must zpool export $TESTPOOL
+	log_must_busy zpool export $TESTPOOL


Did you mean to include this in the PR? It looks like a fix for the occasional zfs_mount_remount failures we see under Fedora 26 (which I'm all for fixing).

Yes: i've seen it failing quite a lot on f26 and that seems to fix it. I tried to sneak it in even if it has nothing to do with the main issue.

Since now we have other recent issues reported on the ZTS (#6627) i'm ok with dropping this here and push it in a "ZTS-only fixes" PR.

Got it. Yeah, let's drop it from this PR and make the fix in the context of #6627.

This change allow (ref)reservation to be set larger than the current ZVOL size: this is safe as we normally set refreservation > volsize at ZVOL creation time when we account for metadata. Signed-off-by: loli10K <ezomori.nozomu@gmail.com>

richardelling · 2017-09-12T15:35:58Z

Looks good, thanks!

This change allow (ref)reservation to be set larger than the current ZVOL size: this is safe as we normally set refreservation > volsize at ZVOL creation time when we account for metadata. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed by: Richard Elling <Richard.Elling@RichardElling.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#2468 Closes openzfs#6610

richardelling reviewed Sep 6, 2017

View reviewed changes

loli10K force-pushed the issue-2468 branch from 1053fe7 to 8fb772e Compare September 7, 2017 20:21

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

loli10K force-pushed the issue-2468 branch 2 times, most recently from 8ff5933 to 502fed8 Compare September 10, 2017 14:18

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

loli10K changed the title ~~Relax refreservation constraints on ZVOLs~~ Relax (ref)reservation constraints on ZVOLs Sep 10, 2017

behlendorf approved these changes Sep 11, 2017

View reviewed changes

behlendorf requested a review from ahrens September 11, 2017 19:28

Relax (ref)reservation constraints on ZVOLs

95c7899

This change allow (ref)reservation to be set larger than the current ZVOL size: this is safe as we normally set refreservation > volsize at ZVOL creation time when we account for metadata. Signed-off-by: loli10K <ezomori.nozomu@gmail.com>

loli10K force-pushed the issue-2468 branch from 502fed8 to 95c7899 Compare September 11, 2017 19:57

ahrens approved these changes Sep 11, 2017

View reviewed changes

gmelikov approved these changes Sep 12, 2017

View reviewed changes

behlendorf merged commit ded8f06 into openzfs:master Sep 12, 2017

loli10K deleted the issue-2468 branch September 17, 2017 10:37

loli10K restored the issue-2468 branch November 1, 2018 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax (ref)reservation constraints on ZVOLs #6610

Relax (ref)reservation constraints on ZVOLs #6610

loli10K commented Sep 6, 2017

richardelling left a comment

richardelling Sep 6, 2017

richardelling Sep 6, 2017

loli10K Sep 7, 2017

richardelling Sep 7, 2017

behlendorf left a comment

behlendorf Sep 11, 2017

loli10K Sep 11, 2017

behlendorf Sep 11, 2017

richardelling commented Sep 12, 2017

		@@ -1459,7 +1459,6 @@ zfs_valid_proplist(libzfs_handle_t hdl, zfs_type_t type, nvlist_t nvl,

		switch (prop) {
		case ZFS_PROP_RESERVATION:

Relax (ref)reservation constraints on ZVOLs #6610

Relax (ref)reservation constraints on ZVOLs #6610

Conversation

loli10K commented Sep 6, 2017

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

richardelling left a comment

Choose a reason for hiding this comment

richardelling Sep 6, 2017

Choose a reason for hiding this comment

richardelling Sep 6, 2017

Choose a reason for hiding this comment

loli10K Sep 7, 2017

Choose a reason for hiding this comment

richardelling Sep 7, 2017

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Sep 11, 2017

Choose a reason for hiding this comment

loli10K Sep 11, 2017

Choose a reason for hiding this comment

behlendorf Sep 11, 2017

Choose a reason for hiding this comment

richardelling commented Sep 12, 2017