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 <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
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)
* KOSAKI Motohiro <[email protected]> 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
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 <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
> ---
> 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
> From f167379b6c2cbad5f4fe8492052693825980ec19 Mon Sep 17 00:00:00 2001
> From: Alan D. Brunelle <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
>
> 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.