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

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

[Arnd Bergmann]

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

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

[Pekka Enberg]

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

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

[Paul Mackerras]

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.

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

[Pekka Enberg]

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

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

[Paul Mackerras]

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.

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

[Arnd Bergmann]

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

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

[Al Viro]

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.

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

[Paul Mackerras]

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 linuxppc64-dev@ozlabs.org) please?

Thanks,
Paul.

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

[Pekka Enberg]

Hi Paul,

On 12/7/05, Paul Mackerras <paulus@samba.org> wrote:
> Could you review the spufs code (i.e. the patches posted by Arnd
> recently to linuxppc64-dev@ozlabs.org) please?

Why not post them to LKML?

                              Pekka

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

[Al Viro]

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 linuxppc64-dev@ozlabs.org) please?

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

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

[Nathan Lynch]

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