[Qemu-devel] [PATCH 0/1] ISCSI: Only set read-event if we have i/o in flight

[Qemu-devel] [PATCH 0/1] ISCSI: Only set read-event if we have i/o in flight

[Ronnie Sahlberg]

List, Kevin

Please find a small patch to the iscsi driver to only set the fd handler for read events IF we have i/o in flight and we are thus waiting for replies coming back from the target.

There are situations where qemu_aio_set_fd_handler(fd, read_callback, ...
will invoke the read_callback even if there is no fd/socket error and there are 
also no bytes to read from the descriptor.
This interacts bad with libiscsi which when invoked for POLLIN assumes that there is data to read and if ioctl(...FIONREAD...) returns 0 bytes that this indicates a socket failure/error/closure.


I dont know if that is expected behaviour from qemu_aio_set_fd_handler or not,
but aside from target-initiated NOPs, we shouldnt expect there to be any reason to read from the socket anyway unless we have i/o in flight and are waiting for replies, so this would also avoid eating some cpu invoking the read_callback when falsely.


regards
ronnie sahlberg

[Qemu-devel] [PATCH] ISCSI: Only set up the read-event if we are actually waiting for data to come back in from the target.

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index d37c4ee..989b5e9 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -105,7 +105,9 @@ iscsi_set_events(IscsiLun *iscsilun)
 {
     struct iscsi_context *iscsi = iscsilun->iscsi;
 
-    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), iscsi_process_read,
+    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi),
+                           (iscsi_queue_length(iscsi) > 0)
+                           ? iscsi_process_read : NULL,
                            (iscsi_which_events(iscsi) & POLLOUT)
                            ? iscsi_process_write : NULL,
                            iscsi_process_flush, iscsilun);
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH] ISCSI: Only set up the read-event if we are actually waiting for data to come back in from the target.

[Paolo Bonzini]

Il 11/05/2012 12:22, Ronnie Sahlberg ha scritto:
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  block/iscsi.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index d37c4ee..989b5e9 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -105,7 +105,9 @@ iscsi_set_events(IscsiLun *iscsilun)
>  {
>      struct iscsi_context *iscsi = iscsilun->iscsi;
>  
> -    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), iscsi_process_read,
> +    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi),
> +                           (iscsi_queue_length(iscsi) > 0)
> +                           ? iscsi_process_read : NULL,
>                             (iscsi_which_events(iscsi) & POLLOUT)
>                             ? iscsi_process_write : NULL,
>                             iscsi_process_flush, iscsilun);

I wonder if iscsi is also susceptible to the same race condition I saw
with NBD, where you can have:

1) select in the iothread exiting and reporting readability

2) the iothread subsequently blocking on the mutex

3) a VCPU thread's qemu_aio_wait() calling iscsi_process_read

4) when the VCPU releases the mutex, the iothread will call
iscsi_process_read again.

This should be easily reproducible with IDE drives, but the above patch
would not fix it.  Perhaps it's better to call iscsi_queue_length in
iscsi_process_read instead.

Paolo

Re: [Qemu-devel] [PATCH] ISCSI: Only set up the read-event if we are actually waiting for data to come back in from the target.

[ronnie sahlberg]

On Fri, May 11, 2012 at 9:27 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/05/2012 12:22, Ronnie Sahlberg ha scritto:
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> ---
>>  block/iscsi.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index d37c4ee..989b5e9 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -105,7 +105,9 @@ iscsi_set_events(IscsiLun *iscsilun)
>>  {
>>      struct iscsi_context *iscsi = iscsilun->iscsi;
>>
>> -    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), iscsi_process_read,
>> +    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi),
>> +                           (iscsi_queue_length(iscsi) > 0)
>> +                           ? iscsi_process_read : NULL,
>>                             (iscsi_which_events(iscsi) & POLLOUT)
>>                             ? iscsi_process_write : NULL,
>>                             iscsi_process_flush, iscsilun);
>
> I wonder if iscsi is also susceptible to the same race condition I saw
> with NBD, where you can have:
>
> 1) select in the iothread exiting and reporting readability
>
> 2) the iothread subsequently blocking on the mutex
>
> 3) a VCPU thread's qemu_aio_wait() calling iscsi_process_read
>
> 4) when the VCPU releases the mutex, the iothread will call
> iscsi_process_read again.
>
> This should be easily reproducible with IDE drives, but the above patch
> would not fix it.  Perhaps it's better to call iscsi_queue_length in
> iscsi_process_read instead.
>

So there is a known condition where the event callbacks can be
triggered like this.
Thanks for confirming!

Let me do a new patch along your suggestion, test it a bit and I will
re-send to the list.


regards
ronnie sahlberg