Re: [BUG REPORT] Kernel panic on 3.9.0-rc7-4-gbb33db7

[Jan Schmidt <list.lkml@jan-o-sch.net>]

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