[PULL 0/7] Block/Multiboot patches for 2.10.0-rc3

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 0/7] Block/Multiboot patches for 2.10.0-rc3

Kevin Wolf-5
The following changes since commit 95766c2cd04395e5712b4d5967b3251f35d537df:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2017-08-10 18:53:39 +0100)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 8565c3ab537e78f3e69977ec2c609dc9417a806e:

  qemu-iotests: fix 185 (2017-08-11 14:44:39 +0200)

----------------------------------------------------------------
Block layer patches for 2.10.0-rc3

----------------------------------------------------------------
Eric Blake (4):
      tests/multiboot: Fix whitespace failure
      vpc: Check failure of bdrv_getlength()
      qcow2: Drop debugging dump_refcounts()
      qcow2: Check failure of bdrv_getlength()

Fam Zheng (2):
      osdep: Add runtime OFD lock detection
      file-posix: Do runtime check for ofd lock API

Vladimir Sementsov-Ogievskiy (1):
      qemu-iotests: fix 185

 include/qemu/osdep.h        |  1 +
 block/file-posix.c          | 19 ++++++-------
 block/qcow2.c               | 26 +++---------------
 block/vpc.c                 |  9 ++++++-
 util/osdep.c                | 66 ++++++++++++++++++++++++++++++++++++++-------
 tests/multiboot/run_test.sh |  2 +-
 tests/qemu-iotests/185      |  4 +++
 7 files changed, 82 insertions(+), 45 deletions(-)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 1/7] tests/multiboot: Fix whitespace failure

Kevin Wolf-5
From: Eric Blake <[hidden email]>

Commit b43671f8 accidentally broke run_test.sh within tests/multiboot;
due to a subtle change in whitespace.

These two commands produce theh same output (at least, for sane $IFS
of space-tab-newline):

echo -e "...$@..."
echo -e "...$*..."

But that's only because echo inserts spaces between multiple arguments
(the $@ case), while the $* form gives a single argument to echo with
the spaces already present.

But when converting to printf %b, there are no automatic spaces between
multiple arguments, so we HAVE to use $*.

It doesn't help that run_test.sh isn't part of 'make check'.

Signed-off-by: Eric Blake <[hidden email]>
Signed-off-by: Kevin Wolf <[hidden email]>
---
 tests/multiboot/run_test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index c8f3da8f37..0278148b43 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -26,7 +26,7 @@ run_qemu() {
     local kernel=$1
     shift
 
-    printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
+    printf %b "\n\n=== Running test case: $kernel $* ===\n\n" >> test.log
 
     $QEMU \
         -kernel $kernel \
--
2.13.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 2/7] vpc: Check failure of bdrv_getlength()

Kevin Wolf-5
In reply to this post by Kevin Wolf-5
From: Eric Blake <[hidden email]>

vpc_open() was checking for bdrv_getlength() failure in one, but
not the other, location.

Reported-by: Markus Armbruster <[hidden email]>
Signed-off-by: Eric Blake <[hidden email]>
Reviewed-by: Philippe Mathieu-Daudé <[hidden email]>
Reviewed-by: Jeff Cody <[hidden email]>
Reviewed-by: John Snow <[hidden email]>
Signed-off-by: Kevin Wolf <[hidden email]>
---
 block/vpc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 574879ba7c..82911ebead 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     uint64_t pagetable_size;
     int disk_type = VHD_DYNAMIC;
     int ret;
+    int64_t bs_size;
 
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
@@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             }
         }
 
-        if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
+        bs_size = bdrv_getlength(bs->file->bs);
+        if (bs_size < 0) {
+            error_setg_errno(errp, -bs_size, "Unable to learn image size");
+            ret = bs_size;
+            goto fail;
+        }
+        if (s->free_data_block_offset > bs_size) {
             error_setg(errp, "block-vpc: free_data_block_offset points after "
                              "the end of file. The image has been truncated.");
             ret = -EINVAL;
--
2.13.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 3/7] qcow2: Drop debugging dump_refcounts()

Kevin Wolf-5
In reply to this post by Kevin Wolf-5
From: Eric Blake <[hidden email]>

It's been #if 0'd since its introduction in 2006, commit 585f8587.
We can revive dead code if we need it, but in the meantime, it has
bit-rotted (for example, not checking for failure in bdrv_getlength()).

