| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 30 Apr 2009 12:56:13 -0400
From: "Alan D. Brunelle" <Alan.Brunelle@...com>
To: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
CC: LKML <linux-kernel@...r.kernel.org>,
Li Zefan <lizf@...fujitsu.com>, Ingo Molnar <mingo@...e.hu>,
Steven Rostedt <srostedt@...hat.com>,
Frederic Weisbecker <fweisbec@...il.com>,
Jens Axboe <jens.axboe@...cle.com>,
Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH] blktrace: swap arg name "from" and "to" of blk_add_trace_remap
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@...fujitsu.com>
> Cc: Jens Axboe <jens.axboe@...cle.com>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists