[PATCH v2 0/5] Multipath Follow-up patches

[PATCH v2 0/5] Multipath Follow-up patches

[Benjamin Marzinski]

These patches resolve various minor issues that Martin had with my
previous patch set.

Changes in v2:

0001:	now uses memcmp() instead of strncmp(), and safe_sprintf()
	instead of strlcpy(), as suggested by Martin Wilck.

Benjamin Marzinski (5):
  multipath: fix issues found by compiling with gcc 10
  libmultipath: turn pp->vpd_data into a pointer
  libmultipath: change loading and resetting in directio
  libmultipath: change directio get_events() timeout handling
  libmultipath: cleanup old issues with directio checker

 kpartx/dasd.c                    |   6 +-
 libmultipath/checkers.c          |  26 ++---
 libmultipath/checkers.h          |   2 +-
 libmultipath/checkers/directio.c |  66 ++----------
 libmultipath/discovery.c         |  17 ++-
 libmultipath/print.c             |  20 ++--
 libmultipath/structs.c           |   3 +
 libmultipath/structs.h           |   2 +-
 multipath/main.c                 |   2 +-
 tests/directio.c                 | 178 ++++++++++++-------------------
 tests/vpd.c                      |   3 +-
 11 files changed, 125 insertions(+), 200 deletions(-)

-- 
2.17.2

[PATCH v2 1/5] multipath: fix issues found by compiling with gcc 10

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/dasd.c        |  6 +++---
 libmultipath/print.c | 16 ++++++++--------
 multipath/main.c     |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index 1486ccaa..14b9d3aa 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -186,7 +186,7 @@ read_dasd_pt(int fd, __attribute__((unused)) struct slice all,
 		goto out;
 	}
 
