[dm-devel] [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.
Chandra Seetharaman
sekharan at us.ibm.com
Mon Jul 30 19:26:37 UTC 2007
Definitions of pg_init_retries and pg_init_count still uses spaces
(instead of tabs, which is the case with rest of the definitions around.
Other than that it looks good.
thanks
chandra
On Mon, 2007-07-30 at 14:54 -0400, Dave Wysochanski wrote:
> On Thu, 2007-07-26 at 12:15 -0700, Chandra Seetharaman wrote:
> > On Thu, 2007-07-26 at 00:44 -0400, dwysocha at redhat.com wrote:
> > > plain text document attachment (dm-mpath-add-retry-pg-init.patch)
> > > This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
> > > core. The flag is a generic one, but in this instance we use it to flag
> > > cases where we must retry a pg_init command. The patch is useful for cases
> > > where a hw handler sends a path initialization command to the storage and
> > > it sees the command complete with an error code indicating the command
> > > should be retried. In this case, the hardware handler should call
> > > dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
> > > to the dm-mpath core to retry the pg_init. However, it is not a guarantee
> > > that the dm-mpath core will actually retry the pg_init. The number of actual
> > > retries is governed by the multipath feature argument "pg_init_retries".
> > > Once the dm-mpath core has retried the command "pg_init_retries" times
> > > without success, a subsequent dm_pg_init_complete() with MP_RETRY will
> > > cause the path to be failed via fail_path(). To specify a value of
> > > pg_init_retries, add a line similar to the following in the 'device' section
> > > of your /etc/multipath.conf file:
> > > features "2 pg_init_retries 7"
> > >
> > >
> > >
> > > Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
> > > +++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > > @@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
> > > #define MP_FAIL_PATH 1
> > > #define MP_BYPASS_PG 2
> > > #define MP_ERROR_IO 4 /* Don't retry this I/O */
> > > +#define MP_RETRY 8
> > >
> > > #endif
> > > Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
> > > +++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > > @@ -75,6 +75,8 @@ struct multipath {
> > > unsigned queue_io; /* Must we queue all I/O? */
> > > unsigned queue_if_no_path; /* Queue I/O if last path fails? */
> > > 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 */
> > >
> > > struct work_struct process_queued_ios;
> > > struct bio_list queued_ios;
> > > @@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
> > > if (hwh->type && hwh->type->pg_init) {
> > > m->pg_init_required = 1;
> > > m->queue_io = 1;
> > > + m->pg_init_count = 0;
> > > } else {
> > > m->pg_init_required = 0;
> > > m->queue_io = 0;
> > > @@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
> > > must_queue = 0;
> > >
> > > if (m->pg_init_required && !m->pg_init_in_progress) {
> > > + m->pg_init_count++;
> > > m->pg_init_required = 0;
> > > m->pg_init_in_progress = 1;
> > > init_required = 1;
> > > @@ -689,9 +693,11 @@ static int parse_features(struct arg_set
> > > int r;
> > > unsigned argc;
> > > struct dm_target *ti = m->ti;
> > > + char *name;
> > >
> > > static struct param _params[] = {
> > > - {0, 1, "invalid number of feature args"},
> > > + {0, 4, "invalid number of feature args"},
> >
> > Isn't it "3" (instead of "4") ?
> >
> > > + {0, 50, "invalid number of pg_init retries"},
> > > };
> > >
> > > r = read_param(_params, shift(as), &argc, &ti->error);
> > > @@ -701,12 +707,26 @@ static int parse_features(struct arg_set
> > > if (!argc)
> > > return 0;
> > >
> > > - if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
> > > - return queue_if_no_path(m, 1, 0);
> > > - else {
> > > - ti->error = "Unrecognised multipath feature request";
> > > - return -EINVAL;
> > > + while (argc && !r) {
> > > + name = shift(as);
> > > + argc--;
> > > + if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
> > > + r = queue_if_no_path(m, 1, 0);
> > > + DMDEBUG("setting queue_if_no_path");
> >
> > Shouldn't this DEBUG be printed only when r == 0 ?
> >
> > > + } else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
> > > + (argc >= 1)) {
> >
> > mixed use of space/tab.
> > > + r = read_param(_params + 1, shift(as),
> > > + &m->pg_init_retries, &ti->error);
> > > + argc--;
> > > + DMDEBUG("setting pg_init_retries to %u",
> > > + m->pg_init_retries);
> >
> > Shouldn't this DEBUG be printed only when r == 0 ?
> > > + } else {
> > > + ti->error = "Unrecognised multipath feature request";
> > > + return -EINVAL;
> > > + }
> > > }
> > > +
> > > + return r;
> > > }
> > >
> > > static int multipath_ctr(struct dm_target *ti, unsigned int argc,
> > > @@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
> > > }
> > >
> > > /*
> > > + * Retry pg_init on the same path group and path
> > > + */
> > > +static void retry_pg(struct multipath *m, struct pgpath *pgpath)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&m->lock, flags);
> > > + if (m->pg_init_count <= m->pg_init_retries)
> > > + m->pg_init_required = 1;
> > > + else
> > > + fail_path(pgpath);
> > > + spin_unlock_irqrestore(&m->lock, flags);
> > > +}
> > > +
> > > +/*
> > > * pg_init must call this when it has completed its initialisation
> > > */
> > > void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
> > > @@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path
> > > if (err_flags & MP_BYPASS_PG)
> > > bypass_pg(m, pg, 1);
> > >
> > > + if (err_flags & MP_RETRY)
> > > + retry_pg(m, pgpath);
> > > +
> > > spin_lock_irqsave(&m->lock, flags);
> > > - if (err_flags) {
> > > + if (err_flags & ~MP_RETRY) {
> > > m->current_pgpath = NULL;
> > > m->current_pg = NULL;
> > > } else if (!m->pg_init_required)
> > > @@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
> > > /* Features */
> > > if (type == STATUSTYPE_INFO)
> > > DMEMIT("1 %u ", m->queue_size);
> > > - else if (m->queue_if_no_path)
> > > + else if (m->queue_if_no_path && !m->pg_init_retries)
> > > DMEMIT("1 queue_if_no_path ");
> > > + else if (!m->queue_if_no_path && m->pg_init_retries)
> > > + DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
> > > + else if (m->queue_if_no_path && m->pg_init_retries)
> > > + DMEMIT("3 queue_if_no_path pg_init_retries %u ",
> > > + m->pg_init_retries);
> > > else
> > > DMEMIT("0 ");
> > >
> > >
>
> The attached patch should address your comments. I removed the DMDEBUG
> statements as they did not seem too useful beyond basic tests.
>
>
--
----------------------------------------------------------------------
Chandra Seetharaman | Be careful what you choose....
- sekharan at us.ibm.com | .......you may get it.
----------------------------------------------------------------------
More information about the dm-devel
mailing list