From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Howells Subject: Re: [PATCH 7/7] FS-Cache: CacheFiles: A cache that backs onto a mounted filesystem Date: Fri, 21 Apr 2006 15:49:57 +0100 Message-ID: <24065.1145630997@warthog.cambridge.redhat.com> References: <20060420181616.0f167b1d.akpm@osdl.org> <20060420165927.9968.33912.stgit@warthog.cambridge.redhat.com> <20060420165941.9968.13602.stgit@warthog.cambridge.redhat.com> Cc: aviro@redhat.com, sct@redhat.com, nfsv4@linux-nfs.org, steved@redhat.com, linux-kernel@vger.kernel.org, David Howells , torvalds@osdl.org, linux-cachefs@redhat.com, linux-fsdevel@vger.kernel.org Return-path: In-Reply-To: <20060420181616.0f167b1d.akpm@osdl.org> To: Andrew Morton List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: nfsv4-bounces@linux-nfs.org Errors-To: nfsv4-bounces@linux-nfs.org List-Id: linux-fsdevel.vger.kernel.org Andrew Morton wrote: > > +unsigned long cachefiles_debug = 0; > > Unneeded initialisation. Yep. > > +static int cachefiles_init(void) > > __init? Yep. > removeable? Yep. > hm, what's going on here? It's strange for a callee to undo an i_mutex > which some caller took. It happens occasionally. The problem here is that I want to call this from three different places, but if I drop the mutex before calling the burial function, I have to get the mutex again to do the unlink; but as it is, I have to drop it before I can do the rename:-/ It's not nice, but... You have to note also that the directory's i_mutex is quite important for interacting with the daemon also. The wonders of working through an existing filesystem, and the the wonders of co-operating with userspace. > > +int > > +generic_file_buffered_write_one_kernel_page(struct file *file, > > + pgoff_t index, > > + struct page *src) > > Some covering comments would be nice. I copied those of generic_file_buffered_write() and rearranged them a bit:-) I'll add a comment to my function. > If the hosts's i_mutex is held (it should be, but there are no comments) > then we can read inode->i_size directly. Minor thing. Ah. Do we though? I just copied generic_file_buffered_write() and cut it down. The same is done there. The comments at the top of that function weren't exactly forthcoming on the preconditions for calling that function. > that's copy_highpage(). Good point. > Sigh. It's all a huge pile of new code. And it's only used by AFS, the > number of users of which can be counted on the fingers of one foot. An NFS > implementation would make a testing phase much more useful. Yes... Whilst I have it working with NFS, the NFS anti-aliasing problems are still there and still need to be sorted. I thought I'd got them nailed, but then Trond changed his mind:-( But that does not preclude putting what I can release up for review. David