* Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7
2013-04-19 5:57 ` [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7 Tejun Heo
@ 2013-04-19 6:17 ` Tejun Heo
2013-04-19 6:30 ` Wanlong Gao
2013-04-19 13:31 ` Jens Axboe
2013-04-19 8:24 ` Jan Schmidt
` (2 subsequent siblings)
3 siblings, 2 replies; 8+ messages in thread
From: Tejun Heo @ 2013-04-19 6:17 UTC (permalink / raw)
To: Wanlong Gao
Cc: Jens Axboe, Steven Rostedt, namhyung, agk, dm-devel, neilb, LKML,
Linus Torvalds, Chris Mason, linux-btrfs
On Thu, Apr 18, 2013 at 10:57:54PM -0700, Tejun Heo wrote:
> No wonder this thing crashes. Chris, can't the original bio carry
> bbio in bi_private and let end_bio_extent_readpage() free the bbio
> instead of abusing bi_bdev like this?
BTW, I think it's a bit too late to fix this properly from btrfs side
unless we're gonna do -rc8, so let's revert the TP patch for now and
sort this out in the next devel cycle. AFAICS, while disturbingly
(ha|yu)cky, the bi_bdev trick should be okay without the new TP.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7
2013-04-19 6:17 ` Tejun Heo
@ 2013-04-19 6:30 ` Wanlong Gao
2013-04-19 13:31 ` Jens Axboe
1 sibling, 0 replies; 8+ messages in thread
From: Wanlong Gao @ 2013-04-19 6:30 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, Steven Rostedt, namhyung, agk, dm-devel, neilb, LKML,
Linus Torvalds, Chris Mason, linux-btrfs, Wanlong Gao
On 04/19/2013 02:17 PM, Tejun Heo wrote:
> On Thu, Apr 18, 2013 at 10:57:54PM -0700, Tejun Heo wrote:
>> No wonder this thing crashes. Chris, can't the original bio carry
>> bbio in bi_private and let end_bio_extent_readpage() free the bbio
>> instead of abusing bi_bdev like this?
>
> BTW, I think it's a bit too late to fix this properly from btrfs side
> unless we're gonna do -rc8, so let's revert the TP patch for now and
> sort this out in the next devel cycle. AFAICS, while disturbingly
> (ha|yu)cky, the bi_bdev trick should be okay without the new TP.
Thank you for investigating this. And sorry for my incompleted panic picture
before that let you detour. :-(
Regards,
Wanlong Gao
>
> Thanks.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7
2013-04-19 6:17 ` Tejun Heo
2013-04-19 6:30 ` Wanlong Gao
@ 2013-04-19 13:31 ` Jens Axboe
1 sibling, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2013-04-19 13:31 UTC (permalink / raw)
To: Tejun Heo
Cc: Wanlong Gao, Steven Rostedt, namhyung, agk, dm-devel, neilb, LKML,
Linus Torvalds, Chris Mason, linux-btrfs
On Thu, Apr 18 2013, Tejun Heo wrote:
> On Thu, Apr 18, 2013 at 10:57:54PM -0700, Tejun Heo wrote:
> > No wonder this thing crashes. Chris, can't the original bio carry
> > bbio in bi_private and let end_bio_extent_readpage() free the bbio
> > instead of abusing bi_bdev like this?
>
> BTW, I think it's a bit too late to fix this properly from btrfs side
> unless we're gonna do -rc8, so let's revert the TP patch for now and
> sort this out in the next devel cycle. AFAICS, while disturbingly
> (ha|yu)cky, the bi_bdev trick should be okay without the new TP.
The tracing commit was already backed out yesterday, so no problems
there. I figured it'd be a bit too late to fix something this nasty in a
suitably clean and straight forward fashion.
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7
2013-04-19 5:57 ` [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7 Tejun Heo
2013-04-19 6:17 ` Tejun Heo
@ 2013-04-19 8:24 ` Jan Schmidt
2013-04-19 12:15 ` Chris Mason
2013-04-19 13:32 ` Jens Axboe
3 siblings, 0 replies; 8+ messages in thread
From: Jan Schmidt @ 2013-04-19 8:24 UTC (permalink / raw)
To: Tejun Heo
Cc: Wanlong Gao, Jens Axboe, Steven Rostedt, namhyung, agk, dm-devel,
neilb, LKML, Linus Torvalds, Chris Mason, linux-btrfs
On Fri, April 19, 2013 at 07:57 (+0200), Tejun Heo wrote:
> (cc'ing btrfs people)
>
> On Fri, Apr 19, 2013 at 11:33:20AM +0800, Wanlong Gao wrote:
>> RIP: 0010:[<ffffffff812484d3>] [<ffffffff812484d3>] ftrace_raw_event_block_bio_complete+0x73/0xf0
> ...
>> [<ffffffff811b6c10>] bio_endio+0x80/0x90
>> [<ffffffffa0790d26>] btrfs_end_bio+0xf6/0x190 [btrfs]
>> [<ffffffff811b6bcd>] bio_endio+0x3d/0x90
>> [<ffffffff81249873>] req_bio_endio+0xa3/0xe0
>
> Ugh....
>
> In fs/btrfs/volumes.c
>
> static void bbio_error(struct btrfs_bio *bbio, struct bio *bio, u64 logical)
> {
> ...
> bio->bi_bdev = (struct block_device *)
> (unsigned long)bbio->mirror_num;
> ...
> }
>
> static void btrfs_end_bio(struct bio *bio, int err)
> {
> ...
> bio->bi_bdev = (struct block_device *)
> (unsigned long)bbio->mirror_num;
>
> ...
> }
>
> In fs/btrfs/extent_io.c
>
> static void end_bio_extent_readpage(struct bio *bio, int err)
> {
> int mirror;
> ...
> mirror = (int)(unsigned long)bio->bi_bdev;
> ...
> }
>
> Ewweeeeeeeeeeeeeeeeeehh........
>
> No wonder this thing crashes. Chris, can't the original bio carry
> bbio in bi_private and let end_bio_extent_readpage() free the bbio
> instead of abusing bi_bdev like this?
Oops.
It's been my patch back in 2011 (commit 2774b2ca3), sent as an RFC-Patch and
just slipped in without further discussion of that exact change. Hackish, yes -
my reasoning was because the block layer changed bio->bi_bdev anyway, no one
would want to look into it after the bio returned (and in fact it didn't hurt
for like two years now). Although the block layer changes bi_bdev, it stays a
valid bdev pointer, I admit.
One way around this would be what you suggest, however that would mean the
caller of (btrfs|btree)_submit_bio_hook gets its completion called in the end,
but must know that the private is in fact a bbio which in turn carries the
caller's private. Doesn't sound clean to me, either.
The best idea I currently have is to add a dispatcher function that does the
freeing of bbio and calls the actual completion with mirror_num as a separate
parameter. That would make all the btrfs completions incompatible with
bio_end_io_t, but it shouldn't hurt.
At least now I know I wasn't invited to LSF for a good reason :-)
-Jan
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7
2013-04-19 5:57 ` [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7 Tejun Heo
2013-04-19 6:17 ` Tejun Heo
2013-04-19 8:24 ` Jan Schmidt
@ 2013-04-19 12:15 ` Chris Mason
2013-04-19 13:32 ` Jens Axboe
3 siblings, 0 replies; 8+ messages in thread
From: Chris Mason @ 2013-04-19 12:15 UTC (permalink / raw)
To: Tejun Heo, Wanlong Gao
Cc: Jens Axboe, Steven Rostedt, namhyung@gmail.com, agk@redhat.com,
dm-devel@redhat.com, neilb@suse.de, LKML, Linus Torvalds,
linux-btrfs@vger.kernel.org
Quoting Tejun Heo (2013-04-19 01:57:54)
>
> Ewweeeeeeeeeeeeeeeeeehh........
>
> No wonder this thing crashes. Chris, can't the original bio carry
> bbio in bi_private and let end_bio_extent_readpage() free the bbio
> instead of abusing bi_bdev like this?
Yes, we can definitely carry bbio up higher in the stack. I'll patch it
up right now. I do agree that it'll be too big for -final, but we'll
have it either way.
-chris
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7
2013-04-19 5:57 ` [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7 Tejun Heo
` (2 preceding siblings ...)
2013-04-19 12:15 ` Chris Mason
@ 2013-04-19 13:32 ` Jens Axboe
2013-04-19 13:52 ` Chris Mason
3 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2013-04-19 13:32 UTC (permalink / raw)
To: Tejun Heo
Cc: Wanlong Gao, Steven Rostedt, namhyung, agk, dm-devel, neilb, LKML,
Linus Torvalds, Chris Mason, linux-btrfs
On Thu, Apr 18 2013, Tejun Heo wrote:
> (cc'ing btrfs people)
>
> On Fri, Apr 19, 2013 at 11:33:20AM +0800, Wanlong Gao wrote:
> > RIP: 0010:[<ffffffff812484d3>] [<ffffffff812484d3>] ftrace_raw_event_block_bio_complete+0x73/0xf0
> ...
> > [<ffffffff811b6c10>] bio_endio+0x80/0x90
> > [<ffffffffa0790d26>] btrfs_end_bio+0xf6/0x190 [btrfs]
> > [<ffffffff811b6bcd>] bio_endio+0x3d/0x90
> > [<ffffffff81249873>] req_bio_endio+0xa3/0xe0
>
> Ugh....
>
> In fs/btrfs/volumes.c
>
> static void bbio_error(struct btrfs_bio *bbio, struct bio *bio, u64 logical)
> {
> ...
> bio->bi_bdev = (struct block_device *)
> (unsigned long)bbio->mirror_num;
> ...
> }
>
> static void btrfs_end_bio(struct bio *bio, int err)
> {
> ...
> bio->bi_bdev = (struct block_device *)
> (unsigned long)bbio->mirror_num;
>
> ...
> }
>
> In fs/btrfs/extent_io.c
>
> static void end_bio_extent_readpage(struct bio *bio, int err)
> {
> int mirror;
> ...
> mirror = (int)(unsigned long)bio->bi_bdev;
> ...
> }
>
> Ewweeeeeeeeeeeeeeeeeehh........
>
> No wonder this thing crashes. Chris, can't the original bio carry
> bbio in bi_private and let end_bio_extent_readpage() free the bbio
> instead of abusing bi_bdev like this?
Ugh, wtf.
Chris, time for a swim in the bay :-)
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7
2013-04-19 13:32 ` Jens Axboe
@ 2013-04-19 13:52 ` Chris Mason
0 siblings, 0 replies; 8+ messages in thread
From: Chris Mason @ 2013-04-19 13:52 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo
Cc: Wanlong Gao, Steven Rostedt, namhyung@gmail.com, agk@redhat.com,
dm-devel@redhat.com, neilb@suse.de, LKML, Linus Torvalds,
linux-btrfs@vger.kernel.org
Quoting Jens Axboe (2013-04-19 09:32:50)
> >
> > No wonder this thing crashes. Chris, can't the original bio carry
> > bbio in bi_private and let end_bio_extent_readpage() free the bbio
> > instead of abusing bi_bdev like this?
>
> Ugh, wtf.
>
> Chris, time for a swim in the bay :-)
Yeah, I can't really defend this one. We needed a space for an int and
I assumed end_io meant the FS was free to do horrible things.
Really though, I'll just take a quick dip in the lake and patch this out
of btrfs.
Jan is probably right about changing around our endio callbacks to
explicitly pass the mirror, it should be less complex and cleaner.
Many thanks to everyone here that tracked it down.
-chris
^ permalink raw reply [flat|nested] 8+ messages in thread