From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikita Danilov Subject: Re: [RFC] [PATCH] Generic mpage_writepage() support Date: Wed, 16 Feb 2005 14:41:41 +0300 Message-ID: <16915.12661.494053.290059@gargle.gargle.HOWL> References: <1108409415.20053.1278.camel@dyn318077bld.beaverton.ibm.com> <20050214134058.1402cfed.akpm@osdl.org> <1108430825.20053.1363.camel@dyn318077bld.beaverton.ibm.com> <20050214190556.07c4a0c9.akpm@osdl.org> <1108485967.20053.1438.camel@dyn318077bld.beaverton.ibm.com> <20050215095443.3e646401.akpm@osdl.org> <1108512141.20053.1490.camel@dyn318077bld.beaverton.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, ext2-devel Received: from [213.85.13.118] ([213.85.13.118]:21376 "EHLO tau.rusteko.ru") by vger.kernel.org with ESMTP id S262006AbVBPLlu (ORCPT ); Wed, 16 Feb 2005 06:41:50 -0500 To: Badari Pulavarty In-Reply-To: <1108512141.20053.1490.camel@dyn318077bld.beaverton.ibm.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org Badari Pulavarty writes: > On Tue, 2005-02-15 at 09:54, Andrew Morton wrote: > > Badari Pulavarty 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.