[PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath

[PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath

[Mike Anderson]

This patch series contains two patches.

1.) The first patch adds a feature flags bit field to the multipath
structure for selected features.

2.) The second patch allows a user to set a feature flag for a dm-mpath
device of "no_abort_q" to indicate that deactivate_path should not call
blk_abort_queue. I tried to select a short feature name to feature output
listed with multipath -l would not be too long "features='2 queue_if_no_path
no_abort_q'

Mike Anderson (2):
  dm: Add feature flags to dm-mpath
  dm: Add feature flag to control call to blk_abort_queue

 drivers/md/dm-mpath.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

[PATCH 1/2] dm: Add feature flags to dm-mpath

[Mike Anderson]

Add a feature flag attribute to the multipath structure.

Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
---
 drivers/md/dm-mpath.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 826bce7..4200d03 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -82,6 +82,7 @@ struct multipath {
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
 	unsigned pg_init_retries;	/* Number of times to retry pg_init */
 	unsigned pg_init_count;		/* Number of times pg_init called */
+	unsigned long features;
 
 	struct work_struct process_queued_ios;
 	struct list_head queued_ios;
@@ -118,6 +119,15 @@ static void trigger_event(struct work_struct *work);
 static void activate_path(struct work_struct *work);
 static void deactivate_path(struct work_struct *work);
 
+static int multipath_test_feature(struct multipath *m, unsigned feature)
+{
+	return test_bit(feature, &m->features);
+}
+
+static void multipath_set_feature(struct multipath *m, unsigned feature)
+{
+	set_bit(feature, &m->features);
+}
 
 /*-----------------------------------------------
  * Allocation routines
-- 
1.6.6.1

Re: [PATCH 1/2] dm: Add feature flags to dm-mpath

[Mike Snitzer]

On Tue, May 04 2010 at 12:01am -0400,
Mike Anderson <andmike@linux.vnet.ibm.com> wrote:

> Add a feature flag attribute to the multipath structure.
> 
> Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
> ---
>  drivers/md/dm-mpath.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 826bce7..4200d03 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -82,6 +82,7 @@ struct multipath {
>  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
>  	unsigned pg_init_retries;	/* Number of times to retry pg_init */
>  	unsigned pg_init_count;		/* Number of times pg_init called */
> +	unsigned long features;

Why not use uint64_t?

>  	struct work_struct process_queued_ios;
>  	struct list_head queued_ios;
> @@ -118,6 +119,15 @@ static void trigger_event(struct work_struct *work);
>  static void activate_path(struct work_struct *work);
>  static void deactivate_path(struct work_struct *work);
>  
> +static int multipath_test_feature(struct multipath *m, unsigned feature)
> +{
> +	return test_bit(feature, &m->features);
> +}
> +
> +static void multipath_set_feature(struct multipath *m, unsigned feature)
> +{
> +	set_bit(feature, &m->features);
> +}

You're using 'unsigned long' for features yet these wrapper functions
take 'unsigned'.  unsigned allows you to use {test,set}_bit but in the
end we have fewer flags to work with...

Granted you're introducing the very first flag but... ;)

Re: [dm-devel] [PATCH 1/2] dm: Add feature flags to dm-mpath

[Mike Anderson]

Mike Snitzer <snitzer@redhat.com> wrote:
> On Tue, May 04 2010 at 12:01am -0400,
> Mike Anderson <andmike@linux.vnet.ibm.com> wrote:
> 
> > +++ b/drivers/md/dm-mpath.c
> > @@ -82,6 +82,7 @@ struct multipath {
> >  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
> >  	unsigned pg_init_retries;	/* Number of times to retry pg_init */
> >  	unsigned pg_init_count;		/* Number of times pg_init called */
> > +	unsigned long features;
> 
> Why not use uint64_t?

I was just following dm.c mapped_device flags as an example, but could be
changed.
> 
> >  	struct work_struct process_queued_ios;
> >  	struct list_head queued_ios;
> > @@ -118,6 +119,15 @@ static void trigger_event(struct work_struct *work);
> >  static void activate_path(struct work_struct *work);
> >  static void deactivate_path(struct work_struct *work);
> >  
> > +static int multipath_test_feature(struct multipath *m, unsigned feature)
> > +{
> > +	return test_bit(feature, &m->features);
> > +}
> > +
> > +static void multipath_set_feature(struct multipath *m, unsigned feature)
> > +{
> > +	set_bit(feature, &m->features);
> > +}
> 
> You're using 'unsigned long' for features yet these wrapper functions
> take 'unsigned'.  unsigned allows you to use {test,set}_bit but in the
> end we have fewer flags to work with...
> 
> Granted you're introducing the very first flag but... ;)
> 

As I indicated above I used dm.c bit setting as an example. The feature
bit nr plus the {test,set}_bit appeared like a good start but as you said
I was just introducing the first flag. I guess it depends on how many
features we think we might need. 

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

[PATCH 2/2] dm: Add feature flag to control call to blk_abort_queue

[Mike Anderson]

Add feature flag for no_abort_q.

Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
---
 drivers/md/dm-mpath.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 4200d03..e55cb41 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -119,6 +119,10 @@ static void trigger_event(struct work_struct *work);
 static void activate_path(struct work_struct *work);
 static void deactivate_path(struct work_struct *work);
 
+enum multipath_feature_flags {
+	MP_FEATURE_NO_ABORT_Q = 0,
+};
+
 static int multipath_test_feature(struct multipath *m, unsigned feature)
 {
 	return test_bit(feature, &m->features);
@@ -156,7 +160,8 @@ static void deactivate_path(struct work_struct *work)
 	struct pgpath *pgpath =
 		container_of(work, struct pgpath, deactivate_path);
 
-	blk_abort_queue(pgpath->path.dev->bdev->bd_disk->queue);
+	if (!multipath_test_feature(pgpath->pg->m, MP_FEATURE_NO_ABORT_Q))
+		blk_abort_queue(pgpath->path.dev->bdev->bd_disk->queue);
 }
 
 static struct priority_group *alloc_priority_group(void)
@@ -822,6 +827,11 @@ static int parse_features(struct arg_set *as, struct multipath *m)
 			continue;
 		}
 
+		if (!strnicmp(param_name, MESG_STR("no_abort_q"))) {
+			multipath_set_feature(m, MP_FEATURE_NO_ABORT_Q);
+			continue;
+		}
+
 		if (!strnicmp(param_name, MESG_STR("pg_init_retries")) &&
 		    (argc >= 1)) {
 			r = read_param(_params + 1, shift(as),
@@ -1381,11 +1391,14 @@ static int multipath_status(struct dm_target *ti, status_type_t type,
 		DMEMIT("2 %u %u ", m->queue_size, m->pg_init_count);
 	else {
 		DMEMIT("%u ", m->queue_if_no_path +
-			      (m->pg_init_retries > 0) * 2);
+			      (m->pg_init_retries > 0) * 2 +
+			      multipath_test_feature(m, MP_FEATURE_NO_ABORT_Q));
 		if (m->queue_if_no_path)
 			DMEMIT("queue_if_no_path ");
 		if (m->pg_init_retries)
 			DMEMIT("pg_init_retries %u ", m->pg_init_retries);
+		if (multipath_test_feature(m, MP_FEATURE_NO_ABORT_Q))
+			DMEMIT("no_abort_q ");
 	}
 
 	if (!m->hw_handler_name || type == STATUSTYPE_INFO)
-- 
1.6.6.1

Re: [PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath

[Moger, Babu]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Mike Anderson
> Sent: Monday, May 03, 2010 11:01 PM
> To: dm-devel@redhat.com
> Cc: linux-scsi@vger.kernel.org
> Subject: [PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath
> 
> This patch series contains two patches.
> 
> 1.) The first patch adds a feature flags bit field to the multipath
> structure for selected features.
> 
> 2.) The second patch allows a user to set a feature flag for a dm-mpath
> device of "no_abort_q" to indicate that deactivate_path should not call
> blk_abort_queue. I tried to select a short feature name to feature
> output
> listed with multipath -l would not be too long "features='2
> queue_if_no_path
> no_abort_q'

Mike, 
 In what special circumstances you recommend to use this feature? It would be great if you could add that explanation in your patch descriptions. 

Re: [PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath

[Mike Anderson]

Moger, Babu <Babu.Moger@lsi.com> wrote:
> > -----Original Message-----
> > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> > owner@vger.kernel.org] On Behalf Of Mike Anderson
> > Sent: Monday, May 03, 2010 11:01 PM
> > To: dm-devel@redhat.com
> > Cc: linux-scsi@vger.kernel.org
> > Subject: [PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath
> > 
> > This patch series contains two patches.
> > 
> > 1.) The first patch adds a feature flags bit field to the multipath
> > structure for selected features.
> > 
> > 2.) The second patch allows a user to set a feature flag for a dm-mpath
> > device of "no_abort_q" to indicate that deactivate_path should not call
> > blk_abort_queue. I tried to select a short feature name to feature
> > output
> > listed with multipath -l would not be too long "features='2
> > queue_if_no_path
> > no_abort_q'
> 
> Mike, 
>  In what special circumstances you recommend to use this feature? It would be great if you could add that explanation in your patch descriptions. 

Yes I will update the descriptions.

To answer your question in general it would be circumstance where the
block_abort_queue during failover is leading to longer recovery instead of
shorter. This could be because of aborting / transport / device
interactions (the work by others in the iSCSI and FC transports have made
things better here).

While there are case where abort makes a big difference (being able to
failover in less than max_retries * IO timeout value). The latency numbers
for simple RDAC initiator failover show only small improvements on my
configurations (others may have better data). A assumption would be that
there could be combinations of transports and devices out there where this
might not give the fastest failover so the user may want control.

On a historically note the call to block_abort_queue came in somewhat
sideways by me linked to the timeout movement from SCSI to blk so it could
have integrated better. I should have had a way to disable / enable it
then and I am trying to provide that now so that the user has some
control.

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

RE: [PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath

[Moger, Babu]

> -----Original Message-----
> From: Mike Anderson [mailto:andmike@linux.vnet.ibm.com]
> Sent: Wednesday, May 05, 2010 8:39 PM
> To: Moger, Babu
> Cc: dm-devel@redhat.com; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath
> 
> Moger, Babu <Babu.Moger@lsi.com> wrote:
> > > -----Original Message-----
> > > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> > > owner@vger.kernel.org] On Behalf Of Mike Anderson
> > > Sent: Monday, May 03, 2010 11:01 PM
> > > To: dm-devel@redhat.com
> > > Cc: linux-scsi@vger.kernel.org
> > > Subject: [PATCH 0/2] dm: Add no_abort_q feature flag to dm-mpath
> > >
> > > This patch series contains two patches.
> > >
> > > 1.) The first patch adds a feature flags bit field to the multipath
> > > structure for selected features.
> > >
> > > 2.) The second patch allows a user to set a feature flag for a dm-
> mpath
> > > device of "no_abort_q" to indicate that deactivate_path should not
> call
> > > blk_abort_queue. I tried to select a short feature name to feature
> > > output
> > > listed with multipath -l would not be too long "features='2
> > > queue_if_no_path
> > > no_abort_q'
> >
> > Mike,
> >  In what special circumstances you recommend to use this feature? It
> would be great if you could add that explanation in your patch
> descriptions.
> 
> Yes I will update the descriptions.
> 
> To answer your question in general it would be circumstance where the
> block_abort_queue during failover is leading to longer recovery instead
> of
> shorter. This could be because of aborting / transport / device
> interactions (the work by others in the iSCSI and FC transports have
> made
> things better here).
> 
> While there are case where abort makes a big difference (being able to
> failover in less than max_retries * IO timeout value). The latency
> numbers
> for simple RDAC initiator failover show only small improvements on my
> configurations (others may have better data). A assumption would be
> that
> there could be combinations of transports and devices out there where
> this
> might not give the fastest failover so the user may want control.
> 
> On a historically note the call to block_abort_queue came in somewhat
> sideways by me linked to the timeout movement from SCSI to blk so it
> could
> have integrated better. I should have had a way to disable / enable it
> then and I am trying to provide that now so that the user has some
> control.

Thanks for the details.