[PATCH] [RFC] multipath: change the DEFAULT_MINIO for the request based multipath

[PATCH] [RFC] multipath: change the DEFAULT_MINIO for the request based multipath

[Malahal Naineni]

The value of 1000 is good for bio based multipath. Seen 50% increase in
I/O ops by setting it to 1 in request based multipath configuration.
This patch would give poor performance for people still using the bio
based multipath!

Is it possible to detect request based multipath and change only for
those configurations?

diff -r e504a50b0db5 -r 91d6dae7d882 libmultipath/defaults.h
--- a/libmultipath/defaults.h	Wed Jan 19 13:16:40 2011 -0800
+++ b/libmultipath/defaults.h	Wed Jan 19 13:29:52 2011 -0800
@@ -5,7 +5,7 @@
 #define DEFAULT_ALIAS_PREFIX	"mpath"
 #define DEFAULT_FEATURES	"0"
 #define DEFAULT_HWHANDLER	"0"
-#define DEFAULT_MINIO		1000
+#define DEFAULT_MINIO		1
 #define DEFAULT_PGPOLICY       FAILOVER
 #define DEFAULT_FAILBACK       -FAILBACK_MANUAL
 #define DEFAULT_RR_WEIGHT      RR_WEIGHT_NONE

Re: [PATCH] [RFC] multipath: change the DEFAULT_MINIO for the request based multipath

[Hannes Reinecke]

On 01/19/2011 10:30 PM, Malahal Naineni wrote:
> The value of 1000 is good for bio based multipath. Seen 50% increase in
> I/O ops by setting it to 1 in request based multipath configuration.
> This patch would give poor performance for people still using the bio
> based multipath!
> 
> Is it possible to detect request based multipath and change only for
> those configurations?
> 
> diff -r e504a50b0db5 -r 91d6dae7d882 libmultipath/defaults.h
> --- a/libmultipath/defaults.h	Wed Jan 19 13:16:40 2011 -0800
> +++ b/libmultipath/defaults.h	Wed Jan 19 13:29:52 2011 -0800
> @@ -5,7 +5,7 @@
>  #define DEFAULT_ALIAS_PREFIX	"mpath"
>  #define DEFAULT_FEATURES	"0"
>  #define DEFAULT_HWHANDLER	"0"
> -#define DEFAULT_MINIO		1000
> +#define DEFAULT_MINIO		1
>  #define DEFAULT_PGPOLICY       FAILOVER
>  #define DEFAULT_FAILBACK       -FAILBACK_MANUAL
>  #define DEFAULT_RR_WEIGHT      RR_WEIGHT_NONE
> 
Heh, that was the main reason for using request-based multipathing :-)

So yes, changing the behaviour is a good idea. But we should equally
be able to detect if request-based multipathing is present; maybe
we can key off the version number of dm-multipath?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Mike Snitzer]

On Thu, Jan 20 2011 at  2:32am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 01/19/2011 10:30 PM, Malahal Naineni wrote:
> > The value of 1000 is good for bio based multipath. Seen 50% increase in
> > I/O ops by setting it to 1 in request based multipath configuration.
> > This patch would give poor performance for people still using the bio
> > based multipath!
> > 
> > Is it possible to detect request based multipath and change only for
> > those configurations?
> > 
> > diff -r e504a50b0db5 -r 91d6dae7d882 libmultipath/defaults.h
> > --- a/libmultipath/defaults.h	Wed Jan 19 13:16:40 2011 -0800
> > +++ b/libmultipath/defaults.h	Wed Jan 19 13:29:52 2011 -0800
> > @@ -5,7 +5,7 @@
> >  #define DEFAULT_ALIAS_PREFIX	"mpath"
> >  #define DEFAULT_FEATURES	"0"
> >  #define DEFAULT_HWHANDLER	"0"
> > -#define DEFAULT_MINIO		1000
> > +#define DEFAULT_MINIO		1
> >  #define DEFAULT_PGPOLICY       FAILOVER
> >  #define DEFAULT_FAILBACK       -FAILBACK_MANUAL
> >  #define DEFAULT_RR_WEIGHT      RR_WEIGHT_NONE
> > 
> Heh, that was the main reason for using request-based multipathing :-)

Exactly, since I/O merging occurs before entering request-based
multipath, frequent round-robin path switching doesn't cause the number
of overall requests to increase (like it did for bio-based).

Given the data from the 2007 Linux Symposim request-based DM paper (see
[1], figure 7), a safer rr_min_io default for both bio-based and
request-based might be 50 or 100.

But regardless of the default, no one size fits all.

Depending on the FC storage backend, a rr_min_io of 1 may not _always_
be ideal for request-based DM.  But clearly additional testing would be
needed.

> So yes, changing the behaviour is a good idea. But we should equally
> be able to detect if request-based multipathing is present; maybe
> we can key off the version number of dm-multipath?

Unfortunately, the multipath target version wasn't bumped when
request-based mpath was introduced (commit f40c67f0f7e2767f).

