blktrace: add missing probe argument to block_bio_complete

PATCH In review Mathieu Desnoyers 3 participants 5 messages Jan 7, 2011

Subsystems (2)

Block layer Tracing

lore

Share & tools

lore

Mathieu Desnoyers

author reviewer

2011-01-07 01:54 UTC #0 lore raw

PATCH 1/1 blktrace: add missing probe argument to block_bio_complete

blktrace.c block bio complete callback needs to gain a new argument to reflect the newly added "error" tracepoint argument. This is needed to match the new block_bio_complete TRACE_EVENT as of
commit de983a7bfcb7c020901ca6e2314cf55a4207ab5a.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Jeff Moyer <[email protected]>

CC Steven Rostedt <[email protected]>

CC Frederic Weisbecker <[email protected]>

CC Ingo Molnar <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Jens Axboe <[email protected]>

CC Li Zefan <[email protected]>

---
 kernel/trace/blktrace.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

kernel/trace/blktrace.c +2 -1

Index: linux-2.6-lttng/kernel/trace/blktrace.c
===================================================================
@@ -785,7 +785,8 @@ static void blk_add_trace_bio_bounce(voi
 }
 
 static void blk_add_trace_bio_complete(void *ignore,
-				       struct request_queue *q, struct bio *bio)
+				       struct request_queue *q, struct bio *bio,
+				       int error)
 {
 	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE);
 }

Signature · 4 lines

Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Jeff Moyer

2011-01-07 14:14 UTC #1 lore raw

Quoted 22 lines · Mathieu Desnoyers

blktrace.c block bio complete callback needs to gain a new argument to reflect the newly added "error" tracepoint argument. This is needed to match the new block_bio_complete TRACE_EVENT as of
commit de983a7bfcb7c020901ca6e2314cf55a4207ab5a.
Signed-off-by Mathieu Desnoyers <[email protected]>
CC Jeff Moyer <[email protected]>
CC Steven Rostedt <[email protected]>
CC Frederic Weisbecker <[email protected]>
CC Ingo Molnar <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Jens Axboe <[email protected]>
CC Li Zefan <[email protected]>
---
 kernel/trace/blktrace.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6-lttng/kernel/trace/blktrace.c
===================================================================

kernel/trace/blktrace.c static void blk_add_trace_bio_bounce(voi · line 792

Quoted 9 lines

kernel/trace/blktrace.c +2 -1

@@ -785,7 +785,8 @@ static void blk_add_trace_bio_bounce(voi
 }
 
 static void blk_add_trace_bio_complete(void *ignore,
-				       struct request_queue *q, struct bio *bio)
+				       struct request_queue *q, struct bio *bio,
+				       int error)
 {
 	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE);
 }

OK, I clearly didn't look closely enough last time. There's no sense passing this information down if it isn't used (as you said initially). blk_add_trace_bio sets the error based on whether or not the BIO_UPTODATE bit is set. So, I think we should instead revert the patch I sent you (Mathieu), and then completely get rid of the error field in the TP macros.

Does that make sense to everyone else?

Cheers,
Jeff

Jens Axboe

maintainer

2011-01-07 15:15 UTC #2 lore raw

Quoted 41 lines · Jeff Moyer

Quoted 22 lines · Mathieu Desnoyers
blktrace.c block bio complete callback needs to gain a new argument to reflect the newly added "error" tracepoint argument. This is needed to match the new block_bio_complete TRACE_EVENT as of
commit de983a7bfcb7c020901ca6e2314cf55a4207ab5a.
Signed-off-by Mathieu Desnoyers <[email protected]>
CC Jeff Moyer <[email protected]>
CC Steven Rostedt <[email protected]>
CC Frederic Weisbecker <[email protected]>
CC Ingo Molnar <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Jens Axboe <[email protected]>
CC Li Zefan <[email protected]>
---
 kernel/trace/blktrace.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6-lttng/kernel/trace/blktrace.c
===================================================================
kernel/trace/blktrace.c static void blk_add_trace_bio_bounce(voi · line 792
Quoted 9 lines
kernel/trace/blktrace.c +2 -1
@@ -785,7 +785,8 @@ static void blk_add_trace_bio_bounce(voi
 }
 
 static void blk_add_trace_bio_complete(void *ignore,
-				       struct request_queue *q, struct bio *bio)
+				       struct request_queue *q, struct bio *bio,
+				       int error)
 {
 	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE);
 }
OK, I clearly didn't look closely enough last time. There's no sense passing this information down if it isn't used (as you said initially). blk_add_trace_bio sets the error based on whether or not the BIO_UPTODATE bit is set. So, I think we should instead revert the patch I sent you (Mathieu), and then completely get rid of the error field in the TP macros.

Quoted 1 line

Does that make sense to everyone else?

We usually use BIO_UPTODATE if we have no other information available. So if we have, then we should use it.

kernel/trace/blktrace.c +13 -9

