Re: [dm-devel] [PATCH] libmultipath: increase path product_id/rev field size for NVMe

[Benjamin Marzinski]

On Fri, Jan 19, 2018 at 12:55:35PM +0100, Martin Wilck wrote:
> NVMe allows longer strings for the model (product) and firmware rev
> than SCSI.

Reviewed-by: Benjamin Marzinski 

> 
> Signed-off-by: Martin Wilck 
> ---
>  libmultipath/config.c|  2 +-
>  libmultipath/discovery.c | 12 ++--
>  libmultipath/structs.h   | 10 --
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 1461a17cddbe..2592990ecccd 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -319,7 +319,7 @@ set_param_str(char * str)
>  static int
>  merge_hwe (struct hwentry * dst, struct hwentry * src)
>  {
> - char id[SCSI_VENDOR_SIZE+SCSI_PRODUCT_SIZE];
> + char id[SCSI_VENDOR_SIZE+PATH_PRODUCT_SIZE];
>   merge_str(vendor);
>   merge_str(product);
>   merge_str(revision);
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 88fc8d732258..88e9f3b61510 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1158,12 +1158,12 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
>  
>   condlog(3, "%s: vendor = %s", pp->dev, pp->vendor_id);
>  
> - if (sysfs_get_model(parent, pp->product_id, SCSI_PRODUCT_SIZE) <= 0)
> + if (sysfs_get_model(parent, pp->product_id, PATH_PRODUCT_SIZE) <= 0)
>   return 1;
>  
>   condlog(3, "%s: product = %s", pp->dev, pp->product_id);
>  
> - if (sysfs_get_rev(parent, pp->rev, SCSI_REV_SIZE) < 0)
> + if (sysfs_get_rev(parent, pp->rev, PATH_REV_SIZE) < 0)
>   return 1;
>  
>   condlog(3, "%s: rev = %s", pp->dev, pp->rev);
> @@ -1223,11 +1223,11 @@ nvme_sysfs_pathinfo (struct path * pp, vector hwtable)
>   pp->sg_id.channel = attr ? atoi(attr) : 0;
>  
>   snprintf(pp->vendor_id, SCSI_VENDOR_SIZE, "NVME");
> - snprintf(pp->product_id, SCSI_PRODUCT_SIZE, "%s",
> + snprintf(pp->product_id, PATH_PRODUCT_SIZE, "%s",
>udev_device_get_sysattr_value(parent, "model"));
>   snprintf(pp->serial, SERIAL_SIZE, "%s",
>udev_device_get_sysattr_value(parent, "serial"));
> - snprintf(pp->rev, SCSI_REV_SIZE, "%s",
> + snprintf(pp->rev, PATH_REV_SIZE, "%s",
>udev_device_get_sysattr_value(parent, "firmware_rev"));
>  
>   condlog(3, "%s: vendor = %s", pp->dev, pp->vendor_id);
> @@ -1342,12 +1342,12 @@ cciss_sysfs_pathinfo (struct path * pp, vector 
> hwtable)
>  
>   condlog(3, "%s: vendor = %s", pp->dev, pp->vendor_id);
>  
> - if (sysfs_get_model(parent, pp->product_id, SCSI_PRODUCT_SIZE) <= 0)
> + if (sysfs_get_model(parent, pp->product_id, PATH_PRODUCT_SIZE) <= 0)
>   return 1;
>  
>   condlog(3, "%s: product = %s", pp->dev, pp->product_id);
>  
> - if (sysfs_get_rev(parent, pp->rev, SCSI_REV_SIZE) <= 0)
> + if (sysfs_get_rev(parent, pp->rev, PATH_REV_SIZE) <= 0)
>   return 1;
>  
>   condlog(3, "%s: rev = %s", pp->dev, pp->rev);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index d132dfcebce4..b951c7b0e157 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -25,6 +25,12 @@
>  #define SCSI_PRODUCT_SIZE17
>  #define SCSI_REV_SIZE5
>  #define SCSI_STATE_SIZE  19
> +#define NVME_MODEL_SIZE 41
> +#define NVME_REV_SIZE   9
> +
> +/* This must be the maximum of SCSI and NVME sizes */
> +#define PATH_PRODUCT_SIZE NVME_MODEL_SIZE
> +#define PATH_REV_SIZE NVME_REV_SIZE
>  
>  #define NO_PATH_RETRY_UNDEF  0
>  #define NO_PATH_RETRY_FAIL   -1
> @@ -214,8 +220,8 @@ struct path {
>   struct hd_geometry geom;
>   char wwid[WWID_SIZE];
>   char vendor_id[SCSI_VENDOR_SIZE];
> - char product_id[SCSI_PRODUCT_SIZE];
> - char rev[SCSI_REV_SIZE];
> + char product_id[PATH_PRODUCT_SIZE];
> + char rev[PATH_REV_SIZE];
>   char serial[SERIAL_SIZE];
>   char tgt_node_name[NODE_NAME_SIZE];
>   unsigned long long size;
> -- 
> 2.15.1

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

[dm-devel] [PATCH 4/7] multipathd: remove unused configure parameter

[Benjamin Marzinski]

configure() is always called with start_waiters=1, so there is no point
in having the parameter. Remove it.

Signed-off-by: Benjamin Marzinski 
---
 multipathd/main.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index cf76241..233d09f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1950,7 +1950,7 @@ checkerloop (void *ap)
 }
 
 int
-configure (struct vectors * vecs, int start_waiters)
+configure (struct vectors * vecs)
 {
struct multipath * mpp;
struct path * pp;
@@ -2049,11 +2049,9 @@ configure (struct vectors * vecs, int start_waiters)
i--;
continue;
}
-   if (start_waiters) {
-   if (start_waiter_thread(mpp, vecs)) {
-   remove_map(mpp, vecs, 1);
-   i--;
-   }
+   if (start_waiter_thread(mpp, vecs)) {
+   remove_map(mpp, vecs, 1);
+   i--;
}
}
return 0;
@@ -2120,7 +2118,7 @@ reconfigure (struct vectors * vecs)
rcu_assign_pointer(multipath_conf, conf);
call_rcu(>rcu, rcu_free_config);
 
-   configure(vecs, 1);
+   configure(vecs);
 
 
return 0;
-- 
2.7.4

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

[dm-devel] [PATCH 1/7] multipath: fix tur checker locking

[Benjamin Marzinski]

Commit 6e2423fd fixed a bug where the tur checker could cancel a
detached thread after it had exitted. However in fixing this, the new
code grabbed a mutex (to call condlog) while holding a spin_lock. To
deal with that, and to try to keep with the maixim "lock data, not
code", I've changed how the tur checker synchronizes with its thread.

Now, the tur checker creates joinable threads, and detaches them when
the thread is finished or has timed out. To track the state of the
threads, I've added a new variable to the checker context, ct->attached.
When a thread starts, attached is set to 1. When the thread finishes, it
saves the value of attached, and then zeros it out, while locked. If
attached was set, it detaches itself.

When the tur checker gives up on a thread, it also saves and decrements
ct->attached, while locked. At the same time it saves the value of
ct->thread.  If attached was set, it cancels the thread, and then
detaches it.

So the values that are protected by the spin lock are now ct->holders,
ct->thread, and ct->attached. There are cases where the tur checker just
wants to know if the thread is running. If so, its safe to simply read
ct->thread without locking.  Also, if it knows that the thread isn't
running, it can freely access all of these variables. I've left the
locking in-place in these cases to make the static analyzers happy.
However I have added comments stating when the locking isn't actually
necessary.

Signed-off-by: Benjamin Marzinski 
---
 libmultipath/checkers/tur.c | 66 -
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index b4a5cb2..6ae9700 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -46,6 +46,7 @@ struct tur_checker_context {
pthread_cond_t active;
pthread_spinlock_t hldr_lock;
int holders;
+   int attached;
char message[CHECKER_MSG_LEN];
 };
 
@@ -98,17 +99,21 @@ void libcheck_free (struct checker * c)
 {
if (c->context) {
struct tur_checker_context *ct = c->context;
-   int holders;
+   int attached, holders;
pthread_t thread;
 
pthread_spin_lock(>hldr_lock);
ct->holders--;
holders = ct->holders;
+   attached = ct->attached;
+   ct->attached = 0;
thread = ct->thread;
pthread_spin_unlock(>hldr_lock);
-   if (holders)
+   if (attached) {
pthread_cancel(thread);
-   else
+   pthread_detach(thread);
+   }
+   if (!holders)
cleanup_context(ct);
c->context = NULL;
}
@@ -218,15 +223,21 @@ retry:
 
 static void cleanup_func(void *data)
 {
-   int holders;
+   int attached, holders;
+   pthread_t thread;
struct tur_checker_context *ct = data;
pthread_spin_lock(>hldr_lock);
ct->holders--;
holders = ct->holders;
+   attached = ct->attached;
+   ct->attached = 0;
+   thread = ct->thread;
ct->thread = 0;
pthread_spin_unlock(>hldr_lock);
if (!holders)
cleanup_context(ct);
+   if (attached)
+   pthread_detach(thread);
 }
 
 static int tur_running(struct tur_checker_context *ct)
@@ -324,7 +335,8 @@ int libcheck_check(struct checker * c)
pthread_attr_t attr;
int tur_status, r;
char devt[32];
-
+   pthread_t thread;
+   int timed_out, attached;
 
if (!ct)
return PATH_UNCHECKED;
@@ -350,18 +362,27 @@ int libcheck_check(struct checker * c)
}
 
if (ct->running) {
-   /*
-* Check if TUR checker is still running. Hold hldr_lock
-* around the pthread_cancel() call to avoid that
-* pthread_cancel() gets called after the (detached) TUR
-* thread has exited.
-*/
-   pthread_spin_lock(>hldr_lock);
-   if (ct->thread) {
-   if (tur_check_async_timeout(c)) {
+   timed_out = tur_check_async_timeout(c);
+   if (timed_out) {
+   pthread_spin_lock(>hldr_lock);
+   attached = ct->attached;
+   ct->attached = 0;
+   thread = ct->thread;
+   pthread_spin_unlock(>hldr_lock);
+   if (attached) {
+   pthread_cancel(thread);
+   pthread_detach(thread);
+   }
+   } else {
+   /* locking here solely to make static analyzers happy */
+   pthread_spin_lock(>hldr_lock);
+   thread = ct->thread;

[dm-devel] [PATCH 3/7] multipathd: remove coalesce_paths from ev_add_map

[Benjamin Marzinski]

If ev_add_map is called for a multipath device that doesn't exist in
device-mapper, it will call coalesce_paths to add it.  This doesn't work
and hasn't for years. It doesn't add the map to the mpvec, or start up
waiters, or do any of the necessary things that do get done when you
call ev_add_map for a map that does exist in device mapper.

Fortunately, there are only two things that call ev_add_map. uev_add_map
makes sure that the device does exist in device-mapper before calling
ev_add_map, and cli_add_map creates the device first and then calls
ev_add_map, if the device doesn't exist.

So, there is no reason for coalesce_paths to be in ev_add_map. This
removes it.

Signed-off-by: Benjamin Marzinski 
---
 multipathd/main.c | 41 +
 1 file changed, 9 insertions(+), 32 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 27cf234..cf76241 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -415,15 +415,11 @@ uev_add_map (struct uevent * uev, struct vectors * vecs)
 int
 ev_add_map (char * dev, char * alias, struct vectors * vecs)
 {
-   char * refwwid;
struct multipath * mpp;
-   int map_present;
-   int r = 1, delayed_reconfig, reassign_maps;
+   int delayed_reconfig, reassign_maps;
struct config *conf;
 
-   map_present = dm_map_present(alias);
-
-   if (map_present && !dm_is_mpath(alias)) {
+   if (!dm_is_mpath(alias)) {
condlog(4, "%s: not a multipath map", alias);
return 0;
}
@@ -468,33 +464,14 @@ ev_add_map (char * dev, char * alias, struct vectors * 
vecs)
/*
 * now we can register the map
 */
-   if (map_present) {
-   if ((mpp = add_map_without_path(vecs, alias))) {
-   sync_map_state(mpp);
-   condlog(2, "%s: devmap %s registered", alias, dev);
-   return 0;
-   } else {
-   condlog(2, "%s: uev_add_map failed", dev);
-   return 1;
-   }
-   }
-   r = get_refwwid(CMD_NONE, dev, DEV_DEVMAP, vecs->pathvec, );
-
-   if (refwwid) {
-   r = coalesce_paths(vecs, NULL, refwwid, FORCE_RELOAD_NONE,
-  CMD_NONE);
-   dm_lib_release();
+   if ((mpp = add_map_without_path(vecs, alias))) {
+   sync_map_state(mpp);
+   condlog(2, "%s: devmap %s registered", alias, dev);
+   return 0;
+   } else {
+   condlog(2, "%s: uev_add_map failed", dev);
+   return 1;
}
-
-   if (!r)
-   condlog(2, "%s: devmap %s added", alias, dev);
-   else if (r == 2)
-   condlog(2, "%s: uev_add_map %s blacklisted", alias, dev);
-   else
-   condlog(0, "%s: uev_add_map %s failed", alias, dev);
-
-   FREE(refwwid);
-   return r;
 }
 
 static int
-- 
2.7.4

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

[dm-devel] [PATCH 6/7] multipathd: change spurious uevent msg priority

[Benjamin Marzinski]

The "spurious uevent, path already in pathvec" is not anything to worry
about, so it should not have the error priority.

Signed-off-by: Benjamin Marzinski 
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 233d09f..cfd0363 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -557,7 +557,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, 
int need_do_map)
if (pp) {
int r;
 
-   condlog(0, "%s: spurious uevent, path already in pathvec",
+   condlog(2, "%s: spurious uevent, path already in pathvec",
uev->kernel);
if (!pp->mpp && !strlen(pp->wwid)) {
condlog(3, "%s: reinitialize path", uev->kernel);
-- 
2.7.4

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

[dm-devel] Help request: setting up /dev/zero using dm-crypt ...

[Ravi Prakash Putchala]

Hi,

We would like to experiment and see the performance results with different
options of dm-crypt. To do this we would like to setup /dev/zero with
dm-crypt as mentioned in
https://www.redhat.com/archives/dm-devel/2015-February/msg00106.html
However, when I did the following no new mapping gets created in
/dev/mapper:

# cryptsetup -y luksFormat /dev/zero
# cryptsetup luksOpen /dev/zero dm-zero
# cryptsetup status /dev/mapper/dm-zero
/dev/mapper/dm-zero is inactive.

Could you please help by giving instructions on how to do this right?

Thank you.

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

Re: [dm-devel] Help request: setting up /dev/zero using dm-crypt ...

[Milan Broz]

On 02/07/2018 09:15 AM, Ravi Prakash Putchala wrote:
> We would like to experiment and see the performance results with different 
> options of dm-crypt. To do this we would like to setup /dev/zero with 
> dm-crypt as mentioned in 
> https://www.redhat.com/archives/dm-devel/2015-February/msg00106.html
> However, when I did the following no new mapping gets created in /dev/mapper:
> 
> # cryptsetup -y luksFormat /dev/zero

This command cannot work, /dev/zero is a character device, moreover all writes 
there are discarded.

Seems cryptsetup does not print error message here (that is a bug) but return 
value of this command
indicates the problem: 4 - that means "wrong device specified" (see man page).

If you need experiment over a memory device, use either ramdisk, scsi_debug 
module to it.
Another alternative is to create dm-zero device and use plain dm-crypt (not 
LUKS).

Milan

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