Re: [rfc][patch 3/5] afs: new aops

[David Howells <dhowells@redhat.com>]

Nick Piggin <npiggin@suse.de> wrote:

> -	ASSERTCMP(start + len, <=, PAGE_SIZE);
> +	ASSERTCMP(len, <=, PAGE_CACHE_SIZE);

Do you guarantee this will work if PAGE_CACHE_SIZE != PAGE_SIZE?  If not, you
can't make this particular change.

Do we ever intend to have PAGE_CACHE_SIZE != PAGE_SIZE?  If not, then surely
the former is redundant and should scrapped to avoid confusion?

> +	i_size = i_size_read(&vnode->vfs_inode);
> +	if (pos + len > i_size)
> +		eof = i_size;
> +	else
> +		eof = PAGE_CACHE_SIZE;
> +
> +	ret = afs_vnode_fetch_data(vnode, key, 0, eof, page);

That can't be right, surely.  Either 'eof' is the size of the file or it's the
length of the data to be read.  It can't be both.  The first case needs eof
masking off.  Also, 'eof' isn't a good choice of name.  'len' would be better
were it not already taken:-/

I notice you removed the stuff that clears holes in the page to be written.  Is
this is now done by the caller?

I notice also that you use afs_fill_page() in place of afs_prepare_page() to
prepare a page.  You can't do this if the region to be filled currently lies
beyond the server's idea of EOF.

I'll try and get a look at fixing this patch tomorrow.

David