FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 02-17-2009, 12:47 PM
Nikanth Karthikesan
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

Delay retry to activate_path if it returns SCSI_DH_RETRY.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 095f77b..af54632 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -65,12 +65,14 @@ struct multipath {
spinlock_t lock;

const char *hw_handler_name;
- struct work_struct activate_path;
+ struct delayed_work activate_path;
struct pgpath *pgpath_to_activate;
unsigned nr_priority_groups;
struct list_head priority_groups;
unsigned pg_init_required; /* pg_init needs calling? */
unsigned pg_init_in_progress; /* Only one pg_init allowed at once */
+ unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */
+#define SCSI_DH_RETRY_DELAY ((HZ * 2))

unsigned nr_valid_paths; /* Total number of usable paths */
struct pgpath *current_pgpath;
@@ -203,7 +205,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
m->queue_io = 1;
INIT_WORK(&m->process_queued_ios, process_queued_ios);
INIT_WORK(&m->trigger_event, trigger_event);
- INIT_WORK(&m->activate_path, activate_path);
+ INIT_DELAYED_WORK(&m->activate_path, activate_path);
m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
if (!m->mpio_pool) {
kfree(m);
@@ -431,6 +433,8 @@ static void process_queued_ios(struct work_struct *work)
struct pgpath *pgpath = NULL;
unsigned init_required = 0, must_queue = 1;
unsigned long flags;
+ unsigned long delay = 0;
+ unsigned long now;

spin_lock_irqsave(&m->lock, flags);

@@ -452,13 +456,20 @@ static void process_queued_ios(struct work_struct *work)
m->pg_init_required = 0;
m->pg_init_in_progress = 1;
init_required = 1;
+ /* Delay retry due to SCSI_DH_RETRY */
+ if (m->pg_init_jiffy) {
+ now = jiffies;
+ if (time_after(now, m->pg_init_jiffy))
+ delay = now - m->pg_init_jiffy;
+ m->pg_init_jiffy = 0;
+ }
}

out:
spin_unlock_irqrestore(&m->lock, flags);

if (init_required)
- queue_work(kmpath_handlerd, &m->activate_path);
+ queue_delayed_work(kmpath_handlerd, &m->activate_path, delay);

if (!must_queue)
dispatch_queued_ios(m);
@@ -1060,6 +1071,7 @@ static void pg_init_done(struct dm_path *path, int errors)
struct priority_group *pg = pgpath->pg;
struct multipath *m = pg->m;
unsigned long flags;
+ bool delay = false;

/* device or driver problems */
switch (errors) {
@@ -1084,8 +1096,11 @@ static void pg_init_done(struct dm_path *path, int errors)
*/
bypass_pg(m, pg, 1);
break;
- /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */
+ /*
+ * For SCSI_DH_RETRY we wait for a couple seconds.
+ */
case SCSI_DH_RETRY:
+ delay = true;
case SCSI_DH_IMM_RETRY:
case SCSI_DH_RES_TEMP_UNAVAIL:
if (pg_init_limit_reached(m, pgpath))
@@ -1112,6 +1127,10 @@ static void pg_init_done(struct dm_path *path, int errors)
}

m->pg_init_in_progress = 0;
+ if (delay)
+ m->pg_init_jiffy = jiffies + SCSI_DH_RETRY_DELAY;
+ else
+ m->pg_init_jiffy = 0;
queue_work(kmultipathd, &m->process_queued_ios);
spin_unlock_irqrestore(&m->lock, flags);
}
@@ -1120,7 +1139,7 @@ static void activate_path(struct work_struct *work)
{
int ret;
struct multipath *m =
- container_of(work, struct multipath, activate_path);
+ container_of(work, struct multipath, activate_path.work);
struct dm_path *path;
unsigned long flags;


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 02-19-2009, 12:55 AM
Chandra Seetharaman
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

Hi Nikanth,

Thanks for the patch.

Please see my comment below.

chandra
On Tue, 2009-02-17 at 19:17 +0530, Nikanth Karthikesan wrote:
> Delay retry to activate_path if it returns SCSI_DH_RETRY.
>
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
>
> ---
>
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 095f77b..af54632 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -65,12 +65,14 @@ struct multipath {
> spinlock_t lock;
>
> const char *hw_handler_name;
> - struct work_struct activate_path;
> + struct delayed_work activate_path;
> struct pgpath *pgpath_to_activate;
> unsigned nr_priority_groups;
> struct list_head priority_groups;
> unsigned pg_init_required; /* pg_init needs calling? */
> unsigned pg_init_in_progress; /* Only one pg_init allowed at once */
> + unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */
> +#define SCSI_DH_RETRY_DELAY ((HZ * 2))
>
> unsigned nr_valid_paths; /* Total number of usable paths */
> struct pgpath *current_pgpath;
> @@ -203,7 +205,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
> m->queue_io = 1;
> INIT_WORK(&m->process_queued_ios, process_queued_ios);
> INIT_WORK(&m->trigger_event, trigger_event);
> - INIT_WORK(&m->activate_path, activate_path);
> + INIT_DELAYED_WORK(&m->activate_path, activate_path);
> m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
> if (!m->mpio_pool) {
> kfree(m);
> @@ -431,6 +433,8 @@ static void process_queued_ios(struct work_struct *work)
> struct pgpath *pgpath = NULL;
> unsigned init_required = 0, must_queue = 1;
> unsigned long flags;
> + unsigned long delay = 0;
> + unsigned long now;
>
> spin_lock_irqsave(&m->lock, flags);
>
> @@ -452,13 +456,20 @@ static void process_queued_ios(struct work_struct *work)
> m->pg_init_required = 0;
> m->pg_init_in_progress = 1;
> init_required = 1;
> + /* Delay retry due to SCSI_DH_RETRY */
> + if (m->pg_init_jiffy) {
> + now = jiffies;
> + if (time_after(now, m->pg_init_jiffy))
> + delay = now - m->pg_init_jiffy;

I think the logic is reversed. Acc to linux/jiffies.h, "time_after(a,b)
returns true if time a is after time b",

We want it other way around, don't we ?

IMO, we need _not_ be so critical of the time. We could just set a flag
in pg_init_done and use 2 seconds in queue_delayed_work().

> + m->pg_init_jiffy = 0;
> + }
> }
>
> out:
> spin_unlock_irqrestore(&m->lock, flags);
>
> if (init_required)
> - queue_work(kmpath_handlerd, &m->activate_path);
> + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay);
>
> if (!must_queue)
> dispatch_queued_ios(m);
> @@ -1060,6 +1071,7 @@ static void pg_init_done(struct dm_path *path, int errors)
> struct priority_group *pg = pgpath->pg;
> struct multipath *m = pg->m;
> unsigned long flags;
> + bool delay = false;
>
> /* device or driver problems */
> switch (errors) {
> @@ -1084,8 +1096,11 @@ static void pg_init_done(struct dm_path *path, int errors)
> */
> bypass_pg(m, pg, 1);
> break;
> - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */
> + /*
> + * For SCSI_DH_RETRY we wait for a couple seconds.
> + */
> case SCSI_DH_RETRY:
> + delay = true;
> case SCSI_DH_IMM_RETRY:
> case SCSI_DH_RES_TEMP_UNAVAIL:
> if (pg_init_limit_reached(m, pgpath))
> @@ -1112,6 +1127,10 @@ static void pg_init_done(struct dm_path *path, int errors)
> }
>
> m->pg_init_in_progress = 0;
> + if (delay)
> + m->pg_init_jiffy = jiffies + SCSI_DH_RETRY_DELAY;
> + else
> + m->pg_init_jiffy = 0;
> queue_work(kmultipathd, &m->process_queued_ios);
> spin_unlock_irqrestore(&m->lock, flags);
> }
> @@ -1120,7 +1139,7 @@ static void activate_path(struct work_struct *work)
> {
> int ret;
> struct multipath *m =
> - container_of(work, struct multipath, activate_path);
> + container_of(work, struct multipath, activate_path.work);
> struct dm_path *path;
> unsigned long flags;
>
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 02-19-2009, 01:11 AM
Alasdair G Kergon
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

On Tue, Feb 17, 2009 at 07:17:37PM +0530, Nikanth Karthikesan wrote:
> Delay retry to activate_path if it returns SCSI_DH_RETRY.

Please write a complete patch header if you'd like this queued for upstream!

E.g. Why? Any cases where this will make matters worse or make no difference?

> + unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */
> +#define SCSI_DH_RETRY_DELAY ((HZ * 2))

Why that particular value?
Please move it into a header file with an explanation of what it does.

Alasdair
--
agk@redhat.com

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 02-19-2009, 06:10 AM
Nikanth Karthikesan
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

On Thursday 19 February 2009 07:41:18 Alasdair G Kergon wrote:
> On Tue, Feb 17, 2009 at 07:17:37PM +0530, Nikanth Karthikesan wrote:
> > Delay retry to activate_path if it returns SCSI_DH_RETRY.
>
> Please write a complete patch header if you'd like this queued for
> upstream!
>
> E.g. Why? Any cases where this will make matters worse or make no
> difference?
>

Sorry for the terse changelog.

> > + unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */
> > +#define SCSI_DH_RETRY_DELAY ((HZ * 2))
>
> Why that particular value?

There is no specific reason. Just picked 2 seconds based on the TODO comment,
"For SCSI_DH_RETRY we should wait a couple seconds".

> Please move it into a header file with an explanation of what it does.
>
done.

Attached is the patch with comments from you and Chandra incorporated.

Thanks
Nikanth


SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry
immediately and SCSI_DH_RETRY in cases where it is better to retry
after some delay.

Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and
SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to
activate a device, if it returns SCSI_DH_RETRY.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 095f77b..6fd76f1 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -65,12 +65,13 @@ struct multipath {
spinlock_t lock;

const char *hw_handler_name;
- struct work_struct activate_path;
+ struct delayed_work activate_path;
struct pgpath *pgpath_to_activate;
unsigned nr_priority_groups;
struct list_head priority_groups;
unsigned pg_init_required; /* pg_init needs calling? */
unsigned pg_init_in_progress; /* Only one pg_init allowed at once */
+ unsigned pg_init_delay; /* delay required before retry? */

unsigned nr_valid_paths; /* Total number of usable paths */
struct pgpath *current_pgpath;
@@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
m->queue_io = 1;
INIT_WORK(&m->process_queued_ios, process_queued_ios);
INIT_WORK(&m->trigger_event, trigger_event);
- INIT_WORK(&m->activate_path, activate_path);
+ INIT_DELAYED_WORK(&m->activate_path, activate_path);
m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
if (!m->mpio_pool) {
kfree(m);
@@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work)
struct pgpath *pgpath = NULL;
unsigned init_required = 0, must_queue = 1;
unsigned long flags;
+ unsigned long delay = 0;

spin_lock_irqsave(&m->lock, flags);

@@ -452,13 +454,15 @@ static void process_queued_ios(struct work_struct *work)
m->pg_init_required = 0;
m->pg_init_in_progress = 1;
init_required = 1;
+ if (m->pg_init_delay)
+ delay = SCSI_DH_RETRY_DELAY;
}

out:
spin_unlock_irqrestore(&m->lock, flags);

if (init_required)
- queue_work(kmpath_handlerd, &m->activate_path);
+ queue_delayed_work(kmpath_handlerd, &m->activate_path, delay);

if (!must_queue)
dispatch_queued_ios(m);
@@ -1060,6 +1064,7 @@ static void pg_init_done(struct dm_path *path, int errors)
struct priority_group *pg = pgpath->pg;
struct multipath *m = pg->m;
unsigned long flags;
+ unsigned delay = 0;

/* device or driver problems */
switch (errors) {
@@ -1084,8 +1089,11 @@ static void pg_init_done(struct dm_path *path, int errors)
*/
bypass_pg(m, pg, 1);
break;
- /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */
+ /*
+ * For SCSI_DH_RETRY we wait before retrying.
+ */
case SCSI_DH_RETRY:
+ delay = 1;
case SCSI_DH_IMM_RETRY:
case SCSI_DH_RES_TEMP_UNAVAIL:
if (pg_init_limit_reached(m, pgpath))
@@ -1112,6 +1120,7 @@ static void pg_init_done(struct dm_path *path, int errors)
}

m->pg_init_in_progress = 0;
+ m->pg_init_delay = delay;
queue_work(kmultipathd, &m->process_queued_ios);
spin_unlock_irqrestore(&m->lock, flags);
}
@@ -1120,7 +1129,7 @@ static void activate_path(struct work_struct *work)
{
int ret;
struct multipath *m =
- container_of(work, struct multipath, activate_path);
+ container_of(work, struct multipath, activate_path.work);
struct dm_path *path;
unsigned long flags;

diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 33efce2..f099d86 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -55,6 +55,10 @@ enum {
SCSI_DH_NOSYS,
SCSI_DH_DRIVER_MAX,
};
+
+/* Time to wait before retry in case of SCSI_DH_RETRY */
+#define SCSI_DH_RETRY_DELAY ((HZ * 2))
+
#if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
extern int scsi_dh_activate(struct request_queue *);
extern int scsi_dh_handler_exist(const char *);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 02-19-2009, 11:45 PM
Chandra Seetharaman
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote:

<snip>

> @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work)
> struct pgpath *pgpath = NULL;
> unsigned init_required = 0, must_queue = 1;
> unsigned long flags;
> + unsigned long delay = 0;

I do not see the reason for this variable, you can as well put the delay
in pg_init_delay and use it directly (and set it to zero after using
it) ?
>
> spin_lock_irqsave(&m->lock, flags);
>
> @@ -452,13 +454,15 @@ static void process_queued_ios(struct work_struct *work)
> m->pg_init_required = 0;
> m->pg_init_in_progress = 1;
> init_required = 1;
> + if (m->pg_init_delay)
> + delay = SCSI_DH_RETRY_DELAY;
> }
>
> out:
> spin_unlock_irqrestore(&m->lock, flags);
>
> if (init_required)
> - queue_work(kmpath_handlerd, &m->activate_path);
> + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay);
>
> if (!must_queue)
> dispatch_queued_ios(m);
> @@ -1060,6 +1064,7 @@ static void pg_init_done(struct dm_path *path, int errors)
> struct priority_group *pg = pgpath->pg;
> struct multipath *m = pg->m;
> unsigned long flags;
> + unsigned delay = 0;

You can get rid of this variable also and set it directly under
SCSI_DH_RETRY.
>
> /* device or driver problems */
> switch (errors) {
> @@ -1084,8 +1089,11 @@ static void pg_init_done(struct dm_path *path, int errors)
> */
> bypass_pg(m, pg, 1);
> break;
> - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */
> + /*
> + * For SCSI_DH_RETRY we wait before retrying.
> + */
> case SCSI_DH_RETRY:
> + delay = 1;
> case SCSI_DH_IMM_RETRY:
> case SCSI_DH_RES_TEMP_UNAVAIL:
> if (pg_init_limit_reached(m, pgpath))
> @@ -1112,6 +1120,7 @@ static void pg_init_done(struct dm_path *path, int errors)
> }
>
> m->pg_init_in_progress = 0;
> + m->pg_init_delay = delay;
> queue_work(kmultipathd, &m->process_queued_ios);
> spin_unlock_irqrestore(&m->lock, flags);
> }
> @@ -1120,7 +1129,7 @@ static void activate_path(struct work_struct *work)
> {
> int ret;
> struct multipath *m =
> - container_of(work, struct multipath, activate_path);
> + container_of(work, struct multipath, activate_path.work);
> struct dm_path *path;
> unsigned long flags;
>
> diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
> index 33efce2..f099d86 100644
> --- a/include/scsi/scsi_dh.h
> +++ b/include/scsi/scsi_dh.h
> @@ -55,6 +55,10 @@ enum {
> SCSI_DH_NOSYS,
> SCSI_DH_DRIVER_MAX,
> };
> +
> +/* Time to wait before retry in case of SCSI_DH_RETRY */
> +#define SCSI_DH_RETRY_DELAY ((HZ * 2))
> +
> #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
> extern int scsi_dh_activate(struct request_queue *);
> extern int scsi_dh_handler_exist(const char *);
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 02-20-2009, 04:03 AM
Nikanth Karthikesan
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

On Friday 20 February 2009 06:15:29 Chandra Seetharaman wrote:
> On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote:
>
> <snip>
>
> > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct
> > *work) struct pgpath *pgpath = NULL;
> > unsigned init_required = 0, must_queue = 1;
> > unsigned long flags;
> > + unsigned long delay = 0;
>
> I do not see the reason for this variable, you can as well put the delay
> in pg_init_delay and use it directly (and set it to zero after using
> it) ?
>

