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

David Howells
2006-04-21T14:50:18Z

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