| Message ID | 1336731722-17743-2-git-send-email-ronniesahlberg@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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);
Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> --- block/iscsi.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)