[PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

mips allmodconfig fails with

ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
undefined!

which is due to LUSTRE using copy_from_user_page which is not exported by any
architecture. Unfortunately, LUSTRE can only be built as module, so there is no
easy fix.

MIPS, SH, and optionally XTENSA implement copy_from_user_page as unexported
functions. Disable LUSTRE for those.

Cc: Peng Tao <bergwolf@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/staging/lustre/lustre/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/Kconfig b/drivers/staging/lustre/lustre/Kconfig
index 4e898e4..07ce43d 100644
--- a/drivers/staging/lustre/lustre/Kconfig
+++ b/drivers/staging/lustre/lustre/Kconfig
@@ -1,6 +1,6 @@
 config LUSTRE_FS
 	tristate "Lustre file system client support"
-	depends on INET && m
+	depends on INET && m && !MIPS && !XTENSA && !SUPERH
 	select LNET
 	select CRYPTO
 	select CRYPTO_CRC32
-- 
1.7.9.7

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Greg Kroah-Hartman]

On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> mips allmodconfig fails with
> 
> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
> undefined!
> 
> which is due to LUSTRE using copy_from_user_page which is not exported by any
> architecture.

Any, or just these arches?

> Unfortunately, LUSTRE can only be built as module, so there is no
> easy fix.

Can't we just export the functions for those arches?  Surely lutre
isn't the first/only driver that needs this?

thanks,

greg k-h

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Ramkumar Ramachandra]

Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>> mips allmodconfig fails with
>>
>> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>> undefined!
>>
>> which is due to LUSTRE using copy_from_user_page which is not exported by any
>> architecture.
>
> Any, or just these arches?

Arches, dungeon masters.

>> Unfortunately, LUSTRE can only be built as module, so there is no
>> easy fix.
>
> Can't we just export the functions for those arches?  Surely lutre
> isn't the first/only driver that needs this?

It's simply necessary to keep up to date: we can't keep building new
arches since the initial cost can be quite high.

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>> mips allmodconfig fails with
>>
>> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>> undefined!
>>
>> which is due to LUSTRE using copy_from_user_page which is not exported by any
>> architecture.
>
> Any, or just these arches?
>
Other architectures implement it as define as far as I can see.

>> Unfortunately, LUSTRE can only be built as module, so there is no
>> easy fix.
>
> Can't we just export the functions for those arches?  Surely lutre
> isn't the first/only driver that needs this?
>
That would be another option.

Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:

https://lkml.org/lkml/2013/9/5/111

So please forget this patch. If sh/xtensa actually need it, we can do the same there.

Guenter

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

On 09/08/2013 07:31 PM, Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
>> On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
>>> On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>>>> mips allmodconfig fails with
>>>>
>>>> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>>>> undefined!
>>>>
>>>> which is due to LUSTRE using copy_from_user_page which is not exported by any
>>>> architecture.
>>>
>>> Any, or just these arches?
>>>
>> Other architectures implement it as define as far as I can see.
>
> Then why would it be a problem?
>
It isn't a problem for those other architectures. Mostly it is mapped to functions like memcpy().

Guenter

>>>> Unfortunately, LUSTRE can only be built as module, so there is no
>>>> easy fix.
>>>
>>> Can't we just export the functions for those arches?  Surely lutre
>>> isn't the first/only driver that needs this?
>>>
>> That would be another option.
>>
>> Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
>>
>> https://lkml.org/lkml/2013/9/5/111
>>
>> So please forget this patch. If sh/xtensa actually need it, we can do the same there.
>
> Sounds good to me, consider it forgotten :)
>
> greg k-h
>
>

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Greg Kroah-Hartman]

On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
> On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
> > On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> >> mips allmodconfig fails with
> >>
> >> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
> >> undefined!
> >>
> >> which is due to LUSTRE using copy_from_user_page which is not exported by any
> >> architecture.
> >
> > Any, or just these arches?
> >
> Other architectures implement it as define as far as I can see.

Then why would it be a problem?

> >> Unfortunately, LUSTRE can only be built as module, so there is no
> >> easy fix.
> >
> > Can't we just export the functions for those arches?  Surely lutre
> > isn't the first/only driver that needs this?
> >
> That would be another option.
> 
> Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
> 
> https://lkml.org/lkml/2013/9/5/111
> 
> So please forget this patch. If sh/xtensa actually need it, we can do the same there.

Sounds good to me, consider it forgotten :)

