All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix discard requests accounting in the diskstats
@ 2009-04-17  6:14 Nikanth Karthikesan
  2009-04-17  6:49 ` Jens Axboe
  2009-04-23 19:50 ` Andrew Morton
  0 siblings, 2 replies; 6+ messages in thread
From: Nikanth Karthikesan @ 2009-04-17  6:14 UTC (permalink / raw)
  To: Jens Axboe; +Cc: David Woodhouse, linux-kernel

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 <knikanth@suse.de>

---

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] Fix discard requests accounting in the diskstats
  2009-04-17  6:14 [PATCH] Fix discard requests accounting in the diskstats Nikanth Karthikesan
@ 2009-04-17  6:49 ` Jens Axboe
  2009-04-17  7:55   ` Nikanth Karthikesan
  2009-04-23 19:50 ` Andrew Morton
  1 sibling, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2009-04-17  6:49 UTC (permalink / raw)
  To: Nikanth Karthikesan; +Cc: David Woodhouse, linux-kernel, jmarchan

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 <knikanth@suse.de>
> 
> ---
> 
> 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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Fix discard requests accounting in the diskstats
  2009-04-17  6:49 ` Jens Axboe
@ 2009-04-17  7:55   ` Nikanth Karthikesan
  2009-04-17  8:59     ` David Woodhouse
  0 siblings, 1 reply; 6+ messages in thread
From: Nikanth Karthikesan @ 2009-04-17  7:55 UTC (permalink / raw)
  To: Jens Axboe; +Cc: David Woodhouse, linux-kernel, jmarchan

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 <knikanth@suse.de>
> >
> > ---
> >
> > 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



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Fix discard requests accounting in the diskstats
  2009-04-17  7:55   ` Nikanth Karthikesan
@ 2009-04-17  8:59     ` David Woodhouse
  2009-04-17  9:08       ` Jens Axboe
  0 siblings, 1 reply; 6+ messages in thread
From: David Woodhouse @ 2009-04-17  8:59 UTC (permalink / raw)
  To: Nikanth Karthikesan; +Cc: Jens Axboe, linux-kernel, jmarchan

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
David.Woodhouse@intel.com                              Intel Corporation


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Fix discard requests accounting in the diskstats
  2009-04-17  8:59     ` David Woodhouse
@ 2009-04-17  9:08       ` Jens Axboe
  0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2009-04-17  9:08 UTC (permalink / raw)
  To: David Woodhouse; +Cc: Nikanth Karthikesan, linux-kernel, jmarchan

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Fix discard requests accounting in the diskstats
  2009-04-17  6:14 [PATCH] Fix discard requests accounting in the diskstats Nikanth Karthikesan
  2009-04-17  6:49 ` Jens Axboe
@ 2009-04-23 19:50 ` Andrew Morton
  1 sibling, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2009-04-23 19:50 UTC (permalink / raw)
  To: Nikanth Karthikesan; +Cc: jens.axboe, David.Woodhouse, linux-kernel

On Fri, 17 Apr 2009 11:44:49 +0530
Nikanth Karthikesan <knikanth@suse.de> 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.


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-04-23 19:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-17  6:14 [PATCH] Fix discard requests accounting in the diskstats Nikanth Karthikesan
2009-04-17  6:49 ` Jens Axboe
2009-04-17  7:55   ` Nikanth Karthikesan
2009-04-17  8:59     ` David Woodhouse
2009-04-17  9:08       ` Jens Axboe
2009-04-23 19:50 ` Andrew Morton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.