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

[Qemu-devel] [PATCH v2 0/5] 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

v2:
* remove duplicate check for sg inside iscsi_co_flush()
* remove DEBUG_BLOCK_PRINT in block/raw-posix.c
* use DPRINTF for debugging in block/raw-posix.c

PS: Paolo suggested to use a tracepoint inside hdev_is_sg() but I chose DPRINTF
instead. It would make sense to add a tracepoint for bdrv_is_sg() (just like
most bdrv_* commands) but this is too much for now since it just returns the
bs->sg flag (and is not an actual driver function). If you insist I'll change
it in v3.

Dimitris Aragiorgis (5):
  block: Use bdrv_is_sg() everywhere
  Fix migration in case of scsi-generic
  raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT
  raw-posix: Use DPRINTF for DEBUG_FLOPPY
  raw-posix: Introduce hdev_is_sg()

 block.c           |    6 ++---
 block/io.c        |    2 +-
 block/iscsi.c     |    4 ---
 block/raw-posix.c |   75 +++++++++++++++++++++++++++++------------------------
 4 files changed, 45 insertions(+), 42 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v2 1/5] 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.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

[Qemu-devel] [PATCH v2 2/5] 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.

Remove the bdrv_is_sg() test from iscsi_co_flush() since this is
now redundant (we flush the underlying protocol at the end
of bdrv_co_flush() which, with this patch, we never reach).

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/io.c    |    2 +-
 block/iscsi.c |    4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

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;
     }
 
diff --git a/block/iscsi.c b/block/iscsi.c
index 502a81f..965978b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -627,10 +627,6 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
     IscsiLun *iscsilun = bs->opaque;
     struct IscsiTask iTask;
 
-    if (bdrv_is_sg(bs)) {
-        return 0;
-    }
-
     if (!iscsilun->force_next_flush) {
         return 0;
     }
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Dimitris Aragiorgis]

Building the QEMU tools fails if we #define DEBUG_BLOCK inside
block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
a simple DPRINTF().

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 24b061f..fbccca8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -97,11 +97,11 @@
 //#define DEBUG_FLOPPY
 
 //#define DEBUG_BLOCK
-#if defined(DEBUG_BLOCK)
-#define DEBUG_BLOCK_PRINT(formatCstr, ...) do { if (qemu_log_enabled()) \
-    { qemu_log(formatCstr, ## __VA_ARGS__); qemu_log_flush(); } } while (0)
+
+#ifdef DEBUG_BLOCK
+#define DPRINTF(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while (0)
 #else
-#define DEBUG_BLOCK_PRINT(formatCstr, ...)
+#define DPRINTF(fmt, ...) do { } while (0)
 #endif
 
 /* OS X does not have O_DSYNC */
@@ -1023,7 +1023,7 @@ static int xfs_write_zeroes(BDRVRawState *s, int64_t offset, uint64_t bytes)
     fl.l_len = bytes;
 
     if (xfsctl(NULL, s->fd, XFS_IOC_ZERO_RANGE, &fl) < 0) {
-        DEBUG_BLOCK_PRINT("cannot write zero range (%s)\n", strerror(errno));
+        DPRINTF("cannot write zero range (%s)\n", strerror(errno));
         return -errno;
     }
 
@@ -1040,7 +1040,7 @@ static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
     fl.l_len = bytes;
 
     if (xfsctl(NULL, s->fd, XFS_IOC_UNRESVSP64, &fl) < 0) {
-        DEBUG_BLOCK_PRINT("cannot punch hole (%s)\n", strerror(errno));
+        DPRINTF("cannot punch hole (%s)\n", strerror(errno));
         return -errno;
     }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 4/5] raw-posix: Use DPRINTF for DEBUG_FLOPPY

[Dimitris Aragiorgis]

Get rid of several #ifdef DEBUG_FLOPPY and substitute them with
DPRINTF.

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index fbccca8..8a38d02 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -2157,16 +2157,12 @@ static int fd_open(BlockDriverState *bs)
         (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_open_time) >= FD_OPEN_TIMEOUT) {
         qemu_close(s->fd);
         s->fd = -1;
-#ifdef DEBUG_FLOPPY
-        printf("Floppy closed\n");
-#endif
+        DPRINTF("Floppy closed\n");
     }
     if (s->fd < 0) {
         if (s->fd_got_error &&
             (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_error_time) < FD_OPEN_TIMEOUT) {
-#ifdef DEBUG_FLOPPY
-            printf("No floppy (open delayed)\n");
-#endif
+            DPRINTF("No floppy (open delayed)\n");
             return -EIO;
         }
         s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
@@ -2175,14 +2171,10 @@ static int fd_open(BlockDriverState *bs)
             s->fd_got_error = 1;
             if (last_media_present)
                 s->fd_media_changed = 1;
-#ifdef DEBUG_FLOPPY
-            printf("No floppy\n");
-#endif
+            DPRINTF("No floppy\n");
             return -EIO;
         }
