I concur with the idea of combining both patches.
Chandra
On Tue, 2012-05-08 at 19:30 +0200, Hannes Reinecke wrote:
> On 05/08/2012 04:46 PM, Mike Snitzer wrote:
> > On Tue, May 08 2012 at 10:18am -0400,
> > Hannes Reinecke<hare@xxxxxxx> wrote:
> >
> >> This patch introduces a 'default_hw_handler' feature for
> >> dm-mpath. When specifying the feature 'default_hw_handler'
> >> dm-multipath will be using the currently attached hardware
> >> handler instead of trying to re-attach with the one
> >> specified during table creation.
> >> If no hardware handler is attached the specified hardware
> >> handler will be used.
> >>
> >> Signed-off-by: Hannes Reinecke<hare@xxxxxxx>
> >> ---
> >> drivers/md/dm-mpath.c | 25 ++++++++++++++++++++++---
> >> 1 files changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> >> index 6d3f2a8..c1ef41d 100644
> >> --- a/drivers/md/dm-mpath.c
> >> +++ b/drivers/md/dm-mpath.c
> >> @@ -57,6 +57,7 @@ struct priority_group {
> >> };
> >>
> >> #define FEATURE_NO_PARTITIONS 1
> >> +#define FEATURE_DEFAULT_HW_HANDLER 2
> >>
> >> /* Multipath context */
> >> struct multipath {
> >> @@ -589,14 +590,24 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
> >>
> >> if (m->hw_handler_name) {
> >> struct request_queue *q = bdev_get_queue(p->path.dev->bdev);
> >> + const char *hw_handler_name = m->hw_handler_name;
> >>
> >> - r = scsi_dh_attach(q, m->hw_handler_name);
> >> + if (m->features& FEATURE_DEFAULT_HW_HANDLER)
> >> + hw_handler_name = NULL;
> >> +
> >> + r = scsi_dh_attach(q, hw_handler_name);
> >> if (r == -EBUSY) {
> >> /*
> >> * Already attached to different hw_handler,
> >> * try to reattach with correct one.
> >> */
> >> scsi_dh_detach(q);
> >> + r = scsi_dh_attach(q, hw_handler_name);
> >> + } else if (r == -EINVAL) {
> >> + /*
> >> + * No hardware handler attached, use
> >> + * the specified one.
> >> + */
> >> r = scsi_dh_attach(q, m->hw_handler_name);
> >> }
> >
> > I like what you've done with the 'default_hw_handler' feature. But
> > you're not establishing m->hw_handler_name. As such the rest of the
> > dm-mpath.c code that keys off of m->hw_handler_name (e.g. reinstate_path,
> > pg_init_done, free_pgpaths) will not work.
> >
> Ah. True.
>
> > Would you be OK with a hybrid of both our approaches? Use your
> > 'default_hw_handler' feature and, rather than pass a NULL name to
> > scsi_dh_attach, use scsi_dh_attached_handler_name like I provided?
> >
> Yes, sure. That sounds like the best approach here.
>
> I'm not _that_ keen on my 'NULL' hw handler name approach, so your idea
> of having a function for that looks like a better idea.
>
> > (I don't see a problem with altering m->hw_handler_name to reflect the
> > scsi_dh that is used by the path.. Babu agreed with this too).
> >
> Yep.
>
> > I'd be happy to merge our dm-mpath patches and attribute authorship to
> > you.
> >
> Oh, cool.
> Please do.
>
> Cheers,
>
> Hannes
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
