From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Thompson Subject: Re: eCryptfs: Request for review Date: Wed, 19 Oct 2005 14:00:05 -0500 Message-ID: References: <20051018193811.GA11545@halcrow.us> <1129736164.25733.19.camel@polarbear.fsl.cs.sunysb.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT Cc: Michael Halcrow , linux-fsdevel@vger.kernel.org Return-path: Received: from xproxy.gmail.com ([66.249.82.206]:4911 "EHLO xproxy.gmail.com") by vger.kernel.org with ESMTP id S1751222AbVJSTAG convert rfc822-to-8bit (ORCPT ); Wed, 19 Oct 2005 15:00:06 -0400 Received: by xproxy.gmail.com with SMTP id i26so108103wxd for ; Wed, 19 Oct 2005 12:00:05 -0700 (PDT) To: "Charles P. Wright" In-Reply-To: <1129736164.25733.19.camel@polarbear.fsl.cs.sunysb.edu> Content-Disposition: inline Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On 10/19/05, Charles P. Wright wrote: > This is just a brief glance, and by no means complete, but here are a > few things that I saw. > > In dentry.c you shouldn't implement d_delete. This will cause problems > with certain lower-level file systems. Not entirely sure I follow the reasoning why here. Obviously, if it needs to be, it can be removed, but all this function is doing is a bunch of sanity checks, then then calling d_delete on the lower filesystems if it exists. Just trying to get a better understand of the code you see :) > In crypto.c:ecryptfs_dump_hex you have variables intermixed with code, > which won't work on some older compilers (AFAIK gcc 2.95 will barf). Noted. Changed. > In ecryptfs_llseek, If you are adding pages for extra information, you > need to convert the offset to a lower-level offset, and then convert the > return back to what user space expects it to be. You should test fsx on > your file system, to sort these types of things out. I believe that this is working, we have run fsx "succesfully" for 10000 iterations. However, its not a total success because we are bugging out in fs/buffer.c:1822 BUG_ON(PageWriteback(page)); Will look a bit more closely though. > You should probably rename ecryptfs_write to ecryptfs_dir_write, or just > use generic_read_dir and NULL for directory read and write, > respectively. Changed. Will look into using the generics, not entirely sure what impact that will have without further investigation. > In inode.c: > Instead of get_parent you can use dget_parent. The core kernel just > seems to have lock_parent as two lines in the function that need it > instead of the function these days. > > The rest of the VFS locking functions should also be converted from 2.4 > to 2.6 primitives (this is a bug that you inherited from the FiST > templates). For example, instead of using double_down, you should use > lock_rename. Thanks for those, adding them to our ever growing TODO list :) > For main.c, you can replace the nasty mount option string processing > with lib/parser.c. Will look into doing that :) Mike