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

[Nick Piggin <npiggin@suse.de>]

On Mon, Nov 12, 2007 at 03:29:14PM +0000, David Howells wrote:
> 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.

PAGE_CACHE_SIZE should be used to address the pagecache.

 
> 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?

Maybe.


> > +	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:-/

Yeah, just missed the mask.

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

It is supposed to bring the page uptodate first. So, no need to clear
AFAIKS?

> 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.

No rush, it won't get into 2.6.24 obviously. But that would be nice, thanks.