I missed resetting pg_init_delay to zero after using it. I have attached the
corrected patch with this. This variable keeps the code cleaner(avoids taking
m->lock). Also having only a boolean in struct multipath keeps it a bit
smaller.

off-topic:
I think struct multipath can be shrunk even further by making various flags
like pg_init_required, pg_init_in_progress, queue_io, queue_if_no_path,
saved_queue_if_no_path in to a single variable. Thoughts?

<snip>

> > @@ -1060,6 +1064,7 @@ static void pg_init_done(struct dm_path *path, int
> > errors) struct priority_group *pg = pgpath->pg;
> > struct multipath *m = pg->m;
> > unsigned long flags;
> > + unsigned delay = 0;
>
> You can get rid of this variable also and set it directly under
> SCSI_DH_RETRY.
>

pg_init_delay is protected by the m->lock. And this variable helps in keeping
the code cleaner.

<snip>

I am attaching the fixed patch(resetting pg_init_delay to zero after using
it).

Thanks
Nikanth

SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry
immediately and SCSI_DH_RETRY in cases where it is better to retry
after some delay.

Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and
SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to
activate a device, if it returns SCSI_DH_RETRY.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 095f77b..7ddf775 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -65,12 +65,13 @@ struct multipath {
spinlock_t lock;

const char *hw_handler_name;
- struct work_struct activate_path;
+ struct delayed_work activate_path;
struct pgpath *pgpath_to_activate;
unsigned nr_priority_groups;
struct list_head priority_groups;
unsigned pg_init_required; /* pg_init needs calling? */
unsigned pg_init_in_progress; /* Only one pg_init allowed at once */
+ unsigned pg_init_delay; /* delay required before retry? */

unsigned nr_valid_paths; /* Total number of usable paths */
struct pgpath *current_pgpath;
@@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
m->queue_io = 1;
INIT_WORK(&m->process_queued_ios, process_queued_ios);
INIT_WORK(&m->trigger_event, trigger_event);
- INIT_WORK(&m->activate_path, activate_path);
+ INIT_DELAYED_WORK(&m->activate_path, activate_path);
m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
if (!m->mpio_pool) {
kfree(m);
@@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work)
struct pgpath *pgpath = NULL;
unsigned init_required = 0, must_queue = 1;
unsigned long flags;
+ unsigned long delay = 0;

spin_lock_irqsave(&m->lock, flags);

@@ -452,13 +454,17 @@ static void process_queued_ios(struct work_struct *work)
m->pg_init_required = 0;
m->pg_init_in_progress = 1;
init_required = 1;
+ if (m->pg_init_delay) {
+ delay = SCSI_DH_RETRY_DELAY;
+ m->pg_init_delay = 0;
+ }
}

out:
spin_unlock_irqrestore(&m->lock, flags);

if (init_required)
- queue_work(kmpath_handlerd, &m->activate_path);
+ queue_delayed_work(kmpath_handlerd, &m->activate_path, delay);

if (!must_queue)
dispatch_queued_ios(m);
@@ -1060,6 +1066,7 @@ static void pg_init_done(struct dm_path *path, int errors)
struct priority_group *pg = pgpath->pg;
struct multipath *m = pg->m;
unsigned long flags;
+ unsigned delay = 0;

/* device or driver problems */
switch (errors) {
@@ -1084,8 +1091,11 @@ static void pg_init_done(struct dm_path *path, int errors)
*/
bypass_pg(m, pg, 1);
break;
- /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */
+ /*
+ * For SCSI_DH_RETRY we wait before retrying.
+ */
case SCSI_DH_RETRY:
+ delay = 1;
case SCSI_DH_IMM_RETRY:
case SCSI_DH_RES_TEMP_UNAVAIL:
if (pg_init_limit_reached(m, pgpath))
@@ -1112,6 +1122,7 @@ static void pg_init_done(struct dm_path *path, int errors)
}

m->pg_init_in_progress = 0;
+ m->pg_init_delay = delay;
queue_work(kmultipathd, &m->process_queued_ios);
spin_unlock_irqrestore(&m->lock, flags);
}
@@ -1120,7 +1131,7 @@ static void activate_path(struct work_struct *work)
{
int ret;
struct multipath *m =
- container_of(work, struct multipath, activate_path);
+ container_of(work, struct multipath, activate_path.work);
struct dm_path *path;
unsigned long flags;

diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 33efce2..f099d86 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -55,6 +55,10 @@ enum {
SCSI_DH_NOSYS,
SCSI_DH_DRIVER_MAX,
};
+
+/* Time to wait before retry in case of SCSI_DH_RETRY */
+#define SCSI_DH_RETRY_DELAY ((HZ * 2))
+
#if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
extern int scsi_dh_activate(struct request_queue *);
extern int scsi_dh_handler_exist(const char *);


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 02-20-2009, 02:08 PM
Konrad Rzeszutek
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

> off-topic:
> I think struct multipath can be shrunk even further by making various flags
> like pg_init_required, pg_init_in_progress, queue_io, queue_if_no_path,
> saved_queue_if_no_path in to a single variable. Thoughts?

Do the benefits (cut down on the size of the structure which is allocated once
for a map) vs readability worth it?

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 02-20-2009, 08:11 PM
Chandra Seetharaman
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

On Fri, 2009-02-20 at 10:33 +0530, Nikanth Karthikesan wrote:
> On Friday 20 February 2009 06:15:29 Chandra Seetharaman wrote:
> > On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote:
> >
> > <snip>
> >
> > > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct
> > > *work) struct pgpath *pgpath = NULL;
> > > unsigned init_required = 0, must_queue = 1;
> > > unsigned long flags;
> > > + unsigned long delay = 0;
> >
> > I do not see the reason for this variable, you can as well put the delay
> > in pg_init_delay and use it directly (and set it to zero after using
> > it) ?
> >
>
> I missed resetting pg_init_delay to zero after using it. I have attached the
> corrected patch with this. This variable keeps the code cleaner(avoids taking
> m->lock). Also having only a boolean in struct multipath keeps it a bit
> smaller.