greg k-h

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Greg Kroah-Hartman]

On Mon, Sep 09, 2013 at 07:48:51AM +0530, Ramkumar Ramachandra wrote:
> Greg Kroah-Hartman wrote:
> > On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> >> Unfortunately, LUSTRE can only be built as module, so there is no
> >> easy fix.
> >
> > Can't we just export the functions for those arches?  Surely lutre
> > isn't the first/only driver that needs this?
> 
> It's simply necessary to keep up to date: we can't keep building new
> arches since the initial cost can be quite high.

What do you mean by this?  What "initial cost"?  You should be able to
cross-compile almost all arches on your desktop machine today with the
compilers we have on kernel.org.  If I can get them set up and working,
they can't be that hard for anyone else :)

thanks,

greg k-h

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Ramkumar Ramachandra]

Greg Kroah-Hartman wrote:
> What do you mean by this?  What "initial cost"?  You should be able to
> cross-compile almost all arches on your desktop machine today with the
> compilers we have on kernel.org.  If I can get them set up and working,
> they can't be that hard for anyone else :)

Won't you need to physically explore hardware? How will the hardware
industry be driven otherwise?

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Greg Kroah-Hartman]

On Mon, Sep 09, 2013 at 08:08:28AM +0530, Ramkumar Ramachandra wrote:
> Greg Kroah-Hartman wrote:
> > What do you mean by this?  What "initial cost"?  You should be able to
> > cross-compile almost all arches on your desktop machine today with the
> > compilers we have on kernel.org.  If I can get them set up and working,
> > they can't be that hard for anyone else :)
> 
> Won't you need to physically explore hardware?

In order to do what?

> How will the hardware industry be driven otherwise?

I have no idea what you are referring to here, please explain.

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Ramkumar Ramachandra]

Greg Kroah-Hartman wrote:
>> How will the hardware industry be driven otherwise?
>
> I have no idea what you are referring to here, please explain.

For stability, hardware needs to be present. When I started out a
couple of days ago, I blamed my monitor for the data corruption: it's
one extra point of leverage.

Ram

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

On 09/08/2013 07:55 PM, Ramkumar Ramachandra wrote:
> Greg Kroah-Hartman wrote:
>>> How will the hardware industry be driven otherwise?
>>
>> I have no idea what you are referring to here, please explain.
>
> For stability, hardware needs to be present. When I started out a
> couple of days ago, I blamed my monitor for the data corruption: it's
> one extra point of leverage.
>

I have no idea whatsoever what you are talking about.

Guenter

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Ramkumar Ramachandra]

Guenter Roeck wrote:
>> For stability, hardware needs to be present. When I started out a
>> couple of days ago, I blamed my monitor for the data corruption: it's
>> one extra point of leverage.
>
> I have no idea whatsoever what you are talking about.

Sorry, my bad. I haven't done hardware driver development.

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Heiko Carstens]

On Sun, Sep 08, 2013 at 07:31:18PM -0700, Guenter Roeck wrote:
> On 09/08/2013 07:31 PM, Greg Kroah-Hartman wrote:
> >On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
> >>On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
> >>>On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> >>>>mips allmodconfig fails with
> >>>>
> >>>>ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
> >>>>undefined!
> >>>>
> >>>>which is due to LUSTRE using copy_from_user_page which is not exported by any
> >>>>architecture.
> >>>
> >>>Any, or just these arches?
> >>>
> >>Other architectures implement it as define as far as I can see.
> >
> >Then why would it be a problem?
> >
> It isn't a problem for those other architectures. Mostly it is mapped to functions like memcpy().
> 
> Guenter
> 
> >>>>Unfortunately, LUSTRE can only be built as module, so there is no
> >>>>easy fix.
> >>>
> >>>Can't we just export the functions for those arches?  Surely lutre
> >>>isn't the first/only driver that needs this?
> >>>
> >>That would be another option.
> >>
> >>Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
> >>
> >>https://lkml.org/lkml/2013/9/5/111
> >>
> >>So please forget this patch. If sh/xtensa actually need it, we can do the same there.
> >
> >Sounds good to me, consider it forgotten :)
> >
> >greg k-h

The proper "fix" seems to be to get rid of this new usage of 
copy_from_user_page() in lustre.
The code in question is nothing but a copy of __access_remote_vm()
from mm/memory.c...

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Christoph Hellwig]

