[PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap

[PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap

[KOSAKI Motohiro]

Subject: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap

Impact: cleanup for improve readability

Currently, blk_add_trace_remap has following prototype.

static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
				       dev_t dev, sector_t from, sector_t to)

but caller pass "from" secter as 4th arg, "to" sector as 5th arg.

example,
--------------------------------------------------------
static inline void blk_partition_remap(struct bio *bio)
{
	struct block_device *bdev = bio->bi_bdev;

	if (bio_sectors(bio) && bdev != bdev->bd_contains) {
		struct hd_struct *p = bdev->bd_part;

		bio->bi_sector += p->start_sect;
		bio->bi_bdev = bdev->bd_contains;

		trace_block_remap(bdev_get_queue(bio->bi_bdev), bio,
				    bdev->bd_dev, bio->bi_sector,
				    bio->bi_sector - p->start_sect);
	}
}
--------------------------------------------------------

Oh my god, it's reverse order.
Fortunately, print logic reverse again. the twice reversing hide problem.

but, but...
It repeatedly confuse reviewer (include me!).
Then, swapping argment name is better.

this patch doesn't change any behavior and ABI.

Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 include/linux/blktrace_api.h |    2 +-
 kernel/trace/blktrace.c      |   17 ++++++++---------
 2 files changed, 9 insertions(+), 10 deletions(-)

Index: b/include/linux/blktrace_api.h
===================================================================
--- a/include/linux/blktrace_api.h	2009-04-30 13:17:56.000000000 +0900
+++ b/include/linux/blktrace_api.h	2009-04-30 13:18:12.000000000 +0900
@@ -118,7 +118,7 @@ struct blk_io_trace {
 struct blk_io_trace_remap {
 	__be32 device;
 	__be32 device_from;
-	__be64 sector;
+	__be64 sector_from;
 };
 
 enum {
Index: b/kernel/trace/blktrace.c
===================================================================
--- a/kernel/trace/blktrace.c	2009-04-30 13:18:09.000000000 +0900
+++ b/kernel/trace/blktrace.c	2009-04-30 13:18:12.000000000 +0900
@@ -839,7 +839,7 @@ static void blk_add_trace_split(struct r
  *
  **/
 static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
-				       dev_t dev, sector_t from, sector_t to)
+				       dev_t dev, sector_t to, sector_t from)
 {
 	struct blk_trace *bt = q->blk_trace;
 	struct blk_io_trace_remap r;
@@ -847,11 +847,11 @@ static void blk_add_trace_remap(struct r
 	if (likely(!bt))
 		return;
 
-	r.device = cpu_to_be32(dev);
+	r.device      = cpu_to_be32(dev);
 	r.device_from = cpu_to_be32(bio->bi_bdev->bd_dev);
-	r.sector = cpu_to_be64(to);
+	r.sector_from = cpu_to_be64(from);
 
-	__blk_add_trace(bt, from, bio->bi_size, bio->bi_rw, BLK_TA_REMAP,
+	__blk_add_trace(bt, to, bio->bi_size, bio->bi_rw, BLK_TA_REMAP,
 			!bio_flagged(bio, BIO_UPTODATE), sizeof(r), &r);
 }
 
@@ -1028,11 +1028,10 @@ static void get_pdu_remap(const struct t
 			  struct blk_io_trace_remap *r)
 {
 	const struct blk_io_trace_remap *__r = pdu_start(ent);
-	__u64 sector = __r->sector;
 
-	r->device = be32_to_cpu(__r->device);
-	r->device_from = be32_to_cpu(__r->device_from);
-	r->sector = be64_to_cpu(sector);
+	r->device	= be32_to_cpu(__r->device);
+	r->device_from	= be32_to_cpu(__r->device_from);
+	r->sector_from	= be64_to_cpu(__r->sector_from);
 }
 
 typedef int (blk_log_action_t) (struct trace_iterator *iter, const char *act);
@@ -1154,7 +1153,7 @@ static int blk_log_remap(struct trace_se
 	return trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
 			       t_sector(ent),
 			       t_sec(ent), MAJOR(r.device), MINOR(r.device),
-			       (unsigned long long)r.sector);
+			       (unsigned long long)r.sector_from);
 }
 
 static int blk_log_plug(struct trace_seq *s, const struct trace_entry *ent)

Re: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap

[Ingo Molnar]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> 
> Subject: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap
> 
> Impact: cleanup for improve readability
> 
> Currently, blk_add_trace_remap has following prototype.
> 
> static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
> 				       dev_t dev, sector_t from, sector_t to)
> 
> but caller pass "from" secter as 4th arg, "to" sector as 5th arg.
> 
> example,
> --------------------------------------------------------
> static inline void blk_partition_remap(struct bio *bio)
> {
> 	struct block_device *bdev = bio->bi_bdev;
> 
> 	if (bio_sectors(bio) && bdev != bdev->bd_contains) {
> 		struct hd_struct *p = bdev->bd_part;
> 
> 		bio->bi_sector += p->start_sect;
> 		bio->bi_bdev = bdev->bd_contains;
> 
> 		trace_block_remap(bdev_get_queue(bio->bi_bdev), bio,
> 				    bdev->bd_dev, bio->bi_sector,
> 				    bio->bi_sector - p->start_sect);
> 	}
> }
> --------------------------------------------------------
> 
> Oh my god, it's reverse order.
> Fortunately, print logic reverse again. the twice reversing hide problem.

heh ... well spotted.

> but, but...
> It repeatedly confuse reviewer (include me!).
> Then, swapping argment name is better.

Agreed. Li, Jens, what do you think?

	Ingo

Re: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap

[Alan D. Brunelle]

KOSAKI Motohiro wrote:
> Subject: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap
> 
> Impact: cleanup for improve readability
> 
> Currently, blk_add_trace_remap has following prototype.
> 
> static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
> 				       dev_t dev, sector_t from, sector_t to)
> 
> but caller pass "from" secter as 4th arg, "to" sector as 5th arg.
> 
> example,
> --------------------------------------------------------
> static inline void blk_partition_remap(struct bio *bio)
> {
> 	struct block_device *bdev = bio->bi_bdev;
> 
> 	if (bio_sectors(bio) && bdev != bdev->bd_contains) {
> 		struct hd_struct *p = bdev->bd_part;
> 
> 		bio->bi_sector += p->start_sect;
> 		bio->bi_bdev = bdev->bd_contains;
> 
> 		trace_block_remap(bdev_get_queue(bio->bi_bdev), bio,
> 				    bdev->bd_dev, bio->bi_sector,
> 				    bio->bi_sector - p->start_sect);
> 	}
> }
> --------------------------------------------------------
> 
> Oh my god, it's reverse order.
> Fortunately, print logic reverse again. the twice reversing hide problem.
> 
> but, but...
> It repeatedly confuse reviewer (include me!).
> Then, swapping argment name is better.
> 
> this patch doesn't change any behavior and ABI.
> 
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> ---
>  include/linux/blktrace_api.h |    2 +-
>  kernel/trace/blktrace.c      |   17 ++++++++---------
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> Index: b/include/linux/blktrace_api.h
> ===================================================================
> --- a/include/linux/blktrace_api.h	2009-04-30 13:17:56.000000000 +0900
> +++ b/include/linux/blktrace_api.h	2009-04-30 13:18:12.000000000 +0900
> @@ -118,7 +118,7 @@ struct blk_io_trace {
>  struct blk_io_trace_remap {
>  	__be32 device;
>  	__be32 device_from;
> -	__be64 sector;
> +	__be64 sector_from;
>  };
>  
>  enum {
> Index: b/kernel/trace/blktrace.c
> ===================================================================
> --- a/kernel/trace/blktrace.c	2009-04-30 13:18:09.000000000 +0900
> +++ b/kernel/trace/blktrace.c	2009-04-30 13:18:12.000000000 +0900
> @@ -839,7 +839,7 @@ static void blk_add_trace_split(struct r
>   *
>   **/
>  static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
> -				       dev_t dev, sector_t from, sector_t to)
> +				       dev_t dev, sector_t to, sector_t from)
>  {
>  	struct blk_trace *bt = q->blk_trace;
>  	struct blk_io_trace_remap r;
> @@ -847,11 +847,11 @@ static void blk_add_trace_remap(struct r
>  	if (likely(!bt))
>  		return;
>  
> -	r.device = cpu_to_be32(dev);
> +	r.device      = cpu_to_be32(dev);
>  	r.device_from = cpu_to_be32(bio->bi_bdev->bd_dev);
> -	r.sector = cpu_to_be64(to);
> +	r.sector_from = cpu_to_be64(from);
>  
> -	__blk_add_trace(bt, from, bio->bi_size, bio->bi_rw, BLK_TA_REMAP,
> +	__blk_add_trace(bt, to, bio->bi_size, bio->bi_rw, BLK_TA_REMAP,
>  			!bio_flagged(bio, BIO_UPTODATE), sizeof(r), &r);
>  }
>  
> @@ -1028,11 +1028,10 @@ static void get_pdu_remap(const struct t
>  			  struct blk_io_trace_remap *r)
>  {
>  	const struct blk_io_trace_remap *__r = pdu_start(ent);
> -	__u64 sector = __r->sector;
>  
> -	r->device = be32_to_cpu(__r->device);
> -	r->device_from = be32_to_cpu(__r->device_from);
> -	r->sector = be64_to_cpu(sector);
> +	r->device	= be32_to_cpu(__r->device);
> +	r->device_from	= be32_to_cpu(__r->device_from);
> +	r->sector_from	= be64_to_cpu(__r->sector_from);
>  }
>  
>  typedef int (blk_log_action_t) (struct trace_iterator *iter, const char *act);
> @@ -1154,7 +1153,7 @@ static int blk_log_remap(struct trace_se
>  	return trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
>  			       t_sector(ent),
>  			       t_sec(ent), MAJOR(r.device), MINOR(r.device),
> -			       (unsigned long long)r.sector);
> +			       (unsigned long long)r.sector_from);
>  }
>  
>  static int blk_log_plug(struct trace_seq *s, const struct trace_entry *ent)
> 
> 

I believe this may be more messed up than you believe: Consider the last
function: blk_log_remap. Notice how you are printing the /device/ and
/sector_from/ as one pair. Isn't that confusing too: wouldn't one expect
/device_from/ and /sector_from/ to be paired up?

I ran into this problem a long time ago - hence the current code in
blkparse contains:

        if (act[0] == 'A') {    /* Remap */
                get_pdu_remap(t, &r);
                t->device = r.device_from;
        }

Which puts the device_from out at the beginning of the line, and then
the 'device' + 'sector' (== 'sector_from') out at the end of the line.
(Which, of course, is all confusing.)

To really fix this, I think one needs to:

(a) adjust the kernel to make "r.device_from" and "r.sector_from" refer
to the same part of the trace, and the "r.device" and "t->sector" refer
to the other part of the trace.

(b) fix blkparse (& btt & ...) to do the same thing - not make
adjustments for the stupid naming...

If we leave the kernel->user space ABI alone:

first u32: device mapped /from/
second u32: device mapped /to/
u64: sector mapped from

We can then change the names. I've followed this e-mail up with a pair
of kernel-side patches and will submit user side fixes for blkptrace later.

Alan D. Brunelle
Hewlett-Packard

Subject: [PATCH] from-sector redundant in trace_block_remap

[Alan D. Brunelle]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0001-from-sector-redundant-in-trace_block_remap.patch --]
[-- Type: text/x-diff, Size: 3158 bytes --]

