Re: [PATCH] fbdefio: add set_page_dirty handler to deferred IO FB

[Ian Campbell]

On Mon, 2008-08-18 at 23:38 -0700, Andrew Morton wrote:
> On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell <ijc@xxxxxxxxxxxxxx> wrote:
> 
> > Fixes kernel BUG at lib/radix-tree.c:473.
> > 
> > Previously the handler was incidentally provided by tmpfs but this was
> > removed with:
> > 
> >   commit 14fcc23fdc78e9d32372553ccf21758a9bd56fa1
> >   Author: Hugh Dickins <hugh@xxxxxxxxxxx>
> >   Date:   Mon Jul 28 15:46:19 2008 -0700
> > 
> >     tmpfs: fix kernel BUG in shmem_delete_inode
> > 
> > relying on this behaviour was incorrect in any case and the BUG
> > also appeared when the device node was on an ext3 filesystem.
> > 
> > Signed-off-by: Ian Campbell <ijc@xxxxxxxxxxxxxx>
> > Acked-by: Jaya Kumar <jayakumar.lkml@xxxxxxxxx>
> > Acked-by: Nick Piggin <npiggin@xxxxxxx>
> > Acked-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> > Cc: Jaya Kumar <jayakumar.lkml@xxxxxxxxx>
> > Cc: Nick Piggin <npiggin@xxxxxxx>
> > Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> > Cc: Hugh Dickins <hugh@xxxxxxxxxxx>
> > Cc: Johannes Weiner <hannes@xxxxxxxxxxxx>
> > Cc: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
> > Cc: Kel Modderman <kel@xxxxxxxxxx>
> > Cc: Markus Armbruster <armbru@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx [14fcc23fd is in 2.6.25.14 and 2.6.26.1]
> > ---
> >  drivers/video/fb_defio.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
> > index 59df132..214bb7c 100644
> > --- a/drivers/video/fb_defio.c
> > +++ b/drivers/video/fb_defio.c
> > @@ -114,11 +114,23 @@ static struct vm_operations_struct fb_deferred_io_vm_ops = {
> >  	.page_mkwrite	= fb_deferred_io_mkwrite,
> >  };
> >  
> > +static int fb_deferred_io_set_page_dirty(struct page *page)
> > +{
> > +	if (!PageDirty(page))
> > +		SetPageDirty(page);
> > +	return 0;
> > +}
> 
> <searches, finds the thread.  "kernel BUG at lib/radix-tree.c:473!">

Sorry, I had an unsent reply to self pointing to
http://marc.info/?l=linux-kernel&m=121869811531858

> Is there actually any benefit in setting these pages dirty?  Or should
> this be an empty function?  I see claims in the above thread that this
> driver uses PG_dirty for optimising writeback but I can't immediately
> locate any code which actually does that.

I'm not really that familiar with this driver, but me neither. It seems
to actually use a list constructed at fault time (in io_mkwrite).

> > +static const struct address_space_operations fb_deferred_io_aops = {
> > +	.set_page_dirty = fb_deferred_io_set_page_dirty,
> > +};
> > +
> >  static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> >  {
> >  	vma->vm_ops = &fb_deferred_io_vm_ops;
> >  	vma->vm_flags |= ( VM_IO | VM_RESERVED | VM_DONTEXPAND );
> >  	vma->vm_private_data = info;
> > +	vma->vm_file->f_mapping->a_ops = &fb_deferred_io_aops;
> >  	return 0;
> >  }
> 
> Seems a bit odd to rewrite the address_space_operations at mmap()-time.
> A filesystem will usually do this on the inode when it is first being
> set up, when no other thread of control can be looking at it.
> 
> 	grep 'if .*[-]>a_ops' */*.c
> 
> and you'll see that lots of code assumes that a_ops doesn't get
> swizzled around (to contain a bunch of NULL pointers!) under its feet. 
> Maybe none of those code paths are applicable to the /dev/fb0 inode,
> but it would be painful to work out which paths _are_ applicable and to
> verify that they're all safe wrt a_ops getting whisked away.
> 
> Rewriting page->mapping within the vm_operations_struct.fault handler
> seems a bit suspect for similar reasons.
> 
> I suspect that we just shouldn't be pretending that this is a regular
> anon/pagecache page to this extent.

Quite possibly, as I say I'm not really familiar with this code, I just
want my framebuffer to work again ;-)

Perhaps applying the band-aid at open time instead would be preferred?
Reworking the guts of this thing doesn't sound like the best option in
an rc4 timeframe and reverting 14fcc23fdc doesn't seem wise either.

FWIW the 2.6.25/2.6.26 stable branches also have 14fcc23fdc in them now
too.

>   Maybe a suitable fix would be to
> teach fb_deferred_io_fault() to instantiate the pte itself
> (vm_insert_page()) and then return VM_FAULT_NOPAGE?

I had a stab at it but naively translating your paragraph into code
didn't work (no change in symptoms), which is hardly surprising since I
haven't had a chance to really examine what's (supposed to be) going
on...

Ian.

> I assume there's a reason why we aren't VM_IO/VM_RESERVED/PG_reserved in
> here.
> 
> 
-- 
Ian Campbell

Who made the world I cannot tell;
'Tis made, and here am I in hell.
My hand, though now my knuckles bleed,
I never soiled with such a deed.
		-- A. E. Housman

Attachment: signature.asc
Description: This is a digitally signed message part