On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> Can't we just export the functions for those arches?  Surely lutre
> isn't the first/only driver that needs this?

Lustre is.  These are core mm helpers, and lustre uses them to
reimplement another core VM function.  It then uses it to access
userspace environment variable.

In short all this code should be nuked, and no new symbols should be
exported.

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Greg Kroah-Hartman]

On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> > Can't we just export the functions for those arches?  Surely lutre
> > isn't the first/only driver that needs this?
> 
> Lustre is.  These are core mm helpers, and lustre uses them to
> reimplement another core VM function.  It then uses it to access
> userspace environment variable.
> 
> In short all this code should be nuked, and no new symbols should be
> exported.

Ugh, you are right, the lustre code needs to be fixed here.

greg k-h

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> > > Can't we just export the functions for those arches?  Surely lutre
> > > isn't the first/only driver that needs this?
> > 
> > Lustre is.  These are core mm helpers, and lustre uses them to
> > reimplement another core VM function.  It then uses it to access
> > userspace environment variable.
> > 
> > In short all this code should be nuked, and no new symbols should be
> > exported.
> 
> Ugh, you are right, the lustre code needs to be fixed here.
> 
Given that, should I send another patch marking it as BROKEN again ?

Guenter

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Greg Kroah-Hartman]

On Mon, Sep 09, 2013 at 10:08:19AM -0700, Guenter Roeck wrote:
> On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> > > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> > > > Can't we just export the functions for those arches?  Surely lutre
> > > > isn't the first/only driver that needs this?
> > > 
> > > Lustre is.  These are core mm helpers, and lustre uses them to
> > > reimplement another core VM function.  It then uses it to access
> > > userspace environment variable.
> > > 
> > > In short all this code should be nuked, and no new symbols should be
> > > exported.
> > 
> > Ugh, you are right, the lustre code needs to be fixed here.
> > 
> Given that, should I send another patch marking it as BROKEN again ?

Well, on those arches it's "broken", so I'll dig up your original patch
on this thread.  It's just "normal" for staging drivers to duplicate
core code, it needs to be fixed up before it can be merged into the
kernel tree, so no need to do anything special.

thanks,

greg k-h

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Geert Uytterhoeven]

On Mon, Sep 9, 2013 at 7:22 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Sep 09, 2013 at 10:08:19AM -0700, Guenter Roeck wrote:
>> On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
>> > On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
>> > > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
>> > > > Can't we just export the functions for those arches?  Surely lutre
>> > > > isn't the first/only driver that needs this?
>> > >
>> > > Lustre is.  These are core mm helpers, and lustre uses them to
>> > > reimplement another core VM function.  It then uses it to access
>> > > userspace environment variable.
>> > >
>> > > In short all this code should be nuked, and no new symbols should be
>> > > exported.
>> >
>> > Ugh, you are right, the lustre code needs to be fixed here.
>> >
>> Given that, should I send another patch marking it as BROKEN again ?
>
> Well, on those arches it's "broken", so I'll dig up your original patch
> on this thread.  It's just "normal" for staging drivers to duplicate
> core code, it needs to be fixed up before it can be merged into the
> kernel tree, so no need to do anything special.

It's not only broken on MIPS, SH, and XTENSA, but also on at least parisc
and m68k[*].

It's no longer broken on sparc64, as the missing export already got
into mainline.
In light of the above, perhaps that should be reverted?

[*] Why does m68k allmodconfig still succeed on kissb???
    It does fail for me, as m68k's copy_from_user_page() calls
    flush_icache_user_range(), which is not exported.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

On Mon, Sep 09, 2013 at 09:11:25PM +0200, Geert Uytterhoeven wrote:
> On Mon, Sep 9, 2013 at 7:22 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Sep 09, 2013 at 10:08:19AM -0700, Guenter Roeck wrote:
> >> On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
> >> > On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> >> > > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> >> > > > Can't we just export the functions for those arches?  Surely lutre
> >> > > > isn't the first/only driver that needs this?
> >> > >
> >> > > Lustre is.  These are core mm helpers, and lustre uses them to
> >> > > reimplement another core VM function.  It then uses it to access
> >> > > userspace environment variable.
> >> > >
> >> > > In short all this code should be nuked, and no new symbols should be
> >> > > exported.
> >> >
> >> > Ugh, you are right, the lustre code needs to be fixed here.
> >> >
> >> Given that, should I send another patch marking it as BROKEN again ?
> >
> > Well, on those arches it's "broken", so I'll dig up your original patch
> > on this thread.  It's just "normal" for staging drivers to duplicate
> > core code, it needs to be fixed up before it can be merged into the
> > kernel tree, so no need to do anything special.
> 
> It's not only broken on MIPS, SH, and XTENSA, but also on at least parisc
> and m68k[*].
> 
> It's no longer broken on sparc64, as the missing export already got
> into mainline.
> In light of the above, perhaps that should be reverted?
> 
Agreed.

