fscache review comments, part 3

[Christoph Hellwig <hch@infradead.org>]

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.