-	if ((!info.FBA_layout) && (!strcmp(info.type, "ECKD")))
+	if ((!info.FBA_layout) && (!memcmp(info.type, "ECKD", 4)))
 		memcpy (&vlabel, data, sizeof(vlabel));
 	else {
 		bzero(&vlabel,4);
@@ -216,7 +216,7 @@ read_dasd_pt(int fd, __attribute__((unused)) struct slice all,
 		sp[0].size  = size - sp[0].start;
 		retval = 1;
 	} else if ((strncmp(type, "VOL1", 4) == 0) &&
-		(!info.FBA_layout) && (!strcmp(info.type, "ECKD"))) {
+		(!info.FBA_layout) && (!memcmp(info.type, "ECKD",4))) {
 		/*
 		 * New style VOL1 labeled disk
 		 */
@@ -265,7 +265,7 @@ read_dasd_pt(int fd, __attribute__((unused)) struct slice all,
 			if (vlabel.ldl_version == 0xf2) {
 				fmt_size = sectors512(vlabel.formatted_blocks,
 						      blocksize);
-			} else if (!strcmp(info.type, "ECKD")) {
+			} else if (!memcmp(info.type, "ECKD",4)) {
 				/* formatted w/o large volume support */
 				fmt_size = geo.cylinders * geo.heads
 					* geo.sectors * (blocksize >> 9);
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 1858d8ea..56f86b2f 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -29,6 +29,7 @@
 #include "uevent.h"
 #include "debug.h"
 #include "discovery.h"
+#include "util.h"
 
 #define MAX(x,y) (((x) > (y)) ? (x) : (y))
 #define MIN(x,y) (((x) > (y)) ? (y) : (x))
@@ -2032,7 +2033,6 @@ int snprint_devices(struct config *conf, char * buff, int len,
 	struct dirent *blkdev;
 	struct stat statbuf;
 	char devpath[PATH_MAX];
-	char *devptr;
 	int threshold = MAX_LINE_LEN;
 	int fwd = 0;
 	int r;
@@ -2048,15 +2048,14 @@ int snprint_devices(struct config *conf, char * buff, int len,
 	}
 	fwd += snprintf(buff + fwd, len - fwd, "available block devices:\n");
 
-	strcpy(devpath,"/sys/block/");
 	while ((blkdev = readdir(blkdir)) != NULL) {
 		if ((strcmp(blkdev->d_name,".") == 0) ||
 		    (strcmp(blkdev->d_name,"..") == 0))
 			continue;
 
-		devptr = devpath + 11;
-		*devptr = '\0';
-		strncat(devptr, blkdev->d_name, PATH_MAX-12);
+		if (safe_sprintf(devpath, "/sys/block/%s", blkdev->d_name))
+			continue;
+
 		if (stat(devpath, &statbuf) < 0)
 			continue;
 
@@ -2068,11 +2067,12 @@ int snprint_devices(struct config *conf, char * buff, int len,
 			return len;
 		}
 
-		fwd += snprintf(buff + fwd, len - fwd, "    %s", devptr);
-		pp = find_path_by_dev(vecs->pathvec, devptr);
+		fwd += snprintf(buff + fwd, len - fwd, "    %s",
+				blkdev->d_name);
+		pp = find_path_by_dev(vecs->pathvec, blkdev->d_name);
 		if (!pp) {
 			r = filter_devnode(conf->blist_devnode,
-					   conf->elist_devnode, devptr);
+					   conf->elist_devnode, blkdev->d_name);
 			if (r > 0)
 				fwd += snprintf(buff + fwd, len - fwd,
 						" devnode blacklisted, unmonitored");
diff --git a/multipath/main.c b/multipath/main.c
index 15007752..cf9d2a28 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -1025,7 +1025,7 @@ main (int argc, char *argv[])
 		if (!dev)
 			goto out;
 
-		strncpy(dev, argv[optind], FILE_NAME_SIZE);
+		strlcpy(dev, argv[optind], FILE_NAME_SIZE);
 		if (dev_type != DEV_UEVENT)
 			dev_type = get_dev_type(dev);
 		if (dev_type == DEV_NONE) {
-- 
2.17.2

Re: [PATCH v2 1/5] multipath: fix issues found by compiling with gcc 10

[Martin Wilck]

On Wed, 2020-02-19 at 14:21 -0600, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  kpartx/dasd.c        |  6 +++---
>  libmultipath/print.c | 16 ++++++++--------
>  multipath/main.c     |  2 +-
>  3 files changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: [dm-devel] [PATCH v2 1/5] multipath: fix issues found by compiling with gcc 10

[Martin Wilck]

Hi Ben,

On Wed, 2020-02-19 at 14:21 -0600, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  kpartx/dasd.c        |  6 +++---
>  libmultipath/print.c | 16 ++++++++--------
>  multipath/main.c     |  2 +-
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/kpartx/dasd.c b/kpartx/dasd.c
> index 1486ccaa..14b9d3aa 100644
> --- a/kpartx/dasd.c
> +++ b/kpartx/dasd.c
> @@ -186,7 +186,7 @@ read_dasd_pt(int fd, __attribute__((unused))
> struct slice all,
>  		goto out;
>  	}
>  
> -	if ((!info.FBA_layout) && (!strcmp(info.type, "ECKD")))
> +	if ((!info.FBA_layout) && (!memcmp(info.type, "ECKD", 4)))
>  		memcpy (&vlabel, data, sizeof(vlabel));
>  	else {
>  		bzero(&vlabel,4);

are you using different compiler / warning flags here than we usually do?

I just found that with the standard flags, gcc 10 does *not* complain about
the old (badly broken, noting that info.type is declared as char[4]) code.
Nor has any other compiler so far, although we're using pretty verbose 
warning options.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer



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

Re: [dm-devel] [PATCH v2 1/5] multipath: fix issues found by compiling with gcc 10

[Benjamin Marzinski]

On Wed, Dec 02, 2020 at 01:54:57PM +0000, Martin Wilck wrote:
> Hi Ben,
> 
> On Wed, 2020-02-19 at 14:21 -0600, Benjamin Marzinski wrote:
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  kpartx/dasd.c        |  6 +++---
> >  libmultipath/print.c | 16 ++++++++--------
> >  multipath/main.c     |  2 +-
> >  3 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/kpartx/dasd.c b/kpartx/dasd.c
> > index 1486ccaa..14b9d3aa 100644
> > --- a/kpartx/dasd.c
> > +++ b/kpartx/dasd.c
> > @@ -186,7 +186,7 @@ read_dasd_pt(int fd, __attribute__((unused))
> > struct slice all,
> >  		goto out;
> >  	}
> >  
> > -	if ((!info.FBA_layout) && (!strcmp(info.type, "ECKD")))
> > +	if ((!info.FBA_layout) && (!memcmp(info.type, "ECKD", 4)))
> >  		memcpy (&vlabel, data, sizeof(vlabel));
> >  	else {
> >  		bzero(&vlabel,4);
> 
> are you using different compiler / warning flags here than we usually do?

When building rhel and fedora packages, there are some flags that differ
from the upstream set.  Unfortunately, when I just tried rebuilding the
package with these fixes removed, I didn't hit any compiler errors.
Perhaps I just noticed these while working on something else, and they
got included in this commit on accident. Sadly I don't remember the
details anymore.

-Ben
> 
> I just found that with the standard flags, gcc 10 does *not* complain about
> the old (badly broken, noting that info.type is declared as char[4]) code.
> Nor has any other compiler so far, although we're using pretty verbose 
> warning options.
> 
> Regards
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE  Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer
> 

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

[PATCH v2 2/5] libmultipath: turn pp->vpd_data into a pointer

[Benjamin Marzinski]

Instead of always allocating space in the path structure for vpd_data,
only allocte it when necessary.

Also, fix comments on vpd tests

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 17 +++++++++++++++--
 libmultipath/print.c     |  4 ++--
 libmultipath/structs.c   |  3 +++
 libmultipath/structs.h   |  2 +-
 tests/vpd.c              |  3 ++-
 5 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 23a7889c..ee3290cd 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1649,8 +1649,21 @@ scsi_ioctl_pathinfo (struct path * pp, int mask)
 	select_vpd_vendor_id(pp);
 	vpd_id = pp->vpd_vendor_id;
 
-	if (vpd_id != VPD_VP_UNDEF && get_vpd_sgio(pp->fd, vpd_vendor_pages[vpd_id].pg, vpd_id, pp->vpd_data, sizeof(pp->vpd_data)) < 0)
-		condlog(3, "%s: failed to get extra vpd data", pp->dev);
+	if (vpd_id != VPD_VP_UNDEF) {
+		char vpd_data[VPD_DATA_SIZE] = {0};
+
+		if (get_vpd_sgio(pp->fd, vpd_vendor_pages[vpd_id].pg, vpd_id,
+		    vpd_data, sizeof(vpd_data)) < 0)
+			condlog(3, "%s: failed to get extra vpd data", pp->dev);
+		else {
+			vpd_data[VPD_DATA_SIZE - 1] = '\0';
+			if (pp->vpd_data)
+				free(pp->vpd_data);
+			pp->vpd_data = strdup(vpd_data);
+			if (!pp->vpd_data)
+				condlog(0, "%s: failed to allocate space for vpd data", pp->dev);
+		}
+	}
 
 	parent = pp->udev;
 	while (parent) {
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 56f86b2f..b944ef32 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -371,7 +371,7 @@ snprint_multipath_vpd_data(char * buff, size_t len,
 
 	vector_foreach_slot(mpp->pg, pgp, i)
 		vector_foreach_slot(pgp->paths, pp, j)
-			if (strlen(pp->vpd_data))
+			if (pp->vpd_data)
 				return snprintf(buff, len, "%s", pp->vpd_data);
 	return snprintf(buff, len, "[undef]");
 }
@@ -710,7 +710,7 @@ snprint_path_marginal(char * buff, size_t len, const struct path * pp)
 static int
 snprint_path_vpd_data(char * buff, size_t len, const struct path * pp)
 {
-	if (strlen(pp->vpd_data) > 0)
+	if (pp->vpd_data)
 		return snprintf(buff, len, "%s", pp->vpd_data);
 	return snprintf(buff, len, "[undef]");
 }
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index cc931e4e..2dd378c4 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -131,6 +131,9 @@ free_path (struct path * pp)
 		udev_device_unref(pp->udev);
 		pp->udev = NULL;
 	}
+	if (pp->vpd_data)
+		free(pp->vpd_data);
+
 	vector_free(pp->hwe);
 
 	FREE(pp);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index b7a01220..9bd39eb1 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -269,7 +269,7 @@ struct path {
 	char rev[PATH_REV_SIZE];
 	char serial[SERIAL_SIZE];
 	char tgt_node_name[NODE_NAME_SIZE];
-	char vpd_data[VPD_DATA_SIZE];
+	char *vpd_data;
 	unsigned long long size;
 	unsigned int checkint;
 	unsigned int tick;
diff --git a/tests/vpd.c b/tests/vpd.c
index 0331c487..3cbad811 100644
--- a/tests/vpd.c
+++ b/tests/vpd.c
@@ -520,8 +520,9 @@ static void test_vpd_eui_ ## len ## _ ## wlen ## _ ## sml(void **state)	\
 	n = create_vpd83(vt->vpdbuf, sizeof(vt->vpdbuf), test_id,	\
 			 2, 0, len);					\
 	if (sml) {							\
-		/* overwrite the page side to DEFAULT_SGIO_LEN + 1 */	\
+		/* overwrite the page size to DEFAULT_SGIO_LEN + 1 */	\
 		put_unaligned_be16(255, vt->vpdbuf + 2);		\
+		/* this causes get_vpd_sgio to do a second ioctl */	\
 		will_return(__wrap_ioctl, n);				\
 		will_return(__wrap_ioctl, vt->vpdbuf);			\
 	}								\
-- 
2.17.2

[PATCH v2 3/5] libmultipath: change loading and resetting in directio

[Benjamin Marzinski]

The directio checker previously made sure to always keep an aio_group
around after loading or resetting the checker. There is no need to do
this, and removing this code simplifies the checker.  With this change,
there is no longer a need for a load or unload checker function, only a
reset function which is run when the checker is reset or unloaded.
Changing this broke a number of tests, so the unit tests have been
updated as well.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers.c          |  26 ++---
 libmultipath/checkers.h          |   2 +-
 libmultipath/checkers/directio.c |  43 +-------
 tests/directio.c                 | 177 +++++++++++++------------------
 4 files changed, 81 insertions(+), 167 deletions(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 5c7d3004..8d2be8a9 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -18,9 +18,7 @@ struct checker_class {
 	int (*init)(struct checker *);       /* to allocate the context */
 	int (*mp_init)(struct checker *);    /* to allocate the mpcontext */
 	void (*free)(struct checker *);      /* to free the context */
-	int (*load)(void);                   /* to allocate global variables */
-	void (*unload)(void);                /* to free global variables */
-	int (*reset)(void);		     /* to reset the global variables */
+	void (*reset)(void);		     /* to reset the global variables */
 	const char **msgtable;
 	short msgtable_size;
 };
@@ -69,8 +67,8 @@ void free_checker_class(struct checker_class *c)
 	}
 	condlog(3, "unloading %s checker", c->name);
 	list_del(&c->node);
-	if (c->unload)
-		c->unload();
+	if (c->reset)
+		c->reset();
 	if (c->handle) {
 		if (dlclose(c->handle) != 0) {
 			condlog(0, "Cannot unload checker %s: %s",
@@ -103,16 +101,14 @@ static struct checker_class *checker_class_lookup(const char *name)
 	return NULL;
 }
 
-int reset_checker_classes(void)
+void reset_checker_classes(void)
 {
-	int ret = 0;
 	struct checker_class *c;
 
 	list_for_each_entry(c, &checkers, node) {
 		if (c->reset)
-			ret = ret? ret : c->reset();
+			c->reset();
 	}
-	return ret;
 }
 
 static struct checker_class *add_checker_class(const char *multipath_dir,
@@ -159,10 +155,8 @@ static struct checker_class *add_checker_class(const char *multipath_dir,
 		goto out;
 
 	c->mp_init = (int (*)(struct checker *)) dlsym(c->handle, "libcheck_mp_init");
-	c->load = (int (*)(void)) dlsym(c->handle, "libcheck_load");
-	c->unload = (void (*)(void)) dlsym(c->handle, "libcheck_unload");
-	c->reset = (int (*)(void)) dlsym(c->handle, "libcheck_reset");
-	/* These 4 functions can be NULL. call dlerror() to clear out any
+	c->reset = (void (*)(void)) dlsym(c->handle, "libcheck_reset");
+	/* These 2 functions can be NULL. call dlerror() to clear out any
 	 * error string */
 	dlerror();
 
@@ -189,12 +183,6 @@ static struct checker_class *add_checker_class(const char *multipath_dir,
 	condlog(3, "checker %s: message table size = %d",
 		c->name, c->msgtable_size);
 
-	if (c->load && c->load() != 0) {
-		condlog(0, "%s: failed to load checker", c->name);
-		c->unload = NULL;
-		goto out;
-	}
-
 done:
 	c->sync = 1;
 	list_add(&c->node, &checkers);
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index d9930467..b458118d 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -150,7 +150,7 @@ void checker_disable (struct checker *);
 int checker_check (struct checker *, int);
 int checker_is_sync(const struct checker *);
 const char *checker_name (const struct checker *);
-int reset_checker_classes(void);
+void reset_checker_classes(void);
 /*
  * This returns a string that's best prepended with "$NAME checker",
  * where $NAME is the return value of checker_name().
diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index 813e61e2..6ad7c885 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -138,23 +138,11 @@ check_orphaned_group(struct aio_group *aio_grp)
 		return;
 	list_for_each(item, &aio_grp->orphans)
 		count++;
-	if (count >= AIO_GROUP_SIZE) {
+	if (count >= AIO_GROUP_SIZE)
 		remove_aio_group(aio_grp);
-		if (list_empty(&aio_grp_list))
-			add_aio_group();
-	}
 }
 
-int libcheck_load (void)
-{
-	if (add_aio_group() == NULL) {
-		LOG(1, "libcheck_load failed: %s", strerror(errno));
-		return 1;
-	}
-	return 0;
-}
-
-void libcheck_unload (void)
+void libcheck_reset (void)
 {
 	struct aio_group *aio_grp, *tmp;
 
@@ -162,33 +150,6 @@ void libcheck_unload (void)
 		remove_aio_group(aio_grp);
 }
 
-int libcheck_reset (void)
-{
-	struct aio_group *aio_grp, *tmp, *reset_grp = NULL;
-
-	/* If a clean existing aio_group exists, use that. Otherwise add a
-	 * new one */
-	list_for_each_entry(aio_grp, &aio_grp_list, node) {
-		if (aio_grp->holders == 0 &&
-		    list_empty(&aio_grp->orphans)) {
-			reset_grp = aio_grp;
-			break;
-		}
-	}
-	if (!reset_grp)
-		reset_grp = add_aio_group();
-	if (!reset_grp) {
-		LOG(1, "checker reset failed");
-		return 1;
-	}
-
-	list_for_each_entry_safe(aio_grp, tmp, &aio_grp_list, node) {
-		if (aio_grp != reset_grp)
-			remove_aio_group(aio_grp);
-	}
-	return 0;
-}
-
 int libcheck_init (struct checker * c)
 {
 	unsigned long pgsize = getpagesize();
diff --git a/tests/directio.c b/tests/directio.c
index 236c514b..23fd2da9 100644
--- a/tests/directio.c
+++ b/tests/directio.c
@@ -217,7 +217,7 @@ void do_check_state(struct checker *c, int sync, int timeout, int chk_state)
 	memset(mock_events, 0, sizeof(mock_events));
 }
 
-void do_libcheck_unload(int nr_aio_grps)
+void do_libcheck_reset(int nr_aio_grps)
 {
 	int count = 0;
 	struct aio_group *aio_grp;
@@ -227,7 +227,7 @@ void do_libcheck_unload(int nr_aio_grps)
 	assert_int_equal(count, nr_aio_grps);
 	for (count = 0; count < nr_aio_grps; count++)
 		will_return(__wrap_io_destroy, 0);
-	libcheck_unload();
+	libcheck_reset();
 	assert_true(list_empty(&aio_grp_list));
 	assert_int_equal(ioctx_count, 0);
 }
@@ -278,31 +278,38 @@ static void check_aio_grp(struct aio_group *aio_grp, int holders,
 	assert_int_equal(orphans, count);
 }
 
-static void do_libcheck_load(void)
+/* simple resetting test */
+static void test_reset(void **state)
 {
-	int count = 0;
-	struct aio_group *aio_grp;
-
 	assert_true(list_empty(&aio_grp_list));
-	will_return(__wrap_io_setup, 0);
-	assert_int_equal(libcheck_load(), 0);
-	list_for_each_entry(aio_grp, &aio_grp_list, node) {
-		assert_int_equal(aio_grp->holders, 0);
-		count++;
-	}
-	assert_int_equal(count, 1);
+	do_libcheck_reset(0);
 }
 
-/* simple loading resetting and unloading test */
-static void test_load_reset_unload(void **state)
+/* tests initializing, then resetting, and then initializing again */
+static void test_init_reset_init(void **state)
 {
-	struct list_head *item;
-	do_libcheck_load();
-	item = aio_grp_list.next;
-	assert_int_equal(libcheck_reset(), 0);
-	assert_false(list_empty(&aio_grp_list));
-	assert_true(item == aio_grp_list.next);
-	do_libcheck_unload(1);
+	struct checker c = {0};
+	struct aio_group *aio_grp, *tmp_grp;
+
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
+	do_libcheck_init(&c, 4096, NULL);
+	aio_grp = get_aio_grp(&c);
+	check_aio_grp(aio_grp, 1, 0);
+	list_for_each_entry(tmp_grp, &aio_grp_list, node)
+		assert_ptr_equal(aio_grp, tmp_grp);
+	libcheck_free(&c);
+	check_aio_grp(aio_grp, 0, 0);
+	do_libcheck_reset(1);
+	will_return(__wrap_io_setup, 0);
+	do_libcheck_init(&c, 4096, NULL);
+	aio_grp = get_aio_grp(&c);
+	check_aio_grp(aio_grp, 1, 0);
+	list_for_each_entry(tmp_grp, &aio_grp_list, node)
+		assert_ptr_equal(aio_grp, tmp_grp);
+	libcheck_free(&c);
+	check_aio_grp(aio_grp, 0, 0);
+	do_libcheck_reset(1);
 }
 
 /* test initializing and then freeing 4096 checkers */
@@ -312,8 +319,8 @@ static void test_init_free(void **state)
 	struct checker c[4096] = {0};
 	struct aio_group *aio_grp;
 
-	do_libcheck_load();
-	aio_grp = list_entry(aio_grp_list.next, typeof(*aio_grp), node);
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	will_return(__wrap_io_setup, 0);
 	will_return(__wrap_io_setup, 0);
 	will_return(__wrap_io_setup, 0);
@@ -328,7 +335,7 @@ static void test_init_free(void **state)
 			do_libcheck_init(&c[i], 4096, NULL);
 		ct = (struct directio_context *)c[i].context;
 		assert_non_null(ct->aio_grp);
-		if (i && (i & 1023) == 0)
+		if ((i & 1023) == 0)
 			aio_grp = ct->aio_grp;
 		else {
 			assert_ptr_equal(ct->aio_grp, aio_grp);
@@ -346,17 +353,9 @@ static void test_init_free(void **state)
 		libcheck_free(&c[i]);
 		assert_int_equal(aio_grp->holders, 1023 - (i & 1023));
 	}
-	count = 0;
-	list_for_each_entry(aio_grp, &aio_grp_list, node) {
+	list_for_each_entry(aio_grp, &aio_grp_list, node)
 		assert_int_equal(aio_grp->holders, 0);
-		count++;
-	}
-	assert_int_equal(count, 4);
-	will_return(__wrap_io_destroy, 0);
-	will_return(__wrap_io_destroy, 0);
-	will_return(__wrap_io_destroy, 0);
-	assert_int_equal(libcheck_reset(), 0);
-	do_libcheck_unload(1);
+	do_libcheck_reset(4);
 }
 
 /* check mixed initializing and freeing 4096 checkers */
@@ -366,7 +365,8 @@ static void test_multi_init_free(void **state)
 	struct checker c[4096] = {0};
 	struct aio_group *aio_grp;
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	will_return(__wrap_io_setup, 0);
 	will_return(__wrap_io_setup, 0);
 	will_return(__wrap_io_setup, 0);
@@ -396,7 +396,7 @@ static void test_multi_init_free(void **state)
 			i++;
 		}
 	}
-	do_libcheck_unload(4);
+	do_libcheck_reset(4);
 }
 
 /* simple single checker sync test */
@@ -406,12 +406,13 @@ static void test_check_state_simple(void **state)
 	struct async_req *req;
 	int res = 0;
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	do_libcheck_init(&c, 4096, &req);
 	return_io_getevents_nr(NULL, 1, &req, &res);
 	do_check_state(&c, 1, 30, PATH_UP);
 	libcheck_free(&c);
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test sync timeout */
@@ -420,7 +421,8 @@ static void test_check_state_timeout(void **state)
 	struct checker c = {0};
 	struct aio_group *aio_grp;
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	do_libcheck_init(&c, 4096, NULL);
 	aio_grp = get_aio_grp(&c);
 	return_io_getevents_none();
@@ -433,7 +435,7 @@ static void test_check_state_timeout(void **state)
 	will_return(__wrap_io_cancel, 0);
 #endif
 	libcheck_free(&c);
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test async timeout */
@@ -442,7 +444,8 @@ static void test_check_state_async_timeout(void **state)
 	struct checker c = {0};
 	struct aio_group *aio_grp;
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	do_libcheck_init(&c, 4096, NULL);
 	aio_grp = get_aio_grp(&c);
 	return_io_getevents_none();
@@ -459,7 +462,7 @@ static void test_check_state_async_timeout(void **state)
 	will_return(__wrap_io_cancel, 0);
 #endif
 	libcheck_free(&c);
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test freeing checkers with outstanding requests */
@@ -470,7 +473,8 @@ static void test_free_with_pending(void **state)
 	struct async_req *req;
 	int res = 0;
 
-        do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
         do_libcheck_init(&c[0], 4096, &req);
 	do_libcheck_init(&c[1], 4096, NULL);
         aio_grp = get_aio_grp(c);
@@ -491,7 +495,7 @@ static void test_free_with_pending(void **state)
 #else
         check_aio_grp(aio_grp, 0, 0);
 #endif
-        do_libcheck_unload(1);
+        do_libcheck_reset(1);
 }
 
 /* test removing orpahed aio_group on free */
@@ -501,7 +505,8 @@ static void test_orphaned_aio_group(void **state)
 	struct aio_group *aio_grp, *tmp_grp;
 	int i;
 
-        do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	for (i = 0; i < AIO_GROUP_SIZE; i++) {
 		do_libcheck_init(&c[i], 4096, NULL);
 		return_io_getevents_none();
@@ -519,17 +524,10 @@ static void test_orphaned_aio_group(void **state)
 		if (i == AIO_GROUP_SIZE - 1) {
 			/* remove the orphaned group and create a new one */
 			will_return(__wrap_io_destroy, 0);
-			will_return(__wrap_io_setup, 0);
 		}
 		libcheck_free(&c[i]);
 	}
-	i = 0;
-	list_for_each_entry(tmp_grp, &aio_grp_list, node) {
-		i++;
-		check_aio_grp(aio_grp, 0, 0);
-	}
-	assert_int_equal(i, 1);
-        do_libcheck_unload(1);
+        do_libcheck_reset(0);
 }
 
 /* test sync timeout with failed cancel and cleanup by another
@@ -542,7 +540,8 @@ static void test_timeout_cancel_failed(void **state)
 	int res[] = {0,0};
 	int i;
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	for (i = 0; i < 2; i++)
 		do_libcheck_init(&c[i], 4096, &reqs[i]);
 	aio_grp = get_aio_grp(c);
@@ -563,7 +562,7 @@ static void test_timeout_cancel_failed(void **state)
 		assert_false(is_checker_running(&c[i]));
 		libcheck_free(&c[i]);
 	}
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test async timeout with failed cancel and cleanup by another
@@ -575,7 +574,8 @@ static void test_async_timeout_cancel_failed(void **state)
 	int res[] = {0,0};
 	int i;
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	for (i = 0; i < 2; i++)
 		do_libcheck_init(&c[i], 4096, &reqs[i]);
 	return_io_getevents_none();
@@ -605,7 +605,7 @@ static void test_async_timeout_cancel_failed(void **state)
 		assert_false(is_checker_running(&c[i]));
 		libcheck_free(&c[i]);
 	}
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test orphaning a request, and having another checker clean it up */
@@ -617,7 +617,8 @@ static void test_orphan_checker_cleanup(void **state)
 	struct aio_group *aio_grp;
 	int i;
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	for (i = 0; i < 2; i++)
 		do_libcheck_init(&c[i], 4096, &reqs[i]);
 	aio_grp = get_aio_grp(c);
@@ -632,7 +633,7 @@ static void test_orphan_checker_cleanup(void **state)
 	check_aio_grp(aio_grp, 1, 0);
 	libcheck_free(&c[1]);
 	check_aio_grp(aio_grp, 0, 0);
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test orphaning a request, and having reset clean it up */
@@ -642,46 +643,8 @@ static void test_orphan_reset_cleanup(void **state)
 	struct aio_group *orphan_aio_grp, *tmp_aio_grp;
 	int found, count;
 
-	do_libcheck_load();
-	do_libcheck_init(&c, 4096, NULL);
-	orphan_aio_grp = get_aio_grp(&c);
-	return_io_getevents_none();
-	do_check_state(&c, 0, 30, PATH_PENDING);
-	will_return(__wrap_io_cancel, -1);
-	check_aio_grp(orphan_aio_grp, 1, 0);
-	libcheck_free(&c);
-	check_aio_grp(orphan_aio_grp, 1, 1);
-	found = count = 0;
-	list_for_each_entry(tmp_aio_grp, &aio_grp_list, node) {
-		count++;
-		if (tmp_aio_grp == orphan_aio_grp)
-			found = 1;
-	}
-	assert_int_equal(count, 1);
-	assert_int_equal(found, 1);
+	assert_true(list_empty(&aio_grp_list));
 	will_return(__wrap_io_setup, 0);
-	will_return(__wrap_io_destroy, 0);
-	assert_int_equal(libcheck_reset(), 0);
-	found = count = 0;
-	list_for_each_entry(tmp_aio_grp, &aio_grp_list, node) {
-		count++;
-		check_aio_grp(tmp_aio_grp, 0, 0);
-		if (tmp_aio_grp == orphan_aio_grp)
-			found = 1;
-	}
-	assert_int_equal(count, 1);
-	assert_int_equal(found, 0);
-	do_libcheck_unload(1);
-}
-
-/* test orphaning a request, and having unload clean it up */
-static void test_orphan_unload_cleanup(void **state)
-{
-	struct checker c;
-	struct aio_group *orphan_aio_grp, *tmp_aio_grp;
-	int found, count;
-
-	do_libcheck_load();
 	do_libcheck_init(&c, 4096, NULL);
 	orphan_aio_grp = get_aio_grp(&c);
 	return_io_getevents_none();
@@ -698,7 +661,7 @@ static void test_orphan_unload_cleanup(void **state)
 	}
 	assert_int_equal(count, 1);
 	assert_int_equal(found, 1);
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test checkers with different blocksizes */
@@ -716,7 +679,8 @@ static void test_check_state_blksize(void **state)
 	int chk_state[] = {PATH_UP, PATH_DOWN, PATH_UP};
 #endif
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	for (i = 0; i < 3; i++)
 		do_libcheck_init(&c[i], blksize[i], &reqs[i]);
 	for (i = 0; i < 3; i++) {
@@ -727,7 +691,7 @@ static void test_check_state_blksize(void **state)
 		assert_false(is_checker_running(&c[i]));
 		libcheck_free(&c[i]);
 	}
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 /* test async checkers pending and getting resovled by another checker
@@ -739,7 +703,8 @@ static void test_check_state_async(void **state)
 	struct async_req *reqs[257];
 	int res[257] = {0};
 
-	do_libcheck_load();
+	assert_true(list_empty(&aio_grp_list));
+	will_return(__wrap_io_setup, 0);
 	for (i = 0; i < 257; i++)
 		do_libcheck_init(&c[i], 4096, &reqs[i]);
 	for (i = 0; i < 256; i++) {
@@ -757,7 +722,7 @@ static void test_check_state_async(void **state)
 		assert_false(is_checker_running(&c[i]));
 		libcheck_free(&c[i]);
 	}
-	do_libcheck_unload(1);
+	do_libcheck_reset(1);
 }
 
 static int setup(void **state)
@@ -782,7 +747,8 @@ static int teardown(void **state)
 int test_directio(void)
 {
 	const struct CMUnitTest tests[] = {
-		cmocka_unit_test(test_load_reset_unload),
+		cmocka_unit_test(test_reset),
+		cmocka_unit_test(test_init_reset_init),
 		cmocka_unit_test(test_init_free),
 		cmocka_unit_test(test_multi_init_free),
 		cmocka_unit_test(test_check_state_simple),
@@ -793,7 +759,6 @@ int test_directio(void)
 		cmocka_unit_test(test_async_timeout_cancel_failed),
 		cmocka_unit_test(test_orphan_checker_cleanup),
 		cmocka_unit_test(test_orphan_reset_cleanup),
-		cmocka_unit_test(test_orphan_unload_cleanup),
 		cmocka_unit_test(test_check_state_blksize),
 		cmocka_unit_test(test_check_state_async),
 		cmocka_unit_test(test_orphaned_aio_group),
-- 
2.17.2

[PATCH v2 4/5] libmultipath: change directio get_events() timeout handling

[Benjamin Marzinski]

get_events() used a NULL or a zeroed timeout to mean "don't wait".
io_getevents() uses a NULL timeout to mean "wait forever" and a
zeroed timeout to mean "don't wait". Make get_events() work like
io_getevents().

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/directio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index 6ad7c885..649961a4 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -262,7 +262,7 @@ get_events(struct aio_group *aio_grp, struct timespec *timeout)
 	struct io_event events[128];
 	int i, nr, got_events = 0;
 	struct timespec zero_timeout = {0};
-	struct timespec *timep = (timeout)? timeout : &zero_timeout;
+	struct timespec *timep = timeout;
 
 	do {
 		errno = 0;
@@ -303,7 +303,6 @@ check_state(int fd, struct directio_context *ct, int sync, int timeout_secs)
 	int		rc;
 	long		r;
 	struct timespec currtime, endtime;
-	struct timespec *timep = &timeout;
 
 	if (fstat(fd, &sb) == 0) {
 		LOG(4, "called for %x", (unsigned) sb.st_rdev);
@@ -339,18 +338,19 @@ check_state(int fd, struct directio_context *ct, int sync, int timeout_secs)
 	endtime.tv_nsec += timeout.tv_nsec;
 	normalize_timespec(&endtime);
 	while(1) {
-		r = get_events(ct->aio_grp, timep);
+		r = get_events(ct->aio_grp, &timeout);
 
 		if (ct->req->state != PATH_PENDING) {
 			ct->running = 0;
 			return ct->req->state;
-		} else if (r == 0 || !timep)
+		} else if (r == 0 ||
+			   (timeout.tv_sec == 0 && timeout.tv_nsec == 0))
 			break;
 
 		get_monotonic_time(&currtime);
 		timespecsub(&endtime, &currtime, &timeout);
 		if (timeout.tv_sec < 0)
-			timep = NULL;
+			timeout.tv_sec = timeout.tv_nsec = 0;
 	}
 	if (ct->running > timeout_secs || sync) {
 		struct io_event event;
-- 
2.17.2

[PATCH v2 5/5] libmultipath: cleanup old issues with directio checker

[Benjamin Marzinski]

The directio checker manually aligns its directio buffer, instead of
using posix_memalign(). It also defaults the block size for the read to
512, which may be too small for 4k devices, and it only waits for 5 ns
for IO completion before giving up and setting the path to pending,
which means that in will virtually always set the path to pending on the
check when the IO is issued.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/directio.c | 13 ++++---------
 tests/directio.c                 |  1 -
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index 649961a4..503519e2 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -38,7 +38,6 @@ struct async_req {
 	struct iocb io;
 	unsigned int blksize;
 	unsigned char *	buf;
-	unsigned char * ptr;
 	struct list_head node;
 	int state; /* PATH_REMOVED means this is an orphan */
 };
@@ -174,7 +173,7 @@ int libcheck_init (struct checker * c)
 
 	if (ioctl(c->fd, BLKBSZGET, &req->blksize) < 0) {
 		c->msgid = MSG_DIRECTIO_BLOCKSIZE;
-		req->blksize = 512;
+		req->blksize = 4096;
 	}
 	if (req->blksize > 4096) {
 		/*
@@ -185,8 +184,7 @@ int libcheck_init (struct checker * c)
 	if (!req->blksize)
 		goto out;
 
-	req->buf = (unsigned char *)malloc(req->blksize + pgsize);
-	if (!req->buf)
+	if (posix_memalign((void **)&req->buf, pgsize, req->blksize) != 0)
 		goto out;
 
 	flags = fcntl(c->fd, F_GETFL);
@@ -199,9 +197,6 @@ int libcheck_init (struct checker * c)
 		ct->reset_flags = 1;
 	}
 
-	req->ptr = (unsigned char *) (((unsigned long)req->buf + pgsize - 1) &
-		  (~(pgsize - 1)));
-
 	/* Successfully initialized, return the context. */
 	ct->req = req;
 	c->context = (void *) ct;
@@ -298,7 +293,7 @@ get_events(struct aio_group *aio_grp, struct timespec *timeout)
 static int
 check_state(int fd, struct directio_context *ct, int sync, int timeout_secs)
 {
-	struct timespec	timeout = { .tv_nsec = 5 };
+	struct timespec	timeout = { .tv_nsec = 1000 };
 	struct stat	sb;
 	int		rc;
 	long		r;
@@ -323,7 +318,7 @@ check_state(int fd, struct directio_context *ct, int sync, int timeout_secs)
 
 		LOG(3, "starting new request");
 		memset(&ct->req->io, 0, sizeof(struct iocb));
-		io_prep_pread(&ct->req->io, fd, ct->req->ptr,
+		io_prep_pread(&ct->req->io, fd, ct->req->buf,
 			      ct->req->blksize, 0);
 		ct->req->state = PATH_PENDING;
 		if (io_submit(ct->aio_grp->ioctx, 1, ios) != 1) {
diff --git a/tests/directio.c b/tests/directio.c
index 23fd2da9..3cd7a520 100644
--- a/tests/directio.c
+++ b/tests/directio.c
@@ -23,7 +23,6 @@
 #include <stddef.h>
 #include <setjmp.h>
 #include <stdlib.h>
-#define UNIT_TESTING /* enable memory testing in directio.c */
 #include <cmocka.h>
 #include "globals.c"
 #include "../libmultipath/checkers/directio.c"
-- 
2.17.2