>From f167379b6c2cbad5f4fe8492052693825980ec19 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <alan.brunelle@hp.com>
Date: Thu, 30 Apr 2009 11:22:55 -0400
Subject: [PATCH] from-sector redundant in trace_block_remap

Removed redundant from-sector parameter: it's /always/ the bio's sector
passed in.

Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
---
 block/blk-core.c        |    5 ++---
 drivers/md/dm.c         |    3 +--
 include/trace/block.h   |    4 ++--
 kernel/trace/blktrace.c |    7 ++++---
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2998fe3..d028baf 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1277,7 +1277,7 @@ static inline void blk_partition_remap(struct bio *bio)
 		bio->bi_bdev = bdev->bd_contains;
 
 		trace_block_remap(bdev_get_queue(bio->bi_bdev), bio,
-				    bdev->bd_dev, bio->bi_sector,
+				    bdev->bd_dev,
 				    bio->bi_sector - p->start_sect);
 	}
 }
@@ -1446,8 +1446,7 @@ static inline void __generic_make_request(struct bio *bio)
 			goto end_io;
 
 		if (old_sector != -1)
-			trace_block_remap(q, bio, old_dev, bio->bi_sector,
-					    old_sector);
+			trace_block_remap(q, bio, old_dev, old_sector);
 
 		trace_block_bio_queue(q, bio);
 
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 424f7b0..e2ee4a7 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -656,8 +656,7 @@ static void __map_bio(struct dm_target *ti, struct bio *clone,
 		/* the bio has been remapped so dispatch it */
 
 		trace_block_remap(bdev_get_queue(clone->bi_bdev), clone,
-				    tio->io->bio->bi_bdev->bd_dev,
-				    clone->bi_sector, sector);
+				    tio->io->bio->bi_bdev->bd_dev, sector);
 
 		generic_make_request(clone);
 	} else if (r < 0 || r == DM_MAPIO_REQUEUE) {
diff --git a/include/trace/block.h b/include/trace/block.h
index 25b7068..8ac945b 100644
--- a/include/trace/block.h
+++ b/include/trace/block.h
@@ -70,7 +70,7 @@ DECLARE_TRACE(block_split,
 
 DECLARE_TRACE(block_remap,
 	TP_PROTO(struct request_queue *q, struct bio *bio, dev_t dev,
-		 sector_t from, sector_t to),
-	      TP_ARGS(q, bio, dev, from, to));
+		 sector_t to),
+	      TP_ARGS(q, bio, dev, to));
 
 #endif
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 921ef5d..6eaef3b 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -817,7 +817,7 @@ static void blk_add_trace_split(struct request_queue *q, struct bio *bio,
  *
  **/
 static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
-				       dev_t dev, sector_t from, sector_t to)
+				       dev_t dev, sector_t to)
 {
 	struct blk_trace *bt = q->blk_trace;
 	struct blk_io_trace_remap r;
@@ -829,8 +829,9 @@ static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
 	r.device_from = cpu_to_be32(bio->bi_bdev->bd_dev);
 	r.sector = cpu_to_be64(to);
 
-	__blk_add_trace(bt, from, bio->bi_size, bio->bi_rw, BLK_TA_REMAP,
-			!bio_flagged(bio, BIO_UPTODATE), sizeof(r), &r);
+	__blk_add_trace(bt, bio->bi_sector, bio->bi_size, bio->bi_rw,
+			BLK_TA_REMAP, !bio_flagged(bio, BIO_UPTODATE),
+			sizeof(r), &r);
 }
 
 /**
-- 
1.6.0.4

[PATCH] blktrace: correct remap names

[Alan D. Brunelle]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0002-blktrace-correct-remap-names.patch --]
[-- Type: text/x-diff, Size: 3918 bytes --]

>From 28019f41f67ac64f0a8b12148c367d45d7279173 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <alan.brunelle@hp.com>
Date: Thu, 30 Apr 2009 12:48:35 -0400
Subject: [PATCH] blktrace: correct remap names

This attempts to clarify names utilized during block I/O remap operations
(partition, volume manager). It correctly matches up the /from/ information for both device & sector. This takes in the concept from osaki Motohiro and extends it to include better naming for the "device_from" field.

Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
---
 include/linux/blktrace_api.h |    4 ++--
 include/trace/block.h        |    4 ++--
 kernel/trace/blktrace.c      |   21 +++++++++++----------
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index d960889..da1a14e 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -116,9 +116,9 @@ struct blk_io_trace {
  * The remap event
  */
 struct blk_io_trace_remap {
-	__be32 device;
 	__be32 device_from;
-	__be64 sector;
+	__be32 device_to;
+	__be64 sector_from;
 };
 
 enum {
diff --git a/include/trace/block.h b/include/trace/block.h
index 8ac945b..5b12efa 100644
--- a/include/trace/block.h
+++ b/include/trace/block.h
@@ -70,7 +70,7 @@ DECLARE_TRACE(block_split,
 
 DECLARE_TRACE(block_remap,
 	TP_PROTO(struct request_queue *q, struct bio *bio, dev_t dev,
-		 sector_t to),
-	      TP_ARGS(q, bio, dev, to));
+		 sector_t from),
+	      TP_ARGS(q, bio, dev, from));
 
 #endif
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 6eaef3b..1492d50 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -817,7 +817,7 @@ static void blk_add_trace_split(struct request_queue *q, struct bio *bio,
  *
  **/
 static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
-				       dev_t dev, sector_t to)
+				       dev_t dev, sector_t from)
 {
 	struct blk_trace *bt = q->blk_trace;
 	struct blk_io_trace_remap r;
@@ -825,9 +825,9 @@ static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
 	if (likely(!bt))
 		return;
 
-	r.device = cpu_to_be32(dev);
-	r.device_from = cpu_to_be32(bio->bi_bdev->bd_dev);
-	r.sector = cpu_to_be64(to);
+	r.device_from = cpu_to_be32(dev);
+	r.device_to   = cpu_to_be32(bio->bi_bdev->bd_dev);
+	r.sector_from = cpu_to_be64(from);
 
 	__blk_add_trace(bt, bio->bi_sector, bio->bi_size, bio->bi_rw,
 			BLK_TA_REMAP, !bio_flagged(bio, BIO_UPTODATE),
@@ -997,11 +997,11 @@ static void get_pdu_remap(const struct trace_entry *ent,
 			  struct blk_io_trace_remap *r)
 {
 	const struct blk_io_trace_remap *__r = pdu_start(ent);
-	__u64 sector = __r->sector;
+	__u64 sector_from = __r->sector_from;
 
-	r->device = be32_to_cpu(__r->device);
 	r->device_from = be32_to_cpu(__r->device_from);
-	r->sector = be64_to_cpu(sector);
+	r->device_to   = be32_to_cpu(__r->device_to);
+	r->sector_from = be64_to_cpu(sector_from);
 }
 
 typedef int (blk_log_action_t) (struct trace_iterator *iter, const char *act);
@@ -1055,13 +1055,14 @@ static int blk_log_with_error(struct trace_seq *s,
 
 static int blk_log_remap(struct trace_seq *s, const struct trace_entry *ent)
 {
-	struct blk_io_trace_remap r = { .device = 0, };
+	struct blk_io_trace_remap r = { .device_from = 0, };
 
 	get_pdu_remap(ent, &r);
 	return trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
 			       t_sector(ent),
-			       t_sec(ent), MAJOR(r.device), MINOR(r.device),
-			       (unsigned long long)r.sector);
+			       t_sec(ent), MAJOR(r.device_from),
+			       MINOR(r.device_from),
+			       (unsigned long long)r.sector_from);
 }
 
 static int blk_log_plug(struct trace_seq *s, const struct trace_entry *ent)
-- 
1.6.0.4

Re: Subject: [PATCH] from-sector redundant in trace_block_remap

[KOSAKI Motohiro]

> From f167379b6c2cbad5f4fe8492052693825980ec19 Mon Sep 17 00:00:00 2001
> From: Alan D. Brunelle <alan.brunelle@hp.com>
> Date: Thu, 30 Apr 2009 11:22:55 -0400
> Subject: [PATCH] from-sector redundant in trace_block_remap
>
> Removed redundant from-sector parameter: it's /always/ the bio's sector
> passed in.

please swap patch 1/2 and 2/2.
Actually this patch remove "to" parameter. but wrong parameter name
confuse patch description.

IOW, this patch description and changing code isn't consistent because
there is argument to-from swapping.
Otherthings, looks good to me.

>
> Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
> ---
>  block/blk-core.c        |    5 ++---
>  drivers/md/dm.c         |    3 +--
>  include/trace/block.h   |    4 ++--
>  kernel/trace/blktrace.c |    7 ++++---
>  4 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2998fe3..d028baf 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1277,7 +1277,7 @@ static inline void blk_partition_remap(struct bio *bio)
>  		bio->bi_bdev = bdev->bd_contains;
>
>  		trace_block_remap(bdev_get_queue(bio->bi_bdev), bio,
> -				    bdev->bd_dev, bio->bi_sector,
> +				    bdev->bd_dev,
>  				    bio->bi_sector - p->start_sect);
>  	}
>  }
> @@ -1446,8 +1446,7 @@ static inline void __generic_make_request(struct bio *bio)
>  			goto end_io;
>
>  		if (old_sector != -1)
> -			trace_block_remap(q, bio, old_dev, bio->bi_sector,
> -					    old_sector);
> +			trace_block_remap(q, bio, old_dev, old_sector);
>
>  		trace_block_bio_queue(q, bio);
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 424f7b0..e2ee4a7 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -656,8 +656,7 @@ static void __map_bio(struct dm_target *ti, struct bio *clone,
>  		/* the bio has been remapped so dispatch it */
>
>  		trace_block_remap(bdev_get_queue(clone->bi_bdev), clone,
> -				    tio->io->bio->bi_bdev->bd_dev,
> -				    clone->bi_sector, sector);
> +				    tio->io->bio->bi_bdev->bd_dev, sector);
>
>  		generic_make_request(clone);
>  	} else if (r < 0 || r == DM_MAPIO_REQUEUE) {
> diff --git a/include/trace/block.h b/include/trace/block.h
> index 25b7068..8ac945b 100644
> --- a/include/trace/block.h
> +++ b/include/trace/block.h
> @@ -70,7 +70,7 @@ DECLARE_TRACE(block_split,
>
>  DECLARE_TRACE(block_remap,
>  	TP_PROTO(struct request_queue *q, struct bio *bio, dev_t dev,
> -		 sector_t from, sector_t to),
> -	      TP_ARGS(q, bio, dev, from, to));
> +		 sector_t to),
> +	      TP_ARGS(q, bio, dev, to));
>
>  #endif
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 921ef5d..6eaef3b 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -817,7 +817,7 @@ static void blk_add_trace_split(struct request_queue *q, struct bio > *bio,
>   *
>   **/
>  static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
> -				       dev_t dev, sector_t from, sector_t to)
> +				       dev_t dev, sector_t to)
>  {
>  	struct blk_trace *bt = q->blk_trace;
>  	struct blk_io_trace_remap r;
> @@ -829,8 +829,9 @@ static void blk_add_trace_remap(struct request_queue *q, struct bio > *bio,
>  	r.device_from = cpu_to_be32(bio->bi_bdev->bd_dev);
>  	r.sector = cpu_to_be64(to);
>
> -	__blk_add_trace(bt, from, bio->bi_size, bio->bi_rw, BLK_TA_REMAP,
> -			!bio_flagged(bio, BIO_UPTODATE), sizeof(r), &r);
> +	__blk_add_trace(bt, bio->bi_sector, bio->bi_size, bio->bi_rw,
> +			BLK_TA_REMAP, !bio_flagged(bio, BIO_UPTODATE),
> +			sizeof(r), &r);
>  }
>
>  /**
> --
> 1.6.0.4
>
>

Re: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap

[KOSAKI Motohiro]

> I believe this may be more messed up than you believe: Consider the last
> function: blk_log_remap. Notice how you are printing the /device/ and
> /sector_from/ as one pair. Isn't that confusing too: wouldn't one expect
> /device_from/ and /sector_from/ to be paired up?
>
> I ran into this problem a long time ago - hence the current code in
> blkparse contains:
>
>        if (act[0] == 'A') {    /* Remap */
>                get_pdu_remap(t, &r);
>                t->device = r.device_from;
>        }
>
> Which puts the device_from out at the beginning of the line, and then
> the 'device' + 'sector' (== 'sector_from') out at the end of the line.
> (Which, of course, is all confusing.)