Ok. I am fine with your justification.

>
> off-topic:
> I think struct multipath can be shrunk even further by making various flags
> like pg_init_required, pg_init_in_progress, queue_io, queue_if_no_path,
> saved_queue_if_no_path in to a single variable. Thoughts?

As Konrad mentioned, it is worth the cost of readability ?


<snip>

> SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry
> immediately and SCSI_DH_RETRY in cases where it is better to retry
> after some delay.
>
> Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and
> SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to
> activate a device, if it returns SCSI_DH_RETRY.
>
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>
>
> ---
>
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 095f77b..7ddf775 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -65,12 +65,13 @@ struct multipath {
> spinlock_t lock;
>
> const char *hw_handler_name;
> - struct work_struct activate_path;
> + struct delayed_work activate_path;
> struct pgpath *pgpath_to_activate;
> unsigned nr_priority_groups;
> struct list_head priority_groups;
> unsigned pg_init_required; /* pg_init needs calling? */
> unsigned pg_init_in_progress; /* Only one pg_init allowed at once */
> + unsigned pg_init_delay; /* delay required before retry? */
>
> unsigned nr_valid_paths; /* Total number of usable paths */
> struct pgpath *current_pgpath;
> @@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
> m->queue_io = 1;
> INIT_WORK(&m->process_queued_ios, process_queued_ios);
> INIT_WORK(&m->trigger_event, trigger_event);
> - INIT_WORK(&m->activate_path, activate_path);
> + INIT_DELAYED_WORK(&m->activate_path, activate_path);
> m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
> if (!m->mpio_pool) {
> kfree(m);
> @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work)
> struct pgpath *pgpath = NULL;
> unsigned init_required = 0, must_queue = 1;
> unsigned long flags;
> + unsigned long delay = 0;
>
> spin_lock_irqsave(&m->lock, flags);
>
> @@ -452,13 +454,17 @@ static void process_queued_ios(struct work_struct *work)
> m->pg_init_required = 0;
> m->pg_init_in_progress = 1;
> init_required = 1;
> + if (m->pg_init_delay) {
> + delay = SCSI_DH_RETRY_DELAY;
> + m->pg_init_delay = 0;
> + }
> }
>
> out:
> spin_unlock_irqrestore(&m->lock, flags);
>
> if (init_required)
> - queue_work(kmpath_handlerd, &m->activate_path);
> + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay);
>
> if (!must_queue)
> dispatch_queued_ios(m);
> @@ -1060,6 +1066,7 @@ static void pg_init_done(struct dm_path *path, int errors)
> struct priority_group *pg = pgpath->pg;
> struct multipath *m = pg->m;
> unsigned long flags;
> + unsigned delay = 0;
>
> /* device or driver problems */
> switch (errors) {
> @@ -1084,8 +1091,11 @@ static void pg_init_done(struct dm_path *path, int errors)
> */
> bypass_pg(m, pg, 1);
> break;
> - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */
> + /*
> + * For SCSI_DH_RETRY we wait before retrying.
> + */
> case SCSI_DH_RETRY:
> + delay = 1;
> case SCSI_DH_IMM_RETRY:
> case SCSI_DH_RES_TEMP_UNAVAIL:
> if (pg_init_limit_reached(m, pgpath))
> @@ -1112,6 +1122,7 @@ static void pg_init_done(struct dm_path *path, int errors)
> }
>
> m->pg_init_in_progress = 0;
> + m->pg_init_delay = delay;
> queue_work(kmultipathd, &m->process_queued_ios);
> spin_unlock_irqrestore(&m->lock, flags);
> }
> @@ -1120,7 +1131,7 @@ static void activate_path(struct work_struct *work)
> {
> int ret;
> struct multipath *m =
> - container_of(work, struct multipath, activate_path);
> + container_of(work, struct multipath, activate_path.work);
> struct dm_path *path;
> unsigned long flags;
>
> diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
> index 33efce2..f099d86 100644
> --- a/include/scsi/scsi_dh.h
> +++ b/include/scsi/scsi_dh.h
> @@ -55,6 +55,10 @@ enum {
> SCSI_DH_NOSYS,
> SCSI_DH_DRIVER_MAX,
> };
> +
> +/* Time to wait before retry in case of SCSI_DH_RETRY */
> +#define SCSI_DH_RETRY_DELAY ((HZ * 2))
> +
> #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
> extern int scsi_dh_activate(struct request_queue *);
> extern int scsi_dh_handler_exist(const char *);
> �

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-02-2009, 09:48 AM
Nikanth Karthikesan
 
Default dm mpath: delay retry activate_path on SCSI_DH_RETRY

On Saturday 21 February 2009 02:41:33 Chandra Seetharaman wrote:
> On Fri, 2009-02-20 at 10:33 +0530, Nikanth Karthikesan wrote:
> > On Friday 20 February 2009 06:15:29 Chandra Seetharaman wrote:
> > > On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote:
> > >
> > > <snip>
> > >
> > > > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct
> > > > *work) struct pgpath *pgpath = NULL;
> > > > unsigned init_required = 0, must_queue = 1;
> > > > unsigned long flags;
> > > > + unsigned long delay = 0;
> > >
> > > I do not see the reason for this variable, you can as well put the
> > > delay in pg_init_delay and use it directly (and set it to zero after
> > > using it) ?
> >
> > I missed resetting pg_init_delay to zero after using it. I have attached
> > the corrected patch with this. This variable keeps the code
> > cleaner(avoids taking m->lock). Also having only a boolean in struct
> > multipath keeps it a bit smaller.
>
> Ok. I am fine with your justification.
>
> > off-topic:
> > I think struct multipath can be shrunk even further by making various
> > flags like pg_init_required, pg_init_in_progress, queue_io,
> > queue_if_no_path, saved_queue_if_no_path in to a single variable.
> > Thoughts?
>
> As Konrad mentioned, it is worth the cost of readability ?
>
>
> <snip>
>
> > SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry
> > immediately and SCSI_DH_RETRY in cases where it is better to retry
> > after some delay.
> >
> > Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and
> > SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to
> > activate a device, if it returns SCSI_DH_RETRY.
> >
> > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
>
> Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>
>


