fscache review comments, part 3

fscache review comments, part 3

[Christoph Hellwig]

Now it gets dirty.  This mail is about the cachefiles module which
I'm actually unhappy with, unlike the previous bits which just got
the usual round of suggestions.

 - as a start please don't mix introducing a new module, adding new
   function in the core code and adding exports in the same patch.
 - the new install_page_waitqueue_monitor function is generally fine,
   but it should get
      a) a kernel-doc comment describing it
      b) a name actual describig what it does, e.g. add_page_wait_queue
         monitoring the names for other waitqueue functionality.
 - generic_file_buffered_write_one_kernel_page seems generally fine, but
   you must not call this directly from cachefiles but rather go through
   a file operation for it..  There's various filesystems where directly
   going to the address_space operations is not fine.  This goes back
   to our kernel_read/write discussion at OLS.  Also please remove the #if 0
   debug code and add a kernel-doc comment.
 - the debug sysctl shouldn't be needed.  We allow run-time changeable
   module parameters now, which sound perfect for this purpose.
 - the procfs interface is insane :) Suggested replacement:

    - the tunables sound like they should be one-value per file sysfs
      entinities.
    - dito for the tag
    - same is true for the root directory, but that one should not
      be specified as a filedescriptor, but a normal pathname.
      This also get rid of the non-acceptable fget_light export.

 - the second argument to cachefiles_proc_add_cache is always NULL,
   you could remove it aswell.
 - intead of setting nd.mnt and nd.dentry to NULL in cachefiles_proc_add_cache
   you should grab your own references to them

 - send_sigurg should be exported _GPL if at all.  In fact I'm not very
   happy about using it at all.  Why can you use a random singnal to
   communicated with your daemon?
 - never use ->getxattr and ->setxattr directly.  Always use the
   vfs_getxattr and vfs_setxattr helpers.
 - similar for unlink, please use vfs_unlink so you don't miss selinux,
   mountpoint and similar checks.  If you need a version that can be
   entered with the lock already held we can talk about that, but I'd
   prefer you to get rid of the requirement
 - the rename case is even worse.  We need a very very good justification
   why this can't be done using vfs_rename
 - in cachefiles_walk_to_object reseting the fsuid/fsgid is not allowed
 - all of cf-namei.c is poking into dcache.c and namei.c internals that
   it absolutely must not.  I'm not going to mention all the details here
   because it's far too much, but I'd say for every single direct invocation
   of a filesystem inode or dcache operation from this file you will
   need a very good explanation - I'm not going to accesspt something
   poking into internals that deeply, it's exactly the kind of layering
   we want to provide via the VFS.
 - style-comment: please don't mention the filename again in the top-of-file
   block comment


There's probably a few more issues here, but I don't want to spend time
on it until the fundamental problems are sorted out.

Re: fscache review comments, part 3

[David Howells]

Christoph Hellwig <hch@infradead.org> wrote:

>  - as a start please don't mix introducing a new module, adding new
>    function in the core code and adding exports in the same patch.

Then people (like one Christoph Hellwig) complain about modules that add stuff
that nothing uses at that point.

>  - the new install_page_waitqueue_monitor function is generally fine,
>    but it should get
>       a) a kernel-doc comment describing it
>       b) a name actual describig what it does, e.g. add_page_wait_queue
>          monitoring the names for other waitqueue functionality.

Ummm... Do you mean you don't like "install_page_waitqueue_monitor"?  It's not
clear from your example if that's what you mean.

>  - generic_file_buffered_write_one_kernel_page seems generally fine, but
>    you must not call this directly from cachefiles but rather go through
>    a file operation for it..  There's various filesystems where directly
>    going to the address_space operations is not fine.  This goes back
>    to our kernel_read/write discussion at OLS.

I'm also told (though not by you) that I don't need to add more file ops or
address space ops as everything I need is there.

>    Also please remove the #if 0 debug code and add a kernel-doc comment.

The former yes; the latter: sigh.

>  - the debug sysctl shouldn't be needed.  We allow run-time changeable
>    module parameters now, which sound perfect for this purpose.

I didn't know that.  What if it's not a module?

>  - the procfs interface is insane :) Suggested replacement:
> 
>     - the tunables sound like they should be one-value per file sysfs
>       entinities.
>     - dito for the tag
>     - same is true for the root directory, but that one should not
>       be specified as a filedescriptor, but a normal pathname.
>       This also get rid of the non-acceptable fget_light export.

