Re: [RFC] [PATCH] Generic mpage_writepage() support

[Nikita Danilov <nikita@clusterfs.com>]

Badari Pulavarty writes:
 > On Tue, 2005-02-15 at 09:54, Andrew Morton wrote:
 > > Badari Pulavarty <pbadari@us.ibm.com> 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.

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.

[...]

 > +	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. :)

 > +		unlock_page(page);
 > +		return 0; /* don't care */
 > +	}
 > +
 > +	/*
 > +	 * The page straddles i_size.  It must be zeroed out on each and every
 > +	 * writepage invokation because it may be mmapped.  "A file is mapped

Typo: should be invocation (at least beyond Australia).

Nikita.