Re: [PATCH] blkback: Fix block I/O latency issue

["Jan Beulich" <JBeulich@novell.com>]

>>> On 02.05.11 at 09:04, "Vincent, Pradeep" <pradeepv@amazon.com> wrote:
> In blkback driver, after I/O requests are submitted to Dom-0 block I/O 
> subsystem, blkback goes to 'sleep' effectively without letting blkfront know 
> about it (req_event isn't set appropriately). Hence blkfront doesn't notify 
> blkback when it submits a new I/O thus delaying the 'dispatch' of the new I/O 
> to Dom-0 block I/O subsystem. The new I/O is dispatched as soon as one of the 
> previous I/Os completes.
> 
> As a result of this issue, the block I/O latency performance is degraded for 
> some workloads on Xen guests using blkfront-blkback stack.
> 
> The following change addresses this issue:
> 
> 
> Signed-off-by: Pradeep Vincent <pradeepv@amazon.com>
> 
> diff --git a/drivers/xen/blkback/blkback.c b/drivers/xen/blkback/blkback.c
> --- a/drivers/xen/blkback/blkback.c
> +++ b/drivers/xen/blkback/blkback.c
> @@ -383,6 +383,12 @@ static int do_block_io_op(blkif_t *blkif)
>   cond_resched();
>   }
> 
> + /* If blkback might go to sleep (i.e. more_to_do == 0) then we better
> +   let blkfront know about it (by setting req_event appropriately) so that
> +   blkfront will bother to wake us up (via interrupt) when it submits a
> +   new I/O */
> +        if (!more_to_do)
> +                 RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do);

To me this contradicts the comment preceding the use of
RING_FINAL_CHECK_FOR_REQUESTS() in make_response()
(there it's supposedly used to avoid unnecessary notification,
here you say it's used to force notification). Albeit I agree that
the change looks consistent with the comments in io/ring.h.

Even if correct, you're not holding blkif->blk_ring_lock here, and
hence I think you'll need to explain how this is not a problem.

>From a formal perspective, you also want to correct usage of tabs,
and (assuming this is intended for the 2.6.18 tree) you'd also need
to indicate so for Keir to pick this up and apply it to that tree (and
it might then also be a good idea to submit an equivalent patch for
the pv-ops trees).

Jan

>   return more_to_do;
>  }