[Qemu-devel] [PULL 0/2] Block patches

[Qemu-devel] [PULL 0/2] Block patches

[Stefan Hajnoczi]

The following changes since commit 4871b51b9241b10f4fd8e04bbb21577886795e25:

  vmgenid-test: use boot-sector infrastructure (2017-07-14 17:03:03 +0100)

are available in the git repository at:

  git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 593ed6f0a3c827a13a274e47f6fa980344234f9c:

  block: fix shadowed variable in bdrv_co_pdiscard (2017-07-17 15:58:37 +0100)

----------------------------------------------------------------

----------------------------------------------------------------

Alistair Francis (1):
  util/aio-win32: Only select on what we are actually waiting for

Denis V. Lunev (1):
  block: fix shadowed variable in bdrv_co_pdiscard

 block/io.c       |  1 -
 util/aio-win32.c | 13 ++++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.9.4

[Qemu-devel] [PULL 1/2] util/aio-win32: Only select on what we are actually waiting for

[Stefan Hajnoczi]

From: Alistair Francis <alistair.francis@xilinx.com>

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 9307b70e9876c4e9e3c4478524a32a23a3d5dd05.1499368180.git.alistair.francis@xilinx.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/aio-win32.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/util/aio-win32.c b/util/aio-win32.c
index bca496a..d6d5e02 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -71,6 +71,7 @@ void aio_set_fd_handler(AioContext *ctx,
         }
     } else {
         HANDLE event;
+        long bitmask = 0;
 
         if (node == NULL) {
             /* Alloc and insert if it's not already there */
@@ -95,10 +96,16 @@ void aio_set_fd_handler(AioContext *ctx,
         node->io_write = io_write;
         node->is_external = is_external;
 
+        if (io_read) {
+            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
+        }
+
+        if (io_write) {
+            bitmask |= FD_WRITE | FD_CONNECT;
+        }
+
         event = event_notifier_get_handle(&ctx->notifier);
-        WSAEventSelect(node->pfd.fd, event,
-                       FD_READ | FD_ACCEPT | FD_CLOSE |
-                       FD_CONNECT | FD_WRITE | FD_OOB);
+        WSAEventSelect(node->pfd.fd, event, bitmask);
     }
 
     qemu_lockcnt_unlock(&ctx->list_lock);
-- 
2.9.4

Re: [Qemu-devel] [PULL 1/2] util/aio-win32: Only select on what we are actually waiting for

[Paolo Bonzini]

> +        if (io_read) {
> +            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
> +        }
> +
> +        if (io_write) {
> +            bitmask |= FD_WRITE | FD_CONNECT;
> +        }
> +
>          event = event_notifier_get_handle(&ctx->notifier);
> -        WSAEventSelect(node->pfd.fd, event,
> -                       FD_READ | FD_ACCEPT | FD_CLOSE |
> -                       FD_CONNECT | FD_WRITE | FD_OOB);
> +        WSAEventSelect(node->pfd.fd, event, bitmask);
>      }
>

As noticed by Eric, if the same socket is in use via both aio-win32 and
another GSource, or via multiple AioContexts, this would break because
there is only one WSAEventSelect mask for each socket handle.

It is probably not going to break anything in the case of aio-win32, but I
think it is worth at least a comment. And because WSAEventSelect is
edge-triggered (e.g. an FD_WRITE event doesn't trigger again until the next
write) it shouldn't be that bad for performance.  If there's time to revert
this patch, I think it would be preferable.

Paolo

Re: [Qemu-devel] [PULL 1/2] util/aio-win32: Only select on what we are actually waiting for

[Alistair Francis]

On Mon, Jul 17, 2017 at 7:23 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> +        if (io_read) {
>> +            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
>> +        }
>> +
>> +        if (io_write) {
>> +            bitmask |= FD_WRITE | FD_CONNECT;
>> +        }
>> +
>>          event = event_notifier_get_handle(&ctx->notifier);
>> -        WSAEventSelect(node->pfd.fd, event,
>> -                       FD_READ | FD_ACCEPT | FD_CLOSE |
>> -                       FD_CONNECT | FD_WRITE | FD_OOB);
>> +        WSAEventSelect(node->pfd.fd, event, bitmask);
>>      }
>>
>
> As noticed by Eric, if the same socket is in use via both aio-win32 and
> another GSource, or via multiple AioContexts, this would break because
> there is only one WSAEventSelect mask for each socket handle.
>
> It is probably not going to break anything in the case of aio-win32, but I
> think it is worth at least a comment. And because WSAEventSelect is
> edge-triggered (e.g. an FD_WRITE event doesn't trigger again until the next
> write) it shouldn't be that bad for performance.  If there's time to revert
> this patch, I think it would be preferable.

It looks like this was applied, I'll be back in the office next week
should I try and revert this or just add a comment explaining the
limitation.

Either that or store the current mask and OR then all together.

Thanks,
Alistair

>
> Paolo

[Qemu-devel] [PULL 2/2] block: fix shadowed variable in bdrv_co_pdiscard

[Stefan Hajnoczi]

From: "Denis V. Lunev" <den@openvz.org>

We've had a shadowed 'ret' variable, which risks returning the wrong
value, introduced in commit b9c64947.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20170710150559.30163-1-den@openvz.org
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index b413727..a73153d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2335,7 +2335,6 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
     assert(max_pdiscard >= bs->bl.request_alignment);
 
     while (bytes > 0) {
-        int ret;
         int num = bytes;
 
         if (head) {
-- 
2.9.4

Re: [Qemu-devel] [PULL 0/2] Block patches

[Peter Maydell]

On 17 July 2017 at 16:40, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The following changes since commit 4871b51b9241b10f4fd8e04bbb21577886795e25:
>
>   vmgenid-test: use boot-sector infrastructure (2017-07-14 17:03:03 +0100)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 593ed6f0a3c827a13a274e47f6fa980344234f9c:
>
>   block: fix shadowed variable in bdrv_co_pdiscard (2017-07-17 15:58:37 +0100)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> Alistair Francis (1):
>   util/aio-win32: Only select on what we are actually waiting for
>
> Denis V. Lunev (1):
>   block: fix shadowed variable in bdrv_co_pdiscard
>
>  block/io.c       |  1 -
>  util/aio-win32.c | 13 ++++++++++---
>  2 files changed, 10 insertions(+), 4 deletions(-)

Applied, thanks.

-- PMM