Re: fscache review comments, part 3

[Christoph Hellwig <hch@infradead.org>]

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