On Mon, Nov 12, 2007 at 03:29:14PM +0000, David Howells wrote: > Nick Piggin <npiggin@xxxxxxx> 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. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
