[Qemu-devel] [PATCH 0/4] Some fixes related to scsi-generic

[Qemu-devel] [PATCH 0/4] Some fixes related to scsi-generic

[Dimitris Aragiorgis]

Hi all,

These four patches make slight changes to the way QEMU handles SCSI
generic devices to fix a number of small problems.

I am sending them against the master branch, since I don't know if they
can be considered bugfixes.

Thanks,
dimara

Dimitris Aragiorgis (4):
  Fix migration in case of scsi-generic
  block: Use bdrv_is_sg() everywhere
  raw-posix: Introduce hdev_is_sg()
  block: Include qemu-log.o in block-obj-y

 Makefile.objs     |    2 +-
 block.c           |    6 +++---
 block/io.c        |    2 +-
 block/iscsi.c     |    2 +-
 block/raw-posix.c |   43 ++++++++++++++++++++++++++++++-------------
 5 files changed, 36 insertions(+), 19 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4] Fix migration in case of scsi-generic

[Dimitris Aragiorgis]

During migration, QEMU uses fsync()/fdatasync() on the open file
descriptor for read-write block devices to flush data just before
stopping the VM.

However, fsync() on a scsi-generic device returns -EINVAL which
causes the migration to fail. This patch skips flushing data in case
of an SG device, since submitting SCSI commands directly via an SG
character device (e.g. /dev/sg0) bypasses the page cache completely,
anyway.

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 1ce62c4..d248a4d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2231,7 +2231,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 {
     int ret;
 
-    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) || bdrv_is_sg(bs)) {
         return 0;
     }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/4] Fix migration in case of scsi-generic

[Paolo Bonzini]

On 08/05/2015 13:38, Dimitris Aragiorgis wrote:
> During migration, QEMU uses fsync()/fdatasync() on the open file
> descriptor for read-write block devices to flush data just before
> stopping the VM.
> 
> However, fsync() on a scsi-generic device returns -EINVAL which
> causes the migration to fail. This patch skips flushing data in case
> of an SG device, since submitting SCSI commands directly via an SG
> character device (e.g. /dev/sg0) bypasses the page cache completely,
> anyway.

Good catch!  The patch is correct, but please remove the same test from
iscsi_co_flush (in block/iscsi.c) because it's now duplicate.

Thanks,

Paolo

> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/io.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 1ce62c4..d248a4d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2231,7 +2231,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>  {
>      int ret;
>  
> -    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> +    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) || bdrv_is_sg(bs)) {
>          return 0;
>      }
>  
> 

[Qemu-devel] [PATCH 2/4] block: Use bdrv_is_sg() everywhere

[Dimitris Aragiorgis]

Instead of checking bs->sg use bdrv_is_sg() consistently throughout
the code.

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block.c           |    6 +++---
 block/iscsi.c     |    2 +-
 block/raw-posix.c |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 7904098..d651b57 100644
--- a/block.c
+++ b/block.c
@@ -566,7 +566,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
     int ret = 0;
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
-    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
+    if (bdrv_is_sg(bs) || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
         *pdrv = &bdrv_raw;
         return ret;
     }
@@ -598,7 +598,7 @@ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
     BlockDriver *drv = bs->drv;
 
     /* Do not attempt drv->bdrv_getlength() on scsi-generic devices */
-    if (bs->sg)
+    if (bdrv_is_sg(bs))
         return 0;
 
     /* query actual device if possible, otherwise just trust the hint */
@@ -890,7 +890,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     }
 
     assert(bdrv_opt_mem_align(bs) != 0);
-    assert((bs->request_alignment != 0) || bs->sg);
+    assert((bs->request_alignment != 0) || bdrv_is_sg(bs));
     return 0;
 
 free_and_fail:
diff --git a/block/iscsi.c b/block/iscsi.c
index 8fca1d3..502a81f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -627,7 +627,7 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
     IscsiLun *iscsilun = bs->opaque;
     struct IscsiTask iTask;
 
