Re: [PATCH] ext4: Need clear buffer_delay after page writeout for delayed allocation

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

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@us.ibm.com>
> > > > > > > 
> > > > > > > Need clear buffer_delay in ext4_da_writepage() after page has been writeout
> > > > > > > 
> > > > > > > Signed-off-by: Mingming Cao <cmm@us.ibm.com>
> > > > > > > 
> > > > > > > ---
> > > > > > 
> > > > > > 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);



> 
> Mingming
> > 
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 26f3626..9d25255 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -2873,6 +2873,7 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
> >  		allocated = max_blocks;
> >  	ext4_ext_show_leaf(inode, path);
> >  	__set_bit(BH_Mapped, &bh_result->b_state);
> > +	__clear_bit(BH_Delay, &bh_result->b_state);
> >  	bh_result->b_bdev = inode->i_sb->s_bdev;
> >  	bh_result->b_blocknr = newblock;
> >  out2:
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 2194aa7..2a2f832 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -917,6 +917,7 @@ int ext4_get_blocks_handle(handle_t *handle, struct inode *inode,
> >  		goto cleanup;
> > 
> >  	set_buffer_new(bh_result);
> > +	clear_buffer_delay(bh_result);
> >  got_it:
> >  	map_bh(bh_result, inode->i_sb, le32_to_cpu(chain[depth-1].key));
> >  	if (count > blocks_to_boundary)


I think we need only the above chage. since we are using buffer_head new
for block request in writepages. The below change can be dropped. It
handles all the error case.



> > diff --git a/fs/mpage.c b/fs/mpage.c
> > index c107728..0d05d7a 100644
> > --- a/fs/mpage.c
> > +++ b/fs/mpage.c
> > @@ -897,6 +897,14 @@ static void mpage_da_map_blocks(struct mpage_da_data *mpd)
> >  	int err = 0, remain = lbh->b_size;
> >  	sector_t next = lbh->b_blocknr;
> >  	struct buffer_head new;
> > +	int delay_buf = 0;
> > +
> > +	/*
> > +	 * We clear the delay bit in get_block so remember
> > +	 * whether we are doing a delay allocation
> > +	 */
> > +	if (buffer_delay(lbh))
> > +		delay_buf = 1;
> > 
> >  	/*
> >  	 * We consider only non-mapped and non-allocated blocks
> > @@ -926,7 +934,7 @@ static void mpage_da_map_blocks(struct mpage_da_data *mpd)
> >  		 * If blocks are delayed marked, we need to
> >  		 * put actual blocknr and drop delayed bit
> >  		 */
> > -		if (buffer_delay(lbh))
> > +		if (delay_buf)
> >  			mpage_put_bnr_to_bhs(mpd, next, &new);
> > 
> >  		/* go for the remaining blocks */
> > 
> 

-aneesh