Re: [PATCH 13/25] io-controller: Wait for requests to complete from last queue before new queue is scheduled

[Nauman Rafique]

On Thu, Jul 2, 2009 at 1:01 PM, Vivek Goyal<vgoyal@xxxxxxxxxx> wrote:
> o Currently one can dispatch requests from multiple queues to the disk. This
>  is true for hardware which supports queuing. So if a disk support queue
>  depth of 31 it is possible that 20 requests are dispatched from queue 1
>  and then next queue is scheduled in which dispatches more requests.
>
> o This multiple queue dispatch introduces issues for accurate accounting of
>  disk time consumed by a particular queue. For example, if one async queue
>  is scheduled in, it can dispatch 31 requests to the disk and then it will
>  be expired and a new sync queue might get scheduled in. These 31 requests
>  might take a long time to finish but this time is never accounted to the
>  async queue which dispatched these requests.
>
> o This patch introduces the functionality where we wait for all the requests
>  to finish from previous queue before next queue is scheduled in. That way
>  a queue is more accurately accounted for disk time it has consumed. Note
>  this still does not take care of errors introduced by disk write caching.
>
> o Because above behavior can result in reduced throughput, this behavior will
>  be enabled only if user sets "fairness" tunable to 2 or higher.

Vivek,
Did you collect any numbers for the impact on throughput from this
patch? It seems like with this change, we can even support NCQ.

>
> o This patch helps in achieving more isolation between reads and buffered
>  writes in different cgroups. buffered writes typically utilize full queue
>  depth and then expire the queue. On the contarary, sequential reads
>  typicaly driver queue depth of 1. So despite the fact that writes are
>  using more disk time it is never accounted to write queue because we don't
>  wait for requests to finish after dispatching these. This patch helps
>  do more accurate accounting of disk time, especially for buffered writes
>  hence providing better fairness hence better isolation between two cgroups
>  running read and write workloads.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
>  block/elevator-fq.c |   31 ++++++++++++++++++++++++++++++-
>  1 files changed, 30 insertions(+), 1 deletions(-)
>
> diff --git a/block/elevator-fq.c b/block/elevator-fq.c
> index 68be1dc..7609579 100644
> --- a/block/elevator-fq.c
> +++ b/block/elevator-fq.c
> @@ -2038,7 +2038,7 @@ STORE_FUNCTION(elv_slice_sync_store, &efqd->elv_slice[1], 1, UINT_MAX, 1);
>  EXPORT_SYMBOL(elv_slice_sync_store);
>  STORE_FUNCTION(elv_slice_async_store, &efqd->elv_slice[0], 1, UINT_MAX, 1);
>  EXPORT_SYMBOL(elv_slice_async_store);
> -STORE_FUNCTION(elv_fairness_store, &efqd->fairness, 0, 1, 0);
> +STORE_FUNCTION(elv_fairness_store, &efqd->fairness, 0, 2, 0);
>  EXPORT_SYMBOL(elv_fairness_store);
>  #undef STORE_FUNCTION
>
> @@ -2952,6 +2952,24 @@ void *elv_fq_select_ioq(struct request_queue *q, int force)
>        }
>
>  expire:
> +       if (efqd->fairness >= 2 && !force && ioq && ioq->dispatched) {
> +               /*
> +                * If there are request dispatched from this queue, don't
> +                * dispatch requests from new queue till all the requests from
> +                * this queue have completed.
> +                *
> +                * This helps in attributing right amount of disk time consumed
> +                * by a particular queue when hardware allows queuing.
> +                *
> +                * Set ioq = NULL so that no more requests are dispatched from
> +                * this queue.
> +                */
> +               elv_log_ioq(efqd, ioq, "select: wait for requests to finish"
> +                               " disp=%lu", ioq->dispatched);
> +               ioq = NULL;
> +               goto keep_queue;
> +       }
> +
>        elv_ioq_slice_expired(q);
>  new_queue:
>        ioq = elv_set_active_ioq(q, new_ioq);
> @@ -3109,6 +3127,17 @@ void elv_ioq_completed_request(struct request_queue *q, struct request *rq)
>                                 */
>                                elv_ioq_arm_slice_timer(q, 1);
>                        } else {
> +                               /* If fairness >=2 and there are requests
> +                                * dispatched from this queue, don't dispatch
> +                                * new requests from a different queue till
> +                                * all requests from this queue have finished.
> +                                * This helps in attributing right disk time
> +                                * to a queue when hardware supports queuing.
> +                                */
> +
> +                               if (efqd->fairness >= 2 && ioq->dispatched)
> +                                       goto done;
> +
>                                /* Expire the queue */
>                                elv_ioq_slice_expired(q);
>                        }
> --
> 1.6.0.6
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/