Organization: Beijing Soul
X-Mailer: Sylpheed version 0.9.8claws (GTK+ 1.2.10; powerpc-unknown-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
On Thu, 1 Jan 2004 19:35:45 +0100
Jens Axboe <[email protected]> wrote:
> Patch is obviously bogus, just look at the comm definition in sched.h:
>
> char comm[16];
>
> IO submission must happen in process context, so we also know that
> current is valid.
You are right. But why add this patch, My laptop not crash when I enable block dump, So I try to find where is the Bug. Final, The bug is in sector_t, I was enable CONFIG_LBD, So sector_t is u64, So We have to change the code when enable CONFIG_LBD.
I'd like the 2.4 style so add count number into printf.
Here is the patch fix it
+
+ if (unlikely(block_dump)) {
+ char b[BDEVNAME_SIZE];
+ printk("%s(%d): %s block %llu/%u on %s\n",
+ current->comm, current->pid,
+ (rw & WRITE) ? "WRITE" : (rw == READA ? "READA" : "READ"),
+ (u64)bio->bi_sector, count, bdevname(bio->bi_bdev,b));
+ }
+
I think, also have this bug in 2.4.23, here is the patch for it, Hope can helpful.
Index: linux-2.4.23/drivers/block/ll_rw_blk.c
===================================================================
--- linux-2.4.23/drivers/block/ll_rw_blk.c (revision 4)
+++ linux-2.4.23/drivers/block/ll_rw_blk.c (working copy)
@@ -1298,7 +1298,7 @@
wake_up(&bh->b_wait);
if (block_dump)
- printk(KERN_DEBUG "%s: %s block %lu/%u on %s\n", current->comm, rw == WRITE ? "WRITE" : "READ", bh->b_rsector, count, kdevname(bh->b_rdev));
+ printk(KERN_DEBUG "%s: %s block %llu/%u on %s\n", current->comm, rw == WRITE ? "WRITE" : "READ", (u64)bh->b_rsector, count, kdevname(bh->b_rdev));
put_bh(bh);
switch (rw) {
--
Hu Gang / Steve
RLU# : 204016 [1999] (Registered Linux user)
GPG Public Key: http://soulinfo.com/~hugang/HuGang.asc
On Fri, Jan 02 2004, Hugang wrote:
> Organization: Beijing Soul
> X-Mailer: Sylpheed version 0.9.8claws (GTK+ 1.2.10; powerpc-unknown-linux-gnu)
> Mime-Version: 1.0
> Content-Type: text/plain; charset=US-ASCII
> Content-Transfer-Encoding: 7bit
>
> On Thu, 1 Jan 2004 19:35:45 +0100
> Jens Axboe <[email protected]> wrote:
>
> > Patch is obviously bogus, just look at the comm definition in sched.h:
> >
> > char comm[16];
> >
> > IO submission must happen in process context, so we also know that
> > current is valid.
>
> You are right. But why add this patch, My laptop not crash when I
> enable block dump, So I try to find where is the Bug. Final, The bug
I dunno, I can't possibly tell since you haven't given any info about
this crash. Where does it crash, do you have an oops? All I could say
from your report + patch is that it wasn't valid. There's just no way
for current->comm to be NULL, so your patch couldn't possibly have made
a difference.
> is in sector_t, I was enable CONFIG_LBD, So sector_t is u64, So We
> have to change the code when enable CONFIG_LBD.
>
> I'd like the 2.4 style so add count number into printf.
>
> Here is the patch fix it
> +
> + if (unlikely(block_dump)) {
> + char b[BDEVNAME_SIZE];
> + printk("%s(%d): %s block %llu/%u on %s\n",
> + current->comm, current->pid,
> + (rw & WRITE) ? "WRITE" : (rw == READA ? "READA" : "READ"),
> + (u64)bio->bi_sector, count, bdevname(bio->bi_bdev,b));
> + }
It's best to keep the line as minimal as possible, count isn't really
very interesting. What is interesting is process, offset (for finding
the file, if you need to), and data direction.
> I think, also have this bug in 2.4.23, here is the patch for it, Hope can helpful.
> Index: linux-2.4.23/drivers/block/ll_rw_blk.c
> ===================================================================
> --- linux-2.4.23/drivers/block/ll_rw_blk.c (revision 4)
> +++ linux-2.4.23/drivers/block/ll_rw_blk.c (working copy)
> @@ -1298,7 +1298,7 @@
> wake_up(&bh->b_wait);
>
> if (block_dump)
> - printk(KERN_DEBUG "%s: %s block %lu/%u on %s\n", current->comm, rw == WRITE ? "WRITE" : "READ", bh->b_rsector, count, kdevname(bh->b_rdev));
> + printk(KERN_DEBUG "%s: %s block %llu/%u on %s\n", current->comm, rw == WRITE ? "WRITE" : "READ", (u64)bh->b_rsector, count, kdevname(bh->b_rdev));
2.4 stock doesn't have 64-bit sectors, please consult (again) the
canonical source (include file). There's no need to cast.
--
Jens Axboe
On Fri, 2 Jan 2004 12:27:33 +0100
Jens Axboe <[email protected]> wrote:
> I dunno, I can't possibly tell since you haven't given any info about
> this crash. Where does it crash, do you have an oops? All I could say
> from your report + patch is that it wasn't valid. There's just no way
> for current->comm to be NULL, so your patch couldn't possibly have made
> a difference.
Attached file is the crashed dmesg, When I disable CONFIG_LBD, the problem not found any more.
thanks.
--
Hu Gang / Steve
RLU# : 204016 [1999] (Registered Linux user)
GPG Public Key: http://soulinfo.com/~hugang/HuGang.asc
On Fri, Jan 02 2004, Hugang wrote:
> On Fri, 2 Jan 2004 12:27:33 +0100
> Jens Axboe <[email protected]> wrote:
>
> > I dunno, I can't possibly tell since you haven't given any info about
> > this crash. Where does it crash, do you have an oops? All I could say
> > from your report + patch is that it wasn't valid. There's just no way
> > for current->comm to be NULL, so your patch couldn't possibly have made
> > a difference.
>
> Attached file is the crashed dmesg, When I disable CONFIG_LBD, the
> problem not found any more.
Ah there you go, then it's just the missing cast to u64. It has nothing
to do with current->comm at all. The compiler should have warned you
about this error, did it not?
The dump printk() needs to be changed anyways, the rw deciphering is not
correct. Something like this is more appropriate:
if (unlikely(block_dump)) {
char b[BDEVNAME_SIZE];
printk("%s(%d): %s block %Lu on %s\n", current->comm,
current->pid, (rw & WRITE) ? "WRITE" : "READ",
(u64) bio->bi_sector, bdevname(bio->bi_bdev, b));
}
--
Jens Axboe
>>>>> "Jens" == Jens Axboe <[email protected]> writes:
Jens> The dump printk() needs to be changed anyways, the rw
Jens> deciphering is not correct. Something like this is more
Jens> appropriate:
Jens> if (unlikely(block_dump)) {
Jens> char b[BDEVNAME_SIZE];
Jens> printk("%s(%d): %s block %Lu on %s\n", current->comm, current-> pid, (rw & WRITE) ? "WRITE" : "READ",
Jens> (u64) bio->bi_sector, bdevname(bio->bi_bdev, b));
Jens> }
Please cast to (unsigned long long) not (u64) because on 64-bit
architectures u64 is unsigned long, and you'll get a compiler warning.
On Sun, Jan 04 2004, Peter Chubb wrote:
> >>>>> "Jens" == Jens Axboe <[email protected]> writes:
>
> Jens> The dump printk() needs to be changed anyways, the rw
> Jens> deciphering is not correct. Something like this is more
> Jens> appropriate:
>
> Jens> if (unlikely(block_dump)) {
> Jens> char b[BDEVNAME_SIZE];
> Jens> printk("%s(%d): %s block %Lu on %s\n", current->comm, current-> pid, (rw & WRITE) ? "WRITE" : "READ",
> Jens> (u64) bio->bi_sector, bdevname(bio->bi_bdev, b));
> Jens> }
>
> Please cast to (unsigned long long) not (u64) because on 64-bit
> architectures u64 is unsigned long, and you'll get a compiler warning.
Yeah Andrew noted the same thing, my bad.
--
Jens Axboe
On Sun, Jan 04 2004, Peter Chubb wrote:
> >>>>> "Jens" == Jens Axboe <[email protected]> writes:
>
> Jens> The dump printk() needs to be changed anyways, the rw
> Jens> deciphering is not correct. Something like this is more
> Jens> appropriate:
>
> Jens> if (unlikely(block_dump)) {
> Jens> char b[BDEVNAME_SIZE];
> Jens> printk("%s(%d): %s block %Lu on %s\n", current->comm, current-> pid, (rw & WRITE) ? "WRITE" : "READ",
> Jens> (u64) bio->bi_sector, bdevname(bio->bi_bdev, b));
> Jens> }
>
> Please cast to (unsigned long long) not (u64) because on 64-bit
> architectures u64 is unsigned long, and you'll get a compiler warning.
Yeah Andrew noted the same thing, my bad.
--
Jens Axboe
>>>>> "Jens" == Jens Axboe <[email protected]> writes:
Jens> The dump printk() needs to be changed anyways, the rw
Jens> deciphering is not correct. Something like this is more
Jens> appropriate:
Jens> if (unlikely(block_dump)) {
Jens> char b[BDEVNAME_SIZE];
Jens> printk("%s(%d): %s block %Lu on %s\n", current->comm, current-> pid, (rw & WRITE) ? "WRITE" : "READ",
Jens> (u64) bio->bi_sector, bdevname(bio->bi_bdev, b));
Jens> }
Please cast to (unsigned long long) not (u64) because on 64-bit
architectures u64 is unsigned long, and you'll get a compiler warning.