Patchwork ISCSI: Only set up the read-event if we are actually waiting for data to come back in from the target.

login
register
mail settings
Submitter ronniesahlberg@gmail.com
Date May 11, 2012, 10:22 a.m.
Message ID <1336731722-17743-2-git-send-email-ronniesahlberg@gmail.com>
Download mbox | patch
Permalink /patch/158474/
State New
Headers show

Comments

ronniesahlberg@gmail.com - May 11, 2012, 10:22 a.m.
Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Paolo Bonzini - May 11, 2012, 11:27 a.m.
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
ronniesahlberg@gmail.com - May 11, 2012, 12:39 p.m.
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

Patch

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);