Prevent g_access calls to bad multipath members
ClosedPublic
Actions

Authored by smh on Dec 7 2015, 4:18 AM.

Details

Reviewers

imp
mav

Commits

rS292289: Prevent g_access calls to bad multipath members

Summary

When a multipath member is orphaned its access members are zeroed before its removed if the marked for wither, so prevent any future calls to g_access on such members.

This prevents a panic on debug kernels which validates the resultant values aren't negative.

Also fix the fail loops use of break instead of continue.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

smh updated this revision to Diff 10833.Dec 7 2015, 4:18 AM

smh retitled this revision from to Prevent g_access calls to bad multipath members.

smh updated this object.

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

Herald added a subscriber: imp. · View Herald TranscriptDec 7 2015, 4:18 AM

smh added a reviewer: mav.Dec 7 2015, 4:20 AM

This looks generally correct. Minor formatting nit is all.

sys/geom/multipath/g_multipath.c
470 ↗	(On Diff #10833)	We've generally not started using C++ comments wholesale.

This revision is now accepted and ready to land.Dec 7 2015, 4:57 AM

smh marked an inline comment as done.Dec 7 2015, 9:25 AM

Style nit fix

This revision now requires review to proceed.Dec 7 2015, 9:27 AM

I haven't looked into this code for quite some time, but I doubt this patch is correct.

sys/geom/multipath/g_multipath.c
474 ↗	(On Diff #10869)	I am not sure that check for MP_BAD is right here. MP_BAD includes several flags, including MP_FAIL, which has nothing to do about access counts.
497 ↗	(On Diff #10869)	In original version "break" here exited loop at the right point if original g_access() failed on this consumer. Now attempt to fix one bug introduced another.

This revision now requires changes to proceed.Dec 7 2015, 6:26 PM

smh added inline comments.Dec 7 2015, 7:48 PM

sys/geom/multipath/g_multipath.c
474 ↗	(On Diff #10869)	I did think that might be the case, which is why I put in for review, so thanks for confirming. Even checking for MP_LOST, which can result in g_mpd calls and hence can lead to g_access reset, its still not guaranteed to have run. Given that maybe adding a new state bit that set if the reset has occurred is the a way forward as then it could be explicitly checked?
497 ↗	(On Diff #10869)	Correct it needs to abort as the previous loop did, will fix.

mav added inline comments.Dec 7 2015, 7:55 PM

sys/geom/multipath/g_multipath.c
474 ↗	(On Diff #10869)	MP_POSTED looks somewhat related. Otherwise -- may be indeed a new flag.

Introduce MP_WITHER which is used to flag a device thats had its access counters reset but required withering hence hasn't yet been destroyed.

Tests to confirm correct operation still pending...

smh marked 5 inline comments as done.Dec 7 2015, 8:25 PM

smh added inline comments.

sys/geom/multipath/g_multipath.c
476 ↗	(On Diff #10890)	It is related but still not enough unfortunately so I've introduced MP_WITHER.

Are you happy with this now mav?

Yes, now it seems fine to me.

This revision is now accepted and ready to land.Dec 15 2015, 8:57 PM

Closed by commit rS292289: Prevent g_access calls to bad multipath members (authored by smh). · Explain WhyDec 15 2015, 9:11 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

head/

sys/

geom/

multipath/

g_multipath.c

20 lines

Diff 11330

View Options

head/sys/geom/multipath/g_multipath.c

Prevent g_access calls to bad multipath membersClosedPublicActions