Re: [PATCH] bounce:fix bug, avoid to flush dcache on slab pagefrom jbd2.

[Jan Kara]

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?

								Honza
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/