swapper_space export

swapper_space export

[Josef Sipek]

I was trying to compile the Unionfs[1] to get it up to sync it up with
the kernel developments from the past few months. Anyway, long story
short...swapper_space (defined in mm/swap_state.c) is not exported
anymore (git commit: 4936967374c1ad0eb3b734f24875e2484c3786cc). This
apparently is not an issue for most modules. Troubles arise when the
modules include mm.h or any of its relatives.

One simply gets a linker error about swapper_space not being defined.
The problem is that it is used in mm.h.

I included a reverse patch to export the symbol again.

Josef "Jeff" Sipek.

[1] http://unionfs.filesystems.org


Export swapper_space because several include files reference it.

Signed-off-by: Josef Sipek <jsipek@cs.sunysb.edu>

--- a/mm/swap_state.c.orig	2006-05-16 18:23:38.000000000 -0400
+++ b/mm/swap_state.c		2006-05-16 18:22:57.000000000 -0400
@@ -43,6 +43,7 @@
 	.i_mmap_nonlinear = LIST_HEAD_INIT(swapper_space.i_mmap_nonlinear),
 	.backing_dev_info = &swap_backing_dev_info,
 };
+EXPORT_SYMBOL(swapper_space);
 
 #define INC_CACHE_INFO(x)	do { swap_cache_info.x++; } while (0)
 

Re: swapper_space export

[Adrian Bunk]

On Tue, May 16, 2006 at 07:24:43PM -0400, Josef Sipek wrote:
> I was trying to compile the Unionfs[1] to get it up to sync it up with
> the kernel developments from the past few months. Anyway, long story
> short...swapper_space (defined in mm/swap_state.c) is not exported
> anymore (git commit: 4936967374c1ad0eb3b734f24875e2484c3786cc). This
> apparently is not an issue for most modules. Troubles arise when the
> modules include mm.h or any of its relatives.
> 
> One simply gets a linker error about swapper_space not being defined.
> The problem is that it is used in mm.h.
> 
> I included a reverse patch to export the symbol again.

Can we discuss this patch when Unionfs gets submitted for inclusion into 
the kernel?

It's in fact a good thing if you and other people regularly notice that 
external modules do not longer work here or there since it creates 
pressure towards getting external modules submitted for inclusion into 
the kernel.

> Josef "Jeff" Sipek.
>...

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: swapper_space export

[Arjan van de Ven]

On Tue, 2006-05-16 at 19:24 -0400, Josef Sipek wrote:
> I was trying to compile the Unionfs[1] to get it up to sync it up with
> the kernel developments from the past few months. Anyway, long story
> short...swapper_space (defined in mm/swap_state.c) is not exported
> anymore (git commit: 4936967374c1ad0eb3b734f24875e2484c3786cc). This
> apparently is not an issue for most modules. Troubles arise when the
> modules include mm.h or any of its relatives.
> 
> One simply gets a linker error about swapper_space not being defined.
> The problem is that it is used in mm.h.


don't you think it's really suspect that no other filesystem, in fact no
other driver in the kernel needs this? Could it just be that unionfs is 
using a wrong API ? Because if that's the case you're patch is just the
wrong thing. Maybe the unionfs people should try to submit their code
for review etc......

Re: swapper_space export

[Andrew Morton]

Arjan van de Ven <arjan@infradead.org> wrote:
>
> On Tue, 2006-05-16 at 19:24 -0400, Josef Sipek wrote:
> > I was trying to compile the Unionfs[1] to get it up to sync it up with
> > the kernel developments from the past few months. Anyway, long story
> > short...swapper_space (defined in mm/swap_state.c) is not exported
> > anymore (git commit: 4936967374c1ad0eb3b734f24875e2484c3786cc). This
> > apparently is not an issue for most modules. Troubles arise when the
> > modules include mm.h or any of its relatives.
> > 
> > One simply gets a linker error about swapper_space not being defined.
> > The problem is that it is used in mm.h.
> 
> 
> don't you think it's really suspect that no other filesystem, in fact no
> other driver in the kernel needs this? Could it just be that unionfs is 
> using a wrong API ? Because if that's the case you're patch is just the
> wrong thing. Maybe the unionfs people should try to submit their code
> for review etc......

They're probably just using page_mapping(), which is a reasonable thing to so.

Probably it's sufficient to use page->mapping.  It depends how they got
hold of the page.  If it's know to be in filesystem pagecache then
page->mapping will be OK.

Re: swapper_space export

[Hugh Dickins]

On Wed, 17 May 2006, Arjan van de Ven wrote:
> On Tue, 2006-05-16 at 19:24 -0400, Josef Sipek wrote:
> > I was trying to compile the Unionfs[1] to get it up to sync it up with
> > the kernel developments from the past few months. Anyway, long story
> > short...swapper_space (defined in mm/swap_state.c) is not exported
> > anymore (git commit: 4936967374c1ad0eb3b734f24875e2484c3786cc). This
> > apparently is not an issue for most modules. Troubles arise when the
> > modules include mm.h or any of its relatives.
> > 
> > One simply gets a linker error about swapper_space not being defined.
> > The problem is that it is used in mm.h.
> 
> don't you think it's really suspect that no other filesystem, in fact no
> other driver in the kernel needs this? Could it just be that unionfs is 
> using a wrong API ? Because if that's the case you're patch is just the
> wrong thing. Maybe the unionfs people should try to submit their code
> for review etc......

Much as I'd love to side with Jeff against you and Adrian ;)
I think you're right.

I see no reference to page_mapping() in the unionfs source (and at
present there's no other justifiable modular use of swapper_space);
but my guess would be that Jeff is being more conscientious than is
called for in getting it to sync up with the kernel -

The unionfs source does contain its own inline "sync_page" which
comments that it "is copied verbatim from mm/filemap.c".  I'm
guessing Jeff has noticed that it's no longer a verbatim copy,
has made it so, and is thereby involving page_mapping().

No need for that here (nor for the smp_mb nor for the io_schedule):
unionfs's sync_page is working on a locked pagecache page of the
lower-level filesystem, that's not going to be a PageSwapCache page
nor a PageAnon page (nor even a truncated page with NULL mapping:
page lock is held).  Just use page->mapping as before.

(But I notice that unionfs better not have a tmpfs in its union:
the unionfs use of grab_cache_page is not strictly compatible with
the way tmpfs pages are swapped out under memory pressure.)

Hugh

Re: swapper_space export

[Josef Sipek]

On Wed, May 17, 2006 at 04:52:36PM +0100, Hugh Dickins wrote:
> On Wed, 17 May 2006, Arjan van de Ven wrote:
> > On Tue, 2006-05-16 at 19:24 -0400, Josef Sipek wrote:
> > > I was trying to compile the Unionfs[1] to get it up to sync it up with
> > > the kernel developments from the past few months. Anyway, long story
> > > short...swapper_space (defined in mm/swap_state.c) is not exported
> > > anymore (git commit: 4936967374c1ad0eb3b734f24875e2484c3786cc). This
> > > apparently is not an issue for most modules. Troubles arise when the
> > > modules include mm.h or any of its relatives.
> > > 
> > > One simply gets a linker error about swapper_space not being defined.
> > > The problem is that it is used in mm.h.
> > 
> > don't you think it's really suspect that no other filesystem, in fact no
> > other driver in the kernel needs this? Could it just be that unionfs is 
> > using a wrong API ? Because if that's the case you're patch is just the
> > wrong thing. Maybe the unionfs people should try to submit their code
> > for review etc......
>
> I see no reference to page_mapping() in the unionfs source (and at
> present there's no other justifiable modular use of swapper_space);
> but my guess would be that Jeff is being more conscientious than is
> called for in getting it to sync up with the kernel -

That's what happens when one is trying to get ready to send the code to
linux-kernel & fs-devel :)

> The unionfs source does contain its own inline "sync_page" which
> comments that it "is copied verbatim from mm/filemap.c".  I'm
> guessing Jeff has noticed that it's no longer a verbatim copy,
> has made it so, and is thereby involving page_mapping().

Good guess, exactly what happened.

> Just use page->mapping as before.

Thanks for the advice.

> (But I notice that unionfs better not have a tmpfs in its union:
> the unionfs use of grab_cache_page is not strictly compatible with
> the way tmpfs pages are swapped out under memory pressure.)

We have some tmpfs nastiness reported in the bugzilla - I guess this is
a good starting point - thanks!

Thanks all,
Josef "Jeff" Sipek.