Hi Alasdair

Can you merge this?

Thanks
Nikanth

> > ---
> >
> > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > index 095f77b..7ddf775 100644
> > --- a/drivers/md/dm-mpath.c
> > +++ b/drivers/md/dm-mpath.c
> > @@ -65,12 +65,13 @@ struct multipath {
> > spinlock_t lock;
> >
> > const char *hw_handler_name;
> > - struct work_struct activate_path;
> > + struct delayed_work activate_path;
> > struct pgpath *pgpath_to_activate;
> > unsigned nr_priority_groups;
> > struct list_head priority_groups;
> > unsigned pg_init_required; /* pg_init needs calling? */
> > unsigned pg_init_in_progress; /* Only one pg_init allowed at once */
> > + unsigned pg_init_delay; /* delay required before retry? */
> >
> > unsigned nr_valid_paths; /* Total number of usable paths */
> > struct pgpath *current_pgpath;
> > @@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct
> > dm_target *ti) m->queue_io = 1;
> > INIT_WORK(&m->process_queued_ios, process_queued_ios);
> > INIT_WORK(&m->trigger_event, trigger_event);
> > - INIT_WORK(&m->activate_path, activate_path);
> > + INIT_DELAYED_WORK(&m->activate_path, activate_path);
> > m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
> > if (!m->mpio_pool) {
> > kfree(m);
> > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct
> > *work) struct pgpath *pgpath = NULL;
> > unsigned init_required = 0, must_queue = 1;
> > unsigned long flags;
> > + unsigned long delay = 0;
> >
> > spin_lock_irqsave(&m->lock, flags);
> >
> > @@ -452,13 +454,17 @@ static void process_queued_ios(struct work_struct
> > *work) m->pg_init_required = 0;
> > m->pg_init_in_progress = 1;
> > init_required = 1;
> > + if (m->pg_init_delay) {
> > + delay = SCSI_DH_RETRY_DELAY;
> > + m->pg_init_delay = 0;
> > + }
> > }
> >
> > out:
> > spin_unlock_irqrestore(&m->lock, flags);
> >
> > if (init_required)
> > - queue_work(kmpath_handlerd, &m->activate_path);
> > + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay);
> >
> > if (!must_queue)
> > dispatch_queued_ios(m);
> > @@ -1060,6 +1066,7 @@ static void pg_init_done(struct dm_path *path, int
> > errors) struct priority_group *pg = pgpath->pg;
> > struct multipath *m = pg->m;
> > unsigned long flags;
> > + unsigned delay = 0;
> >
> > /* device or driver problems */
> > switch (errors) {
> > @@ -1084,8 +1091,11 @@ static void pg_init_done(struct dm_path *path, int
> > errors) */
> > bypass_pg(m, pg, 1);
> > break;
> > - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */
> > + /*
> > + * For SCSI_DH_RETRY we wait before retrying.
> > + */
> > case SCSI_DH_RETRY:
> > + delay = 1;
> > case SCSI_DH_IMM_RETRY:
> > case SCSI_DH_RES_TEMP_UNAVAIL:
> > if (pg_init_limit_reached(m, pgpath))
> > @@ -1112,6 +1122,7 @@ static void pg_init_done(struct dm_path *path, int
> > errors) }
> >
> > m->pg_init_in_progress = 0;
> > + m->pg_init_delay = delay;
> > queue_work(kmultipathd, &m->process_queued_ios);
> > spin_unlock_irqrestore(&m->lock, flags);
> > }
> > @@ -1120,7 +1131,7 @@ static void activate_path(struct work_struct *work)
> > {
> > int ret;
> > struct multipath *m =
> > - container_of(work, struct multipath, activate_path);
> > + container_of(work, struct multipath, activate_path.work);
> > struct dm_path *path;
> > unsigned long flags;
> >
> > diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
> > index 33efce2..f099d86 100644
> > --- a/include/scsi/scsi_dh.h
> > +++ b/include/scsi/scsi_dh.h
> > @@ -55,6 +55,10 @@ enum {
> > SCSI_DH_NOSYS,
> > SCSI_DH_DRIVER_MAX,
> > };
> > +
> > +/* Time to wait before retry in case of SCSI_DH_RETRY */
> > +#define SCSI_DH_RETRY_DELAY ((HZ * 2))
> > +
> > #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
> > extern int scsi_dh_activate(struct request_queue *);
> > extern int scsi_dh_handler_exist(const char *);
> > �

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

Thread Tools




All times are GMT. The time now is 04:57 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org