Similarly, the DM ioctl version wasn't bumped when the DM core was
updated to support request-based (primarily via commits e6ee8c0b767540
and cec47e3d4a861e).

Fortunately, the DM ioctl version was bumped in a release (therefore a
dedicated version bump related to request-based DM changes was deemed
unnecessary).

$ git log --tags --source --oneline include/linux/dm-ioctl.h
...
60935eb v2.6.31-rc1 dm ioctl: support cookies for udev

$ git log --tags --source --oneline drivers/md/dm.c
...
a77e28c v2.6.31-rc9 dm multipath: fix oops when request based io fails when no paths
a732c20 v2.6.31-rc5 dm: remove queue next_ordered workaround for barriers
7878cba v2.6.31-rc2 block: Create bip slabs with embedded integrity vectors
523d929 v2.6.31-rc1 dm: disable interrupt when taking map_lock
5d67aa2 v2.6.31-rc1 dm: do not set QUEUE_ORDERED_DRAIN if request based
e6ee8c0 v2.6.31-rc1 dm: enable request based option
cec47e3 v2.6.31-rc1 dm: prepare for request based option

Long story short, it seems we _could_ use the DM ioctl version bump that
occurred for 2.6.31 (from commit 60935eb):  4.15.0-ioctl (2009-04-01) 

Mike


[1] http://www.linuxsymposium.org/archives/OLS/Reprints-2007/ueda-Reprint.pdf

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Jun'ichi Nomura]

Hi,

On 01/21/11 01:07, Mike Snitzer wrote:
>>> Is it possible to detect request based multipath and change only for
>>> those configurations?
...
> Unfortunately, the multipath target version wasn't bumped when
> request-based mpath was introduced (commit f40c67f0f7e2767f).

Um, sorry I don't remember why..

> Long story short, it seems we _could_ use the DM ioctl version bump that
> occurred for 2.6.31 (from commit 60935eb):  4.15.0-ioctl (2009-04-01) 

Fortunately, the target version of multipath was also bumped
in 2.6.31, from 1.0.5 to 1.1.0 by this commit:

af4874e v2.6.31-rc1 dm target:s introduce iterate devices fn

-- 
Jun'ichi Nomura, NEC Corporation

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Christophe Varoqui]

> 
> Fortunately, the target version of multipath was also bumped
> in 2.6.31, from 1.0.5 to 1.1.0 by this commit:
> 
> af4874e v2.6.31-rc1 dm target:s introduce iterate devices fn
> 
Fortunate indeed.

Should the 'minio' be a tunable in the request-based multipath context,
or should we hardcode its value to '1' ?

If we keep it a tunable, I guess we'll have to introduce a different
'minio' configuration parameter ('rr_min_io_rq' for example) to permit
vendors to set their defaults for distributions shipping a rq-capable
kernel or not. This change would be pretty invasive.

Please advise.
-- 
Christophe Varoqui <christophe.varoqui@opensvc.com>
OpenSVC

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Mike Snitzer]

On Fri, Jan 21 2011 at  3:47am -0500,
Christophe Varoqui <christophe.varoqui@gmail.com> wrote:

> 
> > 
> > Fortunately, the target version of multipath was also bumped
> > in 2.6.31, from 1.0.5 to 1.1.0 by this commit:
> > 
> > af4874e v2.6.31-rc1 dm target:s introduce iterate devices fn
> > 
> Fortunate indeed.
> 
> Should the 'minio' be a tunable in the request-based multipath context,
> or should we hardcode its value to '1' ?

As my earlier response indicated: I'm not convinced _always_ using 1 for
request-based is appropriate/best.
 
> If we keep it a tunable, I guess we'll have to introduce a different
> 'minio' configuration parameter ('rr_min_io_rq' for example) to permit
> vendors to set their defaults for distributions shipping a rq-capable
> kernel or not. This change would be pretty invasive.

I'm not following why we'd need a different configuration parameter.
It is just that the default rr_min_io that would be used would be
conditional on the multipath target version being >= 1.1.0.

Mike

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Christophe Varoqui]

> I'm not following why we'd need a different configuration parameter.
> It is just that the default rr_min_io that would be used would be
> conditional on the multipath target version being >= 1.1.0.
> 
Defaults are layered. For current minio, we have :
[1] one top level default (hardcoded, superseded by config)
[2] per hardware default (hardcoded, superseded by config)
[3] per multipath value (none hardcoded, defined by config)

You suggest multipath-tools to adapt only the top level minio default
depending on dm-rq availability [1], but what of the hwtable defaults
[2] ? Should we provide vendors with a way to describe a with-rq minio
default *and* a without-rq minio default (a new parameter in the hwentry
struct) ? If so, we should also provide a new config file keyword to
override this new hwentry parameter hardcoded value ... then the
reasoning cascades to the mpentry struct minio setting [3].

Actually, [1] is hardly the common case : only unknown hardware resort
to these defaults.

Is my reasoning flawed ?

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Malahal Naineni]