> [*] Why does m68k allmodconfig still succeed on kissb???
>     It does fail for me, as m68k's copy_from_user_page() calls
>     flush_icache_user_range(), which is not exported.
> 
I don't see a build failure in m68k:allmodconfig either.

flush_icache_user_range() is called from copy_to_user_page(), not from
copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
which calls __flush_cache_030(). The first is inline, the second is
assembler, so I would expect it to work. Which doesn't answer
the question why it fails for you.

powerpc and frm export flush_icache_user_range(). Wonder if that is really
necessary or points to other abuses.

On parisc I currently only test defconfig. I'll check if allmodconfig passes
in 3.10 and/or 3.11; if yes I'll add it to my test suite.

Guenter

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Geert Uytterhoeven]

On Mon, Sep 9, 2013 at 10:06 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> [*] Why does m68k allmodconfig still succeed on kissb???
>>     It does fail for me, as m68k's copy_from_user_page() calls
>>     flush_icache_user_range(), which is not exported.
>>
> I don't see a build failure in m68k:allmodconfig either.
>
> flush_icache_user_range() is called from copy_to_user_page(), not from
> copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
> which calls __flush_cache_030(). The first is inline, the second is
> assembler, so I would expect it to work. Which doesn't answer
> the question why it fails for you.

Sorry, I meant copy_to_user_page().

I tried with 2.6.3 from crosstool, and it succeeded, too.

Turns out cfs_access_process_vm() is called with write=0 only.
Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
Mystery solved.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

On Tue, Sep 10, 2013 at 10:49:05AM +0200, Geert Uytterhoeven wrote:
> On Mon, Sep 9, 2013 at 10:06 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> [*] Why does m68k allmodconfig still succeed on kissb???
> >>     It does fail for me, as m68k's copy_from_user_page() calls
> >>     flush_icache_user_range(), which is not exported.
> >>
> > I don't see a build failure in m68k:allmodconfig either.
> >
> > flush_icache_user_range() is called from copy_to_user_page(), not from
> > copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
> > which calls __flush_cache_030(). The first is inline, the second is
> > assembler, so I would expect it to work. Which doesn't answer
> > the question why it fails for you.
> 
> Sorry, I meant copy_to_user_page().
> 
> I tried with 2.6.3 from crosstool, and it succeeded, too.
> 
Do such old versions of gcc still exist ? Just kidding :)

> Turns out cfs_access_process_vm() is called with write=0 only.
> Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
> and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
> Mystery solved.
> 
Still bad. Guess it would still fail with 4.6.3 if optimization is turned off.

Guenter

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Geert Uytterhoeven]

On Tue, Sep 10, 2013 at 6:44 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> I tried with 2.6.3 from crosstool, and it succeeded, too.
>>
> Do such old versions of gcc still exist ? Just kidding :)
>
>> Turns out cfs_access_process_vm() is called with write=0 only.
>> Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
>> and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
>> Mystery solved.
>>
> Still bad. Guess it would still fail with 4.6.3 if optimization is turned off.

For the record: s/2.6.3/4.6.3/.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Peng Tao]

