On Wed, 2005-02-16 at 03:41, Nikita Danilov wrote: > Badari Pulavarty writes: > > On Tue, 2005-02-15 at 09:54, Andrew Morton wrote: > > > Badari Pulavarty <[EMAIL PROTECTED]> wrote: > > > > > > > > Yep. nobh_prepare_write() doesn't add any bufferheads. But > > > > we call block_write_full_page() even for "nobh" case, which > > > > does create bufferheads, attaches to the page and operates > > > > on them.. > > > > > > hmm, yeah, OK, we'll attach bh's in that case. It's a rare case though - > > > when a dirty page falls off the end of the LRU. There's no particular > > > reason why we cannot have a real mpage_writepage() which doesn't use bh's > > > and employ that. > > > > > > I coulda sworn we used to have one. > > > > Hi Andrew, > > > > Here is my first version of mpage_writepage() patch. > > I haven't handle the "confused" case yet - I need to > > pass a function pointer to handle it. Just for > > initial code review. I am still testing it. > > > > Thanks, > > Badari > > > > > > diff -Narup -X dontdiff linux-2.6.10/fs/ext2/inode.c > linux-2.6.10.nobh/fs/ext2/inode.c > > --- linux-2.6.10/fs/ext2/inode.c 2004-12-24 13:33:51.000000000 -0800 > > [...] > > > return ret; > > } > > + > > +/* > > + * The generic ->writepage function for address_spaces > > + */ > > This function doesn't look generic. It only works correctly with file > systems that store pointer to buffer head ring in page->private (at > least temporarily), otherwise code after page_has_buffers(page) check in > __mpage_writepage() will corrupt page->private. > > Actually, this looks confusing. I thought that main idea of mpage.c is > to get rid of buffer heads, and switch everything to bios. But looking > at the current code it seems that buffer heads are striking back: code > simply assumes that PG_private means "buffers in page->private", making > mpage.c effectively useless for file systems using page->private for > something else.
Let me put it this way, mpage.c doesn't create any new buffer heads, but if there are already attached to a page, it can deal with them. Yes. page->private is assumed for the bufferhead usage. Do you really need for handling page->private for non-bufferhead usage ? > > There is another reason why mpage_writepage() is a problematic choice > for ->writepage: __mpage_writepage() calls > page->mapping->a_ops->writepage() in "confused" case, which sounds like > infinite recursion. Yep. I need to fix (as mentioned earlier). > > [...] > > > + if (page->index >= end_index+1 || !offset) { > > + /* > > + * The page may have dirty, unmapped buffers. For example, > > + * they may have been added in ext3_writepage(). Make them > > + * freeable here, so the page does not leak. > > + */ > > + block_invalidatepage(page, 0); > > Shouldn't this be > > page->mapping->a_ops->invalidatepage(page, 0) > > ? To preserve external appearance of "genericity", that is. :) I think so. Thanks, Badari - 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