Christophe Varoqui [christophe.varoqui@gmail.com] wrote:
> 
> > I'm not following why we'd need a different configuration parameter.
> > It is just that the default rr_min_io that would be used would be
> > conditional on the multipath target version being >= 1.1.0.
> > 
> Defaults are layered. For current minio, we have :
> [1] one top level default (hardcoded, superseded by config)
> [2] per hardware default (hardcoded, superseded by config)
> [3] per multipath value (none hardcoded, defined by config)
> 
> You suggest multipath-tools to adapt only the top level minio default
> depending on dm-rq availability [1], but what of the hwtable defaults
> [2] ? Should we provide vendors with a way to describe a with-rq minio
> default *and* a without-rq minio default (a new parameter in the hwentry
> struct) ? If so, we should also provide a new config file keyword to
> override this new hwentry parameter hardcoded value ... then the
> reasoning cascades to the mpentry struct minio setting [3].
> 
> Actually, [1] is hardly the common case : only unknown hardware resort
> to these defaults.
> 
> Is my reasoning flawed ?

I don't think so. Your reasoning is right. How about this:

1. Set DEFAULT_MINIO to -1
2. If bio based mapping and the value is -1, set it to 1000
   (DEFAULT_BIO_MINIO)
3. If request based mapping, set it to DEFAULT_REQUEST_MINIO.

Note that devices can't have individual hardware default for request
based mappings in this method and I think that should be OK. They are
allowed to individual hardware based default for BIO based mappings as
they have now. I will code it up if everyone agrees.

Thanks, Malahal.

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Jun'ichi Nomura]

Hi,

On 01/22/11 02:39, Malahal Naineni wrote:
> Christophe Varoqui [christophe.varoqui@gmail.com] wrote:
>>
>>> I'm not following why we'd need a different configuration parameter.
>>> It is just that the default rr_min_io that would be used would be
>>> conditional on the multipath target version being >= 1.1.0.
>>>
>> Defaults are layered. For current minio, we have :
>> [1] one top level default (hardcoded, superseded by config)
>> [2] per hardware default (hardcoded, superseded by config)
>> [3] per multipath value (none hardcoded, defined by config)
>>
>> You suggest multipath-tools to adapt only the top level minio default
>> depending on dm-rq availability [1], but what of the hwtable defaults
>> [2] ? Should we provide vendors with a way to describe a with-rq minio
>> default *and* a without-rq minio default (a new parameter in the hwentry
>> struct) ? If so, we should also provide a new config file keyword to
>> override this new hwentry parameter hardcoded value ... then the
>> reasoning cascades to the mpentry struct minio setting [3].
>>
>> Actually, [1] is hardly the common case : only unknown hardware resort
>> to these defaults.

Do we really need [2]?

Currently, there seem only 3 minio values in per-hardware default:
  - 1000 (DEFAULT_MINIO)
  - 128
  - 100

I think both 128 and 100 are based on that, in many systems/cases,
max request size (max_sectors) is 512KB and BIO submission
is done in page-size unit (4KB).

If there isn't strong reason for the current default (1000)
(and I think there isn't), it seems we can remove [2]
after changing the default to 128 (or 100).

>> Is my reasoning flawed ?
> 
> I don't think so. Your reasoning is right. How about this:
> 
> 1. Set DEFAULT_MINIO to -1
> 2. If bio based mapping and the value is -1, set it to 1000
>    (DEFAULT_BIO_MINIO)
> 3. If request based mapping, set it to DEFAULT_REQUEST_MINIO.
> 
> Note that devices can't have individual hardware default for request
> based mappings in this method and I think that should be OK. They are
> allowed to individual hardware based default for BIO based mappings as
> they have now. I will code it up if everyone agrees.

Does it mean users can't change rr_min_io value for request-based dm?
I suspect it is possible that someone wants to set non-default
value to minio for request-based dm.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Malahal Naineni]