The intention is, in the future, to make it so that you can open it multiple
times to provide multiple caches with separate parameters, so: request denied.

>  - the second argument to cachefiles_proc_add_cache is always NULL,
>    you could remove it aswell.

Yeah.  I tried setting stuff up in its own namespace, but that caused more
problems than it solved.

>  - intead of setting nd.mnt and nd.dentry to NULL in cachefiles_proc_add_cache
>    you should grab your own references to them

There's precedent for what I'm doing, IIRC.  Maybe Al Viro can comment on
that.  Your suggestion causes unnecessary extra atomic ops to be incurred.

>  - send_sigurg should be exported _GPL if at all.

I can GPL export it if it makes you happier.

>    In fact I'm not very happy about using it at all.

Sorry.

>    Why can you use a random singnal to communicated with your daemon?

Can you rephrase that please? I think you're missing a negation.

Anyway, my daemon is attached to a file descriptor: why not use the fd's
signalling capabilities?

>  - never use ->getxattr and ->setxattr directly.  Always use the
>    vfs_getxattr and vfs_setxattr helpers.
>  - similar for unlink, please use vfs_unlink so you don't miss selinux,
>    mountpoint and similar checks.  If you need a version that can be
>    entered with the lock already held we can talk about that, but I'd
>    prefer you to get rid of the requirement
>  - the rename case is even worse.  We need a very very good justification
>    why this can't be done using vfs_rename
>  - in cachefiles_walk_to_object reseting the fsuid/fsgid is not allowed

Then CacheFiles has to do all its own filesystem ops in a separate thread
which will slow things down a lot.  Every read, every write, most opens and
many closes done to, say, NFS will incur a pair of context switches.

I have to avoid SELinux, fs permission checks and auditing.  You have to
remember that the security details of the accessing process are *not* what I
must access the cache with.

>  - all of cf-namei.c is poking into dcache.c and namei.c internals that
>    it absolutely must not.  I'm not going to mention all the details here
>    because it's far too much, but I'd say for every single direct invocation
>    of a filesystem inode or dcache operation from this file you will
>    need a very good explanation - I'm not going to accesspt something
>    poking into internals that deeply, it's exactly the kind of layering
>    we want to provide via the VFS.

Consider security.

>  - style-comment: please don't mention the filename again in the top-of-file
>    block comment

I trust you'll go and fix that in every other file in the kernel that does so.

But I can do that for my files here.

David

Re: fscache review comments, part 3

[Christoph Hellwig]

On Fri, Sep 29, 2006 at 10:23:57AM +0100, David Howells wrote:
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> >  - as a start please don't mix introducing a new module, adding new
> >    function in the core code and adding exports in the same patch.
> 
> Then people (like one Christoph Hellwig) complain about modules that add stuff
> that nothing uses at that point.

In your years of doing kernel work you must surely have been introduced
to the concept of a patch series.   Stop beeing suck a prick and please
work the way everyone else does.

> 
> >  - the new install_page_waitqueue_monitor function is generally fine,
> >    but it should get
> >       a) a kernel-doc comment describing it
> >       b) a name actual describig what it does, e.g. add_page_wait_queue
> >          monitoring the names for other waitqueue functionality.
> 
> Ummm... Do you mean you don't like "install_page_waitqueue_monitor"?  It's not
> clear from your example if that's what you mean.

No, I meant exactly what I said.  We don't use the term "monitor" for any
of our current waitqueue interface, and you shouldn't introduce it randomly.

> >  - generic_file_buffered_write_one_kernel_page seems generally fine, but
> >    you must not call this directly from cachefiles but rather go through
> >    a file operation for it..  There's various filesystems where directly
> >    going to the address_space operations is not fine.  This goes back
> >    to our kernel_read/write discussion at OLS.
> 
> I'm also told (though not by you) that I don't need to add more file ops or
> address space ops as everything I need is there.

But I tell you we need it and you haven't shown a reason why it's not true.
If you only believe in what people from your company tell you please take
a look at the thread starting at
http://marc.theaimsgroup.com/?l=linux-fsdevel&m=114349617015065&w=2
where the shiny new filesystem from the people with the Red Hats has a problem
with exactly that kind of construct.

> >    module parameters now, which sound perfect for this purpose.
> 
> I didn't know that.  What if it's not a module?

