Re: [Qemu-devel] [PATCH] block/iscsi: handle BUSY condition

[Paolo Bonzini]

Il 29/05/2014 19:31, Peter Lieven ha scritto:
> +static const unsigned iscsi_retry_times[] = {4, 16, 64, 256, 1024, 4096};

This is a pretty fast growth.

> +static void iscsi_retry_timer_expired(void *opaque)
> +{
> +    struct IscsiTask *iTask = opaque;
> +    timer_del(iTask->retry_timer);

This timer_del is not necessary.  Also, since you are introducing a new 
usage I would prefer if you used aio_timer_init instead.

> +    timer_free(iTask->retry_timer);
> +    if (iTask->co) {
> +        qemu_coroutine_enter(iTask->co, NULL);
> +    }
> +}
> +
> +#define RANDRANGE(M, N) (M + rand() / (RAND_MAX / (N - M + 1) + 1))

A minor nit: you should probably use "((double)rand() / RAND_MAX) and 
then cast it back to integer at the end.  You should also make it a 
static inline function for readability, or surround the arguments with 
parentheses.

If you care, it probably would be better to have the random numbers be 
exponentially distributed, even more so since the ranges form a 
geometric progression.  So for example the first retry would be on 
average 8 ms rather than 10 ms.  The difference for the last retry is 
half a second!

You can do that by taking the (natural) logarithm of N and M, applying 
randrange on the logarithms, and then applying exp on the result.

Otherwise looks good, but how did you test the patch?  I'd rather have 
the growth in iscsi_retry_times backed by actual data, but I understand 
that's hard to gather.

Paolo

>  static void
>  iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
>                          void *command_data, void *opaque)
> @@ -156,20 +172,40 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int 
> status,
>      iTask->do_retry = 0;
>      iTask->task = task;
>
> -    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
> -        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
> -        error_report("iSCSI CheckCondition: %s", iscsi_get_error(iscsi));
> -        iTask->do_retry = 1;
> -        goto out;
> -    }
> -
>      if (status != SCSI_STATUS_GOOD) {
> +        if (iTask->retries++ < ISCSI_CMD_RETRIES) {
> +            if (status == SCSI_STATUS_CHECK_CONDITION
> +                && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
> +                error_report("iSCSI CheckCondition: %s",
> +                             iscsi_get_error(iscsi));
> +                iTask->do_retry = 1;
> +                goto out;
> +            }
> +            if (status == SCSI_STATUS_BUSY) {
> +                unsigned retry_time =
> +                    RANDRANGE(iscsi_retry_times[iTask->retries - 1],
> +                              iscsi_retry_times[iTask->retries]);
> +                error_report("iSCSI Busy (retry #%u in %u ms): %s",
> +                             iTask->retries, retry_time,
> +                             iscsi_get_error(iscsi));
> +                iTask->retry_timer = 
> aio_timer_new(iTask->iscsilun->aio_context,
> +                                                   QEMU_CLOCK_REALTIME,
> +                                                   SCALE_MS,
> +                                                   iscsi_retry_timer_expired,
> +                                                   iTask);
> +                timer_mod(iTask->retry_timer,
> +                          qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + retry_time);