Hi Ben, On Wed, 2018-04-04 at 10:04 +0200, Martin Wilck wrote: > On Tue, 2018-04-03 at 16:29 -0500, Benjamin Marzinski wrote: > > > I believe that if TGPS is 0, the device will never be able to > > > support > > > ALUA. The kernel also looks at the TPGS bits and won't try ALUA > > > if > > > they > > > are unset. Once the device is configured and actual ALUA > > > RTPG/STPG > > > calls are performed, they may fail for a variety of temporary > > > reasons - > > > I wanted to avoid resetting the prio algorithm to "const" for > > > such > > > cases. That's my understanding, correct me if I'm wrong. > > > > Devices that were not correctly supporing ALUA returned > 0 for > > get_target_port_group_support, so detect_alua actually does all the > > work > > necessary to verify that it can get a priority. Without doing this, > > multiple deviecs that didn't support ALUA were being detected as > > supporting ALUA. > > So, detect_alua() tests TPGS *and* tries and actual alua call, and > sets > pp->tpgs to anything other than TPGS_NONE only if the latter is > successful. That's fine. My patch was looking at pp->tpgs, so it was > implicitly using this logic of detect_alua(). But does that guarantee > that future alua->getprio() calls will never fail at some later point > in time? > > Maybe I misunderstood your original proposition. What I'm saying is > that resetting the prio algorithm from "alua" to "const" because of > an > error code in get_prio() is wrong, because that error code may be > transient. > > If we give "hardware_handler" config options preference over ALUA > autodetection, and thus enforce hwhandler "1 alua" on such devices > that > have no ALUA support, domap() is guaranteed to fail, because the > kernel > refuses to set up a map with a given hwhandler if any device doesn't > support that handler. > > > By using retain_attached_hwhandler at all, we are implicitly > > requiring > > the scsi_dh_alua module to be loaded before devices with > > indeterminate > > configurations are discovered for them to work correctly. right? > > For > > instance, commit 715c48d93dd00930534ce6a55d0e3705466df5d6 did this > > for > > netapp devices, and that was in 2013. I don't see how this is > > different. > > You're right, we are "implicitly requiring" this sort-of, but we have > no code that enforces the early loading of the device handlers. We > should be shipping a modules-load.d file, or a modprobe.d softdep, or > something similar that would enforce this setting if we _really_ > depend > on it. "Implicit requirements" are bad. We should either make the > requirement explicit, or not hard-depend on it. So far I was thinking > the latter. After all, SCSI device-handler support is configurable in > the kernel. I'm unsure what to do. Do you still reject my patch? Or have you been convinced by Hannes and my arguments? Or are you requesting changes? If yes, what? Regards, Martin -- Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