Good viewpoint!
Currently, 99% blktrace user use blktrace and blkparse command, not
ftrace framework.
So, ABI changing is proper way?

I'd like to hear Jens and Li opinion.



> To really fix this, I think one needs to:
>
> (a) adjust the kernel to make "r.device_from" and "r.sector_from" refer
> to the same part of the trace, and the "r.device" and "t->sector" refer
> to the other part of the trace.
>
> (b) fix blkparse (& btt & ...) to do the same thing - not make
> adjustments for the stupid naming...
>
> If we leave the kernel->user space ABI alone:
>
> first u32: device mapped /from/
> second u32: device mapped /to/
> u64: sector mapped from
>
> We can then change the names. I've followed this e-mail up with a pair
> of kernel-side patches and will submit user side fixes for blkptrace later.

Re: Subject: [PATCH] from-sector redundant in trace_block_remap

[Alan D. Brunelle]

KOSAKI Motohiro wrote:
>> From f167379b6c2cbad5f4fe8492052693825980ec19 Mon Sep 17 00:00:00 2001
>> From: Alan D. Brunelle <alan.brunelle@hp.com>
>> Date: Thu, 30 Apr 2009 11:22:55 -0400
>> Subject: [PATCH] from-sector redundant in trace_block_remap
>>
>> Removed redundant from-sector parameter: it's /always/ the bio's sector
>> passed in.
> 
> please swap patch 1/2 and 2/2.
> Actually this patch remove "to" parameter. but wrong parameter name
> confuse patch description.
> 
> IOW, this patch description and changing code isn't consistent because
> there is argument to-from swapping.
> Otherthings, looks good to me.

