[dm-devel] sg_persist triggers block kernel event ???

[Christophe Varoqui]

Doug,

would this patch address the Linux kernel backward-compatibility concern
you raised ?

Note, you would have to substitute the 3000 version by the appropriate
version that introduced support for prin commands submitted on a open-ro
device ...

--- sg_persist.c.orig 2014-05-04 01:10:01.987981956 +0200
+++ sg_persist.c 2014-05-04 20:44:34.665292548 +0200
@@ -16,6 +16,7 @@
 #include <string.h>
 #include <ctype.h>
 #include <getopt.h>
+#include <sys/ioctl.h>
 #define __STDC_FORMAT_MACROS 1

 #include <inttypes.h>
@@ -26,6 +27,7 @@
 #include "sg_lib.h"
 #include "sg_cmds_basic.h"
 #include "sg_cmds_extra.h"
+#include "sg_io_linux.h"

 static const char * version_str = "0.44 20140202";

@@ -1029,6 +1031,8 @@
     struct sg_simple_inquiry_resp inq_resp;
     const char * cp;
     struct opts_t opts;
+    int omode = 0;
+    const char *omode_desc = "rw";

     memset(&opts, 0, sizeof(opts));
     opts.prin = 1;
@@ -1292,10 +1296,31 @@
         sg_cmds_close_device(sg_fd);
     }

-    if ((sg_fd = sg_cmds_open_device(device_name, 0 /* rw */,
+#ifdef SG_LIB_LINUX
+    /*
+     * PRIN commands do not provoke device changes, so we should avoid to
+     * open the device read-write so that the Linux kernel does not
generate
+     * a change event.
+     * Older kernel do not support PRIN commands submission on a read-only
+     * opened device, so don't try in this case.
+     */
+    int v;
+    if (opts.prin) {
+        sg_fd = sg_cmds_open_device(device_name, 1, opts.verbose);
+        if (sg_fd >= 0) {
+            if (ioctl(sg_fd, SG_GET_VERSION_NUM, &v) >= 0 && v >= 30000) {
+                omode = 1;
+                omode_desc = "ro";
+            }
+            sg_cmds_close_device(sg_fd);
+        }
+    }
+#endif
+
+    if ((sg_fd = sg_cmds_open_device(device_name, omode,
                                      opts.verbose)) < 0) {
-        pr2serr("sg_persist: error opening file (rw): %s: %s\n",
device_name,
-                safe_strerror(-sg_fd));
+        pr2serr("sg_persist: error opening file (%s): %s: %s\n",
omode_desc,
+                device_name, safe_strerror(-sg_fd));
         return SG_LIB_FILE_ERROR;
     }




Best regards,
Christophe Varoqui
www.opensvc.com


On Sun, May 4, 2014 at 8:01 PM, Christophe Varoqui <
christophe.varoqui at opensvc.com> wrote:

> Indeed, I'd rather let udev do its work on legitimate device changes.
> Problem is, this change event caused by sg_persist prin commands in not
> legitimate : no device change can be caused by interrogating the disk pr
> status.
>
> Doug informed me newer kernels accept prin commands inside a
> open-ro=>close sequence, which I now verified by testing with the patch I
> sent ... but older kernels do not. My patch is thus not backward compatible
> as-is.
>
> Any advice on how to treat this backward-compatibility issue ?
>
> For information, here's a more recent version of the patch I sent to Doug
> earlier to address not-Linux platform behaviour stability :
>
> --- sg_persist.c.orig 2014-05-04 01:10:01.987981956 +0200
> +++ sg_persist.c 2014-05-04 08:41:29.482017943 +0200
> @@ -1029,6 +1029,8 @@
>      struct sg_simple_inquiry_resp inq_resp;
>      const char * cp;
>      struct opts_t opts;
> +    int omode = 0;
> +    const char *omode_desc = "rw";
>
>      memset(&opts, 0, sizeof(opts));
>      opts.prin = 1;
> @@ -1292,10 +1294,17 @@
>          sg_cmds_close_device(sg_fd);
>      }
>
> -    if ((sg_fd = sg_cmds_open_device(device_name, 0 /* rw */,
> +#ifdef SG_LIB_LINUX
> +    if (opts.prin) {
> +        omode = 1;
> +        omode_desc = "ro";
> +    }
> +#endif
> +
> +    if ((sg_fd = sg_cmds_open_device(device_name, omode,
>                                       opts.verbose)) < 0) {
> -        pr2serr("sg_persist: error opening file (rw): %s: %s\n",
> device_name,
> -                safe_strerror(-sg_fd));
> +        pr2serr("sg_persist: error opening file (%s): %s: %s\n",
> omode_desc,
> +                device_name, safe_strerror(-sg_fd));
>          return SG_LIB_FILE_ERROR;
>      }
>
> Regards,
> Christophe Varoqui
> www.opensvc.com
>
>
>
> On Sun, May 4, 2014 at 7:23 PM, Zdenek Kabelac <zkabelac at redhat.com>wrote:
>
>> Dne 4.5.2014 18:54, Hannes Reinecke napsal(a):
>>
>>  On 05/04/2014 01:34 AM, Christophe Varoqui wrote:
>>>
>>>> It seems sg_persist is doing an "open rw => close" for --in commands,
>>>> causing a kernel change-event.
>>>>
>>> Yep.
>>>
>>> Look for 'watch' in the udev rules, that's precisely what it's doing.
>>>
>>> (Bloody annoying if you ask me. Generally I recommend to remove that
>>> thing
>>> from the rules).
>>>
>>
>> When watch rule is disabled/removed in  udev rules - your udev db becomes
>> invalid when i.e. you run  command like  'mkfs' - since the udev db will
>> not be updated to list information about newly formatted filesystem.
>>
>> Of course there are many cases where disabling  watch rule makes sense
>> (i.e. you export lots of disks to virtual guests) - but unless you are
>> familiar with udev and you know what you are doing - think twice before
>> disabling.
>>
>> Zdenek
>>
>> --
>> dm-devel mailing list
>> dm-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20140504/c8acd286/attachment.htm>