New service that waits on zvol links to be created #8975

pzakha · 2019-07-01T16:47:26Z

This creates a new service, zfs-volume.service. When this service becomes active, it ensures that all the zvols of imported pools are ready to use and their symlinks are created under /dev. Note that this service doesn't actually do any modifications to the system, it only waits for resources to be ready before returning. Any service that performs operations on zvols at boot time would want to add this service as a dependency.

Motivation and Context

The rtslib-fb-targetctl service that comes with the python-rtslib-fb package provides a userland interface to the LIO framework, which can serve iSCSI LUNs from block devices on the system. ZFS volumes (zvols) can be used as backing block devices. However, there is currently no guarantee that the zvols are ready when that service runs at boot time. If that is the case, then rtslib-fb-targetctl will fail to create LUNs from those zvols. The new zfs-volume service can be added as a dependency to rtslib-fb-targetctl to avoid such situations.

Description

The original Delphix review is here: https://github.com/delphix/zfs/pull/56.
The idea is to list all the zvols and then wait for their links under /dev to be created.

How Has This Been Tested?

See https://github.com/delphix/zfs/pull/56 for original testing.
Made sure that the rtslib-fb-targetctl service doesn't report any errors at boot because of unexisting zvol links after adding a dependency on zfs-volume.service.
Tested the new script converted to POSIX sh:

Reduced the inner loop iterations to test that logic better:

Jun 28 22:11:57 localhost systemd[1]: Starting Wait for ZFS Volume (zvol) /dev links...
Jun 28 22:12:03 localhost zvol-wait[4899]: Testing 1003 zvol links
Jun 28 22:12:04 localhost zvol-wait[4899]: Still waiting on 120 zvol links ...
Jun 28 22:12:04 localhost zvol-wait[4899]: All zvol links are now present.
Jun 28 22:12:04 localhost systemd[1]: Started Wait for ZFS Volume (zvol) /dev links.

Simulated a zvol deletion while the service is running with additional test code:

-- Logs begin at Wed 2019-06-19 14:49:06 UTC, end at Fri 2019-06-28 22:19:04 UTC. --
Jun 28 22:17:59 localhost systemd[1]: Starting Wait for ZFS Volume (zvol) /dev links...
Jun 28 22:18:07 pzakha-lx-4.dcenter zvol-wait[3998]: Testing 1003 zvol links
Jun 28 22:18:07 pzakha-lx-4.dcenter zvol-wait[3998]: TEST CODE: delete domain0/VOLS/zvol333
Jun 28 22:18:13 pzakha-lx-4.dcenter zvol-wait[3998]: Still waiting on 1 zvol links ...
Jun 28 22:18:18 pzakha-lx-4.dcenter zvol-wait[3998]: Still waiting on 1 zvol links ...
Jun 28 22:18:18 pzakha-lx-4.dcenter zvol-wait[3998]: No progress since last loop.
Jun 28 22:18:18 pzakha-lx-4.dcenter zvol-wait[3998]: Checking if any zvols were deleted.
Jun 28 22:18:18 pzakha-lx-4.dcenter zvol-wait[3998]: 1 zvol(s) deleted.
Jun 28 22:18:18 pzakha-lx-4.dcenter zvol-wait[3998]: All zvol links are now present.
Jun 28 22:18:18 pzakha-lx-4.dcenter systemd[1]: Started Wait for ZFS Volume (zvol) /dev links.

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.

pzakha · 2019-07-01T16:47:52Z

cc: @behlendorf @grwilson @jgallag88 @prakashsurya

codecov · 2019-07-02T00:30:38Z

Codecov Report

Merging #8975 into master will decrease coverage by 11.16%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #8975       +/-   ##
===========================================
- Coverage    77.6%   66.44%   -11.17%     
===========================================
  Files         391      325       -66     
  Lines      120531   103708    -16823     
===========================================
- Hits        93544    68913    -24631     
- Misses      26987    34795     +7808

Flag	Coverage Δ
#kernel	`?`
#user	`66.44% <ø> (+0.19%)`	⬆️

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 8062b76...ea70da8. Read the comment docs.

debian/tree/zfsutils-linux/usr/lib/zfs-linux/zvol-wait

etc/systemd/system/Makefile.am

etc/systemd/system/zfs-volume.service.in

cmd/zfs_volume_wait/zfs_volume_wait

man/man1/zfs_volume_wait.1

cmd/Makefile.am

rlaager · 2019-07-09T07:13:55Z

Some random questions. Note that these really are questions, and I'm not implying that I'm in favor of them.

Should this be zfs-volume.target instead (or, more likely, should there be a zfs-volume.target that is after and/or requires a zfs-volume-wait.service)?
Did you consider a template unit that waits for one particular zvol?
Did you consider a generator (probably with a cache, like we use for filesystem mounts), possibly in combination with a template unit?

@aerusso @Fabian-Gruenbichler do you have any thoughts here?