Very good point - will re-work & re-submit.

Thanks,
Alan

Re: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap

[Alan D. Brunelle]

KOSAKI Motohiro wrote:
>> I believe this may be more messed up than you believe: Consider the last
>> function: blk_log_remap. Notice how you are printing the /device/ and
>> /sector_from/ as one pair. Isn't that confusing too: wouldn't one expect
>> /device_from/ and /sector_from/ to be paired up?
>>
>> I ran into this problem a long time ago - hence the current code in
>> blkparse contains:
>>
>>        if (act[0] == 'A') {    /* Remap */
>>                get_pdu_remap(t, &r);
>>                t->device = r.device_from;
>>        }
>>
>> Which puts the device_from out at the beginning of the line, and then
>> the 'device' + 'sector' (== 'sector_from') out at the end of the line.
>> (Which, of course, is all confusing.)
> 
> Good viewpoint!
> Currently, 99% blktrace user use blktrace and blkparse command, not
> ftrace framework.
> So, ABI changing is proper way?
> 
> I'd like to hear Jens and Li opinion.

I'll hold off reposting until Jens and Li concur.
Alan

Re: Subject: [PATCH] from-sector redundant in trace_block_remap

[Li Zefan]

>>From f167379b6c2cbad5f4fe8492052693825980ec19 Mon Sep 17 00:00:00 2001
> From: Alan D. Brunelle <alan.brunelle@hp.com>
> Date: Thu, 30 Apr 2009 11:22:55 -0400
> Subject: [PATCH] from-sector redundant in trace_block_remap
> 
> Removed redundant from-sector parameter: it's /always/ the bio's sector
> passed in.
> 
> Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
> ---
>  block/blk-core.c        |    5 ++---
>  drivers/md/dm.c         |    3 +--
>  include/trace/block.h   |    4 ++--
>  kernel/trace/blktrace.c |    7 ++++---
>  4 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2998fe3..d028baf 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1277,7 +1277,7 @@ static inline void blk_partition_remap(struct bio *bio)
>  		bio->bi_bdev = bdev->bd_contains;
>  
>  		trace_block_remap(bdev_get_queue(bio->bi_bdev), bio,
> -				    bdev->bd_dev, bio->bi_sector,
> +				    bdev->bd_dev,
>  				    bio->bi_sector - p->start_sect);
>  	}
>  }
> @@ -1446,8 +1446,7 @@ static inline void __generic_make_request(struct bio *bio)
>  			goto end_io;
>  
>  		if (old_sector != -1)
> -			trace_block_remap(q, bio, old_dev, bio->bi_sector,
> -					    old_sector);
> +			trace_block_remap(q, bio, old_dev, old_sector);
>  
>  		trace_block_bio_queue(q, bio);
>  
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 424f7b0..e2ee4a7 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -656,8 +656,7 @@ static void __map_bio(struct dm_target *ti, struct bio *clone,
>  		/* the bio has been remapped so dispatch it */
>  
>  		trace_block_remap(bdev_get_queue(clone->bi_bdev), clone,
> -				    tio->io->bio->bi_bdev->bd_dev,
> -				    clone->bi_sector, sector);
> +				    tio->io->bio->bi_bdev->bd_dev, sector);
>  
>  		generic_make_request(clone);
>  	} else if (r < 0 || r == DM_MAPIO_REQUEUE) {
> diff --git a/include/trace/block.h b/include/trace/block.h
> index 25b7068..8ac945b 100644
> --- a/include/trace/block.h
> +++ b/include/trace/block.h
> @@ -70,7 +70,7 @@ DECLARE_TRACE(block_split,
>  
>  DECLARE_TRACE(block_remap,
>  	TP_PROTO(struct request_queue *q, struct bio *bio, dev_t dev,
> -		 sector_t from, sector_t to),
> -	      TP_ARGS(q, bio, dev, from, to));
> +		 sector_t to),
> +	      TP_ARGS(q, bio, dev, to));
>  
>  #endif
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 921ef5d..6eaef3b 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -817,7 +817,7 @@ static void blk_add_trace_split(struct request_queue *q, struct bio *bio,
>   *
>   **/
>  static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
> -				       dev_t dev, sector_t from, sector_t to)
> +				       dev_t dev, sector_t to)

