[dm-devel] 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 at 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 at 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 at opensvc.com>
OpenSVC