[dm-devel] [PATCH v3 10/11] libmultipath: don't try to set hwhandler if it is retained

[Martin Wilck]

On Thu, 2017-06-22 at 08:21 +0200, Hannes Reinecke wrote:
> On 06/21/2017 05:06 PM, Martin Wilck wrote:
> > Setting a device handler only works if retain_attached_hw_handler
> > is 'no', or if the kernel didn't auto-assign a handler. If this
> > is not the case, don't even attempt to set a different handler.
> > 
> > This requires reading the sysfs "dh_state" path attribute.
> > For internal consistency, this attribute must be updated after
> > domap().
> > 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  libmultipath/configure.c | 52
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> >  libmultipath/discovery.c |  4 ++++
> >  libmultipath/propsel.c   | 15 ++++++++++++++
> >  libmultipath/structs.h   |  2 ++
> >  4 files changed, 72 insertions(+), 1 deletion(-)
> > 
> > [...]
>
> Hmm.
> 
> Not sure if I fully agree with this.
> I do see the need to read 'dh_state' from pathinfo(), just to figure
> out
> if an hardware handler is already loaded.
> 
> But once select_hwhandler is done it's quite pointless to update the
> dh_state; what exactly _would_ be the error action in this case?
> Plus the code detects the failure, but then doesn't do anything with
> it...

My concern was that multipathd might carry along wrong state and
possibly print it in log messages, irritating users. It's true, there
is no reasonable error action, and it's quite a lot of code just for
this minor purpose.

> So, please, if you insist on checking dh_state please implement
> correct
> error action here, like updating the 'hwhandler' value to that found
> in
> dh_state or disabling the hardware handler if it's found to be
> detached.

If it's fine with you and other reviewers, I'll happily remove that
part of the patch, and just keep the part in select_hwhandler().

If we want proper error handling, we'd need to check that the handler
of the loaded map as well as the handlers of all paths are set to the
handler configured in multipathd. Unless I'm mistaken, it isn't
guaranteed that all paths will be using the same handler after a map is
set up.

Besides re-reading the dh_state of all paths, this check would also
require re-reading and disassembling the map, and no reasonable error
action is to be seen other then updating the internal state, lots of
code for almost nothing.

Cheers,
Martin

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)