[dm-devel] Re: Patch added to scsi-pending-2.6: [SCSI] scsi_dh: Use SCSI device handler in dm-multipath

[Chandra Seetharaman]

Hi Alasdair,

I have submitted a new set of patches with changes as per your
suggestions. Please see below for details.

Thanks,

chandra
On Tue, 2008-04-29 at 16:45 -0700, Chandra Seetharaman wrote:
> Hi Alasdair,
> 
> Thanks for your feedback.
> 
> MikeC and mikeand, feel free to add.
> 
> On Tue, 2008-04-29 at 15:56 +0100, Alasdair G Kergon wrote:
> > On Thu, Apr 24, 2008 at 04:03:11PM +0000, James Bottomley wrote:
> > >     This patch converts dm-mpath to scsi hw handlers. It does
> > >     not add any new functionality and old behaviors and userspace
> > >     tools work as is except we use the safe clariion default instead
> > >     of using the userspace setting.
> > 
> > OK.  Comments from my first line-by-line reading of this.
> > 
> > Firstly, the patch header is rather too brief for my liking - the patch
> > does make some subtle functional changes that ought to be commented upon
> > to explain to people why they are being made.  (I would have preferred
> 
> will do.

Added comments to appropriate places.

> > this patch to have been broken up into smaller ones in a manner that
> > made the functional changes more obvious i.e. separated from the code
> > reorganisation.)
> 
> will do.

Separated the dm patches to 4 patches to make it more reader-friendly
and separating different aspects.

> 
> > 
> > 1. Currently the supplied hw_handler name is validated at table load
> > time so userspace cannot create a device using a non-existent
> > hw_handler.  After this patch it looks as if that validation is delayed
> > until something starts to access the device - and then the paths simply
> > get failed, a new failure mode for the userspace tools to handle.  Is
> > that change hard to avoid or was it judged not worth the effort to leave
> > things as they were and perform as much validation / resource allocation
> > as possible up front?
> 
> For the current implementation, initialization of the scsi_dh specific
> data need to happen when we see the device the very first time (for
> prep_fn() and check_sense() to be useful). We cannot wait till multipath
> comes along, and the device specific data will exist till the device is
> removed, hence there is no need to hold onto a reference for each device
> for mulipath's sake.
> 
> But, I agree with your point that the failure should happen early.
> 
> Will add a scsi_dh_handler_exist(name) function to make sure that the
> module is available at the time of table load. What do you think ?

Implemented it and tested to make sure that the failure happens during
table load time.

> 
> > 
> > 2.  parse_hw_handler() now ignores hw_handler arguments.
> > Please add a comment to explain that or perhaps log a warning message if
> > any are supplied.
> 
> will do

Changed it to fail when any additional arguments are provided making it
very visible to the user.

> 
> > BTW r looks superfluous now:
> > 
> >         if (read_param(_params, shift(as), &hw_argc, &ti->error))
> >                 return -EINVAL;
> 
> will fix.

done.
> 
> > 
> > 3. The dmsetup table output has changed - it no longer matches the input
> > in the case where arguments got ignored.  I don't think this matters
> > (unlike a similar issue we had with crypt), but it should be noted in
> > comments inline.
> 
> will add comments.

with the change in the arguments, o/p of dmsetup table would be proper.

> 
> > 
> > 4.  /* Fields used by hardware handler usage */
> > Comment doesn't add anything - drop it?
> > 
> ok.

done

> 
> > 5. char *hw_handler_name;
> > const?
> 
> will fix.

done
> 
> > 
> > 6. The code now assumes all hw_handlers have a pg_init function: in
> > practice this is likely to be the case.  Does that permit further
> > simplification of some of the logic?
> 
> will look into it.

Not much except invoking queue_work directly.

> 
> > 
> > 7. I note that the 'bypassed' argument to the pg_init function has been
> > dropped - presumably as none of the existing handlers made any use of
> > it.
> 
> Yes, none of the hardware handlers used it.
> 
> > 
> > 8. A new workqueue is introduced without comment.
> 
> Will add.

Done.
> 
> >   Which of the changes means we need it now? 
> I do not understand this comment.
> 
> >  What's the reason it's not single-threaded (and per-device)?
> Per device might be an overkill. Will do single-threaded.

Made it single-threaded.
> 
> >   Is there a clearer name than "khwhandlerd" -
> > something that tells people it's connected with multipath, and
> 
> how does kmpath_handlerd sound ?

changed to kmpath_handlerd

> 
> > can the name of the struct workqueue_struct variable match this?
> 
> will fix.

done.

> 
> > Previously the hw_handler pg_init function was required to return
> > immediately.  Can its replacement scsi_dh_activate() block (but not in a
> > way that could deadlock of course)? 
> 
> Yes, scsi_dh_activate() can block. But, at the dm-level, it is
> indifferent, due to the usage of the workqueue.
> 
> >  Should some of its functionality be
> > got out of the way at initialisation time within multipath_ctr()?
> 
> You mean hardware handler specific initialization ? It happens when the
> device is found. We do not want to wait till multipath comes along.
> 
> > 
> > 9. What does m->path_to_activate give us that m->current_pgpath
> > doesn't?
> 
> I guess none. Was just sticking with the original interface as it was
> handed off to an asynchronous thread. If it won't matter, I will fix it
> to use it directly from the multipath data structure.

removed path_to_activate and using current_pgpath->path instead.

> 
> > 
> > 10. Is activate_passive_path() an accurate function name?  Is the
> > supplied path argument always 'passive'?
> 
> May be not always. Will change it activate_path().

Changed to activate_path()
> 
> > 
> > 11. Where has the use of pg_init_retries gone?
> 
> Oops.. will fix.

fixed.

> > 
> > I still need to check the updated state machine logic and associated
> > locking once we have a final version of this patch.
> > 
> > Alasdair
> 
> --
> dm-devel mailing list
> dm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel