On Wed, Mar 13, 2013 at 09:50:21AM +0100, Jan Kara wrote: > On Tue 12-03-13 18:10:20, Darrick J. Wong wrote: > > On Tue, Mar 12, 2013 at 03:32:21PM -0700, Andrew Morton wrote: > > > On Fri, 08 Mar 2013 20:37:36 +0800 Shuge <shugelinux@xxxxxxxxx> wrote: > > > > > > > The bounce accept slab pages from jbd2, and flush dcache on them. > > > > When enabling VM_DEBUG, it will tigger VM_BUG_ON in page_mapping(). > > > > So, check PageSlab to avoid it in __blk_queue_bounce(). > > > > > > > > Bug URL: http://lkml.org/lkml/2013/3/7/56 > > > > > > > > ... > > > > > > > > --- a/mm/bounce.c > > > > +++ b/mm/bounce.c > > > > @@ -214,7 +214,8 @@ static void __blk_queue_bounce(struct request_queue > > > > *q, struct bio **bio_orig, > > > > if (rw == WRITE) { > > > > char *vto, *vfrom; > > > > - flush_dcache_page(from->bv_page); > > > > + if (unlikely(!PageSlab(from->bv_page))) > > > > + flush_dcache_page(from->bv_page); > > > > vto = page_address(to->bv_page) + to->bv_offset; > > > > vfrom = kmap(from->bv_page) + from->bv_offset; > > > > memcpy(vto, vfrom, to->bv_len); > > > > > > I guess this is triggered by Catalin's f1a0c4aa0937975b ("arm64: Cache > > > maintenance routines"), which added a page_mapping() call to arm64's > > > arch/arm64/mm/flush.c:flush_dcache_page(). > > > > > > What's happening is that jbd2 is using kmalloc() to allocate buffer_head > > > data. That gets submitted down the BIO layer and __blk_queue_bounce() > > > calls flush_dcache_page() which in the arm64 case calls page_mapping() > > > and page_mapping() does VM_BUG_ON(PageSlab) and splat. > > > > > > The unusual thing about all of this is that the payload for some disk > > > IO is coming from kmalloc, rather than being a user page. It's oddball > > > but we've done this for ages and should continue to support it. > > > > > > > > > Now, the page from kmalloc() cannot be in highmem, so why did the > > > bounce code decide to bounce it? > > > > > > __blk_queue_bounce() does > > > > > > /* > > > * is destination page below bounce pfn? > > > */ > > > if (page_to_pfn(page) <= queue_bounce_pfn(q) && !force) > > > continue; > > > > > > and `force' comes from must_snapshot_stable_pages(). But > > > must_snapshot_stable_pages() must have returned false, because if it > > > had returned true then it would have been must_snapshot_stable_pages() > > > which went BUG, because must_snapshot_stable_pages() calls page_mapping(). > > > > > > So my tentative diagosis is that arm64 is fishy. A page which was > > > allocated via jbd2_alloc(GFP_NOFS)->kmem_cache_alloc() ended up being > > > above arm64's queue_bounce_pfn(). Can you please do a bit of > > > investigation to work out if this is what is happening? Find out why > > > __blk_queue_bounce() decided to bounce a page which shouldn't have been > > > bounced? > > > > That sure is strange. I didn't see any obvious reasons why we'd end up with a > > kmalloc above queue_bounce_pfn(). But then I don't have any arm64s either. > > > > > This is all terribly fragile :( afaict if someone sets > > > bdi_cap_stable_pages_required() against that jbd2 queue, we're going to > > > hit that BUG_ON() again, via must_snapshot_stable_pages()'s > > > page_mapping() call. (Darrick, this means you ;)) > > > > Wheeee. You're right, we shouldn't be calling page_mapping on slab pages. > > We can keep walking the bio segments to find a non-slab page that can tell us > > MS_SNAP_STABLE is set, since we probably won't need the bounce buffer anyway. > > > > How does something like this look? (+ the patch above) > Umm, this won't quite work. We can have a bio which has just PageSlab > page attached and so you won't be able to get to the superblock. Heh, isn't > the whole page_mapping() thing in must_snapshot_stable_pages() wrong? When we > do direct IO, these pages come directly from userspace and hell knows where > they come from. Definitely their page_mapping() doesn't give us anything > useful... Sorry for not realizing this earlier when reviewing the patch. > > ... remembering why we need to get to sb and why ext3 needs this ... So > maybe a better solution would be to have a bio flag meaning that pages need > bouncing? And we would set it from filesystems that need it - in case of > ext3 only writeback of data from kjournald actually needs to bounce the > pages. Thoughts? What about dirty pages that don't result in journal transactions? I think ext3_sync_file() eventually calls ext3_ordered_writepage, which then calls __block_write_full_page, which in turn calls submit_bh(). --D > > Honza > -- > Jan Kara <jack@xxxxxxx> > SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