index fab31af..153562d 100644
@@ -758,54 +758,58 @@ static void blk_add_trace_rq_complete(void *ignore,
  * @q:		queue the io is for
  * @bio:	the source bio
  * @what:	the action
+ * @error:	error, if any
  *
  * Description:
  *     Records an action against a bio. Will log the bio offset + size.
  *
  **/
 static void blk_add_trace_bio(struct request_queue *q, struct bio *bio,
-				     u32 what)
+			      u32 what, int error)
 {
 	struct blk_trace *bt = q->blk_trace;
 
 	if (likely(!bt))
 		return;
 
+	if (!error && !bio_flagged(bio, BIO_UPTODATE))
+		error = EIO;
+
 	__blk_add_trace(bt, bio->bi_sector, bio->bi_size, bio->bi_rw, what,
-			!bio_flagged(bio, BIO_UPTODATE), 0, NULL);
+			error, 0, NULL);
 }
 
 static void blk_add_trace_bio_bounce(void *ignore,
 				     struct request_queue *q, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_BOUNCE);
+	blk_add_trace_bio(q, bio, BLK_TA_BOUNCE, 0);
 }
 
 static void blk_add_trace_bio_complete(void *ignore,
 				       struct request_queue *q, struct bio *bio,
 				       int error)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE);
+	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE, error);
 }
 
 static void blk_add_trace_bio_backmerge(void *ignore,
 					struct request_queue *q,
 					struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_BACKMERGE);
+	blk_add_trace_bio(q, bio, BLK_TA_BACKMERGE, 0);
 }
 
 static void blk_add_trace_bio_frontmerge(void *ignore,
 					 struct request_queue *q,
 					 struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_FRONTMERGE);
+	blk_add_trace_bio(q, bio, BLK_TA_FRONTMERGE, 0);
 }
 
 static void blk_add_trace_bio_queue(void *ignore,
 				    struct request_queue *q, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_QUEUE);
+	blk_add_trace_bio(q, bio, BLK_TA_QUEUE, 0);
 }
 
 static void blk_add_trace_getrq(void *ignore,

@@ -813,7 +817,7 @@ static void blk_add_trace_getrq(void *ignore,
 				struct bio *bio, int rw)
 {
 	if (bio)
-		blk_add_trace_bio(q, bio, BLK_TA_GETRQ);
+		blk_add_trace_bio(q, bio, BLK_TA_GETRQ, 0);
 	else {
 		struct blk_trace *bt = q->blk_trace;

@@ -828,7 +832,7 @@ static void blk_add_trace_sleeprq(void *ignore,
 				  struct bio *bio, int rw)
 {
 	if (bio)
-		blk_add_trace_bio(q, bio, BLK_TA_SLEEPRQ);
+		blk_add_trace_bio(q, bio, BLK_TA_SLEEPRQ, 0);
 	else {
 		struct blk_trace *bt = q->blk_trace;

Signature · 1 line

Jens Axboe

Jeff Moyer

Reviewed-by

2011-01-07 15:28 UTC #3 lore raw

Quoted 42 lines · Jens Axboe

Quoted 41 lines · Jeff Moyer
Quoted 22 lines · Mathieu Desnoyers
blktrace.c block bio complete callback needs to gain a new argument to reflect the newly added "error" tracepoint argument. This is needed to match the new block_bio_complete TRACE_EVENT as of
commit de983a7bfcb7c020901ca6e2314cf55a4207ab5a.
Signed-off-by Mathieu Desnoyers <[email protected]>
CC Jeff Moyer <[email protected]>
CC Steven Rostedt <[email protected]>
CC Frederic Weisbecker <[email protected]>
CC Ingo Molnar <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Jens Axboe <[email protected]>
CC Li Zefan <[email protected]>
---
 kernel/trace/blktrace.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6-lttng/kernel/trace/blktrace.c
===================================================================
kernel/trace/blktrace.c static void blk_add_trace_bio_bounce(voi · line 792
Quoted 9 lines
kernel/trace/blktrace.c +2 -1
@@ -785,7 +785,8 @@ static void blk_add_trace_bio_bounce(voi
 }
 
 static void blk_add_trace_bio_complete(void *ignore,
-				       struct request_queue *q, struct bio *bio)
+				       struct request_queue *q, struct bio *bio,
+				       int error)
 {
 	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE);
 }
OK, I clearly didn't look closely enough last time. There's no sense passing this information down if it isn't used (as you said initially). blk_add_trace_bio sets the error based on whether or not the BIO_UPTODATE bit is set. So, I think we should instead revert the patch I sent you (Mathieu), and then completely get rid of the error field in the TP macros.

Quoted 4 lines

Quoted 1 line
Does that make sense to everyone else?
We usually use BIO_UPTODATE if we have no other information available. So if we have, then we should use it.

OK, I had contemplated doing it that way, too, but I wasn't sure how often the error differed from EIO. Anyway, this looks better.

Reviewed-by Jeff Moyer <[email protected]>

Quoted 88 lines

kernel/trace/blktrace.c +13 -9

index fab31af..153562d 100644
@@ -758,54 +758,58 @@ static void blk_add_trace_rq_complete(void *ignore,
  * @q:		queue the io is for
  * @bio:	the source bio
  * @what:	the action
+ * @error:	error, if any
  *
  * Description:
  *     Records an action against a bio. Will log the bio offset + size.
  *
  **/
 static void blk_add_trace_bio(struct request_queue *q, struct bio *bio,
-				     u32 what)
+			      u32 what, int error)
 {
 	struct blk_trace *bt = q->blk_trace;
 
 	if (likely(!bt))
 		return;
 
+	if (!error && !bio_flagged(bio, BIO_UPTODATE))
+		error = EIO;
+
 	__blk_add_trace(bt, bio->bi_sector, bio->bi_size, bio->bi_rw, what,
-			!bio_flagged(bio, BIO_UPTODATE), 0, NULL);
+			error, 0, NULL);
 }
 
 static void blk_add_trace_bio_bounce(void *ignore,
 				     struct request_queue *q, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_BOUNCE);
+	blk_add_trace_bio(q, bio, BLK_TA_BOUNCE, 0);
 }
 
 static void blk_add_trace_bio_complete(void *ignore,
 				       struct request_queue *q, struct bio *bio,
 				       int error)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE);
+	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE, error);
 }
 
 static void blk_add_trace_bio_backmerge(void *ignore,
 					struct request_queue *q,
 					struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_BACKMERGE);
