On Mon, 2008-06-02 at 13:35 +0530, Aneesh Kumar K.V wrote: > On Mon, Jun 02, 2008 at 12:04:07AM -0700, Mingming Cao wrote: > > On Mon, 2008-06-02 at 12:05 +0530, Aneesh Kumar K.V wrote: > > > On Sun, Jun 01, 2008 at 10:38:35PM -0700, Mingming Cao wrote: > > > > On Mon, 2008-06-02 at 09:39 +0530, Aneesh Kumar K.V wrote: > > > > > On Sun, Jun 01, 2008 at 08:50:32PM -0700, Mingming Cao wrote: > > > > > > On Mon, 2008-06-02 at 08:44 +0530, Aneesh Kumar K.V wrote: > > > > > > > On Sun, Jun 01, 2008 at 02:10:02PM -0700, Mingming Cao wrote: > > > > > > > > ext4: Need clear buffer_delay after page writeout for delayed allocation > > > > > > > > > > > > > > > > From: Mingming Cao <cmm@xxxxxxxxxx> > > > > > > > > > > > > > > > > Need clear buffer_delay in ext4_da_writepage() after page has been writeout > > > > > > > > > > > > > > > > Signed-off-by: Mingming Cao <cmm@xxxxxxxxxx> > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > We do that in mpage_put_bnr_to_bhs. > > > > > > > > > > > > > Normally delayed buffer could be cleared in that case, but if allocation > > > > > > failed in __mapge_da_writepages(), it will keep buffer_delay marked and > > > > > > deferring to later ext4_da_writepage() to do block allocation. This > > > > > > patch handles clear bh delay bit in this case. > > > > > > > > > > > > > > > > Why not do it in ext4_da_get_block_write then. > > > > > > > > The buffer head passed to ext4_da_get_block_write() calling from > > > > mpage_da_map_blocks is a dummy one, to store the allocated extent, not > > > > the bh that need map. > > > > > > > > > ie true when ext4_da_get_block_write is called via writepages. In > > > that case mpage_put_bnr_to_bhs clears the delay bit properly. How about > > > the changes below. > > > > > I see your patch below is trying to address how to detect and assign > > blocks with your suggestion(i.e clear delayed bit in get_block). But I > > don;t think it's needed. > > > > My last email I mean the buffer head new in mpage_da_map_blocks() is a > > dummy bh, the real buffer head lbh is not passed to get_block. We could > > clear the delayed bit on successful return of get_block, > > mpage_put_bnr_to_bhs() ignore that dummy bh anyway. But that seems > > twisted, unccessary. > > > > I still think clear the bit in the ext4_da_write_page() is more clean > > way. the original patch clears the delayed bit on success case. > > > > For the error case I think we could handle properly by only clear the > > delayed bit if buffer is mapped. > > > Buffer marked as delay is also mapped > fs/ext4/inode.c > > 1437 map_bh(bh_result, inode->i_sb, 0); > 1438 set_buffer_new(bh_result); > 1439 set_buffer_delay(bh_result); > > I find a better place to handle this, it make sense to clear this bit in block_write_full_page() after get_block() returns successfully. This handles partial error case smoothly. Updated patch below (to replace the original patch) ext4: Need clear buffer_delay in block_write_full_page() after allocation From: Mingming Cao <cmm@xxxxxxxxxx> Normally delayed buffer could be cleared in mpage_da_map_blocks(), after blocks are successfully allocated. But if allocation failed, it will keep buffer_delay marked and deferring to later ext4_da_writepage()(via block_write_full_page()) to do block allocation. This patch handles clear bh delay bit in this case. Clear buffer_delay in block_write_full_page() after the block is allocated. This patch also fixed a bug in block_write_full_page() error case, we need to check the delayed flag before flush bh to disk when trying to recover from error. Signed-off-by: Mingming Cao <cmm@xxxxxxxxxx> --- fs/buffer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Index: linux-2.6.26-rc4/fs/buffer.c =================================================================== --- linux-2.6.26-rc4.orig/fs/buffer.c 2008-06-02 21:34:30.000000000 -0700 +++ linux-2.6.26-rc4/fs/buffer.c 2008-06-02 21:35:17.000000000 -0700 @@ -1697,6 +1697,7 @@ static int __block_write_full_page(struc err = get_block(inode, block, bh, 1); if (err) goto recover; + clear_buffer_delay(bh); if (buffer_new(bh)) { /* blockdev mappings never come here */ clear_buffer_new(bh); @@ -1775,7 +1776,8 @@ recover: bh = head; /* Recovery: lock and submit the mapped buffers */ do { - if (buffer_mapped(bh) && buffer_dirty(bh)) { + if (buffer_mapped(bh) && buffer_dirty(bh) + && !buffer_delay(bh)) { lock_buffer(bh); mark_buffer_async_write(bh); } else { -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html