On Mon, Sep 9, 2013 at 1:01 PM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> On Sun, Sep 08, 2013 at 07:31:18PM -0700, Guenter Roeck wrote:
>> On 09/08/2013 07:31 PM, Greg Kroah-Hartman wrote:
>> >On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
>> >>On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
>> >>>On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>> >>>>mips allmodconfig fails with
>> >>>>
>> >>>>ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>> >>>>undefined!
>> >>>>
>> >>>>which is due to LUSTRE using copy_from_user_page which is not exported by any
>> >>>>architecture.
>> >>>
>> >>>Any, or just these arches?
>> >>>
>> >>Other architectures implement it as define as far as I can see.
>> >
>> >Then why would it be a problem?
>> >
>> It isn't a problem for those other architectures. Mostly it is mapped to functions like memcpy().
>>
>> Guenter
>>
>> >>>>Unfortunately, LUSTRE can only be built as module, so there is no
>> >>>>easy fix.
>> >>>
>> >>>Can't we just export the functions for those arches?  Surely lutre
>> >>>isn't the first/only driver that needs this?
>> >>>
>> >>That would be another option.
>> >>
>> >>Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
>> >>
>> >>https://lkml.org/lkml/2013/9/5/111
>> >>
>> >>So please forget this patch. If sh/xtensa actually need it, we can do the same there.
>> >
>> >Sounds good to me, consider it forgotten :)
>> >
>> >greg k-h
>
> The proper "fix" seems to be to get rid of this new usage of
> copy_from_user_page() in lustre.
> The code in question is nothing but a copy of __access_remote_vm()
> from mm/memory.c...
>
The problem is access_process_vm() is not exported since certain
version of kernel including the latest. According to Christoph in the
other mail, access_process_vm() is also a core mm function that is not
supposed to be exported. Then what kind of change shall we make in
order to keep current functionality?

Thanks,
Bergwolf

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Peng Tao]

On Tue, Sep 10, 2013 at 4:49 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Sep 9, 2013 at 10:06 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> [*] Why does m68k allmodconfig still succeed on kissb???
>>>     It does fail for me, as m68k's copy_from_user_page() calls
>>>     flush_icache_user_range(), which is not exported.
>>>
>> I don't see a build failure in m68k:allmodconfig either.
>>
>> flush_icache_user_range() is called from copy_to_user_page(), not from
>> copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
>> which calls __flush_cache_030(). The first is inline, the second is
>> assembler, so I would expect it to work. Which doesn't answer
>> the question why it fails for you.
>
> Sorry, I meant copy_to_user_page().
>
> I tried with 2.6.3 from crosstool, and it succeeded, too.
>
> Turns out cfs_access_process_vm() is called with write=0 only.
> Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
> and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
> Mystery solved.
>
Thanks for chasing down the root cause. I was also wondering why I
didn't see this when building on MIPS some time ago.

Thanks,
Tao

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Christoph Hellwig]

On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
> The problem is access_process_vm() is not exported since certain
> version of kernel including the latest. According to Christoph in the
> other mail, access_process_vm() is also a core mm function that is not
> supposed to be exported. Then what kind of change shall we make in
> order to keep current functionality?

You should remove the higher level functionality, kernel modules are
not supposed to look at userspace environment variables.

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Peng Tao]

On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>> The problem is access_process_vm() is not exported since certain
>> version of kernel including the latest. According to Christoph in the
>> other mail, access_process_vm() is also a core mm function that is not
>> supposed to be exported. Then what kind of change shall we make in
>> order to keep current functionality?
>
> You should remove the higher level functionality, kernel modules are
> not supposed to look at userspace environment variables.
>
OK. I've looked at the specific case that Lustre uses
access_process_vm() to get the jobid environment variable and package
it into the RPC requests to server. However, it turns out that in the
latest Lustre server code, the jobid in a request is not used
anywhere. So it looks like we can just get rid of it.

Andreas, could you please confirm this? Is the jobid an obsolete
parameter that can be abandoned? Or is there plan to use it somehow in
the future?

Thanks,
Tao

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Guenter Roeck]

On Wed, Sep 11, 2013 at 10:25:57AM +0800, Peng Tao wrote:
> On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
> >> The problem is access_process_vm() is not exported since certain
> >> version of kernel including the latest. According to Christoph in the
> >> other mail, access_process_vm() is also a core mm function that is not
> >> supposed to be exported. Then what kind of change shall we make in
> >> order to keep current functionality?
> >
> > You should remove the higher level functionality, kernel modules are
> > not supposed to look at userspace environment variables.
> >
> OK. I've looked at the specific case that Lustre uses
> access_process_vm() to get the jobid environment variable and package
> it into the RPC requests to server. However, it turns out that in the
> latest Lustre server code, the jobid in a request is not used
> anywhere. So it looks like we can just get rid of it.
> 
> Andreas, could you please confirm this? Is the jobid an obsolete
> parameter that can be abandoned? Or is there plan to use it somehow in
> the future?
> 
"Plan to use it in the future" is not a reason or argument to keep it today,
especially if it is something you are not supposed to do to start with.
If you ever need it, you should be able to find some other means to
support a similar functionality.

Guenter

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Peng Tao]

