[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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 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 redhat com
https://www.redhat.com/mailman/listinfo/dm-devel



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]