When 2 discard requests are merged, the stats gets updated, but we do not
update statistics normally when a discard request is issued or completed. For
example the in_flight counter would be decremented when 2 discard requests are
merged, but it was not at all incremented when they were issued, and in_flight
counter will not be decremented, when they are completed as well.
This patch fixes this by adding discard requests to the statistics.
Signed-off-by: Nikanth Karthikesan <[email protected]>
---
diff --git a/block/blk-core.c b/block/blk-core.c
index 07ab754..ee960a7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -68,7 +68,7 @@ static void drive_stat_acct(struct request *rq, int new_io)
int rw = rq_data_dir(rq);
int cpu;
- if (!blk_fs_request(rq) || !blk_do_io_stat(rq))
+ if (!rq_accountable(rq) || !blk_do_io_stat(rq))
return;
cpu = part_stat_lock();
@@ -1698,7 +1698,7 @@ static void blk_account_io_done(struct request *req)
* IO on queueing nor completion. Accounting the containing
* request is enough.
*/
- if (blk_fs_request(req) && req != &req->q->bar_rq) {
+ if (rq_accountable(req) && req != &req->q->bar_rq) {
unsigned long duration = jiffies - req->start_time;
const int rw = rq_data_dir(req);
struct hd_struct *part;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ba54c83..9483c02 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -667,7 +667,9 @@ static inline void blk_clear_queue_full(struct request_queue *q, int sync)
(REQ_NOMERGE | REQ_STARTED | REQ_HARDBARRIER | REQ_SOFTBARRIER)
#define rq_mergeable(rq) \
(!((rq)->cmd_flags & RQ_NOMERGE_FLAGS) && \
- (blk_discard_rq(rq) || blk_fs_request((rq))))
+ (blk_discard_rq(rq) || blk_fs_request(rq)))
+#define rq_accountable(rq) \
+ (blk_discard_rq(rq) || blk_fs_request(rq))
/*
* q->prep_rq_fn return values
On Fri, Apr 17 2009, Nikanth Karthikesan wrote:
> When 2 discard requests are merged, the stats gets updated, but we do not
> update statistics normally when a discard request is issued or completed. For
> example the in_flight counter would be decremented when 2 discard requests are
> merged, but it was not at all incremented when they were issued, and in_flight
> counter will not be decremented, when they are completed as well.
>
> This patch fixes this by adding discard requests to the statistics.
This needs rebasing on top of this one:
http://lkml.org/lkml/2009/4/16/159
whenever that gets resubmitted as a proper, working patch.
>
> Signed-off-by: Nikanth Karthikesan <[email protected]>
>
> ---
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 07ab754..ee960a7 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -68,7 +68,7 @@ static void drive_stat_acct(struct request *rq, int new_io)
> int rw = rq_data_dir(rq);
> int cpu;
>
> - if (!blk_fs_request(rq) || !blk_do_io_stat(rq))
> + if (!rq_accountable(rq) || !blk_do_io_stat(rq))
> return;
>
> cpu = part_stat_lock();
> @@ -1698,7 +1698,7 @@ static void blk_account_io_done(struct request *req)
> * IO on queueing nor completion. Accounting the containing
> * request is enough.
> */
> - if (blk_fs_request(req) && req != &req->q->bar_rq) {
> + if (rq_accountable(req) && req != &req->q->bar_rq) {
> unsigned long duration = jiffies - req->start_time;
> const int rw = rq_data_dir(req);
> struct hd_struct *part;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index ba54c83..9483c02 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -667,7 +667,9 @@ static inline void blk_clear_queue_full(struct request_queue *q, int sync)
> (REQ_NOMERGE | REQ_STARTED | REQ_HARDBARRIER | REQ_SOFTBARRIER)
> #define rq_mergeable(rq) \
> (!((rq)->cmd_flags & RQ_NOMERGE_FLAGS) && \
> - (blk_discard_rq(rq) || blk_fs_request((rq))))
> + (blk_discard_rq(rq) || blk_fs_request(rq)))
> +#define rq_accountable(rq) \
> + (blk_discard_rq(rq) || blk_fs_request(rq))
>
> /*
> * q->prep_rq_fn return values
>
--
Jens Axboe
On Friday 17 April 2009 12:19:12 Jens Axboe wrote:
> On Fri, Apr 17 2009, Nikanth Karthikesan wrote:
> > When 2 discard requests are merged, the stats gets updated, but we do not
> > update statistics normally when a discard request is issued or completed.
> > For example the in_flight counter would be decremented when 2 discard
> > requests are merged, but it was not at all incremented when they were
> > issued, and in_flight counter will not be decremented, when they are
> > completed as well.
> >
> > This patch fixes this by adding discard requests to the statistics.
>
> This needs rebasing on top of this one:
>
> http://lkml.org/lkml/2009/4/16/159
>
> whenever that gets resubmitted as a proper, working patch.
>
Ok. I will resend after Jerome resubmits that patch.
Thanks
Nikanth
> > Signed-off-by: Nikanth Karthikesan <[email protected]>
> >
> > ---
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 07ab754..ee960a7 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -68,7 +68,7 @@ static void drive_stat_acct(struct request *rq, int
> > new_io) int rw = rq_data_dir(rq);
> > int cpu;
> >
> > - if (!blk_fs_request(rq) || !blk_do_io_stat(rq))
> > + if (!rq_accountable(rq) || !blk_do_io_stat(rq))
> > return;
> >
> > cpu = part_stat_lock();
> > @@ -1698,7 +1698,7 @@ static void blk_account_io_done(struct request
> > *req) * IO on queueing nor completion. Accounting the containing
> > * request is enough.
> > */
> > - if (blk_fs_request(req) && req != &req->q->bar_rq) {
> > + if (rq_accountable(req) && req != &req->q->bar_rq) {
> > unsigned long duration = jiffies - req->start_time;
> > const int rw = rq_data_dir(req);
> > struct hd_struct *part;
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index ba54c83..9483c02 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -667,7 +667,9 @@ static inline void blk_clear_queue_full(struct
> > request_queue *q, int sync) (REQ_NOMERGE | REQ_STARTED | REQ_HARDBARRIER
> > | REQ_SOFTBARRIER) #define rq_mergeable(rq) \
> > (!((rq)->cmd_flags & RQ_NOMERGE_FLAGS) && \
> > - (blk_discard_rq(rq) || blk_fs_request((rq))))
> > + (blk_discard_rq(rq) || blk_fs_request(rq)))
> > +#define rq_accountable(rq) \
> > + (blk_discard_rq(rq) || blk_fs_request(rq))
> >
> > /*
> > * q->prep_rq_fn return values
On Fri, 2009-04-17 at 08:55 +0100, Nikanth Karthikesan wrote:
> On Friday 17 April 2009 12:19:12 Jens Axboe wrote:
> > On Fri, Apr 17 2009, Nikanth Karthikesan wrote:
> > > When 2 discard requests are merged, the stats gets updated, but we do not
> > > update statistics normally when a discard request is issued or completed.
> > > For example the in_flight counter would be decremented when 2 discard
> > > requests are merged, but it was not at all incremented when they were
> > > issued, and in_flight counter will not be decremented, when they are
> > > completed as well.
> > >
> > > This patch fixes this by adding discard requests to the statistics.
> >
> > This needs rebasing on top of this one:
> >
> > http://lkml.org/lkml/2009/4/16/159
> >
> > whenever that gets resubmitted as a proper, working patch.
> >
>
> Ok. I will resend after Jerome resubmits that patch.
There are more interesting issues with discard requests and elevators
for now, such as the fact that elevators only actually prevent write
requests from crossing each other on the queue if the {start,end} range
precisely matches, not merely if they happen to overlap.
So all discard requests are submitted as soft barriers for now anyway.
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
On Fri, Apr 17 2009, David Woodhouse wrote:
> On Fri, 2009-04-17 at 08:55 +0100, Nikanth Karthikesan wrote:
> > On Friday 17 April 2009 12:19:12 Jens Axboe wrote:
> > > On Fri, Apr 17 2009, Nikanth Karthikesan wrote:
> > > > When 2 discard requests are merged, the stats gets updated, but we do not
> > > > update statistics normally when a discard request is issued or completed.
> > > > For example the in_flight counter would be decremented when 2 discard
> > > > requests are merged, but it was not at all incremented when they were
> > > > issued, and in_flight counter will not be decremented, when they are
> > > > completed as well.
> > > >
> > > > This patch fixes this by adding discard requests to the statistics.
> > >
> > > This needs rebasing on top of this one:
> > >
> > > http://lkml.org/lkml/2009/4/16/159
> > >
> > > whenever that gets resubmitted as a proper, working patch.
> > >
> >
> > Ok. I will resend after Jerome resubmits that patch.
>
> There are more interesting issues with discard requests and elevators
> for now, such as the fact that elevators only actually prevent write
> requests from crossing each other on the queue if the {start,end} range
> precisely matches, not merely if they happen to overlap.
>
> So all discard requests are submitted as soft barriers for now anyway.
But they are still marked mergeable, so the accounting should be correct
regardless.
--
Jens Axboe
On Fri, 17 Apr 2009 11:44:49 +0530
Nikanth Karthikesan <[email protected]> wrote:
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -667,7 +667,9 @@ static inline void blk_clear_queue_full(struct request_queue *q, int sync)
> (REQ_NOMERGE | REQ_STARTED | REQ_HARDBARRIER | REQ_SOFTBARRIER)
> #define rq_mergeable(rq) \
> (!((rq)->cmd_flags & RQ_NOMERGE_FLAGS) && \
> - (blk_discard_rq(rq) || blk_fs_request((rq))))
> + (blk_discard_rq(rq) || blk_fs_request(rq)))
> +#define rq_accountable(rq) \
> + (blk_discard_rq(rq) || blk_fs_request(rq))
Both these macros reference their argument multiple times and are
potentially buggy and/or inefficient if passed an expression with
side-effects.
There was (afaict) no reason for them to have been implemented as
macros.