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

["Alan D. Brunelle" <Alan.Brunelle@hp.com>]

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