You should also update the kernel-doc:

  * @dev:	target device
- * @from:	source sector
  * @to:	target sector

With this fix:

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

>  {
>  	struct blk_trace *bt = q->blk_trace;
>  	struct blk_io_trace_remap r;
> @@ -829,8 +829,9 @@ static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
>  	r.device_from = cpu_to_be32(bio->bi_bdev->bd_dev);
>  	r.sector = cpu_to_be64(to);
>  
> -	__blk_add_trace(bt, from, bio->bi_size, bio->bi_rw, BLK_TA_REMAP,
> -			!bio_flagged(bio, BIO_UPTODATE), sizeof(r), &r);
> +	__blk_add_trace(bt, bio->bi_sector, bio->bi_size, bio->bi_rw,
> +			BLK_TA_REMAP, !bio_flagged(bio, BIO_UPTODATE),
> +			sizeof(r), &r);
>  }
>  
>  /**
> -- 
> 1.6.0.4

Re: [PATCH] blktrace: correct remap names

[Li Zefan]

> Subject: [PATCH] blktrace: correct remap names
> 
> This attempts to clarify names utilized during block I/O remap operations
> (partition, volume manager). It correctly matches up the /from/ information for both device & sector. This takes in the concept from osaki Motohiro and extends it to include better naming for the "device_from" field.
> 

I also noticed this..

> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
> ---
>  include/linux/blktrace_api.h |    4 ++--
>  include/trace/block.h        |    4 ++--
>  kernel/trace/blktrace.c      |   21 +++++++++++----------
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> index d960889..da1a14e 100644
> --- a/include/linux/blktrace_api.h
> +++ b/include/linux/blktrace_api.h
> @@ -116,9 +116,9 @@ struct blk_io_trace {
>   * The remap event
>   */
>  struct blk_io_trace_remap {
> -	__be32 device;
>  	__be32 device_from;
> -	__be64 sector;
> +	__be32 device_to;
> +	__be64 sector_from;
>  };
>  