On Wed, Sep 11, 2013 at 10:30 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Sep 11, 2013 at 10:25:57AM +0800, Peng Tao wrote:
>> On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> > On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>> >> The problem is access_process_vm() is not exported since certain
>> >> version of kernel including the latest. According to Christoph in the
>> >> other mail, access_process_vm() is also a core mm function that is not
>> >> supposed to be exported. Then what kind of change shall we make in
>> >> order to keep current functionality?
>> >
>> > You should remove the higher level functionality, kernel modules are
>> > not supposed to look at userspace environment variables.
>> >
>> OK. I've looked at the specific case that Lustre uses
>> access_process_vm() to get the jobid environment variable and package
>> it into the RPC requests to server. However, it turns out that in the
>> latest Lustre server code, the jobid in a request is not used
>> anywhere. So it looks like we can just get rid of it.
>>
>> Andreas, could you please confirm this? Is the jobid an obsolete
>> parameter that can be abandoned? Or is there plan to use it somehow in
>> the future?
>>
> "Plan to use it in the future" is not a reason or argument to keep it today,
> especially if it is something you are not supposed to do to start with.
> If you ever need it, you should be able to find some other means to
> support a similar functionality.
>
I'm not fighting against removing the piece of code. But if there is a
strong reason to keep the functionality, we need to find a way to
implement it. The convenience of using environment variables is that
job scheduler can set the environment and other existing applications
don't have to change. Are there other means to do the same? ioctl and
upcall both need application change AFAIK.

Again, if the code is just obsolete, which is quite likely but needs
Andreas' confirmation, we can just remove it.

Thanks,
Tao

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Christoph Hellwig]

On Wed, Sep 11, 2013 at 10:51:50AM +0800, Peng Tao wrote:
> I'm not fighting against removing the piece of code. But if there is a
> strong reason to keep the functionality, we need to find a way to
> implement it. The convenience of using environment variables is that
> job scheduler can set the environment and other existing applications
> don't have to change. Are there other means to do the same? ioctl and
> upcall both need application change AFAIK.

There is no use case for it, the kernel has no business looking at these
variables.  Given that you think it's not even used I don't even know
why we're having this discussion.

Talking about nasty code, the whole linux-curproc.c is highly
questionable:

 - cfs_curproc_groups_nr:
   	unused and should be removed

 - cfs_cap_raise/cfs_cap_lower/cfs_cap_raised:
   	needs to go away, modyules must not change access permissions
	on behalf of processes

 - the whole cfs_cap_t handling also needs to go away, passing around
   capabilities is not a concept the kernel supports for a reason

 - current_is_32bit:
	Code should just use is_compat_task directly.


I've just taken the time to walk through this one file, but it seems
like most of libcfs is just as bad.

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Dilger, Andreas]

On 2013/09/10 8:25 PM, "Peng Tao" <bergwolf@gmail.com> wrote:

>On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <hch@infradead.org>
>wrote:
>> On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>>> The problem is access_process_vm() is not exported since certain
>>> version of kernel including the latest. According to Christoph in the
>>> other mail, access_process_vm() is also a core mm function that is not
>>> supposed to be exported. Then what kind of change shall we make in
>>> order to keep current functionality?
>>
>> You should remove the higher level functionality, kernel modules are
>> not supposed to look at userspace environment variables.
>>
>OK. I've looked at the specific case that Lustre uses
>access_process_vm() to get the jobid environment variable and package
>it into the RPC requests to server. However, it turns out that in the
>latest Lustre server code, the jobid in a request is not used
>anywhere. So it looks like we can just get rid of it.
>
>Andreas, could you please confirm this? Is the jobid an obsolete
>parameter that can be abandoned? Or is there plan to use it somehow in
>the future?

The jobid code is relatively new and in use, I'm not sure why you think
it is not in use.  It is actually a feature that a bunch of users
requested.

The jobid feature allows tracking IO request stats for parallel user
processes
running on possibly thousands of different client nodes onto the servers.
This is easy to do with a single node and a single process via PID/PPID
and blktrace or equivalent, but otherwise impossible in a parallel
processing
environment where there may be users running hundreds of different jobs.
The PID/PPID is not consistent across client nodes, and the server threads
will randomly handle requests from all users.

By all means, I'd prefer to just use access_process_vm() directly, instead
of making a copy in the Lustre code.  Not being able to access the process
environment seems a bit ridiculous - the kernel stores and manages this for
the process, and it isn't even doing anything nasty like accessing the
environment from a different process, just its own environment variables.

