On Thu, Jun 05, 2008 at 11:02:40AM -0700, Mingming Cao wrote:
> On Thu, 2008-06-05 at 15:25 +0530, Aneesh Kumar K.V wrote:
> > f) clear the delay bit in ext4_da_get_block_write instead of
> > __block_write_full_page
> > so that we clear the delay bit for every successfull block allocation.
> > We may fail
> > while marking inode dirty in ext4_da_get_block_write after allocating
> > block. So
> > it is better to clear the delay bit in ext4_da_get_block_write rather
> > than
> > __block_write_full_page
> >
>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> > ---
>
> > @@ -1555,7 +1565,15 @@ static int ext4_da_get_block_write(struct inode
> > *inode, sector_t iblock,
> > bh_result->b_size = (ret << inode->i_blkbits);
> >
> > /* release reserved-but-unused meta blocks */
> > - ext4_da_release_space(inode, ret, 0);
> > + if (buffer_delay(bh_result)) {
> > + ext4_da_release_space(inode, ret, 0);
> > + /*
> > + * clear the delay bit now that we allocated
> > + * blocks. If it is not a single block request
> > + * we clear the delay bit in
> > mpage_put_bnr_to_bhs
> > + */
> > + clear_buffer_delay(bh_result);
> > + }
> >
> > /*
> > * Update on-disk size along with block allocation
>
> It seems with this fix, the buffer_delay bit is still cleared before the
> ext4_mark_inode_dirty() could return error? Actually the already
> allocated blocks are leaked if mark_inode-dirty() returns error, and we
> cleared the buffer_delay for the buffer needs block.
>
How abou the below ? For single block request we are ok to clear the
delay bit as shown by the above patch. For multiple block request we
clear the delay bit if the buffer_head passed to the get_block have
its delay bit cleared. That should take care of the block leaking you
mentioned above.
diff --git a/fs/mpage.c b/fs/mpage.c
index c4376ec..2c90350 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -908,25 +908,41 @@ static void mpage_da_map_blocks(struct mpage_da_data *mpd)
new.b_blocknr = 0;
new.b_size = remain;
err = mpd->get_block(mpd->inode, next, &new, 1);
- if (err) {
+ /*
+ * we may have successfully allocated block. But
+ * failed to mark inode dirty. If we have allocated
+ * blocks update the buffer_head mappings
+ */
+ if (buffer_new(&new)) {
/*
- * Rather than implement own error handling
- * here, we just leave remaining blocks
- * unallocated and try again with ->writepage()
+ * buffer_head is only makred new if we have
+ * a successfull block allocation
*/
- break;
- }
- BUG_ON(new.b_size == 0);
-
- if (buffer_new(&new))
__unmap_underlying_blocks(mpd->inode, &new);
+ }
/*
* If blocks are delayed marked, we need to
* put actual blocknr and drop delayed bit
*/
- if (buffer_delay(lbh))
+ if (buffer_delay(lbh) && !buffer_delay(&new)) {
+ /*
+ * get_block if successfully allocated
+ * block will clear the delay bit of
+ * new buffer_head
+ */
mpage_put_bnr_to_bhs(mpd, next, &new);
+ }
+
+ if (err) {
+ /*
+ * Rather than implement own error handling
+ * here, we just leave remaining blocks
+ * unallocated and try again with ->writepage()
+ */
+ break;
+ }
+ BUG_ON(new.b_size == 0);
/* go for the remaining blocks */
next += new.b_size >> mpd->inode->i_blkbits;
--
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
