Re: [PATCH 0/13: eCryptfs] eCryptfs Patch Set

[Christoph Hellwig <hch@infradead.org>]

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.