Re: [rfc][patch 3/5] afs: new aops
On Thu, Nov 15, 2007 at 12:15:41PM +, David Howells wrote: > Nick Piggin <[EMAIL PROTECTED]> wrote: > > > > So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an > > > area of PAGE_SIZE? > > > > No, a pagecache page is PAGE_CACHE_SIZE. > > That doesn't answer my question. I didn't ask about 'pagecache pages' per se. > > Are you saying then that a page struct always represents an area of PAGE_SIZE > to, say, the page allocator and PAGE_CACHE_SIZE to a filesystem's address > operations? Yes. > How about I state it this way: Please define what the coverage of a > (non-compound) struct page is, and how this relates to PAGE_SIZE and > PAGE_CACHE_SIZE. If it's well-defined then this cannot be hard, right? No it's easy. It's PAGE_SIZE (which also happens to be PAGE_CACHE_SIZE). An implementation that would (not trivially, but without changing the basic concepts) allow a larger pagecache size is with compound pages. And actually hugetlbfs already does this. > > And not all struct pages control the same amount of data anyway, with > > compound pages. > > Compound pages are irrelevant to my question. A compound page is actually a > regulated by a series of page structs, each of which represents a 'page' of > real memory. And it can also represent a PAGE_CACHE_SIZE page of pagecache. > Do you say, then, that all, say, readpage() and readpages() methods must > handle a compound page if that is given to them? I'm not talking about a specific implementation that allows PAGE_CACHE_SIZE > PAGE_SIZE. So no, I don't say anything about that. I say that pagecache pages are PAGE_CACHE_SIZE! Yes it is easy now because that is the same as PAGE_SIZE. Yes it will be harder if you wanted to change that. What you would not have to change is the assumption that pagecache pages are in PAGE_SIZE units. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
Nick Piggin <[EMAIL PROTECTED]> wrote: > > So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an > > area of PAGE_SIZE? > > No, a pagecache page is PAGE_CACHE_SIZE. That doesn't answer my question. I didn't ask about 'pagecache pages' per se. Are you saying then that a page struct always represents an area of PAGE_SIZE to, say, the page allocator and PAGE_CACHE_SIZE to a filesystem's address operations? How about I state it this way: Please define what the coverage of a (non-compound) struct page is, and how this relates to PAGE_SIZE and PAGE_CACHE_SIZE. If it's well-defined then this cannot be hard, right? > And not all struct pages control the same amount of data anyway, with > compound pages. Compound pages are irrelevant to my question. A compound page is actually a regulated by a series of page structs, each of which represents a 'page' of real memory. Do you say, then, that all, say, readpage() and readpages() methods must handle a compound page if that is given to them? David - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
On Wed, Nov 14, 2007 at 03:57:46PM +, David Howells wrote: > Nick Piggin <[EMAIL PROTECTED]> wrote: > > > In core code, the PAGE_CACHE_SIZE is for page cache struct pages. Single > > struct pages (not page arrays). Take a look at generic mapping read or > > something. > > So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an > area of PAGE_SIZE? No, a pagecache page is PAGE_CACHE_SIZE. And not all struct pages control the same amount of data anyway, with compound pages. > Is a struct page then a purely pagecache concept? > > > Documentation is the opposite of convention ;) > > If it's not Documented, then it's irrelevant. But you can't just decide yourself that it is irrelevant because you don't grep hard enough ;) include/linux/mm.h: * A page may belong to an inode's memory mapping. In this case, page->mapping * is the pointer to the inode, and page->index is the file offset of the page, * in units of PAGE_CACHE_SIZE. include/linux/mm_types.h unsigned long vm_pgoff; /* Offset (within vm_file) in PAGE_SIZE units, *not* PAGE_CACHE_SIZE */ Looks like documentation to me. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
Nick Piggin <[EMAIL PROTECTED]> wrote: > In core code, the PAGE_CACHE_SIZE is for page cache struct pages. Single > struct pages (not page arrays). Take a look at generic mapping read or > something. So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an area of PAGE_SIZE? Is a struct page then a purely pagecache concept? > Documentation is the opposite of convention ;) If it's not Documented, then it's irrelevant. David - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
On Wed, Nov 14, 2007 at 12:18:43PM +, David Howells wrote: > Nick Piggin <[EMAIL PROTECTED]> wrote: > > > > The problem is that the code called assumes that the struct page * > > > argument points to a single page, not an array of pages as would > > > presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE. > > > > Incorrect. Christoph's patch for example does this by using compound pages. > > Now I personally don't like the patch or see the point in PAGE_CACHE_SIZE / > > PAGE_SIZE distinction, but I'm just telling you what the convention is. > > There > > is no point you arguing against it, that's simply how it is. > > No! You are wrong. I wrote the AFS code. I *know* it can only deal with No I'm talking about core code. In core code, the PAGE_CACHE_SIZE is for page cache struct pages. Single struct pages (not page arrays). Take a look at generic mapping read or something. There is nothing to deal with page arrays there either, but that's simply the convention. > > > So: you may not change the assertion unless you also fix the lower > > > functions. > > > > I won't change the assertion, because you haven't been following said > > convention, so just changing it in one place is stupider than not changing > > it at all, but not for the reason cited. > > The convention is not precisely clear. Just grep for PAGE_CACHE_SIZE in > Documentation/. It's only mentioned twice, and in neither case does it give > any information about what PAGE_CACHE_SIZE is, what it represents, or where it > applies. Therefore it's an ill-defined concept. > > If you look in Documentation/filesystems/vfs.txt, you'll see that it almost > always talks about 'pages'. It only mentions 'pagecache pages' once - in the > description of write_begin(), but it's not clear whether that means anything. Documentation is the opposite of convention ;) Look in mm/. > However, I've now noted that I need to fix my code, so just keep the assertion > for now and I'll fix my code to handle multipage blocks. I'm not saying you need to do that. Leave it at PAGE_SIZE, really it doesn't matter that much at present. This has just blown out of proportion. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
Nick Piggin <[EMAIL PROTECTED]> wrote: > > The problem is that the code called assumes that the struct page * > > argument points to a single page, not an array of pages as would > > presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE. > > Incorrect. Christoph's patch for example does this by using compound pages. > Now I personally don't like the patch or see the point in PAGE_CACHE_SIZE / > PAGE_SIZE distinction, but I'm just telling you what the convention is. There > is no point you arguing against it, that's simply how it is. No! You are wrong. I wrote the AFS code. I *know* it can only deal with single pages. It has no knowledge of compound pages and does not handle page arrays. This may be a flaw in my code, but it's there nonetheless. The assertion is a guard against that. *That* was the point of my statement. Perhaps I should've said 'my code' rather than 'the code'. If Christoph has a patch to deal with that, it's either not upstream yet or it hasn't altered AFS. > > So: you may not change the assertion unless you also fix the lower > > functions. > > I won't change the assertion, because you haven't been following said > convention, so just changing it in one place is stupider than not changing > it at all, but not for the reason cited. The convention is not precisely clear. Just grep for PAGE_CACHE_SIZE in Documentation/. It's only mentioned twice, and in neither case does it give any information about what PAGE_CACHE_SIZE is, what it represents, or where it applies. Therefore it's an ill-defined concept. If you look in Documentation/filesystems/vfs.txt, you'll see that it almost always talks about 'pages'. It only mentions 'pagecache pages' once - in the description of write_begin(), but it's not clear whether that means anything. However, I've now noted that I need to fix my code, so just keep the assertion for now and I'll fix my code to handle multipage blocks. David - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
On Tue, Nov 13, 2007 at 10:56:25AM +, David Howells wrote: > Nick Piggin <[EMAIL PROTECTED]> wrote: > > > It takes a pagecache page, yes. If you follow convention, you use > > PAGE_CACHE_SIZE for that guy. You don't have to allow PAGE_CACHE_SIZE != > > PAGE_SIZE, and if all the rest of your code is in units of PAGE_SIZE, then > > obviously my changing of just the one unit is even more confusing than > > the current arrangement ;) > > The problem is that the code called assumes that the struct page * argument > points to a single page, not an array of pages as would presumably be the case > if PAGE_CACHE_SIZE > PAGE_SIZE. Incorrect. Christoph's patch for example does this by using compound pages. Now I personally don't like the patch or see the point in PAGE_CACHE_SIZE / PAGE_SIZE distinction, but I'm just telling you what the convention is. There is no point you arguing against it, that's simply how it is. > If I should allow for an array of pages then > the lower functions (specifically afs_deliver_fs_fetch_data()) need to change, > and until that time occurs, the assertion *must* remain as it is now. It > defends the lower functions against being asked to do something they weren't > designed to do. > > So: you may not change the assertion unless you also fix the lower functions. I won't change the assertion, because you haven't been following said convention, so just changing it in one place is stupider than not changing it at all, but not for the reason cited. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
Nick Piggin <[EMAIL PROTECTED]> wrote: > It takes a pagecache page, yes. If you follow convention, you use > PAGE_CACHE_SIZE for that guy. You don't have to allow PAGE_CACHE_SIZE != > PAGE_SIZE, and if all the rest of your code is in units of PAGE_SIZE, then > obviously my changing of just the one unit is even more confusing than > the current arrangement ;) The problem is that the code called assumes that the struct page * argument points to a single page, not an array of pages as would presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE. If I should allow for an array of pages then the lower functions (specifically afs_deliver_fs_fetch_data()) need to change, and until that time occurs, the assertion *must* remain as it is now. It defends the lower functions against being asked to do something they weren't designed to do. So: you may not change the assertion unless you also fix the lower functions. David - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
On Tue, Nov 13, 2007 at 12:30:05AM +, David Howells wrote: > Nick Piggin <[EMAIL PROTECTED]> wrote: > > > PAGE_CACHE_SIZE should be used to address the pagecache. > > Perhaps, but the function being called from there takes pages not page cache > slots. If I have to allow for PAGE_CACHE_SIZE > PAGE_SIZE then I need to > modify my code, if not then the assertion needs to remain what it is. It takes a pagecache page, yes. If you follow convention, you use PAGE_CACHE_SIZE for that guy. You don't have to allow PAGE_CACHE_SIZE != PAGE_SIZE, and if all the rest of your code is in units of PAGE_SIZE, then obviously my changing of just the one unit is even more confusing than the current arrangement ;) > > > 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? > > Hmmm... I suppose. However, it is wasteful in the common case as it is then > bringing the page up to date by filling/clearing the whole of it and not just > the bits that are not going to be written. Yes, that's where you come in. You are free (and encouraged) to optimise this. Let's see, for a network filesystem this is what you could do: - if the page is not uptodate at write_begin time, do not bring it uptodate (at least, not the region that is going to be written to) - if the page is not uptodate at write_end time, but the copy was fully completed, just mark it uptodate (provided you brought the regions outside the copy uptodate). - if the page is not uptodate and you have a short copy, simply do not mark the page uptodate or dirty, and return 0 from write_end, indicating that you have committed 0 bytes. The generic code should DTRT. Or you could: Pass back a temporary (not pagecache) page in *pagep, and copy that yourself into the _real_ pagecache page at write_end time, so you know exactly how big the copy will be (this is basically what the 2copy method does now... it is probably not as good as the first method I described, but for a high latency filesystem it may be preferable to always bringing the page uptodate). Or: keep track of sub-page dirty / uptodate status eg. with a light weight buffer_head like structure, so you can actually have partially dirty pages that are not completely uptodate. Or... if you can think of another way... - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
Nick Piggin <[EMAIL PROTECTED]> wrote: > PAGE_CACHE_SIZE should be used to address the pagecache. Perhaps, but the function being called from there takes pages not page cache slots. If I have to allow for PAGE_CACHE_SIZE > PAGE_SIZE then I need to modify my code, if not then the assertion needs to remain what it is. > > 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? Hmmm... I suppose. However, it is wasteful in the common case as it is then bringing the page up to date by filling/clearing the whole of it and not just the bits that are not going to be written. David - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
On Mon, Nov 12, 2007 at 03:29:14PM +, David Howells wrote: > Nick Piggin <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rfc][patch 3/5] afs: new aops
Nick Piggin <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[rfc][patch 3/5] afs: new aops
Convert afs to new aops.
Cannot assume writes will fully complete, so this conversion goes the easy
way and always brings the page uptodate before the write.
Signed-off-by: Nick Piggin <[EMAIL PROTECTED]>
---
Index: linux-2.6/fs/afs/file.c
===
--- linux-2.6.orig/fs/afs/file.c
+++ linux-2.6/fs/afs/file.c
@@ -50,8 +50,8 @@ const struct address_space_operations af
.launder_page = afs_launder_page,
.releasepage= afs_releasepage,
.invalidatepage = afs_invalidatepage,
- .prepare_write = afs_prepare_write,
- .commit_write = afs_commit_write,
+ .write_begin= afs_write_begin,
+ .write_end = afs_write_end,
.writepage = afs_writepage,
.writepages = afs_writepages,
};
Index: linux-2.6/fs/afs/internal.h
===
--- linux-2.6.orig/fs/afs/internal.h
+++ linux-2.6/fs/afs/internal.h
@@ -731,8 +731,12 @@ extern int afs_volume_release_fileserver
*/
extern int afs_set_page_dirty(struct page *);
extern void afs_put_writeback(struct afs_writeback *);
-extern int afs_prepare_write(struct file *, struct page *, unsigned, unsigned);
-extern int afs_commit_write(struct file *, struct page *, unsigned, unsigned);
+extern int afs_write_begin(struct file *file, struct address_space *mapping,
+ loff_t pos, unsigned len, unsigned flags,
+ struct page **pagep, void **fsdata);
+extern int afs_write_end(struct file *file, struct address_space *mapping,
+ loff_t pos, unsigned len, unsigned copied,
+ struct page *page, void *fsdata);
extern int afs_writepage(struct page *, struct writeback_control *);
extern int afs_writepages(struct address_space *, struct writeback_control *);
extern int afs_write_inode(struct inode *, int);
Index: linux-2.6/fs/afs/write.c
===
--- linux-2.6.orig/fs/afs/write.c
+++ linux-2.6/fs/afs/write.c
@@ -84,15 +84,23 @@ void afs_put_writeback(struct afs_writeb
* partly or wholly fill a page that's under preparation for writing
*/
static int afs_fill_page(struct afs_vnode *vnode, struct key *key,
-unsigned start, unsigned len, struct page *page)
+ loff_t pos, unsigned len, struct page *page)
{
+ loff_t i_size;
+ unsigned eof;
int ret;
- _enter(",,%u,%u", start, len);
+ _enter(",,%llu,%u", (unsigned long long)pos, len);
- ASSERTCMP(start + len, <=, PAGE_SIZE);
+ ASSERTCMP(len, <=, PAGE_CACHE_SIZE);
- ret = afs_vnode_fetch_data(vnode, key, start, len, page);
+ 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);
if (ret < 0) {
if (ret == -ENOENT) {
_debug("got NOENT from server"
@@ -107,109 +115,55 @@ static int afs_fill_page(struct afs_vnod
}
/*
- * prepare a page for being written to
- */
-static int afs_prepare_page(struct afs_vnode *vnode, struct page *page,
- struct key *key, unsigned offset, unsigned to)
-{
- unsigned eof, tail, start, stop, len;
- loff_t i_size, pos;
- void *p;
- int ret;
-
- _enter("");
-
- if (offset == 0 && to == PAGE_SIZE)
- return 0;
-
- p = kmap_atomic(page, KM_USER0);
-
- i_size = i_size_read(&vnode->vfs_inode);
- pos = (loff_t) page->index << PAGE_SHIFT;
- if (pos >= i_size) {
- /* partial write, page beyond EOF */
- _debug("beyond");
- if (offset > 0)
- memset(p, 0, offset);
- if (to < PAGE_SIZE)
- memset(p + to, 0, PAGE_SIZE - to);
- kunmap_atomic(p, KM_USER0);
- return 0;
- }
-
- if (i_size - pos >= PAGE_SIZE) {
- /* partial write, page entirely before EOF */
- _debug("before");
- tail = eof = PAGE_SIZE;
- } else {
- /* partial write, page overlaps EOF */
- eof = i_size - pos;
- _debug("overlap %u", eof);
- tail = max(eof, to);
- if (tail < PAGE_SIZE)
- memset(p + tail, 0, PAGE_SIZE - tail);
- if (offset > eof)
- memset(p + eof, 0, PAGE_SIZE - eof);
- }
-
- kunmap_atomic(p, KM_USER0);
-
- ret = 0;
- if (offset > 0 || eof > to) {
- /* need to fill one or two bits that aren't going to be written
-* (cover both fillers in one read if there are two) */
- start = (offset > 0) ? 0 : to;
- stop = (eof > to) ?
