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

This is a discussion on [PATCH] fbdefio: add set_page_dirty handler to deferred IO FB - Kernel ; 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 Date: Mon Jul 28 15:46:19 2008 -0700 tmpfs: fix kernel BUG in shmem_delete_inode relying on this behaviour ...

+ Reply to Thread
Results 1 to 13 of 13

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

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

    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
    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
    Acked-by: Jaya Kumar
    Acked-by: Nick Piggin
    Acked-by: Peter Zijlstra
    Cc: Jaya Kumar
    Cc: Nick Piggin
    Cc: Peter Zijlstra
    Cc: Hugh Dickins
    Cc: Johannes Weiner
    Cc: Jeremy Fitzhardinge
    Cc: Kel Modderman
    Cc: Markus Armbruster
    Cc: stable@vger.kernel.org [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;
    +}
    +
    +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;
    }

    --
    1.5.6.3

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell 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
    > 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
    > Acked-by: Jaya Kumar
    > Acked-by: Nick Piggin
    > Acked-by: Peter Zijlstra
    > Cc: Jaya Kumar
    > Cc: Nick Piggin
    > Cc: Peter Zijlstra
    > Cc: Hugh Dickins
    > Cc: Johannes Weiner
    > Cc: Jeremy Fitzhardinge
    > Cc: Kel Modderman
    > Cc: Markus Armbruster
    > Cc: stable@vger.kernel.org [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;
    > +}




    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.

    > +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. 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 assume there's a reason why we aren't VM_IO/VM_RESERVED/PG_reserved in
    here.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon, 2008-08-18 at 23:38 -0700, Andrew Morton wrote:
    > On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell 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
    > > 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
    > > Acked-by: Jaya Kumar
    > > Acked-by: Nick Piggin
    > > Acked-by: Peter Zijlstra
    > > Cc: Jaya Kumar
    > > Cc: Nick Piggin
    > > Cc: Peter Zijlstra
    > > Cc: Hugh Dickins
    > > Cc: Johannes Weiner
    > > Cc: Jeremy Fitzhardinge
    > > Cc: Kel Modderman
    > > Cc: Markus Armbruster
    > > Cc: stable@vger.kernel.org [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;
    > > +}

    >
    >
    >
    > 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.
    >
    > > +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. 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 assume there's a reason why we aren't VM_IO/VM_RESERVED/PG_reserved in
    > here.


    IIRC there is the requirement to mmap the framebuffer from multiple
    processes, so we need a shared mapping - this can be done using
    vm_insertpage() if I understand the thing correctly.

    We also need the fault on write, vma_want_writenotify() will generally
    fail on VM_INSERTPAGE pages, except when you set a page_mkwrite()
    handler, so here we're good too.

    However, we also need page_mkclean() to work, and that requires rmap -
    which if memory serves me, vm_insertpage() does not do.



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon, 2008-08-18 at 23:38 -0700, Andrew Morton wrote:
    > On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell 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
    > > 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
    > > Acked-by: Jaya Kumar
    > > Acked-by: Nick Piggin
    > > Acked-by: Peter Zijlstra
    > > Cc: Jaya Kumar
    > > Cc: Nick Piggin
    > > Cc: Peter Zijlstra
    > > Cc: Hugh Dickins
    > > Cc: Johannes Weiner
    > > Cc: Jeremy Fitzhardinge
    > > Cc: Kel Modderman
    > > Cc: Markus Armbruster
    > > Cc: stable@vger.kernel.org [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;
    > > +}

    >
    >


    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

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (GNU/Linux)

    iEYEABECAAYFAkir0iMACgkQM0+0qS9rzVkgnACg0zygBPLKFt oMOlKcTR0L/vHV
    Jw4An2jDxbsCXAgwk1YM8DrRo9s3nDuY
    =GfcL
    -----END PGP SIGNATURE-----


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

    On Wed, 20 Aug 2008 09:13:23 +0100 Ian Campbell wrote:

    > Perhaps applying the band-aid at open time instead would be preferred?


    That would be less racy, I expect. Implement fb_ops.fb_open() within
    drivers/video/fb_defio.c and do the address_space_operations overwrite
    there. Hopefully that will ensure that the address_space_operations
    instance is stable before anyone uses it for anything serious.

    It'd be better to hook in at inode creation time but afaict that's not
    available for the /dev/fb0 node.



    OK, seems that fb_ops.fb_open() has no way of getting at the `struct
    file *' which is being opened (wtf?). Screwed. Need to change
    fb_ops.fb_open(), or add a new fb_ops.fb_open_sane().




    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Tue, Aug 19, 2008 at 2:38 AM, Andrew Morton
    wrote:
    > On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell wrote:
    >
    >> +static int fb_deferred_io_set_page_dirty(struct page *page)
    >> +{
    >> + if (!PageDirty(page))
    >> + SetPageDirty(page);
    >> + return 0;
    >> +}

    >
    >
    >
    > 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.


    Hi Andrew,

    I hope I have understood your question. You are right that PG_dirty
    isn't used directly in defio. The defio portion does use each
    page_mkwrite callback to build a list of the pages of the framebuffer
    that were written to and then passes that list to pvfb (in this case).
    pvfb then optimizes writeback by interpreting that list according to
    its framebuffer and sending it to its actual destination. I think
    Markus's code in xenfb_deferred_io() and xenfb_send* is doing the
    latter.

    >
    > I suspect that we just shouldn't be pretending that this is a regular
    > anon/pagecache page to this extent. 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?
    >


    Ok, I haven't understood all the details of vm_insert_page() yet. I
    noticed Peter's message that this won't work with page_mkclean (which
    I need to do so that we can re-receive fault notification after the
    driver has done writeback). I see your remark and Ian's that it would
    be better to try to do the mapping and a_ops setup at open time. I
    think I have some ideas of how to do this without breaking fb code
    much. I will try to work on this issue this weekend. I'm sorry that
    I'm so slow.

    Thanks,
    jaya
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    "Jaya Kumar" writes:

    > On Tue, Aug 19, 2008 at 2:38 AM, Andrew Morton
    > wrote:
    >> On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell wrote:
    >>
    >>> +static int fb_deferred_io_set_page_dirty(struct page *page)
    >>> +{
    >>> + if (!PageDirty(page))
    >>> + SetPageDirty(page);
    >>> + return 0;
    >>> +}

    >>
    >>
    >>
    >> 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.

    >
    > Hi Andrew,
    >
    > I hope I have understood your question. You are right that PG_dirty
    > isn't used directly in defio. The defio portion does use each
    > page_mkwrite callback to build a list of the pages of the framebuffer
    > that were written to and then passes that list to pvfb (in this case).
    > pvfb then optimizes writeback by interpreting that list according to
    > its framebuffer and sending it to its actual destination. I think
    > Markus's code in xenfb_deferred_io() and xenfb_send* is doing the
    > latter.


    Exactly. xenfb_deferred_io() is the callback that receives the list
    of pages that have been dirtied from fb_deferred_io_work(). It
    computes a rectangle covering all these pages, and passes it to
    xenfb_refresh(). Which takes care of sending it to the backend.

    [...]
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Jaya Kumar wrote:
    > I hope I have understood your question. You are right that PG_dirty
    > isn't used directly in defio. The defio portion does use each
    > page_mkwrite callback to build a list of the pages of the framebuffer
    > that were written to and then passes that list to pvfb (in this case).
    > pvfb then optimizes writeback by interpreting that list according to
    > its framebuffer and sending it to its actual destination. I think
    > Markus's code in xenfb_deferred_io() and xenfb_send* is doing the
    > latter.
    >


    The original Xen-specific implementation of this did use PG_dirty, which
    has the nice property of using the hardware feature without invoking any
    pagefaults. It would be nice if defio could be extended to allow this
    on platforms where it makes sense.

    J
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    [correcting stable@]

    On Wed, 2008-08-20 at 01:37 -0700, Andrew Morton wrote:
    > On Wed, 20 Aug 2008 09:13:23 +0100 Ian Campbell wrote:
    >
    > > Perhaps applying the band-aid at open time instead would be preferred?

    >
    > That would be less racy, I expect.

    [...]
    >
    >
    > OK, seems that fb_ops.fb_open() has no way of getting at the `struct
    > file *' which is being opened (wtf?). Screwed. Need to change
    > fb_ops.fb_open(), or add a new fb_ops.fb_open_sane().


    Ah yes, I remember why I did it in mmap() now...

    How about this version? Not as clean as overriding fb_open() but
    involves less frobbing with unrelated drivers.

    From ae2f7f118518fbfd4006c985b136a5d3d1a314af Mon Sep 17 00:00:00 2001
    From: Ian Campbell
    Date: Wed, 20 Aug 2008 19:54:50 +0100
    Subject: [PATCH] fbdefio: add set_page_dirty handler to deferred IO FB

    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
    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.

    v2: override a_ops at open() time rather than mmap() time to minimise
    races per AKPM's concerns.

    Signed-off-by: Ian Campbell
    Acked-by: Jaya Kumar
    Acked-by: Nick Piggin
    Acked-by: Peter Zijlstra
    Cc: Jaya Kumar
    Cc: Nick Piggin
    Cc: Peter Zijlstra
    Cc: Hugh Dickins
    Cc: Johannes Weiner
    Cc: Jeremy Fitzhardinge
    Cc: Kel Modderman
    Cc: Markus Armbruster
    Cc: stable@kernel.org [14fcc23fd is in 2.6.25.14 and 2.6.26.1]
    ---
    drivers/video/fb_defio.c | 19 +++++++++++++++++++
    drivers/video/fbmem.c | 4 ++++
    include/linux/fb.h | 3 +++
    3 files changed, 26 insertions(+), 0 deletions(-)

    diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
    index 59df132..4835bdc 100644
    --- a/drivers/video/fb_defio.c
    +++ b/drivers/video/fb_defio.c
    @@ -114,6 +114,17 @@ 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;
    +}
    +
    +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;
    @@ -163,6 +174,14 @@ void fb_deferred_io_init(struct fb_info *info)
    }
    EXPORT_SYMBOL_GPL(fb_deferred_io_init);

    +void fb_deferred_io_open(struct fb_info *info,
    + struct inode *inode,
    + struct file *file)
    +{
    + file->f_mapping->a_ops = &fb_deferred_io_aops;
    +}
    +EXPORT_SYMBOL_GPL(fb_deferred_io_open);
    +
    void fb_deferred_io_cleanup(struct fb_info *info)
    {
    void *screen_base = (void __force *) info->screen_base;
    diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
    index 6b48780..98843c2 100644
    --- a/drivers/video/fbmem.c
    +++ b/drivers/video/fbmem.c
    @@ -1344,6 +1344,10 @@ fb_open(struct inode *inode, struct file *file)
    if (res)
    module_put(info->fbops->owner);
    }
    +#ifdef CONFIG_FB_DEFERRED_IO
    + if (info->fbdefio)
    + fb_deferred_io_open(info, inode, file);
    +#endif
    out:
    unlock_kernel();
    return res;
    diff --git a/include/linux/fb.h b/include/linux/fb.h
    index 3b8870e..531ccd5 100644
    --- a/include/linux/fb.h
    +++ b/include/linux/fb.h
    @@ -976,6 +976,9 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32d_pitch,

    /* drivers/video/fb_defio.c */
    extern void fb_deferred_io_init(struct fb_info *info);
    +extern void fb_deferred_io_open(struct fb_info *info,
    + struct inode *inode,
    + struct file *file);
    extern void fb_deferred_io_cleanup(struct fb_info *info);
    extern int fb_deferred_io_fsync(struct file *file, struct dentry *dentry,
    int datasync);
    --
    1.5.6.3




    --
    Ian Campbell

    Hope that the day after you die is a nice day.

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (GNU/Linux)

    iEYEABECAAYFAkisaTAACgkQM0+0qS9rzVmWEQCgsya+jPMH9k ERCHGZczRsJxjS
    KO4AoKiSf1ugeh+x55QjKysJicopWEjM
    =4l8B
    -----END PGP SIGNATURE-----


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

    On Wed, 20 Aug 2008 19:57:53 +0100
    Ian Campbell wrote:

    > [correcting stable@]
    >
    > On Wed, 2008-08-20 at 01:37 -0700, Andrew Morton wrote:
    > > On Wed, 20 Aug 2008 09:13:23 +0100 Ian Campbell wrote:
    > >
    > > > Perhaps applying the band-aid at open time instead would be preferred?

    > >
    > > That would be less racy, I expect.

    > [...]
    > >
    > >
    > > OK, seems that fb_ops.fb_open() has no way of getting at the `struct
    > > file *' which is being opened (wtf?). Screwed. Need to change
    > > fb_ops.fb_open(), or add a new fb_ops.fb_open_sane().

    >
    > Ah yes, I remember why I did it in mmap() now...
    >
    > How about this version? Not as clean as overriding fb_open() but
    > involves less frobbing with unrelated drivers.
    >
    > From ae2f7f118518fbfd4006c985b136a5d3d1a314af Mon Sep 17 00:00:00 2001
    > From: Ian Campbell
    > Date: Wed, 20 Aug 2008 19:54:50 +0100
    > Subject: [PATCH] fbdefio: add set_page_dirty handler to deferred IO FB
    >
    > 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
    > 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.
    >
    > v2: override a_ops at open() time rather than mmap() time to minimise
    > races per AKPM's concerns.
    >
    > ...
    >
    > --- a/drivers/video/fbmem.c
    > +++ b/drivers/video/fbmem.c
    > @@ -1344,6 +1344,10 @@ fb_open(struct inode *inode, struct file *file)
    > if (res)
    > module_put(info->fbops->owner);
    > }
    > +#ifdef CONFIG_FB_DEFERRED_IO
    > + if (info->fbdefio)
    > + fb_deferred_io_open(info, inode, file);
    > +#endif


    eww, hacky, but drivers/video/fbmem.c already got hacky:

    #ifdef CONFIG_FB_DEFERRED_IO
    .fsync = fb_deferred_io_fsync,
    #endif

    so it's not an original sin.

    Does it work?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Wed, 2008-08-20 at 12:30 -0700, Andrew Morton wrote:
    > On Wed, 20 Aug 2008 19:57:53 +0100 Ian Campbell
    > wrote:
    > }
    > > +#ifdef CONFIG_FB_DEFERRED_IO
    > > + if (info->fbdefio)
    > > + fb_deferred_io_open(info, inode, file);
    > > +#endif

    >
    > eww, hacky, but drivers/video/fbmem.c already got hacky:
    >
    > #ifdef CONFIG_FB_DEFERRED_IO
    > .fsync = fb_deferred_io_fsync,
    > #endif
    >
    > so it's not an original sin.


    That's what I figured.

    > Does it work?


    Yep.

    Ian.
    --
    Ian Campbell

    Nobody knows what goes between his cold toes and his warm ears.
    -- Roy Harper

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (GNU/Linux)

    iEYEABECAAYFAkisc0YACgkQM0+0qS9rzVl13QCfYVR1yXFoQj HOPfvaUmJRn9RV
    ev8AoNyZhImYmVIINbZ8uFw226/8+5hC
    =v/nI
    -----END PGP SIGNATURE-----


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

    On Wed, 20 Aug 2008 20:40:54 +0100
    Ian Campbell wrote:

    > On Wed, 2008-08-20 at 12:30 -0700, Andrew Morton wrote:
    > > On Wed, 20 Aug 2008 19:57:53 +0100 Ian Campbell
    > > wrote:
    > > }
    > > > +#ifdef CONFIG_FB_DEFERRED_IO
    > > > + if (info->fbdefio)
    > > > + fb_deferred_io_open(info, inode, file);
    > > > +#endif

    > >
    > > eww, hacky, but drivers/video/fbmem.c already got hacky:
    > >
    > > #ifdef CONFIG_FB_DEFERRED_IO
    > > .fsync = fb_deferred_io_fsync,
    > > #endif
    > >
    > > so it's not an original sin.

    >
    > That's what I figured.


    A better implementation would be to change the fb_ops.fb_open()
    arguments, or to add fb_ops.fb_open2() with the file*.

    Also, that .fsync thing should be done properly via a new fb_ops.fb_fsync().

    But neither are pressing issues and I guess can be left for when Jaya
    is feeling bored?

    > > Does it work?

    >
    > Yep.


    OK, thanks, I guess we're done with this for now.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Wed, Aug 20, 2008 at 3:50 PM, Andrew Morton
    wrote:
    > On Wed, 20 Aug 2008 20:40:54 +0100
    > Ian Campbell wrote:
    >
    >> On Wed, 2008-08-20 at 12:30 -0700, Andrew Morton wrote:
    >> > On Wed, 20 Aug 2008 19:57:53 +0100 Ian Campbell
    >> > wrote:
    >> > }
    >> > > +#ifdef CONFIG_FB_DEFERRED_IO
    >> > > + if (info->fbdefio)
    >> > > + fb_deferred_io_open(info, inode, file);
    >> > > +#endif
    >> >
    >> > eww, hacky, but drivers/video/fbmem.c already got hacky:
    >> >
    >> > #ifdef CONFIG_FB_DEFERRED_IO
    >> > .fsync = fb_deferred_io_fsync,
    >> > #endif
    >> >
    >> > so it's not an original sin.

    >>
    >> That's what I figured.

    >
    > A better implementation would be to change the fb_ops.fb_open()
    > arguments, or to add fb_ops.fb_open2() with the file*.
    >
    > Also, that .fsync thing should be done properly via a new fb_ops.fb_fsync().
    >
    > But neither are pressing issues and I guess can be left for when Jaya
    > is feeling bored?



    hehe, i haven't bene bored ever since i started playing with linux. :-)
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread