(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/0xe0Ugh.... 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? -- tejun
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
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. >
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
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
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
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
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
Apparently Analagous Threads
- [PATCH 0/3] Btrfs: add IO error device stats
- [PATCH v5 0/3] Btrfs: add IO error device stats
- [PATCH 00/11] Btrfs: some patches for 3.3
- [PATCH] Btrfs: fix crash in scrub repair code when device is missing
- Re: [Bugme-new] [Bug 29302] New: Null pointer dereference with large max_sectors_kb