Still works, then it's a boot-time/run-time thing instead.

> >  - the procfs interface is insane :) Suggested replacement:
> > 
> >     - the tunables sound like they should be one-value per file sysfs
> >       entinities.
> >     - dito for the tag
> >     - same is true for the root directory, but that one should not
> >       be specified as a filedescriptor, but a normal pathname.
> >       This also get rid of the non-acceptable fget_light export.
> 
> The intention is, in the future, to make it so that you can open it multiple
> times to provide multiple caches with separate parameters, so: request denied.

So what?  Make the sysfs parameters per-cacheobject - thast's exactly
how it's supposed to work.  And btw, this was not a request, this was
a clear NACK for the current broken interface with a strongly suggested
replacement.

> >    Why can you use a random singnal to communicated with your daemon?
> 
> Can you rephrase that please? I think you're missing a negation.
> 
> Anyway, my daemon is attached to a file descriptor: why not use the fd's
> signalling capabilities?

Because we're not doing in anywhere else and having some code in fs/
beeing the only user of it except for the traditional networking use
is a very good sign you're doing something rather fishy here.  I'm
still open for arguments why its wonderfull, but it needs a good
explanation.

> Then CacheFiles has to do all its own filesystem ops in a separate thread
> which will slow things down a lot.  Every read, every write, most opens and
> many closes done to, say, NFS will incur a pair of context switches.
>
> 
> I have to avoid SELinux, fs permission checks and auditing.  You have to
> remember that the security details of the accessing process are *not* what I
> must access the cache with.

No, you must store access control credentials with your files and allow
auditing - everything else is a bypass of the security model.  And yes,
performances is far third after correctness and maintainability.

> 
> >  - all of cf-namei.c is poking into dcache.c and namei.c internals that
> >    it absolutely must not.  I'm not going to mention all the details here
> >    because it's far too much, but I'd say for every single direct invocation
> >    of a filesystem inode or dcache operation from this file you will
> >    need a very good explanation - I'm not going to accesspt something
> >    poking into internals that deeply, it's exactly the kind of layering
> >    we want to provide via the VFS.
> 
> Consider security.

Exactly, what you do meansa you're bypassing all kinds of checks and balcances
in the VFS code, which is exactly what we want to avoid.

> 
> >  - style-comment: please don't mention the filename again in the top-of-file
> >    block comment
> 
> I trust you'll go and fix that in every other file in the kernel that does so.
> 
> But I can do that for my files here.

Fixing existing code is for the kernel-janitors project, while reviews
try to keep sneaking bad habits into new code.  Having a bad example
somewhere in the tree is not a good reason to get it in one more time.

(I'm really tired of writing all this crap again and again because people
obviously aren't listening, does someone who writes a better english then
me want to something about this to SubmittingPatches or a similar document?)

Re: fscache review comments, part 3

[David Howells]

Christoph Hellwig <hch@infradead.org> wrote:

>  - generic_file_buffered_write_one_kernel_page seems generally fine, but
>    you must not call this directly from cachefiles but rather go through
>    a file operation for it.

I can't do that for one very good reason: you insisted that I take out[*] all
the provision of a struct file * for doing I/O to the cache, and without that
I can't call file ops.  You can't have it both ways.  Sorry.

David

[*] You may remember the ENFILE avoidance patch that you objected strenuously
    to.

Re: fscache review comments, part 3

[Christoph Hellwig]

On Fri, Oct 06, 2006 at 02:25:54PM +0100, David Howells wrote:
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> >  - generic_file_buffered_write_one_kernel_page seems generally fine, but
> >    you must not call this directly from cachefiles but rather go through
> >    a file operation for it.
> 
> I can't do that for one very good reason: you insisted that I take out[*] all
> the provision of a struct file * for doing I/O to the cache, and without that
> I can't call file ops.  You can't have it both ways.  Sorry.

A file operation doesn't actually have to take a file struct, they're
also available from the inode.  But in some way you are right, doing that
as the file_operations level without a struct file is indeed rather odd.

The important point here is that we definitly need one of the operation
vectors to go through instead of a direct call.   I'm rather tired of all
this arguing here, so if the higher gods think we absolutely need cachefs
now just add a method somewhere with the signature of your function and
hopefull someone will clean up the utter mess later.  It's not like our
current set of read/write methods makes any sense.