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

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

[Alasdair G Kergon]

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
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.)

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?

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.
BTW r looks superfluous now:

        if (read_param(_params, shift(as), &hw_argc, &ti->error))
                return -EINVAL;

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.

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

5. char *hw_handler_name;
const?

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?

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.

8. A new workqueue is introduced without comment.  Which of the changes
means we need it now?  What's the reason it's not single-threaded
(and per-device)?  Is there a clearer name than "khwhandlerd" -
something that tells people it's connected with multipath, and
can the name of the struct workqueue_struct variable match this?
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)?  Should some of its functionality be
got out of the way at initialisation time within multipath_ctr()?

9. What does m->path_to_activate give us that m->current_pgpath
doesn't?

10. Is activate_passive_path() an accurate function name?  Is the
supplied path argument always 'passive'?

11. Where has the use of pg_init_retries gone?

I still need to check the updated state machine logic and associated
locking once we have a final version of this patch.

Alasdair
-- 
agk@redhat.com

Different Path priorities for iSCSI and FC paths to a combination lun.

[Srinivas Ramani]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

Hi Folks;
                   I would be grateful to learn from this forum on how one would be able to have
                  different path priorities for a FC / iSCSI  paths to a multiprotocol FC/iSCSI
                  combination lun using device mapper .
                  The intention is that a  FC path should have a higher priority than an iSCSI path to the same lun.
                  Is it possible to do it with a configuration change or would one need to  add this feature to the
                  mpath_prio_alua  code.
 
 
With Regards
Srini
 
Senior Software Engineer
Pillar Data Systems.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4611 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: Different Path priorities for iSCSI and FC paths to a combination lun.

[Tore Anderson]

Hi,

* Srinivas Ramani

> I would be grateful to learn from this forum on how one would be able
> to have different path priorities for a FC / iSCSI  paths to a
> multiprotocol FC/iSCSI combination lun using device mapper . The
> intention is that a  FC path should have a higher priority than an
> iSCSI path to the same lun. Is it possible to do it with a
> configuration change or would one need to  add this feature to the 
> mpath_prio_alua  code.

Make a small wrapper script that invokes mpath_prio_alua and also checks
if the path is to a iSCSI volume or not (you can probably determine this
by looking somewhere beneath /sys/block/$dev/).  If it is, add 100 to
whatever mpath_prio_alua returned, and print the sum.

Regards
-- 
Tore Anderson

RE: Different Path priorities for iSCSI and FC paths toa combination lun.

[Srinivas Ramani]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

Thanks Tore .
I plan to add mpath_prio_alua_pillar to the code base to return different priorities for iSCSI and FC paths for
combination luns .
 
 
With Regards
Srini

________________________________

From: dm-devel-bounces@redhat.com on behalf of Tore Anderson
Sent: Tue 4/29/2008 1:06 PM
To: device-mapper development
Subject: Re: [dm-devel] Different Path priorities for iSCSI and FC paths toa combination lun.



Hi,

* Srinivas Ramani

> I would be grateful to learn from this forum on how one would be able
> to have different path priorities for a FC / iSCSI  paths to a
> multiprotocol FC/iSCSI combination lun using device mapper . The
> intention is that a  FC path should have a higher priority than an
> iSCSI path to the same lun. Is it possible to do it with a
> configuration change or would one need to  add this feature to the
> mpath_prio_alua  code.

Make a small wrapper script that invokes mpath_prio_alua and also checks
if the path is to a iSCSI volume or not (you can probably determine this
by looking somewhere beneath /sys/block/$dev/).  If it is, add 100 to
whatever mpath_prio_alua returned, and print the sum.

Regards
--
Tore Anderson

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4729 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Chandra Seetharaman]

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.
> 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.

> 
> 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 ?

> 
> 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

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

will fix.

> 
> 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.

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

> 5. char *hw_handler_name;
> const?

will fix.

> 
> 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.

> 
> 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.

>   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.

>   Is there a clearer name than "khwhandlerd" -
> something that tells people it's connected with multipath, and

how does kmpath_handlerd sound ?

> can the name of the struct workqueue_struct variable match this?

will fix.

> 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.

> 
> 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().

> 
> 11. Where has the use of pg_init_retries gone?

Oops.. will fix.
> 
> I still need to check the updated state machine logic and associated
> locking once we have a final version of this patch.
> 
> Alasdair

Re: 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@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel