Re: [PATCH 7/7] FS-Cache: CacheFiles: A cache that backs onto a mounted filesystem

[David Howells <dhowells@redhat.com>]

Andrew Morton <akpm@osdl.org> 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