Add a sysctl to allow ZFS pools backed by zvols
ClosedPublic
Actions

Authored by asomers on Jan 19 2016, 11:54 PM.

Details

Reviewers

delphij
se
• lidl

Commits

rS295045: Add a sysctl to allow ZFS pools backed by zvols

Summary

Change 294329 removed the ability to build ZFS pools that are backed by zvols,
because having that ability (even if it's not used) leads to deadlocks. By
popular demand, I'm adding an off-by-default sysctl to reenable that ability.

Test Plan

Create a mirrored pool named foo
Create a pool named bar, and create a zvol on it
Remove one of foo's disks
In one terminal, do "zpool status foo" in a while loop
In another terminal, do "zpool online foo <removed disk's guid>" in a while loop
Verify that no deadlocks result after a few minutes, then kill both loops

Try to create a zpool on bar's zvol. It should fail.
Do "sysctl vfs.zfs.vol.recursive=1".
Try to create a zpool on bar's zvol. It should succeed.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

asomers updated this revision to Diff 12484.Jan 19 2016, 11:54 PM

asomers retitled this revision from to Add a sysctl to allow ZFS pools backed by zvols.

asomers updated this object.

asomers edited the test plan for this revision. (Show Details)

asomers added reviewers: delphij, • lidl, se.

asomers added subscribers: bdrewery, smh, lifanov_mail.lifanov.com.

Herald added a subscriber: imp. · View Herald TranscriptJan 19 2016, 11:54 PM

Just some style nits.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zvol.c
138 ↗	(On Diff #12484)	style(9) line length
1119 ↗	(On Diff #12484)	style(9) define at the top of method and just init here.

asomers added inline comments.Jan 20 2016, 12:22 AM

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zvol.c
138 ↗	(On Diff #12484)	I'll fix it.
1119 ↗	(On Diff #12484)	locked is only used when #ifndef illumos. Therefore, it must be declared in a #ifndef illumos section. It's current location is the highest point in the function when illumos is not defined.

style(9) fixed suggested by smh

I built a kernel with this patch in it.

I ran my script without setting the sysctl. It failed, as expected.
I set the sysctl, and re-ran my script. It succeeded.

I am satisfied with this solution.

This revision is now accepted and ready to land.Jan 21 2016, 2:17 AM

avg added a subscriber: avg.Jan 22 2016, 7:50 AM

avg added inline comments.

UPDATING
36 ↗	(On Diff #12486)	I would prefer that "never really worked" statement was toned down a little bit.
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zvol.c
138 ↗	(On Diff #12486)	I am a little bit concerned about what happens if this knob is switched off at run-time with some zpools-on-zvols already active.

asomers added inline comments.Jan 25 2016, 5:38 PM

UPDATING
36 ↗	(On Diff #12486)	How about "That feature has never worked safely; it's always been prone to deadlocks."?
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zvol.c
138 ↗	(On Diff #12486)	Nothing will happen, since the knob only affects tasting. If you have an active pool on zvols and you switch the knob off, I/O to the pool will still work. You can even export the pool. But then you won't be able to import it again, and you won't be able to do operations like offline a zvol vdev and online it, or replace a zvol vdev with another zvol.