Bug 225960
| Summary: | zfs: g_access leak when unmounting UFS on a zvol | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Alan Somers <asomers> | ||||
| Component: | kern | Assignee: | Andriy Gapon <avg> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | avg, daveb, fs | ||||
| Priority: | --- | ||||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
|
Description
Alan Somers
2018-02-16 20:35:13 UTC
A commit references this bug: Author: asomers Date: Fri Feb 16 20:40:05 UTC 2018 New revision: 329408 URL: https://svnweb.freebsd.org/changeset/base/329408 Log: Skip zfs_copies_006_pos It frequently triggers a g_access leak, creating a non-destroyable pool PR: 225960 Sponsored by: Spectra Logic Corp Changes: projects/zfsd/head/tests/sys/cddl/zfs/tests/cli_root/zfs_copies/zfs_copies_test.sh I've just seen this. I tried to zfs destroy a volume with a mounted (ro) UFS filesystem on it. The destroy failed with EBUSY, as expected. But after I unmounted the filesystem the destroy would still fail in the same way. Could be a g_label problem though:
<geom id="0xfffff80023b33200">
<class ref="0xffffffff80d86a90"/>
<name>zvol/pond/zvol2s1</name>
<rank>3</rank>
<config>
</config>
<consumer id="0xfffff8002215e300">
<geom ref="0xfffff80023b33200"/>
<provider ref="0xfffff8002291ad00"/>
<mode>r0w0e2</mode>
<config>
</config>
</consumer>
<provider id="0xfffff80023b33100">
<geom ref="0xfffff80023b33200"/>
<mode>r0w0e0</mode>
<name>ufsid/4e6d250390a79ac0</name>
<mediasize>1073705472</mediasize>
<sectorsize>512</sectorsize>
<stripesize>4096</stripesize>
<stripeoffset>3584</stripeoffset>
<config>
<index>0</index>
<length>1073705472</length>
<seclength>2097081</seclength>
<offset>0</offset>
<secoffset>0</secoffset>
</config>
</provider>
</geom>
I don't think that the consumer should have r0w0e2 mode while the provider has r0w0e0. The class is LABEL and apparently it's the ufsid variety.
Yeah, and kern.geom.label.ufsid.enable=0 makes the problem go away. I now think that this could be a / the deep problem in GEOM.
The problem is that g_access() must be called with the GEOM topology lock held.
And that gives a false impression that the lock is indeed held across the call.
But this isn't true because many classes, ZVOL being one of the many, need to perform I/O in their access method. So, they must drop and pick up the topology lock.
That, of course, can break many assumptions.
Specifically, looking at g_slice_access() we can see that the code assumes that all invocations are serialized. Indeed, if another call to g_slice_access() is permitted while the consumer's access bits are not updated yet, then the following condition can be true multiple times:
/* On first open, grab an extra "exclusive" bit */
if (cp->acr == 0 && cp->acw == 0 && cp->ace == 0)
de++;
And if that ever happens and we have extra grabs on cp->ace, then the following condition will never be true (because cp->ace + de > 1):
/* ... and let go of it on last close */
if ((cp->acr + dr) == 0 && (cp->acw + dw) == 0 && (cp->ace + de) == 1)
de--;
Created attachment 190802 [details]
possible solution
I have come up with a possible solution, haven't tested it yet.
Perhaps there is a better way to fix the issue in GEOM core code, but I couldn'r think of one.
Your patch works for me. I created a review request for this https://reviews.freebsd.org/D14458 Maybe there is a smarter way to tackle the problem, but I couldn't come up with anything else... (In reply to Andriy Gapon from comment #8) FWIW, I have never been able to reproduce the original problem. avg, I made a mistake with my reproduction instructions. You should run zfs_copies_006_pos, not 001_pos. And zfsd must be running at the same time. sudo service zfsd onestart cd /usr/tests/sys/cddl/zfs/tests/cli_root/zfs_copies sudo kyua debug zfs_copies_test:zfs_copies_006_pos (In reply to Alan Somers from comment #10) I figured out the correct test name from your commit to disable it, so I was already using it. I wasn't running zfsd, I am doing doing that now. But still can't reproduce. Maybe because of my GEOM configuration where I use multiple slices of a single disk to emulate multiple disks. (In reply to Andriy Gapon from comment #11) Do you have Alan's latest zfsd changes ? this is what exasperated the condition. (In reply to Dave Baukus from comment #12) Specifically, the problem should be easiest to reproduce with zfsd built from at least revision 329284 but less than 329344. A more generic patch for the problem: https://reviews.freebsd.org/D14533 A commit references this bug: Author: avg Date: Thu Mar 15 09:16:11 UTC 2018 New revision: 330977 URL: https://svnweb.freebsd.org/changeset/base/330977 Log: g_access: deal with races created by geoms that drop the topology lock The problem is that g_access() must be called with the GEOM topology lock held. And that gives a false impression that the lock is indeed held across the call. But this isn't always true because many classes, ZVOL being one of the many, need to drop the lock. It's either to perform an I/O on the first open or to acquire a different lock (like in g_mirror_access). That, of course, can break many assumptions. For example, g_slice_access() adds an extra exclusive count on the first open. As described above, an underlying geom may drop the topology lock and that would open a race with another thread that would also request another extra exclusive count. In general, two consumers may be granted incompatible accesses. To avoid this problem the code is changed to mark a geom with special flag before calling its access method and clear the flag afterwards. If another thread sees that flag, then it means that the topology lock has been dropped (either by the geom in question or downstream from it), so it is not safe to make another access call. So, the second thread would use g_topology_sleep() to wait until the flag is cleared and only then would it proceed with the access. Also see http://docs.freebsd.org/cgi/mid.cgi?809d9254-ee56-59d8-69a4-08838e985cea PR: 225960 Reported by: asomers Reviewed by: markj, mav MFC after: 3 weeks Differential Revision: https://reviews.freebsd.org/D14533 Changes: head/sys/geom/geom.h head/sys/geom/geom_subr.c A commit references this bug: Author: avg Date: Thu Mar 15 09:28:11 UTC 2018 New revision: 330979 URL: https://svnweb.freebsd.org/changeset/base/330979 Log: re-enable zfs_copies_006_pos test after a fix in r330977 The test was disabled in r329408. PR: 225960 Changes: head/tests/sys/cddl/zfs/tests/cli_root/zfs_copies/zfs_copies_test.sh A commit references this bug: Author: avg Date: Fri Apr 6 12:13:32 UTC 2018 New revision: 332095 URL: https://svnweb.freebsd.org/changeset/base/332095 Log: MFC r330977: g_access: deal with races created by geoms that drop the topology lock PR: 225960 Changes: _U stable/11/ stable/11/sys/geom/geom.h stable/11/sys/geom/geom_subr.c A commit references this bug: Author: avg Date: Fri Apr 6 12:24:00 UTC 2018 New revision: 332096 URL: https://svnweb.freebsd.org/changeset/base/332096 Log: MFC r330977: g_access: deal with races created by geoms that drop the topology lock PR: 225960 Changes: _U stable/10/ stable/10/sys/geom/geom.h stable/10/sys/geom/geom_subr.c |