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
