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.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