+	blk_add_trace_bio(q, bio, BLK_TA_BACKMERGE, 0);
 }
 
 static void blk_add_trace_bio_frontmerge(void *ignore,
 					 struct request_queue *q,
 					 struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_FRONTMERGE);
+	blk_add_trace_bio(q, bio, BLK_TA_FRONTMERGE, 0);
 }
 
 static void blk_add_trace_bio_queue(void *ignore,
 				    struct request_queue *q, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_QUEUE);
+	blk_add_trace_bio(q, bio, BLK_TA_QUEUE, 0);
 }
 
 static void blk_add_trace_getrq(void *ignore,

@@ -813,7 +817,7 @@ static void blk_add_trace_getrq(void *ignore,
 				struct bio *bio, int rw)
 {
 	if (bio)
-		blk_add_trace_bio(q, bio, BLK_TA_GETRQ);
+		blk_add_trace_bio(q, bio, BLK_TA_GETRQ, 0);
 	else {
 		struct blk_trace *bt = q->blk_trace;

@@ -828,7 +832,7 @@ static void blk_add_trace_sleeprq(void *ignore,
 				  struct bio *bio, int rw)
 {
 	if (bio)
-		blk_add_trace_bio(q, bio, BLK_TA_SLEEPRQ);
+		blk_add_trace_bio(q, bio, BLK_TA_SLEEPRQ, 0);
 	else {
 		struct blk_trace *bt = q->blk_trace;

Mathieu Desnoyers

author reviewer Acked-by

2011-01-07 17:41 UTC #4 lore raw

In reply to Jens Axboe

Quoted 42 lines · * Jens Axboe ([email protected]) wrote:

Quoted 41 lines · Jeff Moyer
Quoted 22 lines · Mathieu Desnoyers
blktrace.c block bio complete callback needs to gain a new argument to reflect the newly added "error" tracepoint argument. This is needed to match the new block_bio_complete TRACE_EVENT as of
commit de983a7bfcb7c020901ca6e2314cf55a4207ab5a.
Signed-off-by Mathieu Desnoyers <[email protected]>
CC Jeff Moyer <[email protected]>
CC Steven Rostedt <[email protected]>
CC Frederic Weisbecker <[email protected]>
CC Ingo Molnar <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Jens Axboe <[email protected]>
CC Li Zefan <[email protected]>
---
 kernel/trace/blktrace.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6-lttng/kernel/trace/blktrace.c
===================================================================
kernel/trace/blktrace.c static void blk_add_trace_bio_bounce(voi · line 792
Quoted 9 lines
kernel/trace/blktrace.c +2 -1
@@ -785,7 +785,8 @@ static void blk_add_trace_bio_bounce(voi
 }
 
 static void blk_add_trace_bio_complete(void *ignore,
-				       struct request_queue *q, struct bio *bio)
+				       struct request_queue *q, struct bio *bio,
+				       int error)
 {
 	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE);
 }
OK, I clearly didn't look closely enough last time. There's no sense passing this information down if it isn't used (as you said initially). blk_add_trace_bio sets the error based on whether or not the BIO_UPTODATE bit is set. So, I think we should instead revert the patch I sent you (Mathieu), and then completely get rid of the error field in the TP macros.

Quoted 4 lines

Quoted 1 line
Does that make sense to everyone else?
We usually use BIO_UPTODATE if we have no other information available. So if we have, then we should use it.

Good point. I tested your patch below applied after "trace event block fix unassigned field", and it builds fine. Please feel free to add my

Acked-by Mathieu Desnoyers <[email protected]>

Are you planning to apply it on top of "blktrace: add missing probe argument to block_bio_complete" ? (which I see you have merged in block for-next)

Thanks,

Mathieu

Signature · 4 lines

Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com