If we are really fine with this change, then:

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

>  enum {
> diff --git a/include/trace/block.h b/include/trace/block.h
> index 8ac945b..5b12efa 100644
> --- a/include/trace/block.h
> +++ b/include/trace/block.h
> @@ -70,7 +70,7 @@ DECLARE_TRACE(block_split,
>  
>  DECLARE_TRACE(block_remap,
>  	TP_PROTO(struct request_queue *q, struct bio *bio, dev_t dev,
> -		 sector_t to),
> -	      TP_ARGS(q, bio, dev, to));
> +		 sector_t from),
> +	      TP_ARGS(q, bio, dev, from));
>  
>  #endif
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 6eaef3b..1492d50 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -817,7 +817,7 @@ static void blk_add_trace_split(struct request_queue *q, struct bio *bio,
>   *
>   **/
>  static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
> -				       dev_t dev, sector_t to)
> +				       dev_t dev, sector_t from)
>  {
>  	struct blk_trace *bt = q->blk_trace;
>  	struct blk_io_trace_remap r;
> @@ -825,9 +825,9 @@ static void blk_add_trace_remap(struct request_queue *q, struct bio *bio,
>  	if (likely(!bt))
>  		return;
>  
> -	r.device = cpu_to_be32(dev);
> -	r.device_from = cpu_to_be32(bio->bi_bdev->bd_dev);
> -	r.sector = cpu_to_be64(to);
> +	r.device_from = cpu_to_be32(dev);
> +	r.device_to   = cpu_to_be32(bio->bi_bdev->bd_dev);
> +	r.sector_from = cpu_to_be64(from);
>  
>  	__blk_add_trace(bt, bio->bi_sector, bio->bi_size, bio->bi_rw,
>  			BLK_TA_REMAP, !bio_flagged(bio, BIO_UPTODATE),
> @@ -997,11 +997,11 @@ static void get_pdu_remap(const struct trace_entry *ent,
>  			  struct blk_io_trace_remap *r)
>  {
>  	const struct blk_io_trace_remap *__r = pdu_start(ent);
> -	__u64 sector = __r->sector;
> +	__u64 sector_from = __r->sector_from;
>  
> -	r->device = be32_to_cpu(__r->device);
>  	r->device_from = be32_to_cpu(__r->device_from);
> -	r->sector = be64_to_cpu(sector);
> +	r->device_to   = be32_to_cpu(__r->device_to);
> +	r->sector_from = be64_to_cpu(sector_from);
>  }
>  
>  typedef int (blk_log_action_t) (struct trace_iterator *iter, const char *act);
> @@ -1055,13 +1055,14 @@ static int blk_log_with_error(struct trace_seq *s,
>  
>  static int blk_log_remap(struct trace_seq *s, const struct trace_entry *ent)
>  {
> -	struct blk_io_trace_remap r = { .device = 0, };
> +	struct blk_io_trace_remap r = { .device_from = 0, };
>  
>  	get_pdu_remap(ent, &r);
>  	return trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
>  			       t_sector(ent),
> -			       t_sec(ent), MAJOR(r.device), MINOR(r.device),
> -			       (unsigned long long)r.sector);
> +			       t_sec(ent), MAJOR(r.device_from),
> +			       MINOR(r.device_from),
> +			       (unsigned long long)r.sector_from);

Minor nits. I think this is better:

return trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
			t_sector(ent), t_sec(ent),
			MAJOR(r.device_from), MINOR(r.device_from),
			(unsigned long long)r.sector_from);

or this:

return trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
			t_sector(ent),
			t_sec(ent),
			MAJOR(r.device_from),
			MINOR(r.device_from),
			(unsigned long long)r.sector_from);

>  }
>  
>  static int blk_log_plug(struct trace_seq *s, const struct trace_entry *ent)
> -- 
> 1.6.0.4