Cheers, Andreas
-- 
Andreas Dilger

Lustre Software Architect
Intel High Performance Data Division

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Dilger, Andreas]

On 2013/09/11 10:29 AM, "Christoph Hellwig" <hch@infradead.org> wrote:
>Talking about nasty code, the whole linux-curproc.c is highly
>questionable:
>
> - cfs_curproc_groups_nr:
>   	unused and should be removed

This is already removed in the upstream Lustre code, it just hasn't made
it into the kernel yet.

>- cfs_cap_raise/cfs_cap_lower/cfs_cap_raised:
>   	needs to go away, modyules must not change access permissions
>	on behalf of processes

These are only used on the server so that threads running as user UID/GID
can write to recovery log files.  There is likely a different way that
this could be done, it has probably been this way from years ago when we
used the VFS to do file IO on the server and it was doing file permission
checking again.

> - the whole cfs_cap_t handling also needs to go away, passing around
>   capabilities is not a concept the kernel supports for a reason
>
> - current_is_32bit:
>	Code should just use is_compat_task directly.

This was already removed in the upstream Lustre code.

>I've just taken the time to walk through this one file, but it seems
>like most of libcfs is just as bad.

Sure, and that's why Lustre is in drivers/staging and not fs/, and I don't
make any claims to the contrary.  We are slowly cleaning out these macros
(added when we wanted to get Lustre working on MacOS and WinNT), but it
will take some time.  XFS lived with an IRIX shim layer for years, and
still
has a whole vnode abstraction layer that nobody seems to be complaining
about, so I don't think it is unreasonable for us to take some time to
clean
up Lustre.

Cheers, Andreas
-- 
Andreas Dilger

Lustre Software Architect
Intel High Performance Data Division

Re: [PATCH] staging: Disable lustre file system for MIPS, SH, and XTENSA

[Peng Tao]

On Thu, Sep 12, 2013 at 4:48 AM, Dilger, Andreas
<andreas.dilger@intel.com> wrote:
> On 2013/09/10 8:25 PM, "Peng Tao" <bergwolf@gmail.com> wrote:
>
>>On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <hch@infradead.org>
>>wrote:
>>> On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>>>> The problem is access_process_vm() is not exported since certain
>>>> version of kernel including the latest. According to Christoph in the
>>>> other mail, access_process_vm() is also a core mm function that is not
>>>> supposed to be exported. Then what kind of change shall we make in
>>>> order to keep current functionality?
>>>
>>> You should remove the higher level functionality, kernel modules are
>>> not supposed to look at userspace environment variables.
>>>
>>OK. I've looked at the specific case that Lustre uses
>>access_process_vm() to get the jobid environment variable and package
>>it into the RPC requests to server. However, it turns out that in the
>>latest Lustre server code, the jobid in a request is not used
>>anywhere. So it looks like we can just get rid of it.
>>
>>Andreas, could you please confirm this? Is the jobid an obsolete
>>parameter that can be abandoned? Or is there plan to use it somehow in
>>the future?
>
> The jobid code is relatively new and in use, I'm not sure why you think
> it is not in use.  It is actually a feature that a bunch of users
> requested.
>
You are right. Sorry I misread the code yesterday. I accidentally
searched for callers of lustre_msg_get_jobid() in a kernel tree
instead of a Lustre tree. Now I see that jobid is used by server for
request tracking purpose. Thank you for pointing it out.

> The jobid feature allows tracking IO request stats for parallel user
> processes
> running on possibly thousands of different client nodes onto the servers.
> This is easy to do with a single node and a single process via PID/PPID
> and blktrace or equivalent, but otherwise impossible in a parallel
> processing
> environment where there may be users running hundreds of different jobs.
> The PID/PPID is not consistent across client nodes, and the server threads
> will randomly handle requests from all users.
>
> By all means, I'd prefer to just use access_process_vm() directly, instead
> of making a copy in the Lustre code.  Not being able to access the process
> environment seems a bit ridiculous - the kernel stores and manages this for
> the process, and it isn't even doing anything nasty like accessing the
> environment from a different process, just its own environment variables.
>
Or, can we read from /proc/self/environ? Instead of reading from
mm->env_start/env_end, we can let proc_environ_operations do the same
and avoid calling access_process_vm() directly.

Thanks,
Tao