On Sat, May 20, 2006 at 10:57:40AM +0100, Christoph Hellwig wrote: > I gave the code in -mm a quick look, and it still needs a lot of work. looking over the code again: > - please kill ECRYPTFS_SET_FLAG, ECRYPTFS_CLEAR_FLAG and ECRYPTFS_CHECK_FLAG > and just opencode them, it'll make the code a whole lot more readable these are still there.. > - please make sure touch_atime goes down to ->setattr for atime updates, > that way you don't need all that mess in your read/write. and in -mm > those routines need update for vectored and aio support you don't seem to have updated the common code for this yet. > - NEVER EVER do things like copying locks_delete_block and > posix_lock_file_wait (as ecryptfs_posix_lock and based on a previous > version) to you code. It will get stale and create a maintaince nightmare. > talk with the subsystem maintainers on how to make the core functionality > accesible to you. > - similarly ecryptfs_setlk is totally non-acceptable. find a way with the > maintainer to reuse things from fcntl_setlk with a common helper dito > - copying things like lock_parent, unlock_parent and unlock_dir still there. sorry, but there's zero changes to get things merged that opencode > > - please split all the generic stackable filesystem passthorugh routines > into a separated stackfs layer, in a few files in fs/stackfs/ that > you depend on. They'll get _GPL exported to all possible stackable > filesystem. They'll need their own store underlying object helpers, > but that can be made to work by embedding the generic stackfs data > as first thing in the ecryptfs object. > > > More comments: - what protects accessing d_parent in lock_parent / unlock_parent? - no need to cast the return value of kmem_cache_alloc, it's void * - any reason to use the SLAB_* flags instead of GFP_? I'm a bit surprised SLAB_* still exists at all.. - follow_link handling is wrong. you need to call the underlying ->follow_link in your follow_link implementation and you need to keep a separate nameidata for it - please kill that ugly ecryptfs_allocated_caches hack and do normal goto based unwinding on failure - using iget with the lower filesystems i_ino does not work. There are various filesystems were i_ino does not uniqueuely identify an inode. You will probably need your own sequence numbers. Also please don't use iget in new code but always the iget_locked variant. And a more general issue with implementing stackable filesystems: I think it's probably much better to not stack ontop of a part of the existing namespace but rather let ecryptfs mount the underlying filesystem internally using vfs_kern_mount. This gets out of the way of possible lock order problems when doing namespace operation involving both the stacked and underlying filesystem aswell as allowing using a stackable filesystem as the root filesystem. - 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
