2005-12-06 18:47:54

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

On Dinsdag 06 Dezember 2005 12:38, Pekka Enberg wrote:
> On Dinsdag 06 Dezember 2005 01:51, Paul Mackerras wrote:
> > > Remind me again why spufs is under arch/powerpc/ rather than fs/ ?
>
> On Tue, 2005-12-06 at 11:18 +0100, Arnd Bergmann wrote:
> > We had a discussion about this in August, after the patch
> > at http://patchwork.ozlabs.org/linuxppc64/patch?id=2140
> >
> > Nobody had voiced any objections against the arch/powerpc location,
> > and Pekka had good reasons against fs/, so I changed it.
>
> It had arch specific hooks which IMHO do not belong into fs/.

Since the discussion came up again in irc, I looked up the existing file
systems.

outside of fs/, we have the following file systems.

find -name \*.c | grep -v ^./fs | xargs grep struct.file_system_type.*=
./arch/ia64/kernel/perfmon.c:static struct file_system_type pfm_fs_type = {
./drivers/infiniband/core/uverbs_main.c:static struct file_system_type uverbs_event_fs = {
./drivers/isdn/capi/capifs.c:static struct file_system_type capifs_fs_type = {
./drivers/misc/ibmasm/ibmasmfs.c:static struct file_system_type ibmasmfs_type = {
./drivers/oprofile/oprofilefs.c:static struct file_system_type oprofilefs_type = {
./drivers/usb/core/inode.c:static struct file_system_type usb_fs_type = {
./drivers/usb/gadget/inode.c:static struct file_system_type gadgetfs_type = {
./ipc/mqueue.c:static struct file_system_type mqueue_fs_type = {
./kernel/cpuset.c:static struct file_system_type cpuset_fs_type = {
./kernel/futex.c:static struct file_system_type futex_fs_type = {
./mm/shmem.c:static struct file_system_type tmpfs_fs_type = {
./mm/tiny-shmem.c:static struct file_system_type tmpfs_fs_type = {
./net/socket.c:static struct file_system_type sock_fs_type = {
./net/sunrpc/rpc_pipe.c:static struct file_system_type rpc_pipe_fs_type = {
./security/inode.c:static struct file_system_type fs_type = {
./security/selinux/selinuxfs.c:static struct file_system_type sel_fs_type = {

In fs/, most code deals with actual files stored on a disk or similar,
with the exception of:

./fs/binfmt_misc.c:static struct file_system_type bm_fs_type = {
./fs/block_dev.c:static struct file_system_type bd_type = {
./fs/debugfs/inode.c:static struct file_system_type debug_fs_type = {
./fs/devfs/base.c:static struct file_system_type devfs_fs_type = {
./fs/devpts/inode.c:static struct file_system_type devpts_fs_type = {
./fs/eventpoll.c:static struct file_system_type eventpoll_fs_type = {
./fs/hugetlbfs/inode.c:static struct file_system_type hugetlbfs_fs_type = {
./fs/inotify.c:static struct file_system_type inotify_fs_type = {
./fs/openpromfs/inode.c:static struct file_system_type openprom_fs_type = {
./fs/pipe.c:static struct file_system_type pipe_fs_type = {
./fs/proc/root.c:static struct file_system_type proc_fs_type = {
./fs/relayfs/inode.c:static struct file_system_type relayfs_fs_type = {
./fs/sysfs/mount.c:static struct file_system_type sysfs_fs_type = {

I guess there is no strict rule where these file systems go to, e.g.
hugetlbs could just as well live near mm/shmem.c or any of those outside
of fs/ could be moved in there.

I don't really care where I put spufs, but I would prefer to move
the files only one more time at most.
Initially, they were in fs/spufs, and I moved them to
arch/powerpc/platforms/cell/spufs at Pekkas suggestion.

Arnd <><


2005-12-06 19:05:50

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Hi,

On Tue, 2005-12-06 at 19:49 +0100, Arnd Bergmann wrote:
> I guess there is no strict rule where these file systems go to, e.g.
> hugetlbs could just as well live near mm/shmem.c or any of those outside
> of fs/ could be moved in there.

hugetlbs does not contain architecture specific code so I don't see it
as a problem.

On Tue, 2005-12-06 at 19:49 +0100, Arnd Bergmann wrote:
> I don't really care where I put spufs, but I would prefer to move
> the files only one more time at most.
> Initially, they were in fs/spufs, and I moved them to
> arch/powerpc/platforms/cell/spufs at Pekkas suggestion.

I would prefer them to stay in arch/powerpc/. As far as I understand,
spufs will never have any use for platforms other than cell, so I really
don't see any point in putting it in fs/.

Pekka

2005-12-06 21:23:17

by Paul Mackerras

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Pekka Enberg writes:

> I would prefer them to stay in arch/powerpc/. As far as I understand,
> spufs will never have any use for platforms other than cell, so I really
> don't see any point in putting it in fs/.

The point is that people making changes to the filesystem interfaces
will be much more likely to notice and fix stuff that is under fs/
than code that is buried deep under arch/ somewhere. Filesystems
should go under fs/ for the sake of long-term maintainability. The
fact that it's only used on one architecture is irrelevant - you
simply make sure (with the appropriate Kconfig bits) that it's only
offered on that architecture.

Paul.

2005-12-06 21:41:40

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Hi Paul,

On Wed, 2005-12-07 at 08:10 +1100, Paul Mackerras wrote:
> The point is that people making changes to the filesystem interfaces
> will be much more likely to notice and fix stuff that is under fs/
> than code that is buried deep under arch/ somewhere. Filesystems
> should go under fs/ for the sake of long-term maintainability. The
> fact that it's only used on one architecture is irrelevant - you
> simply make sure (with the appropriate Kconfig bits) that it's only
> offered on that architecture.

I think the fact that it is highly architecture specific is relevant. I
have no way of testing spufs changes except on cell, no? And if I am
developing on a cell, I probably will notice it in arch/ all the same.
So I don't quite buy your the maintenace argument.

But as Arnd said, there are no clear rules on what kind of filesystems
should go into fs/ so please do whatever you must.

Pekka

2005-12-06 22:19:15

by Paul Mackerras

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Pekka Enberg writes:

> I think the fact that it is highly architecture specific is relevant. I
> have no way of testing spufs changes except on cell, no? And if I am
> developing on a cell, I probably will notice it in arch/ all the same.
> So I don't quite buy your the maintenace argument.

Think about someone changing the VFS layer interface and fixing up all
the filesystems to accommodate that change. That person is doing some
of your work for you, so you want to make it easy for him/her to find
your filesystem. That's the sort of thing I was referring to as
maintenance.

As for changes on the cell-specific side, the people doing those
changes will know where to find it, so it isn't a problem having it in
fs/.

Having it in fs/ also means that it is more likely that people
familiar with VFS internals will look through your code and comment on
it. I know that can be painful in the short term, but in the long
term it will lead to better code.

Paul.

2005-12-06 22:27:48

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Am Dienstag 06 Dezember 2005 23:19 schrieb Paul Mackerras:
> Having it in fs/ also means that it is more likely that people
> familiar with VFS internals will look through your code and comment on
> it. ?I know that can be painful in the short term, but in the long
> term it will lead to better code.

Yes, that is an excellent point. How should we proceed to get the code
there?
Do you want to move the files around in your git tree or do you
prefer me to send a full set of patches again and kill the existing
copy? Obviously, I'd prefer the former, since it would mean less
work for me with the same result.

Arnd <><

2005-12-07 02:26:16

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

On Wed, Dec 07, 2005 at 09:19:28AM +1100, Paul Mackerras wrote:
> Think about someone changing the VFS layer interface and fixing up all
> the filesystems to accommodate that change. That person is doing some
> of your work for you, so you want to make it easy for him/her to find
> your filesystem. That's the sort of thing I was referring to as
> maintenance.

FWIW, I think it's not a serious argument. Interface changes => grep time.
And that means grep over the tree anyway.

> As for changes on the cell-specific side, the people doing those
> changes will know where to find it, so it isn't a problem having it in
> fs/.
>
> Having it in fs/ also means that it is more likely that people
> familiar with VFS internals will look through your code and comment on
> it. I know that can be painful in the short term, but in the long
> term it will lead to better code.

That's solved by asking for review...

As far as I'm concerned, the only thing here that looks like a possible
reason to move the entire thing is highly unusual semantics of final
close and interesting use of VFS interfaces in spu_create(). I.e. it's
not that we have a filesystem there.

OTOH, if you go looking for analogs as far as unusual interaction with VFS
is concerned... net/unix is unlikely to get moved.

2005-12-07 03:15:19

by Paul Mackerras

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Al Viro writes:

> FWIW, I think it's not a serious argument. Interface changes => grep time.
> And that means grep over the tree anyway.

OK, well, where would you prefer the spufs code to go?

> That's solved by asking for review...

Could you review the spufs code (i.e. the patches posted by Arnd
recently to [email protected]) please?

Thanks,
Paul.

2005-12-07 08:21:52

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Hi Paul,

On 12/7/05, Paul Mackerras <[email protected]> wrote:
> Could you review the spufs code (i.e. the patches posted by Arnd
> recently to [email protected]) please?

Why not post them to LKML?

Pekka

2005-12-07 10:17:20

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

On Wed, Dec 07, 2005 at 02:15:09PM +1100, Paul Mackerras wrote:
> Al Viro writes:
>
> > FWIW, I think it's not a serious argument. Interface changes => grep time.
> > And that means grep over the tree anyway.
>
> OK, well, where would you prefer the spufs code to go?

Up to ppc folks, really - I don't see any serious objections to arch/powerpc/
variants; it could go there, it could go to fs/*. Objections along the lines
of "it won't be found" are BS - any interface change is going to start with
grep over the entire tree anyway.

> > That's solved by asking for review...
>
> Could you review the spufs code (i.e. the patches posted by Arnd
> recently to [email protected]) please?

If it's what you have in powerpc.git - see comments on IRC yesterday...

2005-12-06 22:14:41

by Nathan Lynch

[permalink] [raw]
Subject: Re: [PATCH 02/14] spufs: fix local store page refcounting

Paul Mackerras wrote:
> Pekka Enberg writes:
>
> > I would prefer them to stay in arch/powerpc/. As far as I understand,
> > spufs will never have any use for platforms other than cell, so I really
> > don't see any point in putting it in fs/.
>
> The point is that people making changes to the filesystem interfaces
> will be much more likely to notice and fix stuff that is under fs/
> than code that is buried deep under arch/ somewhere. Filesystems
> should go under fs/ for the sake of long-term maintainability. The
> fact that it's only used on one architecture is irrelevant - you
> simply make sure (with the appropriate Kconfig bits) that it's only
> offered on that architecture.

openpromfs seems to be a precedent here. It makes sense only on sparc
and sparc64 but it lives in fs/.