-    if (bs->sg) {
+    if (bdrv_is_sg(bs)) {
         return 0;
     }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 24d8582..24b061f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -302,9 +302,9 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     BDRVRawState *s = bs->opaque;
     char *buf;
 
-    /* For /dev/sg devices the alignment is not really used.
+    /* For SG devices the alignment is not really used.
        With buffered I/O, we don't have any restrictions. */
-    if (bs->sg || !s->needs_alignment) {
+    if (bdrv_is_sg(bs) || !s->needs_alignment) {
         bs->request_alignment = 1;
         s->buf_align = 1;
         return;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/4] block: Use bdrv_is_sg() everywhere

[Paolo Bonzini]

On 08/05/2015 13:38, Dimitris Aragiorgis wrote:
> Instead of checking bs->sg use bdrv_is_sg() consistently throughout
> the code.
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block.c           |    6 +++---
>  block/iscsi.c     |    2 +-
>  block/raw-posix.c |    4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7904098..d651b57 100644
> --- a/block.c
> +++ b/block.c
> @@ -566,7 +566,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
>      int ret = 0;
>  
>      /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
> +    if (bdrv_is_sg(bs) || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>          *pdrv = &bdrv_raw;
>          return ret;
>      }
> @@ -598,7 +598,7 @@ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
>      BlockDriver *drv = bs->drv;
>  
>      /* Do not attempt drv->bdrv_getlength() on scsi-generic devices */
> -    if (bs->sg)
> +    if (bdrv_is_sg(bs))
>          return 0;
>  
>      /* query actual device if possible, otherwise just trust the hint */
> @@ -890,7 +890,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>      }
>  
>      assert(bdrv_opt_mem_align(bs) != 0);
> -    assert((bs->request_alignment != 0) || bs->sg);
> +    assert((bs->request_alignment != 0) || bdrv_is_sg(bs));
>      return 0;
>  
>  free_and_fail:
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 8fca1d3..502a81f 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -627,7 +627,7 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
>      IscsiLun *iscsilun = bs->opaque;
>      struct IscsiTask iTask;
>  
> -    if (bs->sg) {
> +    if (bdrv_is_sg(bs)) {
>          return 0;
>      }
>  
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 24d8582..24b061f 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -302,9 +302,9 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>      BDRVRawState *s = bs->opaque;
>      char *buf;
>  
> -    /* For /dev/sg devices the alignment is not really used.
> +    /* For SG devices the alignment is not really used.
>         With buffered I/O, we don't have any restrictions. */
> -    if (bs->sg || !s->needs_alignment) {
> +    if (bdrv_is_sg(bs) || !s->needs_alignment) {
>          bs->request_alignment = 1;
>          s->buf_align = 1;
>          return;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH 3/4] raw-posix: Introduce hdev_is_sg()

[Dimitris Aragiorgis]

Until now, an SG device was identified only by checking if its path
started with "/dev/sg". Then, hdev_open() set bs->sg accordingly.
This is very fragile, e.g. it fails with symlinks or relative paths.
We should rely on the actual properties of the device instead of the
specified file path.

Test for an SG device (e.g. /dev/sg0) by ensuring that all of the
following holds:

 - The device supports the SG_GET_VERSION_NUM ioctl
 - The device supports the SG_GET_SCSI_ID ioctl
 - The specified file name corresponds to a character device

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/raw-posix.c |   39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 24b061f..d35e5ac 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -57,6 +57,7 @@
 #include <linux/fd.h>
 #include <linux/fs.h>
 #include <linux/hdreg.h>
+#include <scsi/sg.h>
 #ifdef __s390__
 #include <asm/dasd.h>
 #endif
@@ -2073,15 +2074,38 @@ static void hdev_parse_filename(const char *filename, QDict *options,
     qdict_put_obj(options, "filename", QOBJECT(qstring_from_str(filename)));
 }
 
+static int hdev_is_sg(BlockDriverState *bs)
+{
+
+#if defined(__linux__)
+
+    struct stat st;
+    struct sg_scsi_id scsiid;
+    int sg_version;
+
+    if (!bdrv_ioctl(bs, SG_GET_VERSION_NUM, &sg_version) &&
+        !bdrv_ioctl(bs, SG_GET_SCSI_ID, &scsiid) &&
+        stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode)) {
+        DEBUG_BLOCK_PRINT("SG device found: type=%d, version=%d\n",
+            scsiid.scsi_type, sg_version);
+        return 1;
+    }
+
+#endif
+
+    return 0;
+}
+
 static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     Error *local_err = NULL;
     int ret;
-    const char *filename = qdict_get_str(options, "filename");
 
 #if defined(__APPLE__) && defined(__MACH__)
+    const char *filename = qdict_get_str(options, "filename");
+
     if (strstart(filename, "/dev/cdrom", NULL)) {
         kern_return_t kernResult;
         io_iterator_t mediaIterator;
@@ -2110,16 +2134,6 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
 #endif
 
     s->type = FTYPE_FILE;
-#if defined(__linux__)
-    {
-        char resolved_path[ MAXPATHLEN ], *temp;
-
-        temp = realpath(filename, resolved_path);
-        if (temp && strstart(temp, "/dev/sg", NULL)) {
-            bs->sg = 1;
-        }
-    }
-#endif
 
     ret = raw_open_common(bs, options, flags, 0, &local_err);
     if (ret < 0) {
@@ -2129,6 +2143,9 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
+    /* Since this does ioctl the device must be already opened */
+    bs->sg = hdev_is_sg(bs);
+
     if (flags & BDRV_O_RDWR) {
         ret = check_hdev_writable(s);
         if (ret < 0) {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/4] raw-posix: Introduce hdev_is_sg()

[Paolo Bonzini]

On 08/05/2015 13:38, Dimitris Aragiorgis wrote:
> Until now, an SG device was identified only by checking if its path
> started with "/dev/sg". Then, hdev_open() set bs->sg accordingly.
> This is very fragile, e.g. it fails with symlinks or relative paths.
> We should rely on the actual properties of the device instead of the
> specified file path.
> 
> Test for an SG device (e.g. /dev/sg0) by ensuring that all of the
> following holds:
> 
>  - The device supports the SG_GET_VERSION_NUM ioctl
>  - The device supports the SG_GET_SCSI_ID ioctl
>  - The specified file name corresponds to a character device
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/raw-posix.c |   39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 24b061f..d35e5ac 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -57,6 +57,7 @@
>  #include <linux/fd.h>
>  #include <linux/fs.h>
>  #include <linux/hdreg.h>
> +#include <scsi/sg.h>
>  #ifdef __s390__
>  #include <asm/dasd.h>
>  #endif
> @@ -2073,15 +2074,38 @@ static void hdev_parse_filename(const char *filename, QDict *options,
>      qdict_put_obj(options, "filename", QOBJECT(qstring_from_str(filename)));
>  }
>  
> +static int hdev_is_sg(BlockDriverState *bs)
> +{
> +
> +#if defined(__linux__)
> +
> +    struct stat st;
> +    struct sg_scsi_id scsiid;
> +    int sg_version;
> +
> +    if (!bdrv_ioctl(bs, SG_GET_VERSION_NUM, &sg_version) &&
> +        !bdrv_ioctl(bs, SG_GET_SCSI_ID, &scsiid) &&
> +        stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode)) {
> +        DEBUG_BLOCK_PRINT("SG device found: type=%d, version=%d\n",
> +            scsiid.scsi_type, sg_version);

Please replace the DEBUG_BLOCK_PRINT with a tracepoint (see the
trace-events file).  With this change, the patch is okay and definitely
an improvement.

Paolo

> +        return 1;
> +    }
> +
> +#endif
> +
> +    return 0;
> +}
> +
>  static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>                       Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
>      Error *local_err = NULL;
>      int ret;
> -    const char *filename = qdict_get_str(options, "filename");
>  
>  #if defined(__APPLE__) && defined(__MACH__)
> +    const char *filename = qdict_get_str(options, "filename");
> +
>      if (strstart(filename, "/dev/cdrom", NULL)) {
>          kern_return_t kernResult;
>          io_iterator_t mediaIterator;
> @@ -2110,16 +2134,6 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>  #endif
>  
>      s->type = FTYPE_FILE;
> -#if defined(__linux__)
> -    {
> -        char resolved_path[ MAXPATHLEN ], *temp;
> -
> -        temp = realpath(filename, resolved_path);
> -        if (temp && strstart(temp, "/dev/sg", NULL)) {
> -            bs->sg = 1;
> -        }
> -    }
> -#endif
>  
>      ret = raw_open_common(bs, options, flags, 0, &local_err);
>      if (ret < 0) {
> @@ -2129,6 +2143,9 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>          return ret;
>      }
>  
> +    /* Since this does ioctl the device must be already opened */
> +    bs->sg = hdev_is_sg(bs);
> +
>      if (flags & BDRV_O_RDWR) {
>          ret = check_hdev_writable(s);
>          if (ret < 0) {
> 

[Qemu-devel] [PATCH 4/4] block: Include qemu-log.o in block-obj-y

[Dimitris Aragiorgis]

Building the QEMU tools fails if we #define DEBUG_BLOCK inside
block/raw-posix.c. This happens because qemu-log.o is missing from
block-obj-y, which causes the link to fail. Fix this.

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 Makefile.objs |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.objs b/Makefile.objs
index 28999d3..98f6e02 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -6,7 +6,7 @@ util-obj-y = util/ qobject/ qapi/ qapi-types.o qapi-visit.o qapi-event.o
 #######################################################################
 # block-obj-y is code used by both qemu system emulation and qemu-img
 
-block-obj-y = async.o thread-pool.o
+block-obj-y = async.o thread-pool.o qemu-log.o
 block-obj-y += nbd.o block.o blockjob.o
 block-obj-y += main-loop.o iohandler.o qemu-timer.o
 block-obj-$(CONFIG_POSIX) += aio-posix.o
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/4] block: Include qemu-log.o in block-obj-y

[Paolo Bonzini]

On 08/05/2015 13:38, Dimitris Aragiorgis wrote:
> Building the QEMU tools fails if we #define DEBUG_BLOCK inside
> block/raw-posix.c. This happens because qemu-log.o is missing from
> block-obj-y, which causes the link to fail. Fix this.
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>

Let's just remove DEBUG_BLOCK messages.  There are only two and they
only trigger for error cases.

Paolo

> ---
>  Makefile.objs |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 28999d3..98f6e02 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -6,7 +6,7 @@ util-obj-y = util/ qobject/ qapi/ qapi-types.o qapi-visit.o qapi-event.o
>  #######################################################################
>  # block-obj-y is code used by both qemu system emulation and qemu-img
>  
> -block-obj-y = async.o thread-pool.o
> +block-obj-y = async.o thread-pool.o qemu-log.o
>  block-obj-y += nbd.o block.o blockjob.o
>  block-obj-y += main-loop.o iohandler.o qemu-timer.o
>  block-obj-$(CONFIG_POSIX) += aio-posix.o
>