| Message ID | 1246564917-19603-14-git-send-email-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jul 2, 2009 at 1:01 PM, Vivek Goyal<vgoyal@redhat.com> 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@redhat.com> > --- > Â 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 > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Jul 02, 2009 at 01:09:14PM -0700, Nauman Rafique wrote: > On Thu, Jul 2, 2009 at 1:01 PM, Vivek Goyal<vgoyal@redhat.com> 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. > Hi Nauman, Not yet. I will try to do some impact analysis of this change and post the results. Thanks Vivek > > > > 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@redhat.com> > > --- > > Â 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 > > > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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); }
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. 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@redhat.com> --- block/elevator-fq.c | 31 ++++++++++++++++++++++++++++++- 1 files changed, 30 insertions(+), 1 deletions(-)