Re: [PATCH] ext4: When reading from fallocated blocks make sure we return zero.

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

On Mon, Feb 18, 2008 at 04:14:34PM -0800, Mingming Cao wrote:
> On Sat, 2008-02-16 at 08:53 +0530, Aneesh Kumar K.V wrote:
> 
> How about the following patch?
> 
> Regards,
> Mingming
> 
> ext4: ext4_get_blocks_wrap fix for writing to preallocated
> From: Mingming Cao <cmm@us.ibm.com>
> 
> This patch fixed a issue with wrting to a preallocated blocks.
> A write hit a BUG_ON() in fs/buffer.c saying the buffer is not mapped.
> 
> On the write path, ext4_get_block_wrap() is called with create=1, but it
> will pass create=0 down to the underlying ext4ext_get_blocks()
> to do a look up first.  In the preallocation case, ext4_ext_get_blocks()
> with create = 0, will return number of blocks pre-allocated and buffer
> head unmapped. ext4_get_blocks_wrap() thinks it succeeds too early, without
> checking if it needs again call ext4_ext_get_blocks with create = 1
> which would do proper handling for writing to preallocated blocks:
> split the extent to initialized and uninitialized one and
> returns the mapped buffer head.
> 
> Treating preallocated blocks as holes equally(i.e. ignoring the number of blocks
> pre-allocated and returns 0) when get_blocks() is called with create = 0 is not enough.
> ext4_ext_get_blocks() needs to differentiate these two cases for delayed allocation
> purpose, as for holes it need to do reservation and prepare for later
> delayed allocation, but for pre-allocated blocks it needs skip that work.
> 
> It would makes things more clear if we have clear definition of what
> get_blocks() return value means.
> 
> Similar to ext4_get_blocks_handle(),  the following 
> * return > 0, # of blocks already allocated
> *          if these are pre-allocated blocks and create = 0
> *             buffer head is unmapped
> *          otherwise blocks are mapped.
> *
> * return = 0, if plain look up failed (blocks have not been allocated)
> *          buffer head is unmapped
> *
> * return < 0, error case.
> 
> The for the write path, at ext4_ext_get_blocks_wrap(), it could check the
> buffer_mapped() status for preallocated extent before quit too early.
> 
> Signed-off-by: Mingming Cao <cmm@us.ibm.com>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.co>



I guess we also need to make sure the buffer head have the mapped bit
set. Something like the patch below.

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index bc7081f..69ccda9 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2294,6 +2294,7 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 	struct ext4_allocation_request ar;
 
 	__clear_bit(BH_New, &bh_result->b_state);
+	__clear_bit(BH_Mapped, &bh_result->b_state);
 	ext_debug("blocks %u/%lu requested for inode %u\n",
 			iblock, max_blocks, inode->i_ino);