Re: [PATCH] blktrace: correct remap names

[Ingo Molnar]

* Li Zefan <lizf@cn.fujitsu.com> wrote:

> > Subject: [PATCH] blktrace: correct remap names
> > 
> > This attempts to clarify names utilized during block I/O remap operations
> > (partition, volume manager). It correctly matches up the /from/ information for both device & sector. This takes in the concept from osaki Motohiro and extends it to include better naming for the "device_from" field.
> > 
> 
> I also noticed this..
> 
> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Li Zefan <lizf@cn.fujitsu.com>
> > Cc: Jens Axboe <jens.axboe@oracle.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
> > ---
> >  include/linux/blktrace_api.h |    4 ++--
> >  include/trace/block.h        |    4 ++--
> >  kernel/trace/blktrace.c      |   21 +++++++++++----------
> >  3 files changed, 15 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> > index d960889..da1a14e 100644
> > --- a/include/linux/blktrace_api.h
> > +++ b/include/linux/blktrace_api.h
> > @@ -116,9 +116,9 @@ struct blk_io_trace {
> >   * The remap event
> >   */
> >  struct blk_io_trace_remap {
> > -	__be32 device;
> >  	__be32 device_from;
> > -	__be64 sector;
> > +	__be32 device_to;
> > +	__be64 sector_from;
> >  };
> >  
> 
> If we are really fine with this change, then:
> 
> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

is it acceptable to break the sector output ordering of all past 
blktrace+blkparse binaries, as far as remap events go?

	Ingo

Re: [PATCH] blktrace: correct remap names

[Alan D. Brunelle]

Ingo Molnar wrote:
> * Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
>>> Subject: [PATCH] blktrace: correct remap names
>>>
>>> This attempts to clarify names utilized during block I/O remap operations
>>> (partition, volume manager). It correctly matches up the /from/ information for both device & sector. This takes in the concept from osaki Motohiro and extends it to include better naming for the "device_from" field.
>>>
>> I also noticed this..
>>
>>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>> Cc: Li Zefan <lizf@cn.fujitsu.com>
>>> Cc: Jens Axboe <jens.axboe@oracle.com>
>>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>>> Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
>>> ---
>>>  include/linux/blktrace_api.h |    4 ++--
>>>  include/trace/block.h        |    4 ++--
>>>  kernel/trace/blktrace.c      |   21 +++++++++++----------
>>>  3 files changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
>>> index d960889..da1a14e 100644
>>> --- a/include/linux/blktrace_api.h
>>> +++ b/include/linux/blktrace_api.h
>>> @@ -116,9 +116,9 @@ struct blk_io_trace {
>>>   * The remap event
>>>   */
>>>  struct blk_io_trace_remap {
>>> -	__be32 device;
>>>  	__be32 device_from;
>>> -	__be64 sector;
>>> +	__be32 device_to;
>>> +	__be64 sector_from;
>>>  };
>>>  
>> If we are really fine with this change, then:
>>
>> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> is it acceptable to break the sector output ordering of all past 
> blktrace+blkparse binaries, as far as remap events go?
> 
> 	Ingo

The binary order of data sent remains the same: the field names for the
structure & parameters passed around were wrong^Wmisleading (as was
pointed out previously by Kosaki Motoiro). I've tested this and the
blkparse output remains identical (both with the previous version of
blktrace/blkparse & with a posted patch to blkparse which also fixes the
field names of blk_io_trace_remap). The testing included permutations of
(old/new) kernel traces & (old/new) blkparse. They all resulted in the
same blkparse output.

Alan

Re: [PATCH] blktrace: correct remap names

[Ingo Molnar]

* Alan D. Brunelle <Alan.Brunelle@hp.com> wrote:

> Ingo Molnar wrote:
> > * Li Zefan <lizf@cn.fujitsu.com> wrote:
> > 
> >>> Subject: [PATCH] blktrace: correct remap names
> >>>
> >>> This attempts to clarify names utilized during block I/O remap operations
> >>> (partition, volume manager). It correctly matches up the /from/ information for both device & sector. This takes in the concept from osaki Motohiro and extends it to include better naming for the "device_from" field.
> >>>
> >> I also noticed this..
> >>
> >>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> >>> Cc: Li Zefan <lizf@cn.fujitsu.com>
> >>> Cc: Jens Axboe <jens.axboe@oracle.com>
> >>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> >>> Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
> >>> ---
> >>>  include/linux/blktrace_api.h |    4 ++--
> >>>  include/trace/block.h        |    4 ++--
> >>>  kernel/trace/blktrace.c      |   21 +++++++++++----------
> >>>  3 files changed, 15 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> >>> index d960889..da1a14e 100644
> >>> --- a/include/linux/blktrace_api.h
> >>> +++ b/include/linux/blktrace_api.h
> >>> @@ -116,9 +116,9 @@ struct blk_io_trace {
> >>>   * The remap event
> >>>   */
> >>>  struct blk_io_trace_remap {
> >>> -	__be32 device;
> >>>  	__be32 device_from;
> >>> -	__be64 sector;
> >>> +	__be32 device_to;
> >>> +	__be64 sector_from;
> >>>  };
> >>>  
> >> If we are really fine with this change, then:
> >>
> >> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> > 
> > is it acceptable to break the sector output ordering of all past 
> > blktrace+blkparse binaries, as far as remap events go?
> > 
> > 	Ingo
> 
> The binary order of data sent remains the same: the field names 
> for the structure & parameters passed around were 
> wrong^Wmisleading (as was pointed out previously by Kosaki 
> Motoiro). I've tested this and the blkparse output remains 
> identical (both with the previous version of blktrace/blkparse & 
> with a posted patch to blkparse which also fixes the field names 
> of blk_io_trace_remap). The testing included permutations of 
> (old/new) kernel traces & (old/new) blkparse. They all resulted in 
> the same blkparse output.

Ah, ok - my worry was baseless then - thanks! I'll queue them up in 
tip/tracing/blktrace.

	Ingo