Fabian-Gruenbichler · 2019-07-09T08:01:06Z

@rlaager I had many of the same thoughts/questions, but not yet found the time to write them up.

Especially in combination with encryption it would be nice to have individual units for zvols (that could then also be ordered after/depend on the resp. zfs-load-key units, if necessary).

I am also not sure whether we really want to tie in this new service(s)/target into the main zfs.target. IMHO it would make more sense for other services (like iSCSI targets, hypervisors that have guest volumes as zvols, ...) that actually depend on the links being there to explicitly pull it/them in. This would avoid artificially slowing down the default boot - there were no guarantees so far that all zvol links are available, so nobody should rely on this (yet).

aerusso · 2019-07-09T14:10:10Z

@behlendorf At the risk of seeing everything as a nail (because of the hammer that I've wielded), I agree that the granularity of having zfs-volume@.service looks appealing. You might also be able to use .path units instead of shellscript to check for zvol readiness.

I have not tested the below, but this is roughly what I'm thinking:

aerusso@ce2abd4 in https://github.com/aerusso/zfs/commits/pulls/systemd-zvols

behlendorf · 2019-07-09T20:41:57Z

the granularity of having zfs-volume@.service looks appealing. You might also be able to use .path units instead of shellscript to check for zvol readiness.

The granularity would be nice, but it would be nice to not have that additional requirement block this PR. I'm not familiar with .path units would this be something which could co-exist with the proposed approach? Similar to what's done with zfs-mount service and generator?

I do think we want to have a zfs-volume.target and have it require a zfs-volume-wait.service.

pzakha · 2019-07-09T22:05:38Z

Thanks for the feedback. I do think that having more granular units could be very useful in some cases, but even with the implementation stub from @aerusso we would probably need to spend much more time testing it and ironing out any corner cases. If others do not object, I'd prefer going forward with this approach for now since it does provide quite some value already. With zfs-volume.target in place, we can later have it target a more granular set of units.

pzakha · 2019-07-09T22:08:06Z

This would avoid artificially slowing down the default boot - there were no guarantees so far that all zvol links are available, so nobody should rely on this (yet).

For now I will not add any WantedBy for zfs-volume.target, so if no one needs it, it will not incur the extra zfs list call from zfs-volume-wait.service

pzakha · 2019-07-10T18:25:43Z

I have added the new zfs-volume.target unit and tested that it is not being run by default unless it is specifically Wanted or Required by another service.

etc/systemd/system/50-zfs.preset.in

loli10K · 2019-07-15T17:29:21Z

This implementation does not seem to handle volmode=none:

root@linux:/usr/src/zfs# zfs create -p $POOLNAME/backups/path/to/zvol -V 1G -s
root@linux:/usr/src/zfs# 
root@linux:/usr/src/zfs# time cmd/zvol_wait/zvol_wait
Testing 1 zvol links
All zvol links are now present.

real	0m0.008s
user	0m0.000s
sys	0m0.000s
root@linux:/usr/src/zfs# 
root@linux:/usr/src/zfs# zfs set volmode=none $POOLNAME/backups/path/to/zvol
root@linux:/usr/src/zfs# 
root@linux:/usr/src/zfs# time cmd/zvol_wait/zvol_wait
Testing 1 zvol links
Still waiting on 1 zvol links ...
No progress since last loop.
Checking if any zvols were deleted.

< more logging >

Still waiting on 1 zvol links ...
No progress since last loop.
Checking if any zvols were deleted.
Remaining zvols:
testpool/backups/path/to/zvol
Timed out waiting on zvol links

real	10m1.814s
user	0m0.052s
sys	0m0.296s
root@linux:/usr/src/zfs#

Systems with at least 1 "hidden" (volmode=none) ZVOL should not have to wait 10m, especially if the ZVOL is not expected to show up by user choice.

pzakha · 2019-07-15T17:36:25Z

This implementation does not seem to handle volmode=none

Thanks for the review. I was not aware of this property. I'll update the review to filter out zvols with volmode=none.

pzakha · 2019-07-16T19:18:18Z

Updated code with the following changes:

Account for volmode=none
Rename zfs-volume.target -> zfs-volumes.target
Added WantedBy=zfs.target to zfs-volumes.target so that the target is reached on boot when enabled.

behlendorf · 2019-07-16T19:37:15Z

Thanks for addressing those last few issues. I'll get this merged pending the final CI results.

behlendorf · 2019-07-17T01:25:30Z

@pzakha bad timing, I believe we had some zimport failures due to the unrelated log space map changes being integrated. Can you rebase this one last time on master.

The zfs-volume service scans existing zvols and waits for their links under /dev to be created. Any service that depends on zvol links to be there should add a dependency on zfs-volume.service. Signed-off-by: Pavel Zakharov <pzakharov@delphix.com>

The zfs-volume-wait.service scans existing zvols and waits for their links under /dev to be created. Any service that depends on zvol links to be there should add a dependency on zfs-volumes.target. By default, this target is not enabled. Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> Reviewed-by: Antonio Russo <antonio.e.russo@gmail.com> Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: John Gallagher <john.gallagher@delphix.com> Reviewed-by: George Wilson <gwilson@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Pavel Zakharov <pzakharov@delphix.com> Closes openzfs#8975

119: DLPX-64724 rtslib-fb-targetctl service should wait on domain0 zvol links before starting r=pzakha a=pzakha This leverages the functionality added in openzfs/zfs#8975. ## Testing ab-pre-push + migration testing: http://selfservice.jenkins.delphix.com/job/devops-gate/job/master/job/appliance-build-orchestrator-pre-push/1836/ Co-authored-by: Pavel Zakharov <pavel.zakharov@delphix.com>

The zfs-volume-wait.service scans existing zvols and waits for their links under /dev to be created. Any service that depends on zvol links to be there should add a dependency on zfs-volumes.target. By default, this target is not enabled. Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> Reviewed-by: Antonio Russo <antonio.e.russo@gmail.com> Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: John Gallagher <john.gallagher@delphix.com> Reviewed-by: George Wilson <gwilson@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Pavel Zakharov <pzakharov@delphix.com> Closes openzfs#8975

The zfs-volume-wait.service scans existing zvols and waits for their links under /dev to be created. Any service that depends on zvol links to be there should add a dependency on zfs-volumes.target. By default, this target is not enabled. Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> Reviewed-by: Antonio Russo <antonio.e.russo@gmail.com> Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: John Gallagher <john.gallagher@delphix.com> Reviewed-by: George Wilson <gwilson@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Pavel Zakharov <pzakharov@delphix.com> Closes #8975

behlendorf added the Status: Code Review Needed Ready for review and testing label Jul 2, 2019

behlendorf reviewed Jul 2, 2019

View reviewed changes

debian/tree/zfsutils-linux/usr/lib/zfs-linux/zvol-wait Outdated Show resolved Hide resolved

etc/systemd/system/Makefile.am Outdated Show resolved Hide resolved

etc/systemd/system/zfs-volume.service.in Outdated Show resolved Hide resolved

jgallag88 approved these changes Jul 8, 2019

View reviewed changes

cmd/zfs_volume_wait/zfs_volume_wait Outdated Show resolved Hide resolved

man/man1/zfs_volume_wait.1 Outdated Show resolved Hide resolved

behlendorf reviewed Jul 9, 2019

View reviewed changes

cmd/Makefile.am Outdated Show resolved Hide resolved

grwilson approved these changes Jul 9, 2019

View reviewed changes

aerusso mentioned this pull request Jul 11, 2019

systemd zvol target #9016

Open

12 tasks

jgallag88 approved these changes Jul 11, 2019

View reviewed changes

behlendorf reviewed Jul 13, 2019

View reviewed changes

etc/systemd/system/50-zfs.preset.in Show resolved Hide resolved

behlendorf approved these changes Jul 16, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 16, 2019

behlendorf merged commit 26b6047 into openzfs:master Jul 17, 2019

pzakha mentioned this pull request Aug 2, 2019

DLPX-64724 rtslib-fb-targetctl service should wait on domain0 zvol links before starting delphix/delphix-platform#119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New service that waits on zvol links to be created #8975

New service that waits on zvol links to be created #8975

pzakha commented Jul 1, 2019

pzakha commented Jul 1, 2019

codecov bot commented Jul 2, 2019 •

edited

Loading

rlaager commented Jul 9, 2019

Fabian-Gruenbichler commented Jul 9, 2019

aerusso commented Jul 9, 2019

behlendorf commented Jul 9, 2019 •

edited

Loading

pzakha commented Jul 9, 2019

pzakha commented Jul 9, 2019

pzakha commented Jul 10, 2019

loli10K commented Jul 15, 2019

pzakha commented Jul 15, 2019

pzakha commented Jul 16, 2019

behlendorf commented Jul 16, 2019

behlendorf commented Jul 17, 2019

New service that waits on zvol links to be created #8975

New service that waits on zvol links to be created #8975

Conversation

pzakha commented Jul 1, 2019

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

pzakha commented Jul 1, 2019

codecov bot commented Jul 2, 2019 • edited Loading

Codecov Report

rlaager commented Jul 9, 2019

Fabian-Gruenbichler commented Jul 9, 2019

aerusso commented Jul 9, 2019

behlendorf commented Jul 9, 2019 • edited Loading

pzakha commented Jul 9, 2019

pzakha commented Jul 9, 2019

pzakha commented Jul 10, 2019

loli10K commented Jul 15, 2019

pzakha commented Jul 15, 2019

pzakha commented Jul 16, 2019

behlendorf commented Jul 16, 2019

behlendorf commented Jul 17, 2019

codecov bot commented Jul 2, 2019 •

edited

Loading

behlendorf commented Jul 9, 2019 •

edited

Loading