[Qemu-devel] [PATCH] block: Make BSG detection more sane in hdev_open()

[Qemu-devel] [PATCH] block: Make BSG detection more sane in hdev_open()

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Greetings hch, tomo and Co,

This patch changes the Linux BSG backstore detect logic in hdev_open()
in order to determine when to actually set 'bs->sg = BDS_BSG;' by obtaining
the BSG major from a SysFS attribute in /sys/class/bsg/$H:C:T:L/dev,
instead of the original hardcoded 254 major check.

This patch has been tested with Linux KVM guest v2.6.26 using the megasas
HBA emulation SGL passthrough from a TCM_Loop IBLOCK backstore running on
v2.6.35 x86_64.

Thanks to Mark Harvey for initially contributing code for doing this with
tgt.git/usr/bs_sg.c!

Thanks!

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 block/raw-posix.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e7afc4a..487e7f0 100644
+++ b/block/raw-posix.c
@@ -842,7 +842,10 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
 #if defined(__linux__)
-    struct stat st;
+    struct stat st, st2;
+    FILE *file;
+    char major[8], dev[64], path[128], *p, *buf;
+    int ch, i;
 #endif
 
 #ifdef CONFIG_COCOA
@@ -881,11 +884,51 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
     if (S_ISCHR(st.st_mode)) {
         if (major(st.st_rdev) == SCSI_GENERIC_MAJOR) {
             bs->sg = BDS_SCSI_GENERIC;
-        } else if (major(st.st_rdev) == 254) {
-            /* This is not yet defined in include/linux/major.h.. */
-            bs->sg = BDS_BSG;
+        } else { /* Extract major for BSG backstore usage */
+            memset(major, 0, 8);
+            memset(dev, 0, 64);
+            memset(path, 0, 128);
+
+            buf = strdup(filename);
+            if (!(buf))
+                goto out;
+            /*
+             * Locate the device name from the path, we are interested
+             * in the last strsep() token..
+             */
+            while ((p = strsep(&buf, "/")))
+                snprintf(dev, 64, "%s", p);
+            /*
+             * Check to sure the sysfs entry exists before calling open
+             */
+            snprintf(path, 128, "/sys/class/bsg/%s/dev", dev);
+            if (stat(path, &st2) < 0)
+                goto out;
+
+            file = fopen(path, "r");
+            if (!(file)) {
+                printf("fopen() failed for BSG sysfs path: %s\n", path);
+                goto out;
+            }
+            ch = fgetc(file);
+            for (i = 0; i < 7; i++) {
+                if (ch == ':') {
+                    major[i] = '\0';
+                    break;
+                }
+                major[i] = ch;
+                ch = fgetc(file);
+            }
+            fclose(file);
+            /*
+             * If the major returned by /sys/class/bsg/$H:C:T:L/dev matches
+             * stat(), then we signal BDS_BSG usage.
+             */ 
+            if (major(st.st_rdev) == atoi(major))
+                bs->sg = BDS_BSG;
         }
     }
+out:
 #endif
 
     return raw_open_common(bs, filename, flags, 0);
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH] block: Make BSG detection more sane in hdev_open()

[Nicholas A. Bellinger]

