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

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

[Ian Campbell]

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@veritas.com>
  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@hellion.org.uk>
Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
Acked-by: Nick Piggin <npiggin@suse.de>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jaya Kumar <jayakumar.lkml@gmail.com>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Hugh Dickins <hugh@veritas.com>
Cc: Johannes Weiner <hannes@saeurebad.de>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Kel Modderman <kel@otaku42.de>
Cc: Markus Armbruster <armbru@redhat.com>
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

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

[Andrew Morton]

On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell <ijc@hellion.org.uk> 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@veritas.com>
>   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@hellion.org.uk>
> Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> Acked-by: Nick Piggin <npiggin@suse.de>
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Jaya Kumar <jayakumar.lkml@gmail.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Hugh Dickins <hugh@veritas.com>
> Cc: Johannes Weiner <hannes@saeurebad.de>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Kel Modderman <kel@otaku42.de>
> Cc: Markus Armbruster <armbru@redhat.com>
> 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;
> +}

<searches, finds the thread.  "kernel BUG at lib/radix-tree.c:473!">

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.

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

[Peter Zijlstra]

On Mon, 2008-08-18 at 23:38 -0700, Andrew Morton wrote:
> On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell <ijc@hellion.org.uk> 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@veritas.com>
> >   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@hellion.org.uk>
> > Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> > Acked-by: Nick Piggin <npiggin@suse.de>
> > Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Jaya Kumar <jayakumar.lkml@gmail.com>
> > Cc: Nick Piggin <npiggin@suse.de>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Hugh Dickins <hugh@veritas.com>
> > Cc: Johannes Weiner <hannes@saeurebad.de>
> > Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> > Cc: Kel Modderman <kel@otaku42.de>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > 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;
> > +}
> 
> <searches, finds the thread.  "kernel BUG at lib/radix-tree.c:473!">
> 
> 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.

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

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 4882 bytes --]

On Mon, 2008-08-18 at 23:38 -0700, Andrew Morton wrote:
> On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell <ijc@hellion.org.uk> 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@veritas.com>
> >   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@hellion.org.uk>
> > Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> > Acked-by: Nick Piggin <npiggin@suse.de>
> > Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Jaya Kumar <jayakumar.lkml@gmail.com>
> > Cc: Nick Piggin <npiggin@suse.de>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Hugh Dickins <hugh@veritas.com>
> > Cc: Johannes Weiner <hannes@saeurebad.de>
> > Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> > Cc: Kel Modderman <kel@otaku42.de>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > 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;
> > +}
> 
> <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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Andrew Morton]

On Wed, 20 Aug 2008 09:13:23 +0100 Ian Campbell <ijc@hellion.org.uk> 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.

<tries to write a patch>

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

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

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 4460 bytes --]

[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 <ijc@hellion.org.uk> wrote:
> 
> > Perhaps applying the band-aid at open time instead would be preferred?
> 
> That would be less racy, I expect.
[...]
> <tries to write a patch>
> 
> 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 <ijc@hellion.org.uk>
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 <hugh@veritas.com>
  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 <ijc@hellion.org.uk>
Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
Acked-by: Nick Piggin <npiggin@suse.de>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jaya Kumar <jayakumar.lkml@gmail.com>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Hugh Dickins <hugh@veritas.com>
Cc: Johannes Weiner <hannes@saeurebad.de>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Kel Modderman <kel@otaku42.de>
Cc: Markus Armbruster <armbru@redhat.com>
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, u32 d_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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Andrew Morton]

On Wed, 20 Aug 2008 19:57:53 +0100
Ian Campbell <ijc@hellion.org.uk> 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 <ijc@hellion.org.uk> wrote:
> > 
> > > Perhaps applying the band-aid at open time instead would be preferred?
> > 
> > That would be less racy, I expect.
> [...]
> > <tries to write a patch>
> > 
> > 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 <ijc@hellion.org.uk>
> 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 <hugh@veritas.com>
>   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?

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

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Wed, 2008-08-20 at 12:30 -0700, Andrew Morton wrote:
> On Wed, 20 Aug 2008 19:57:53 +0100 Ian Campbell <ijc@hellion.org.uk>
> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Andrew Morton]

On Wed, 20 Aug 2008 20:40:54 +0100
Ian Campbell <ijc@hellion.org.uk> wrote:

> On Wed, 2008-08-20 at 12:30 -0700, Andrew Morton wrote:
> > On Wed, 20 Aug 2008 19:57:53 +0100 Ian Campbell <ijc@hellion.org.uk>
> > 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.

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

[Jaya Kumar]

On Wed, Aug 20, 2008 at 3:50 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 20 Aug 2008 20:40:54 +0100
> Ian Campbell <ijc@hellion.org.uk> wrote:
>
>> On Wed, 2008-08-20 at 12:30 -0700, Andrew Morton wrote:
>> > On Wed, 20 Aug 2008 19:57:53 +0100 Ian Campbell <ijc@hellion.org.uk>
>> > 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. :-)

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

[Jaya Kumar]

On Tue, Aug 19, 2008 at 2:38 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell <ijc@hellion.org.uk> wrote:
>
>> +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!">
>
> 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

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

[Markus Armbruster]

"Jaya Kumar" <jayakumar.lkml@gmail.com> writes:

> On Tue, Aug 19, 2008 at 2:38 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Tue, 19 Aug 2008 07:02:45 +0100 Ian Campbell <ijc@hellion.org.uk> wrote:
>>
>>> +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!">
>>
>> 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.

[...]

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

[Jeremy Fitzhardinge]

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