Signed-off-by: Eric Blake <[hidden email]>
Reviewed-by: Philippe Mathieu-Daudé <[hidden email]>
Reviewed-by: Jeff Cody <[hidden email]>
Reviewed-by: Kevin Wolf <[hidden email]>
Reviewed-by: John Snow <[hidden email]>
Signed-off-by: Kevin Wolf <[hidden email]>
---
 block/qcow2.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d7c600b5a2..99407403ea 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3798,27 +3798,6 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
     return spec_info;
 }
 
-#if 0
-static void dump_refcounts(BlockDriverState *bs)
-{
-    BDRVQcow2State *s = bs->opaque;
-    int64_t nb_clusters, k, k1, size;
-    int refcount;
-
-    size = bdrv_getlength(bs->file->bs);
-    nb_clusters = size_to_clusters(s, size);
-    for(k = 0; k < nb_clusters;) {
-        k1 = k;
-        refcount = get_refcount(bs, k);
-        k++;
-        while (k < nb_clusters && get_refcount(bs, k) == refcount)
-            k++;
-        printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount,
-               k - k1);
-    }
-}
-#endif
-
 static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
                               int64_t pos)
 {
--
2.13.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 4/7] qcow2: Check failure of bdrv_getlength()

Kevin Wolf-5
In reply to this post by Kevin Wolf-5
From: Eric Blake <[hidden email]>

qcow2_co_pwritev_compressed() should not call bdrv_truncate()
if determining the size failed.

Reported-by: Markus Armbruster <[hidden email]>
Signed-off-by: Eric Blake <[hidden email]>
Reviewed-by: Philippe Mathieu-Daudé <[hidden email]>
Reviewed-by: Jeff Cody <[hidden email]>
Reviewed-by: Kevin Wolf <[hidden email]>
Reviewed-by: John Snow <[hidden email]>
Signed-off-by: Kevin Wolf <[hidden email]>
---
 block/qcow2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 99407403ea..40ba26c111 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     z_stream strm;
     int ret, out_len;
     uint8_t *buf, *out_buf;
-    uint64_t cluster_offset;
+    int64_t cluster_offset;
 
     if (bytes == 0) {
         /* align end of file to a sector boundary to ease reading with
            sector based I/Os */
         cluster_offset = bdrv_getlength(bs->file->bs);
+        if (cluster_offset < 0) {
+            return cluster_offset;
+        }
         return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
     }
 
--
2.13.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 5/7] osdep: Add runtime OFD lock detection

Kevin Wolf-5
In reply to this post by Kevin Wolf-5
From: Fam Zheng <[hidden email]>

Build time check of OFD lock is not sufficient and can cause image open
errors when the runtime environment doesn't support it.

Add a helper function to probe it at runtime, additionally. Also provide
a qemu_has_ofd_lock() for callers to check the status.

Signed-off-by: Fam Zheng <[hidden email]>
Signed-off-by: Kevin Wolf <[hidden email]>
---
 include/qemu/osdep.h |  1 +
 util/osdep.c         | 66 ++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 3b74f6fcb2..6855b94bbf 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -357,6 +357,7 @@ int qemu_dup(int fd);
 int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
 int qemu_unlock_fd(int fd, int64_t start, int64_t len);
 int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
+bool qemu_has_ofd_lock(void);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index a2863c8e53..a479fedc4a 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -38,14 +38,6 @@ extern int madvise(caddr_t, size_t, int);
 #include "qemu/error-report.h"
 #include "monitor/monitor.h"
 
-#ifdef F_OFD_SETLK
-#define QEMU_SETLK F_OFD_SETLK
-#define QEMU_GETLK F_OFD_GETLK
-#else
-#define QEMU_SETLK F_SETLK
-#define QEMU_GETLK F_GETLK
-#endif
-
 static bool fips_enabled = false;
 
 static const char *hw_version = QEMU_HW_VERSION;
@@ -82,6 +74,10 @@ int qemu_madvise(void *addr, size_t len, int advice)
 }
 
 #ifndef _WIN32
+
+static int fcntl_op_setlk = -1;
+static int fcntl_op_getlk = -1;
+
 /*
  * Dups an fd and sets the flags
  */
@@ -149,6 +145,54 @@ static int qemu_parse_fdset(const char *param)
     return qemu_parse_fd(param);
 }
 
+static void qemu_probe_lock_ops(void)
+{
+    if (fcntl_op_setlk == -1) {
+#ifdef F_OFD_SETLK
+        int fd;
+        int ret;
+        struct flock fl = {
+            .l_whence = SEEK_SET,
+            .l_start  = 0,
+            .l_len    = 0,
+            .l_type   = F_WRLCK,
+        };
+
+        fd = open("/dev/null", O_RDWR);
+        if (fd < 0) {
+            fprintf(stderr,
+                    "Failed to open /dev/null for OFD lock probing: %s\n",
+                    strerror(errno));
+            fcntl_op_setlk = F_SETLK;
+            fcntl_op_getlk = F_GETLK;
+            return;
+        }
+        ret = fcntl(fd, F_OFD_GETLK, &fl);
+        close(fd);
+        if (!ret) {
+            fcntl_op_setlk = F_OFD_SETLK;
+            fcntl_op_getlk = F_OFD_GETLK;
+        } else {
+            fcntl_op_setlk = F_SETLK;
+            fcntl_op_getlk = F_GETLK;
+        }
+#else
+        fcntl_op_setlk = F_SETLK;
+        fcntl_op_getlk = F_GETLK;
+#endif
+    }
+}
+
+bool qemu_has_ofd_lock(void)
+{
+    qemu_probe_lock_ops();
+#ifdef F_OFD_SETLK
+    return fcntl_op_setlk == F_OFD_SETLK;
+#else
+    return false;
+#endif
+}
+
 static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
 {
     int ret;
@@ -158,7 +202,8 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
         .l_len    = len,
         .l_type   = fl_type,
     };
-    ret = fcntl(fd, QEMU_SETLK, &fl);
+    qemu_probe_lock_ops();
+    ret = fcntl(fd, fcntl_op_setlk, &fl);
     return ret == -1 ? -errno : 0;
 }
 
@@ -181,7 +226,8 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
         .l_len    = len,
         .l_type   = exclusive ? F_WRLCK : F_RDLCK,
     };
-    ret = fcntl(fd, QEMU_GETLK, &fl);
+    qemu_probe_lock_ops();
+    ret = fcntl(fd, fcntl_op_getlk, &fl);
     if (ret == -1) {
         return -errno;
     } else {
--
2.13.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 6/7] file-posix: Do runtime check for ofd lock API

Kevin Wolf-5
In reply to this post by Kevin Wolf-5
From: Fam Zheng <[hidden email]>

It is reported that on Windows Subsystem for Linux, ofd operations fail
with -EINVAL. In other words, QEMU binary built with system headers that
exports F_OFD_SETLK doesn't necessarily run in an environment that
actually supports it:

$ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 \
    -device virtio-blk-pci,drive=hd0
qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock byte 100

As a matter of fact this is not WSL specific. It can happen when running
a QEMU compiled against a newer glibc on an older kernel, such as in
a containerized environment.

Let's do a runtime check to cope with that.

Reported-by: Andrew Baumann <[hidden email]>
Reviewed-by: Eric Blake <[hidden email]>
Signed-off-by: Fam Zheng <[hidden email]>
Signed-off-by: Kevin Wolf <[hidden email]>
---
 block/file-posix.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index f4de022ae0..cb3bfce147 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -457,22 +457,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     switch (locking) {
     case ON_OFF_AUTO_ON:
         s->use_lock = true;
-#ifndef F_OFD_SETLK
-        fprintf(stderr,
-                "File lock requested but OFD locking syscall is unavailable, "
-                "falling back to POSIX file locks.\n"
-                "Due to the implementation, locks can be lost unexpectedly.\n");
-#endif
+        if (!qemu_has_ofd_lock()) {
+            fprintf(stderr,
+                    "File lock requested but OFD locking syscall is "
+                    "unavailable, falling back to POSIX file locks.\n"
+                    "Due to the implementation, locks can be lost "
+                    "unexpectedly.\n");
+        }
         break;
     case ON_OFF_AUTO_OFF:
         s->use_lock = false;
         break;
     case ON_OFF_AUTO_AUTO:
-#ifdef F_OFD_SETLK
-        s->use_lock = true;
-#else
-        s->use_lock = false;
-#endif
+        s->use_lock = qemu_has_ofd_lock();
         break;
     default:
         abort();
--
2.13.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PULL 7/7] qemu-iotests: fix 185

Kevin Wolf-5
In reply to this post by Kevin Wolf-5
From: Vladimir Sementsov-Ogievskiy <[hidden email]>

185 can sometimes produce wrong output like this:

    185 2s ... - output mismatch (see 185.out.bad)
    --- /work/src/qemu/master/tests/qemu-iotests/185.out    2017-07-14 \
        15:14:29.520343805 +0300
    +++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
    @@ -37,7 +37,7 @@
     {"return": {}}
     {"return": {}}
     {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
         "event": "SHUTDOWN", "data": {"guest": false}}
    -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
        "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
            "len": 4194304, "offset": 4194304, "speed": 65536, "type": \
                "mirror"}}
    +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, \
        "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
            "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}

     === Start backup job and exit qemu ===

    Failures: 185
    Failed 1 of 1 tests

This is because, under heavy load, the quit can happen before the first
iteration of the mirror request has occurred.  To make sure we've had
time to iterate, let's just add a sleep for 0.5 seconds before quitting.

Signed-off-by: Vladimir Sementsov-Ogievskiy <[hidden email]>
Reviewed-by: Eric Blake <[hidden email]>
Signed-off-by: Kevin Wolf <[hidden email]>
---
 tests/qemu-iotests/185 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
index 0eda371f27..f5b47e4c1a 100755
--- a/tests/qemu-iotests/185
+++ b/tests/qemu-iotests/185
@@ -156,6 +156,10 @@ _send_qemu_cmd $h \
                       'speed': 65536 } }" \
     "return"
 
+# If we don't sleep here 'quit' command may be handled before
+# the first mirror iteration is done
+sleep 0.5
+
 _send_qemu_cmd $h "{ 'execute': 'quit' }" "return"
 wait=1 _cleanup_qemu
 
--
2.13.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PULL 0/7] Block/Multiboot patches for 2.10.0-rc3

Peter Maydell-5
In reply to this post by Kevin Wolf-5
On 11 August 2017 at 15:05, Kevin Wolf <[hidden email]> wrote:

> The following changes since commit 95766c2cd04395e5712b4d5967b3251f35d537df:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2017-08-10 18:53:39 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 8565c3ab537e78f3e69977ec2c609dc9417a806e:
>
>   qemu-iotests: fix 185 (2017-08-11 14:44:39 +0200)
>
> ----------------------------------------------------------------
> Block layer patches for 2.10.0-rc3
>
> ----------------------------------------------------------------

I get an intermittent failure on aarch64 test-aio-multithread:

MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
gtester -k --verbose -m=quick tests/test-aio-multithread
TEST: tests/test-aio-multithread... (pid=19863)
  /aio/multi/lifecycle:                                                OK
  /aio/multi/schedule:                                                 OK
  /aio/multi/mutex/contended:                                          OK
  /aio/multi/mutex/handoff:                                            OK
  /aio/multi/mutex/mcs:                                                **
ERROR:/home/pm215/qemu/tests/test-aio-multithread.c:368:test_multi_fair_mutex:
assertion failed (counter == atomic_counter): (343406 == 343407)
FAIL
GTester: last random seed: R02S227b39277b8c54976c98f0e990305851
(pid=21145)
  /aio/multi/mutex/pthread:                                            OK
FAIL: tests/test-aio-multithread


but I've pushed this to master on the optimistic assumption that
it's not the fault of anything in this pullreq... (will
investigate further)

thanks
-- PMM

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PULL 0/7] Block/Multiboot patches for 2.10.0-rc3

Peter Maydell-5
On 11 August 2017 at 18:10, Peter Maydell <[hidden email]> wrote:

> I get an intermittent failure on aarch64 test-aio-multithread:
>
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> gtester -k --verbose -m=quick tests/test-aio-multithread
> TEST: tests/test-aio-multithread... (pid=19863)
>   /aio/multi/lifecycle:                                                OK
>   /aio/multi/schedule:                                                 OK
>   /aio/multi/mutex/contended:                                          OK
>   /aio/multi/mutex/handoff:                                            OK
>   /aio/multi/mutex/mcs:                                                **
> ERROR:/home/pm215/qemu/tests/test-aio-multithread.c:368:test_multi_fair_mutex:
> assertion failed (counter == atomic_counter): (343406 == 343407)
> FAIL
> GTester: last random seed: R02S227b39277b8c54976c98f0e990305851
> (pid=21145)
>   /aio/multi/mutex/pthread:                                            OK
> FAIL: tests/test-aio-multithread
>
>
> but I've pushed this to master on the optimistic assumption that
> it's not the fault of anything in this pullreq... (will
> investigate further)

I can repro this on 95766c2cd043 so it definitely isn't this patchset's
fault.

thanks
-- PMM

Loading...