Jun'ichi Nomura [j-nomura@ce.jp.nec.com] wrote:
> >> Defaults are layered. For current minio, we have :
> >> [1] one top level default (hardcoded, superseded by config)
> >> [2] per hardware default (hardcoded, superseded by config)
> >> [3] per multipath value (none hardcoded, defined by config)
> >>
> >> You suggest multipath-tools to adapt only the top level minio default
> >> depending on dm-rq availability [1], but what of the hwtable defaults
> >> [2] ? Should we provide vendors with a way to describe a with-rq minio
> >> default *and* a without-rq minio default (a new parameter in the hwentry
> >> struct) ? If so, we should also provide a new config file keyword to
> >> override this new hwentry parameter hardcoded value ... then the
> >> reasoning cascades to the mpentry struct minio setting [3].
> >>
> >> Actually, [1] is hardly the common case : only unknown hardware resort
> >> to these defaults.
> 
> Do we really need [2]?
> 
> Currently, there seem only 3 minio values in per-hardware default:
>   - 1000 (DEFAULT_MINIO)
>   - 128
>   - 100
> 
> I think both 128 and 100 are based on that, in many systems/cases,
> max request size (max_sectors) is 512KB and BIO submission
> is done in page-size unit (4KB).
> 
> If there isn't strong reason for the current default (1000)
> (and I think there isn't), it seems we can remove [2]
> after changing the default to 128 (or 100).
> 
> > 1. Set DEFAULT_MINIO to -1
> > 2. If bio based mapping and the value is -1, set it to 1000
> >    (DEFAULT_BIO_MINIO)
> > 3. If request based mapping, set it to DEFAULT_REQUEST_MINIO.
> 
> Does it mean users can't change rr_min_io value for request-based dm?
> I suspect it is possible that someone wants to set non-default
> value to minio for request-based dm.

Your reading is correct that my proposal doesn't allow it. If we get away
with [2] above (hwatable.c based values), then we can have something
like this:

1. Remove it from hwentry structure or set it to -1
2. If rr_minio is specified in the conf file (either through default or
   device section etc), use it.
3. If not specified in the config file, use DEFAULT_BIO_MINIO or
   DEFAULT_REQUEST_MINIO based on the multipath module version.

In other words, every device out there uses DEFAULT_BIO_MINIO or
DEFAULT_REQUEST_MINIO if not specified by the administrator.
Admin can override it in /etc/multipath.conf file.

CON: No per device controller default  (aka hwtable entry value)

Thanks, Malahal.

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[nishant mungse]

[-- Attachment #1.1: Type: text/plain, Size: 3235 bytes --]

On Tue, Jan 25, 2011 at 9:23 PM, Malahal Naineni <malahal@us.ibm.com> wrote:

> Jun'ichi Nomura [j-nomura@ce.jp.nec.com] wrote:
> > >> Defaults are layered. For current minio, we have :
> > >> [1] one top level default (hardcoded, superseded by config)
> > >> [2] per hardware default (hardcoded, superseded by config)
> > >> [3] per multipath value (none hardcoded, defined by config)
> > >>
> > >> You suggest multipath-tools to adapt only the top level minio default
> > >> depending on dm-rq availability [1], but what of the hwtable defaults
> > >> [2] ? Should we provide vendors with a way to describe a with-rq minio
> > >> default *and* a without-rq minio default (a new parameter in the
> hwentry
> > >> struct) ? If so, we should also provide a new config file keyword to
> > >> override this new hwentry parameter hardcoded value ... then the
> > >> reasoning cascades to the mpentry struct minio setting [3].
> > >>
> > >> Actually, [1] is hardly the common case : only unknown hardware resort
> > >> to these defaults.
> >
> > Do we really need [2]?
> >
> > Currently, there seem only 3 minio values in per-hardware default:
> >   - 1000 (DEFAULT_MINIO)
> >   - 128
> >   - 100
> >
> > I think both 128 and 100 are based on that, in many systems/cases,
> > max request size (max_sectors) is 512KB and BIO submission
> > is done in page-size unit (4KB).
> >
> > If there isn't strong reason for the current default (1000)
> > (and I think there isn't), it seems we can remove [2]
> > after changing the default to 128 (or 100).
> >
> > > 1. Set DEFAULT_MINIO to -1
> > > 2. If bio based mapping and the value is -1, set it to 1000
> > >    (DEFAULT_BIO_MINIO)
> > > 3. If request based mapping, set it to DEFAULT_REQUEST_MINIO.
> >
> > Does it mean users can't change rr_min_io value for request-based dm?
> > I suspect it is possible that someone wants to set non-default
> > value to minio for request-based dm.
>
> Your reading is correct that my proposal doesn't allow it. If we get away
> with [2] above (hwatable.c based values), then we can have something
> like this:
>
> 1. Remove it from hwentry structure or set it to -1
> 2. If rr_minio is specified in the conf file (either through default or
>   device section etc), use it.
> 3. If not specified in the config file, use DEFAULT_BIO_MINIO or
>   DEFAULT_REQUEST_MINIO based on the multipath module version.
>
> In other words, every device out there uses DEFAULT_BIO_MINIO or
> DEFAULT_REQUEST_MINIO if not specified by the administrator.
> Admin can override it in /etc/multipath.conf file.
>
> CON: No per device controller default  (aka hwtable entry value)
>
> Thanks, Malahal.
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>


hello all,

There is one doubt regarding the dm-raid1. As far as i know, in dm-raid1 the
data is written parallelly  on all the mirrors of mirrorset and if any of
the mirror fails to write the data then dm-mirror adds this mirror to fail
list by increasing the error count in "fail mirror" function in dm-raid1.
Actually my doubt is where this error count is decremented? i.e after kcpyd
or before and where exactly this error count is decremented?

Regards,
Nishant.

[-- Attachment #1.2: Type: text/html, Size: 4163 bytes --]

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

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[nishant mungse]

[-- Attachment #1.1: Type: text/plain, Size: 4228 bytes --]

On Wed, Jan 26, 2011 at 12:27 PM, nishant mungse <nishantmungse@gmail.com>wrote:

>
>
> On Tue, Jan 25, 2011 at 9:23 PM, Malahal Naineni <malahal@us.ibm.com>wrote:
>
>> Jun'ichi Nomura [j-nomura@ce.jp.nec.com] wrote:
>> > >> Defaults are layered. For current minio, we have :
>> > >> [1] one top level default (hardcoded, superseded by config)
>> > >> [2] per hardware default (hardcoded, superseded by config)
>> > >> [3] per multipath value (none hardcoded, defined by config)
>> > >>
>> > >> You suggest multipath-tools to adapt only the top level minio default
>> > >> depending on dm-rq availability [1], but what of the hwtable defaults
>> > >> [2] ? Should we provide vendors with a way to describe a with-rq
>> minio
>> > >> default *and* a without-rq minio default (a new parameter in the
>> hwentry
>> > >> struct) ? If so, we should also provide a new config file keyword to
>> > >> override this new hwentry parameter hardcoded value ... then the
>> > >> reasoning cascades to the mpentry struct minio setting [3].
>> > >>
>> > >> Actually, [1] is hardly the common case : only unknown hardware
>> resort
>> > >> to these defaults.
>> >
>> > Do we really need [2]?
>> >
>> > Currently, there seem only 3 minio values in per-hardware default:
>> >   - 1000 (DEFAULT_MINIO)
>> >   - 128
>> >   - 100
>> >
>> > I think both 128 and 100 are based on that, in many systems/cases,
>> > max request size (max_sectors) is 512KB and BIO submission
>> > is done in page-size unit (4KB).
>> >
>> > If there isn't strong reason for the current default (1000)
>> > (and I think there isn't), it seems we can remove [2]
>> > after changing the default to 128 (or 100).
>> >
>> > > 1. Set DEFAULT_MINIO to -1
>> > > 2. If bio based mapping and the value is -1, set it to 1000
>> > >    (DEFAULT_BIO_MINIO)
>> > > 3. If request based mapping, set it to DEFAULT_REQUEST_MINIO.
>> >
>> > Does it mean users can't change rr_min_io value for request-based dm?
>> > I suspect it is possible that someone wants to set non-default
>> > value to minio for request-based dm.
>>
>> Your reading is correct that my proposal doesn't allow it. If we get away
>> with [2] above (hwatable.c based values), then we can have something
>> like this:
>>
>> 1. Remove it from hwentry structure or set it to -1
>> 2. If rr_minio is specified in the conf file (either through default or
>>   device section etc), use it.
>> 3. If not specified in the config file, use DEFAULT_BIO_MINIO or
>>   DEFAULT_REQUEST_MINIO based on the multipath module version.
>>
>> In other words, every device out there uses DEFAULT_BIO_MINIO or
>> DEFAULT_REQUEST_MINIO if not specified by the administrator.
>> Admin can override it in /etc/multipath.conf file.
>>
>> CON: No per device controller default  (aka hwtable entry value)
>>
>> Thanks, Malahal.
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
>>
>
>
> hello all,
>
> There is one doubt regarding the dm-raid1. As far as i know, in dm-raid1
> the data is written parallelly  on all the mirrors of mirrorset and if any
> of the mirror fails to write the data then dm-mirror adds this mirror to
> fail list by increasing the error count in "fail mirror" function in
> dm-raid1.
> Actually my doubt is where this error count is decremented? i.e after kcpyd
> or before and where exactly this error count is decremented?
>
> Regards,
> Nishant.
>



Hi,

I've created a device mapper target and I wanna test its read and
write mechanisms. I created a device using this target and tried to
write a filesystem on to the device.

My target is creating roughly two outgoing bios per incoming bio.
Which is maybe why I'm not able to get a proper log after running the
mke2fs command as while writing the filesystem, there are massive
amounts of writes performed on the device.

So, I tried to use the cat command to directly write to the device
file. The write is being performed for sure because I'm able to view
the data written to the underlying device.

But there is no output when I try to read the device file contents using
cat.

Is there any other way to test the read/write workflows of a dm target
that can give out a proper log ?

Regards,
Nishant.

[-- Attachment #1.2: Type: text/html, Size: 5547 bytes --]

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

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Christophe Varoqui]

On mar., 2011-01-25 at 18:23 -0800, Malahal Naineni wrote:
> Jun'ichi Nomura [j-nomura@ce.jp.nec.com] wrote:
> > >> Defaults are layered. For current minio, we have :
> > >> [1] one top level default (hardcoded, superseded by config)
> > >> [2] per hardware default (hardcoded, superseded by config)
> > >> [3] per multipath value (none hardcoded, defined by config)
> > >>
> > >> You suggest multipath-tools to adapt only the top level minio default
> > >> depending on dm-rq availability [1], but what of the hwtable defaults
> > >> [2] ? Should we provide vendors with a way to describe a with-rq minio
> > >> default *and* a without-rq minio default (a new parameter in the hwentry
> > >> struct) ? If so, we should also provide a new config file keyword to
> > >> override this new hwentry parameter hardcoded value ... then the
> > >> reasoning cascades to the mpentry struct minio setting [3].
> > >>
> > >> Actually, [1] is hardly the common case : only unknown hardware resort
> > >> to these defaults.
> > 
> > Do we really need [2]?
> > 
> > Currently, there seem only 3 minio values in per-hardware default:
> >   - 1000 (DEFAULT_MINIO)
> >   - 128
> >   - 100
> > 
> > I think both 128 and 100 are based on that, in many systems/cases,
> > max request size (max_sectors) is 512KB and BIO submission
> > is done in page-size unit (4KB).
> > 
> > If there isn't strong reason for the current default (1000)
> > (and I think there isn't), it seems we can remove [2]
> > after changing the default to 128 (or 100).
> > 
> > > 1. Set DEFAULT_MINIO to -1
> > > 2. If bio based mapping and the value is -1, set it to 1000
> > >    (DEFAULT_BIO_MINIO)
> > > 3. If request based mapping, set it to DEFAULT_REQUEST_MINIO.
> > 
> > Does it mean users can't change rr_min_io value for request-based dm?
> > I suspect it is possible that someone wants to set non-default
> > value to minio for request-based dm.
> 
> Your reading is correct that my proposal doesn't allow it. If we get away
> with [2] above (hwatable.c based values), then we can have something
> like this:
> 
> 1. Remove it from hwentry structure or set it to -1
> 2. If rr_minio is specified in the conf file (either through default or
>    device section etc), use it.
> 3. If not specified in the config file, use DEFAULT_BIO_MINIO or
>    DEFAULT_REQUEST_MINIO based on the multipath module version.
> 
> In other words, every device out there uses DEFAULT_BIO_MINIO or
> DEFAULT_REQUEST_MINIO if not specified by the administrator.
> Admin can override it in /etc/multipath.conf file.
> 
> CON: No per device controller default  (aka hwtable entry value)
> 
I pushed an implementation of the fully capable solution.

The behaviour is unchanged with dm-multipath <= 1.1.0
For newer driver, if no additional config is added to multipath.conf the
minio will be set to 1.

If Mike S. tests prove this value is not always optimal, it can be tuned
per device in hwtable (hwe->minio_rq) and overridden in multipath.conf
device{} and multipath{} section.

This seemed to be the most consistent behaviour, to me. But feel free to
disagree.

Please test carefully as the patch is quite invasive. I built and tested
lightly with a NetApp iSCSI simulator. The session produced the
following output

Feb 01 00:18:51 | --------start up--------
Feb 01 00:18:51 | read /etc/multipath.conf
Feb 01 00:18:51 | activate request-based multipathing mode (driver >=
v1.1.0)
...
Feb 01 00:18:51 | 360a980006e424377536f576c6274694f: rr_weight = 1
(controller setting)
Feb 01 00:18:51 | 360a980006e424377536f576c6274694f: minio = 2 rq
(controller setting)
Feb 01 00:18:51 | 360a980006e424377536f576c6274694f: no_path_retry =
NONE (internal default)
Feb 01 00:18:51 | 360a980006e424377536f576c6274694f: pg_timeout = NONE
(internal default)
Feb 01 00:18:51 | 360a980006e424377536f576c6274694f: set ACT_CREATE (map
does not exist)
Feb 01 00:18:51 | 360a980006e424377536f576c6274694f: load table [0 30720
multipath 1 queue_if_no_path 0 1 1 round-robin 0 1 1 8:16 2]
...

Seems right.

As a side effect, the new devmapper.c:dm_drv_get_version() can be used
by further developments ... like Mike S. 'pg_num == 0' recent patch.

Regards,
-- 
Christophe Varoqui <christophe.varoqui@opensvc.com>
OpenSVC

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Malahal Naineni]

Christophe Varoqui [christophe.varoqui@gmail.com] wrote:
> I pushed an implementation of the fully capable solution.
> 
> The behaviour is unchanged with dm-multipath <= 1.1.0
> For newer driver, if no additional config is added to multipath.conf the
> minio will be set to 1.
> 
> If Mike S. tests prove this value is not always optimal, it can be tuned
> per device in hwtable (hwe->minio_rq) and overridden in multipath.conf
> device{} and multipath{} section.
> 
> This seemed to be the most consistent behaviour, to me. But feel free to
> disagree.
> 
> Please test carefully as the patch is quite invasive. I built and tested
> lightly with a NetApp iSCSI simulator. The session produced the
> following output

Where is the patch? I would like test it. I have a patch set that does
what I said and was planning on adding a field to hwentry structure
without adding a new config keyword in /etc/multipath.conf. It doesn't
appear that big a change, but would like to see your patch.

Thanks, Malahal.

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Mike Snitzer]

On Mon, Jan 31 2011 at  9:21pm -0500,
Malahal Naineni <malahal@us.ibm.com> wrote:

> Christophe Varoqui [christophe.varoqui@gmail.com] wrote:
> > I pushed an implementation of the fully capable solution.
> > 
> > The behaviour is unchanged with dm-multipath <= 1.1.0
> > For newer driver, if no additional config is added to multipath.conf the
> > minio will be set to 1.
> > 
> > If Mike S. tests prove this value is not always optimal, it can be tuned
> > per device in hwtable (hwe->minio_rq) and overridden in multipath.conf
> > device{} and multipath{} section.
> > 
> > This seemed to be the most consistent behaviour, to me. But feel free to
> > disagree.
> > 
> > Please test carefully as the patch is quite invasive. I built and tested
> > lightly with a NetApp iSCSI simulator. The session produced the
> > following output
> 
> Where is the patch? I would like test it. I have a patch set that does
> what I said and was planning on adding a field to hwentry structure
> without adding a new config keyword in /etc/multipath.conf. It doesn't
> appear that big a change, but would like to see your patch.

Christophe committed it already, see the following multipath-tools
commit:
2b68b83 Support different 'minio' values for rq and bio based dm-multipath

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Malahal Naineni]

Mike Snitzer [snitzer@redhat.com] wrote:
> Christophe committed it already, see the following multipath-tools
> commit:
> 2b68b83 Support different 'minio' values for rq and bio based dm-multipath

Thank you Mike for pointing it out.

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Malahal Naineni]

>Christophe Varoqui [christophe.varoqui@gmail.com] wrote:

>+dm_drv_get_rq (void)
>+{
>+	unsigned int minv_dmrq[3] = {1, 1, 0};
>+	unsigned int *v;
>+
>+	v = zalloc(3);
>+	if (!v)
>+		return 0;
>+
>+	if (dm_drv_version(v, TGT_MPATH)) {
>+		/* in doubt return least capable */
>+		return 0;
>+	}

Looks like the 'v' is NOT freed. Local stack allocation looks much
cleaner, why not do that? You missed the same thing at other places, so
I imagine you started with the on stack local structure but changed
later???

>+static int
>+dm_drvprereq (char * str)
>+{
>+	unsigned int minv[3] = {1, 0, 3};
>+	unsigned int *v;
>+
>+	v = zalloc(3);
>+	if (!v)
>+		return 0;
>+
>+	if (dm_drv_version(v, str)) {
>+		/* in doubt return not capable */
>+		return 1;
>+	}

Missed freeing 'v'. Also, this function taking the target driver name as
'str' doesn't make sense as the minimum version is hard coded internally
to this function. Take no arguments and pass 'TGT_MPATH' while calling
dm_drv_version.

> static int
>+def_minio_rq_handler(vector strvec)
>+{
>+	char * buff;
>+
>+	buff = set_value(strvec);
>+
>+	if (!buff)
>+		return 1;
>+
>+	conf->minio_rq = atoi(buff);
>+	FREE(buff);
>+
>+	return 0;
>+}

I was thinking why introduce minio and minio_rq in the
/etc/multipath.conf file. By default we ship empty /etc/multipath.conf
file. If the admin wants to override the default, he knows if he is
going to use the BIO or REQUEST based multipath. So here is my approach
to avoid introducing another similar looking config keyword:

If we detect minio setting from the config file (either in the
multipath, device or default section), we use it. The code internally
has minio_rq just as you did. In other words, the only change I am
proposing is, use the existing routines that handle minio keyword and
set minio_rq there.

E.g: def_minio_handler() will set conf->minio_rq = conf->minio
hw_minio_handler() will set hwe->minio_rq = hwe->minio
mp_minio_handler() will set mpe->minio_rq = mpe->minio

Thanks, Malahal.

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Christophe Varoqui]

On mar., 2011-02-01 at 00:13 -0800, Malahal Naineni wrote:
> >Christophe Varoqui [christophe.varoqui@gmail.com] wrote:
> 
> >+dm_drv_get_rq (void)
> >+{
> >+	unsigned int minv_dmrq[3] = {1, 1, 0};
> >+	unsigned int *v;
> >+
> >+	v = zalloc(3);
> >+	if (!v)
> >+		return 0;
> >+
> >+	if (dm_drv_version(v, TGT_MPATH)) {
> >+		/* in doubt return least capable */
> >+		return 0;
> >+	}
> 
> Looks like the 'v' is NOT freed. Local stack allocation looks much
> cleaner, why not do that? You missed the same thing at other places, so
> I imagine you started with the on stack local structure but changed
> later???
> 
Sure, fixed.

> >+static int
> >+dm_drvprereq (char * str)
> >+{
> >+	unsigned int minv[3] = {1, 0, 3};
> >+	unsigned int *v;
> >+
> >+	v = zalloc(3);
> >+	if (!v)
> >+		return 0;
> >+
> >+	if (dm_drv_version(v, str)) {
> >+		/* in doubt return not capable */
> >+		return 1;
> >+	}
> 
> Missed freeing 'v'. Also, this function taking the target driver name as
> 'str' doesn't make sense as the minimum version is hard coded internally
> to this function. Take no arguments and pass 'TGT_MPATH' while calling
> dm_drv_version.
> 
Indeed. Fixed.

> > static int
> >+def_minio_rq_handler(vector strvec)
> >+{
> >+	char * buff;
> >+
> >+	buff = set_value(strvec);
> >+
> >+	if (!buff)
> >+		return 1;
> >+
> >+	conf->minio_rq = atoi(buff);
> >+	FREE(buff);
> >+
> >+	return 0;
> >+}
> 
> I was thinking why introduce minio and minio_rq in the
> /etc/multipath.conf file. By default we ship empty /etc/multipath.conf
> file. If the admin wants to override the default, he knows if he is
> going to use the BIO or REQUEST based multipath. So here is my approach
> to avoid introducing another similar looking config keyword:
> 
That's because there are /etc/multipath.conf in the wild right now,
created with non rq capable kernels. rr_min_io meant something then. It
seems not fair to change the meaning of that tunable upon upgrade.
People do cut-and-paste from old docs (corp or googled) and from peers
systems ... this approach minimize the risk of killing the perf by
accident. And I don't see downsides.


--
commit 035ea75c47302fc95d5742e854971f951419ec81
Author: Christophe Varoqui <christophe.varoqui@opensvc.com>
Date:   Tue Feb 1 09:53:20 2011 +0100

    Fix Malahal Naineni commit 2b68b83 review highlights
    
    1/ use local stack allocation in dm_drv_get_rq() and dm_drv_prereq()
    
    2/ don't pass argument to dm_drv_prereq() as it's dm-multipath
specific
    
    Also rename dm_drvprereq() to dm_drv_prereq() and dm_libprereq() to
    dm_lib_prereq() for consistency with dm_drv_get_rq() naming.

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e34c41d..50cdf98 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -79,7 +79,7 @@ dm_init(void) {
 )
 
 static int
-dm_libprereq (void)
+dm_lib_prereq (void)
 {
        char version[64];
        int v[3];
@@ -143,11 +143,8 @@ int
 dm_drv_get_rq (void)
 {
        unsigned int minv_dmrq[3] = {1, 1, 0};
-       unsigned int *v;
-
-       v = zalloc(3);
-       if (!v)
-               return 0;
+       unsigned int version[3] = {0, 0, 0};
+        unsigned int * v = version;
 
        if (dm_drv_version(v, TGT_MPATH)) {
                /* in doubt return least capable */
@@ -165,16 +162,13 @@ dm_drv_get_rq (void)
 }
 
 static int
-dm_drvprereq (char * str)
+dm_drv_prereq (void)
 {
        unsigned int minv[3] = {1, 0, 3};
-       unsigned int *v;
+       unsigned int version[3] = {0, 0, 0};
+        unsigned int * v = version;
 
-       v = zalloc(3);
-       if (!v)
-               return 0;
-
-       if (dm_drv_version(v, str)) {
+       if (dm_drv_version(v, TGT_MPATH)) {
                /* in doubt return not capable */
                return 1;
        }
@@ -194,9 +188,9 @@ dm_drvprereq (char * str)
 extern int
 dm_prereq (void)
 {
-       if (dm_libprereq())
+       if (dm_lib_prereq())
                return 1;
-       return dm_drvprereq(TGT_MPATH);
+       return dm_drv_prereq();
 }
 
 static int



-- 
Christophe Varoqui <christophe.varoqui@opensvc.com>
OpenSVC

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Malahal Naineni]

Christophe Varoqui [christophe.varoqui@gmail.com] wrote:
> That's because there are /etc/multipath.conf in the wild right now,
> created with non rq capable kernels. rr_min_io meant something then. It
> seems not fair to change the meaning of that tunable upon upgrade.
> People do cut-and-paste from old docs (corp or googled) and from peers
> systems ... this approach minimize the risk of killing the perf by
> accident. And I don't see downsides.

Agreed.

>  dm_drv_get_rq (void)
>  {
>         unsigned int minv_dmrq[3] = {1, 1, 0};
> -       unsigned int *v;
> -
> -       v = zalloc(3);
> -       if (!v)
> -               return 0;
> +       unsigned int version[3] = {0, 0, 0};
> +        unsigned int * v = version;

You could just say 'unsigned int v[3] = {0, 0, 0};' and remove the extra
pointer variable.

> +       unsigned int version[3] = {0, 0, 0};
> +        unsigned int * v = version;

Same as above.

Looks good. Thank you.

--Malahal.

Re: multipath: change the DEFAULT_MINIO for the request based multipath

[Mike Snitzer]

On Fri, Jan 21 2011 at  2:03am -0500,
Jun'ichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> Hi,
> 
> On 01/21/11 01:07, Mike Snitzer wrote:
> >>> Is it possible to detect request based multipath and change only for
> >>> those configurations?
> ...
> > Unfortunately, the multipath target version wasn't bumped when
> > request-based mpath was introduced (commit f40c67f0f7e2767f).
> 
> Um, sorry I don't remember why..
> 
> > Long story short, it seems we _could_ use the DM ioctl version bump that
> > occurred for 2.6.31 (from commit 60935eb):  4.15.0-ioctl (2009-04-01) 
> 
> Fortunately, the target version of multipath was also bumped
> in 2.6.31, from 1.0.5 to 1.1.0 by this commit:
> 
> af4874e v2.6.31-rc1 dm target:s introduce iterate devices fn

Ah, very good point.  I completely overlooked that.

Thanks,
Mike