On Sat, 2010-08-21 at 16:01 -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Greetings hch, tomo and Co,
> 
> This patch changes the Linux BSG backstore detect logic in hdev_open()
> in order to determine when to actually set 'bs->sg = BDS_BSG;' by obtaining
> the BSG major from a SysFS attribute in /sys/class/bsg/$H:C:T:L/dev,
> instead of the original hardcoded 254 major check.
> 
> This patch has been tested with Linux KVM guest v2.6.26 using the megasas
> HBA emulation SGL passthrough from a TCM_Loop IBLOCK backstore running on
> v2.6.35 x86_64.
> 
> Thanks to Mark Harvey for initially contributing code for doing this with
> tgt.git/usr/bs_sg.c!
>
> Thanks!
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  block/raw-posix.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e7afc4a..487e7f0 100644
> +++ b/block/raw-posix.c
> @@ -842,7 +842,10 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>  {
>      BDRVRawState *s = bs->opaque;
>  #if defined(__linux__)
> -    struct stat st;
> +    struct stat st, st2;
> +    FILE *file;
> +    char major[8], dev[64], path[128], *p, *buf;
> +    int ch, i;
>  #endif
>  
>  #ifdef CONFIG_COCOA
> @@ -881,11 +884,51 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>      if (S_ISCHR(st.st_mode)) {
>          if (major(st.st_rdev) == SCSI_GENERIC_MAJOR) {
>              bs->sg = BDS_SCSI_GENERIC;
> -        } else if (major(st.st_rdev) == 254) {
> -            /* This is not yet defined in include/linux/major.h.. */
> -            bs->sg = BDS_BSG;
> +        } else { /* Extract major for BSG backstore usage */
> +            memset(major, 0, 8);
> +            memset(dev, 0, 64);
> +            memset(path, 0, 128);
> +
> +            buf = strdup(filename);
> +            if (!(buf))
> +                goto out;


Ugh, one quick followup on this to address missing free(buf) calls from
the usage of buf = strdup(filename).

http://git.kernel.org/?p=virt/kvm/nab/qemu-kvm.git;a=commitdiff;h=9cc774b1766c07065dee4637febe9f7d9237ff7e

Thanks,

--nab

> +            /*
> +             * Locate the device name from the path, we are interested
> +             * in the last strsep() token..
> +             */
> +            while ((p = strsep(&buf, "/")))
> +                snprintf(dev, 64, "%s", p);
> +            /*
> +             * Check to sure the sysfs entry exists before calling open
> +             */
> +            snprintf(path, 128, "/sys/class/bsg/%s/dev", dev);
> +            if (stat(path, &st2) < 0)
> +                goto out;
> +
> +            file = fopen(path, "r");
> +            if (!(file)) {
> +                printf("fopen() failed for BSG sysfs path: %s\n", path);
> +                goto out;
> +            }
> +            ch = fgetc(file);
> +            for (i = 0; i < 7; i++) {
> +                if (ch == ':') {
> +                    major[i] = '\0';
> +                    break;
> +                }
> +                major[i] = ch;
> +                ch = fgetc(file);
> +            }
> +            fclose(file);
> +            /*
> +             * If the major returned by /sys/class/bsg/$H:C:T:L/dev matches
> +             * stat(), then we signal BDS_BSG usage.
> +             */ 
> +            if (major(st.st_rdev) == atoi(major))
> +                bs->sg = BDS_BSG;
>          }
>      }
> +out:
>  #endif
>  
>      return raw_open_common(bs, filename, flags, 0);
> -- 
> 1.5.6.5
> 

[Qemu-devel] Re: [PATCH] block: Make BSG detection more sane in hdev_open()

[Christoph Hellwig]

On Sat, Aug 21, 2010 at 04:01:15PM -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Greetings hch, tomo and Co,

What tree is this against?  I can't see any specificc BSG support in qemu.

Even more I think all this in the wrong place.  The only reason
SG_IO support was shoe-horned into raw-posix is that we only had -drive
and no way to specify other devices.  We now have -device and a scsi
generic device should be just that without an attached driver, so that
we can get rid of all the special casing for sd devices.

[Qemu-devel] Re: [PATCH] block: Make BSG detection more sane in hdev_open()

[Nicholas A. Bellinger]

On Sun, 2010-08-22 at 10:31 +0200, Christoph Hellwig wrote:
> On Sat, Aug 21, 2010 at 04:01:15PM -0700, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Greetings hch, tomo and Co,
> 
> What tree is this against?  I can't see any specificc BSG support in qemu.
> 

The Megasas / SGL passthrough friendly qemu-kvm.git/scsi-bsg updated to
v0.12.5 is here:

http://git.kernel.org/?p=virt/kvm/nab/qemu-kvm.git;a=shortlog;h=refs/heads/scsi-bsg

and the BSG driver is here:

http://git.kernel.org/?p=virt/kvm/nab/qemu-kvm.git;a=blob;f=hw/scsi-bsg.c;hb=refs/heads/scsi-bsg


> Even more I think all this in the wrong place.  The only reason
> SG_IO support was shoe-horned into raw-posix is that we only had -drive
> and no way to specify other devices.  We now have -device and a scsi
> generic device should be just that without an attached driver, so that
> we can get rid of all the special casing for sd devices.
> 

Hmmm, Ok, I think I see what you mean here..

So dropping the explict checking in raw-posix.c to rensure a '-drive
file=/dev/bsg/H:C:T:L' used with '-device scsi-bsg' is really a BSG
major is OK.  And this should instead do similar checks to my rev1 patch
in hw/scsi-bsg.c:bsg_generic_initfn(), and just drop the bdrv_is_bsg()
(and brdv_is_sg() for hw/scsi-generic.c) stuff altogether, yes..?

Thanks!

--nab