Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: handle zero events from iscsi_which_events

Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: handle zero events from iscsi_which_events

[Stefan Hajnoczi]

On Tue, Apr 7, 2015 at 9:08 PM, Peter Lieven <pl@kamp.de> wrote:

Please CC qemu-devel@nongnu.org in the future.  All patches must be on
the qemu-devel mailing list so they can be merged (for transparency).
I have added qemu-devel to CC.

> +    /* newer versions of libiscsi may return zero events. In this
> +     * case start a timer to ensure we are able to return to service
> +     * once this situation changes. */
> +    if (!ev) {
> +        timer_mod(iscsilun->event_timer,
> +                  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
>      }

This suggests that libiscsi is waiting on its own internal timer.  It
would be cleaner for libiscsi to provide a timeout value so the
application doesn't need to poll the library.  Is there still scope to
modify libiscsi to do this?

Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: handle zero events from iscsi_which_events

[ronnie sahlberg]

On Wed, Apr 8, 2015 at 1:38 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Apr 7, 2015 at 9:08 PM, Peter Lieven <pl@kamp.de> wrote:
>
> Please CC qemu-devel@nongnu.org in the future.  All patches must be on
> the qemu-devel mailing list so they can be merged (for transparency).
> I have added qemu-devel to CC.
>
>> +    /* newer versions of libiscsi may return zero events. In this
>> +     * case start a timer to ensure we are able to return to service
>> +     * once this situation changes. */
>> +    if (!ev) {
>> +        timer_mod(iscsilun->event_timer,
>> +                  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
>>      }
>
> This suggests that libiscsi is waiting on its own internal timer.  It
> would be cleaner for libiscsi to provide a timeout value so the
> application doesn't need to poll the library.  Is there still scope to
> modify libiscsi to do this?

I think that returning a timeout value would be a bigger change in the
API that Peter wanted to avoid.
We discussed that as an option by my take from it was that this would
require that qemu and libiscsi
again would have to be updated in lockstep,

In this particular case with creating a delay between failed reconnect
attempts, i.e. the target is restarting and
returning a RST to the SYN request until it has fully restarted, the
correct amount of time to wait until re-trying is at best a guess
anyway.
I don't have any particular feelings on whether the decision on how
long to wait is done inside libiscsi and communicated to qemu,
or if it is done in qemu itself.

The nice part with the current patch of Peter is that qemu and
libiscsi can be upgraded/downgraded independently.

Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: handle zero events from iscsi_which_events

[Stefan Hajnoczi]

On Wed, Apr 8, 2015 at 6:08 PM, ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
> The nice part with the current patch of Peter is that qemu and
> libiscsi can be upgraded/downgraded independently.

That's fine for avoiding hassles for existing apps, like QEMU, and I'm
happy to merge the patch.

For the library's API design it would be return the timeout so that
new applications can avoid polling, but that's just a suggestion.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: handle zero events from iscsi_which_events

[Peter Lieven]

Am 08.04.2015 um 21:22 schrieb Stefan Hajnoczi:
> On Wed, Apr 8, 2015 at 6:08 PM, ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
>> The nice part with the current patch of Peter is that qemu and
>> libiscsi can be upgraded/downgraded independently.
> That's fine for avoiding hassles for existing apps, like QEMU, and I'm
> happy to merge the patch.
>
> For the library's API design it would be return the timeout so that
> new applications can avoid polling, but that's just a suggestion.

You are right. Paolo also brought up this idea, but as Ronnie said the
idea was not to bump the API version. The whole async reconnect stuff
works without a change in the client.

But polling every 250ms is magnitutes better than having a hung Qemu :-)
If you feel 250ms is too often we could also go to 1000ms. The timeout
we are talking about is in the order of 1-30 seconds. It only happens when
a reconnect is initiated either bei Qemu (due to NOP timeout) or libiscsi (protocol,
socket error etc.) and the first reconnect try is not successful.

Another idea I had was to encode the timeout in the return value of iscsi_which_events.
This would work with all Qemu versions, but I do not know how poll reacts if it gets
unknown events passed. So all users other than Qemu that pass the result of iscsi_which_events
directly to poll would need to mask the result. Thats not a problem for the tools provided with libiscsi,
but I do not know how much other real users except Qemu are out there?

Peter

Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: handle zero events from iscsi_which_events

[Stefan Hajnoczi]

On Tue, Apr 7, 2015 at 9:08 PM, Peter Lieven <pl@kamp.de> wrote:

CCing the entirety of this patch to qemu-devel@nongnu.org.  It is also
available in the qemu-block@nongnu.org mailing list archives:
http://permalink.gmane.org/gmane.comp.emulators.qemu.block/460

> newer libiscsi versions may return zero events from iscsi_which_events.
>
> In this case iscsi_service will return immediately without any progress.
> To avoid busy waiting for iscsi_which_events to change we deregister all
> read and write handlers in this case and schedule a timer to periodically
> check iscsi_which_events for changed events.
>
> Next libiscsi version will introduce async reconnects and zero events
> are returned while libiscsi is waiting for a reconnect retry.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 3e34b1f..ba33290 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -56,6 +56,7 @@ typedef struct IscsiLun {
>      uint64_t num_blocks;
>      int events;
>      QEMUTimer *nop_timer;
> +    QEMUTimer *event_timer;
>      uint8_t lbpme;
>      uint8_t lbprz;
>      uint8_t has_write_same;
> @@ -95,6 +96,7 @@ typedef struct IscsiAIOCB {
>  #endif
>  } IscsiAIOCB;
>
> +#define EVENT_INTERVAL 250
>  #define NOP_INTERVAL 5000
>  #define MAX_NOP_FAILURES 3
>  #define ISCSI_CMD_RETRIES ARRAY_SIZE(iscsi_retry_times)
> @@ -256,21 +258,30 @@ static void
>  iscsi_set_events(IscsiLun *iscsilun)
>  {
>      struct iscsi_context *iscsi = iscsilun->iscsi;
> -    int ev;
> +    int ev = iscsi_which_events(iscsi);
>
> -    /* We always register a read handler.  */
> -    ev = POLLIN;
> -    ev |= iscsi_which_events(iscsi);
>      if (ev != iscsilun->events) {
>          aio_set_fd_handler(iscsilun->aio_context,
>                             iscsi_get_fd(iscsi),
> -                           iscsi_process_read,
> +                           (ev & POLLIN) ? iscsi_process_read : NULL,
>                             (ev & POLLOUT) ? iscsi_process_write : NULL,
>                             iscsilun);
> +        iscsilun->events = ev;
> +    }
>
> +    /* newer versions of libiscsi may return zero events. In this
> +     * case start a timer to ensure we are able to return to service
> +     * once this situation changes. */
> +    if (!ev) {
> +        timer_mod(iscsilun->event_timer,
> +                  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
>      }
> +}
>
> -    iscsilun->events = ev;
> +static void iscsi_timed_set_events(void *opaque)
> +{
> +    IscsiLun *iscsilun = opaque;
> +    iscsi_set_events(iscsilun);
>  }
>
>  static void
> @@ -1214,6 +1225,11 @@ static void iscsi_detach_aio_context(BlockDriverState *bs)
>          timer_free(iscsilun->nop_timer);
>          iscsilun->nop_timer = NULL;
>      }
> +    if (iscsilun->event_timer) {
> +        timer_del(iscsilun->event_timer);
> +        timer_free(iscsilun->event_timer);
> +        iscsilun->event_timer = NULL;
> +    }
>  }
>
>  static void iscsi_attach_aio_context(BlockDriverState *bs,
> @@ -1230,6 +1246,11 @@ static void iscsi_attach_aio_context(BlockDriverState *bs,
>                                          iscsi_nop_timed_event, iscsilun);
>      timer_mod(iscsilun->nop_timer,
>                qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> +
> +    /* Prepare a timer for a delayed call to iscsi_set_events */
> +    iscsilun->event_timer = aio_timer_new(iscsilun->aio_context,
> +                                          QEMU_CLOCK_REALTIME, SCALE_MS,
> +                                          iscsi_timed_set_events, iscsilun);
>  }
>
>  static bool iscsi_is_write_protected(IscsiLun *iscsilun)
> --
> 1.7.9.5
>
>