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

[Mingming Cao <cmm@us.ibm.com>]

On Tue, 2008-02-19 at 08:49 +0530, Aneesh Kumar K.V wrote:
> 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.
> 
Good point. I modified the patch with clear_buffer_mapped() added at the
begining of the wrapper function.

Mingming
> 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);
>