Christoph Hellwig <hch@xxxxxxxxxxxxx> 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 - 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