-#ifdef DEBUG_FLOPPY
-        printf("Floppy opened\n");
-#endif
+        DPRINTF("Floppy opened\n");
     }
     if (!last_media_present)
         s->fd_media_changed = 1;
@@ -2450,9 +2442,7 @@ static int floppy_media_changed(BlockDriverState *bs)
     fd_open(bs);
     ret = s->fd_media_changed;
     s->fd_media_changed = 0;
-#ifdef DEBUG_FLOPPY
-    printf("Floppy changed=%d\n", ret);
-#endif
+    DPRINTF("Floppy changed=%d\n", ret);
     return ret;
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 5/5] 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 8a38d02..06128db 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)) {
+        DPRINTF("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 v2 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1622 bytes --]

On 05/08/2015 11:47 AM, Dimitris Aragiorgis wrote:
> Building the QEMU tools fails if we #define DEBUG_BLOCK inside
> block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
> so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
> a simple DPRINTF().
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/raw-posix.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 24b061f..fbccca8 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -97,11 +97,11 @@
>  //#define DEBUG_FLOPPY
>  
>  //#define DEBUG_BLOCK
> -#if defined(DEBUG_BLOCK)
> -#define DEBUG_BLOCK_PRINT(formatCstr, ...) do { if (qemu_log_enabled()) \
> -    { qemu_log(formatCstr, ## __VA_ARGS__); qemu_log_flush(); } } while (0)
> +
> +#ifdef DEBUG_BLOCK
> +#define DPRINTF(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while (0)
>  #else
> -#define DEBUG_BLOCK_PRINT(formatCstr, ...)
> +#define DPRINTF(fmt, ...) do { } while (0)

Please fix this to ensure that we avoid bit-rot even when debugging is
not turned on.  Something like:

#ifdef DEBUG_BLOCK
# define DEBUG_BLOCK_PRINT 1
#else
# define DEBUG_BLOCK_PRINT 0
#endif
#define DPRINTF(fmt, ...) \
do \
    if (DEBUG_BLOCK_PRINT) { \
        printf(fmt, ## __VA_ARGS__); \
while (0)

that way, the compiler checks that the printf format arguments are valid
while still optimizing out the if(0) code.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 0/5] Some fixes related to scsi-generic

[Paolo Bonzini]

On 08/05/2015 19:47, Dimitris Aragiorgis wrote:
> 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
> 
> v2:
> * remove duplicate check for sg inside iscsi_co_flush()
> * remove DEBUG_BLOCK_PRINT in block/raw-posix.c
> * use DPRINTF for debugging in block/raw-posix.c
> 
> PS: Paolo suggested to use a tracepoint inside hdev_is_sg() but I chose DPRINTF
> instead. It would make sense to add a tracepoint for bdrv_is_sg() (just like
> most bdrv_* commands) but this is too much for now since it just returns the
> bs->sg flag (and is not an actual driver function). If you insist I'll change
> it in v3.
> 
> Dimitris Aragiorgis (5):
>   block: Use bdrv_is_sg() everywhere
>   Fix migration in case of scsi-generic
>   raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT
>   raw-posix: Use DPRINTF for DEBUG_FLOPPY
>   raw-posix: Introduce hdev_is_sg()
> 
>  block.c           |    6 ++---
>  block/io.c        |    2 +-
>  block/iscsi.c     |    4 ---
>  block/raw-posix.c |   75 +++++++++++++++++++++++++++++------------------------
>  4 files changed, 45 insertions(+), 42 deletions(-)
> 

I am okay with the debug printf, even though the problem with debug
printfs is that no one uses them and they bitrot.

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

Paolo

Re: [Qemu-devel] [PATCH v2 1/5] block: Use bdrv_is_sg() everywhere

[Kevin Wolf]

Am 08.05.2015 um 19:47 hat Dimitris Aragiorgis geschrieben:
> Instead of checking bs->sg use bdrv_is_sg() consistently throughout
> the code.
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c           |    6 +++---
>  block/iscsi.c     |    2 +-
>  block/raw-posix.c |    4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)

> 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 you want to change this comment, why not "SCSI generic" instead of
letting people guess what sg might mean in this context?

Kevin

Re: [Qemu-devel] [PATCH v2 2/5] Fix migration in case of scsi-generic

[Kevin Wolf]

Am 08.05.2015 um 19:47 hat Dimitris Aragiorgis geschrieben:
> 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.

fsync() doesn't only flush the kernel page cache, but also the disk
cache. The full justification includes at least that the scsi-generic
device never sends flushes, and for migration it's assumed that the same
SCSI device is used by the destination host (rather than another way to
access the same backing storage). If this were not enough, we would have
to send a SCSI flush command.

On another note, I expect we still neglect to check bs_is_sg() in too
many places. Any monitor command that does normal I/O on such a BDS
should actually fail.

> Remove the bdrv_is_sg() test from iscsi_co_flush() since this is
> now redundant (we flush the underlying protocol at the end
> of bdrv_co_flush() which, with this patch, we never reach).
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/io.c    |    2 +-
>  block/iscsi.c |    4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> 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)) {

This line exceeds 80 characters.

Kevin

Re: [Qemu-devel] [PATCH v2 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Kevin Wolf]

Am 08.05.2015 um 19:47 hat Dimitris Aragiorgis geschrieben:
> Building the QEMU tools fails if we #define DEBUG_BLOCK inside
> block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
> so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
> a simple DPRINTF().
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>

Why is this better than linking in qemu-log.o?

>  block/raw-posix.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 24b061f..fbccca8 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -97,11 +97,11 @@
>  //#define DEBUG_FLOPPY
>  
>  //#define DEBUG_BLOCK
> -#if defined(DEBUG_BLOCK)
> -#define DEBUG_BLOCK_PRINT(formatCstr, ...) do { if (qemu_log_enabled()) \
> -    { qemu_log(formatCstr, ## __VA_ARGS__); qemu_log_flush(); } } while (0)
> +
> +#ifdef DEBUG_BLOCK
> +#define DPRINTF(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while (0)

Please use stderr at least if you don't respect the log file setting.

>  #else
> -#define DEBUG_BLOCK_PRINT(formatCstr, ...)
> +#define DPRINTF(fmt, ...) do { } while (0)
>  #endif

Kevin

Re: [Qemu-devel] [PATCH v2 4/5] raw-posix: Use DPRINTF for DEBUG_FLOPPY

[Kevin Wolf]

Am 08.05.2015 um 19:47 hat Dimitris Aragiorgis geschrieben:
> Get rid of several #ifdef DEBUG_FLOPPY and substitute them with
> DPRINTF.
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>

Hm, this removes the option of selectively enabling debug messages. It's
probably not a big probem in this case, though.

However, please do remove the "//#define DEBUG_FLOPPY" line.

Kevin

Re: [Qemu-devel] [PATCH v2 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Paolo Bonzini]

On 11/05/2015 12:16, Kevin Wolf wrote:
> Am 08.05.2015 um 19:47 hat Dimitris Aragiorgis geschrieben:
>> > Building the QEMU tools fails if we #define DEBUG_BLOCK inside
>> > block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
>> > so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
>> > a simple DPRINTF().
>> > 
>> > Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> 
> Why is this better than linking in qemu-log.o?

Speaking as the one who suggested this, because no one else uses
qemu-log in the backends, and because qemu_log is tied to command-line
processing that the tools do not do (qemu_set_log, qemu_set_log_filename).

Paolo

Re: [Qemu-devel] [PATCH v2 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On 05/08/2015 02:10 PM, Eric Blake wrote:
> On 05/08/2015 11:47 AM, Dimitris Aragiorgis wrote:
>> Building the QEMU tools fails if we #define DEBUG_BLOCK inside
>> block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
>> so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
>> a simple DPRINTF().
>>

> Please fix this to ensure that we avoid bit-rot even when debugging is
> not turned on.  Something like:
> 
> #ifdef DEBUG_BLOCK
> # define DEBUG_BLOCK_PRINT 1
> #else
> # define DEBUG_BLOCK_PRINT 0
> #endif
> #define DPRINTF(fmt, ...) \
> do \
>     if (DEBUG_BLOCK_PRINT) { \
>         printf(fmt, ## __VA_ARGS__); \
> while (0)

Obviously, with the missing } that allows compilation :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/5] Fix migration in case of scsi-generic

[Dimitris Aragiorgis]

[-- Attachment #1: Type: text/plain, Size: 2801 bytes --]

Hello Kevin,

* Kevin Wolf <kwolf@redhat.com> [2015-05-11 12:12:23 +0200]:

> Am 08.05.2015 um 19:47 hat Dimitris Aragiorgis geschrieben:
> > 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.
> 
> fsync() doesn't only flush the kernel page cache, but also the disk
> cache. The full justification includes at least that the scsi-generic
> device never sends flushes, and for migration it's assumed that the same
> SCSI device is used by the destination host (rather than another way to
> access the same backing storage). If this were not enough, we would have
> to send a SCSI flush command.
> 

If I understand correctly you mean that QEMU will not issue any
SCSI SYNCHRONIZE CACHE (10) command and thus the disk cache does not get
flushed during migration. This requires, as you say, that the same SCSI
device is used by the destination host.

Note taken. I have added your comment in the commit message of the
corresponding patch in v3 just sent to the list. Thanks for clarifying
this.

Also note that QEMU seems to flush the disk cache explicitly in case of
iSCSI, using iscsi_synchronizecache10_task() from libiscsi inside
iscsi_co_flush().

Perhaps we can also extend this approach to scsi-generic e.g. by calling
sg_ll_sync_cache_10() from libsgutils2. This is a distinct, orthogonal
patch that could be added in the future.

> On another note, I expect we still neglect to check bs_is_sg() in too
> many places. Any monitor command that does normal I/O on such a BDS
> should actually fail.
> 
> > Remove the bdrv_is_sg() test from iscsi_co_flush() since this is
> > now redundant (we flush the underlying protocol at the end
> > of bdrv_co_flush() which, with this patch, we never reach).
> > 
> > Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> > ---
> >  block/io.c    |    2 +-
> >  block/iscsi.c |    4 ----
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> > 
> > 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)) {
> 
> This line exceeds 80 characters.
> 
> Kevin

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [Qemu-devel] [PATCH v2 2/5] Fix migration in case of scsi-generic

[Paolo Bonzini]

On 14/05/2015 13:02, Dimitris Aragiorgis wrote:
> Also note that QEMU seems to flush the disk cache explicitly in
> case of iSCSI, using iscsi_synchronizecache10_task() from libiscsi
> inside iscsi_co_flush().

Right, QEMU does that if type is DISK only.

> Perhaps we can also extend this approach to scsi-generic e.g. by
> calling sg_ll_sync_cache_10() from libsgutils2. This is a distinct,
> orthogonal patch that could be added in the future.

Yes.  But do not use libsgutils2, just do it manually in QEMU.  See
for example get_stream_blocksize in hw/scsi/scsi-generic.c.

Paolo