Re: [PATCH, RFC 2/2] ext4: Convert callers of ext4_get_blocks() to use ext4_map_blocks()

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

On Mon,  3 May 2010 18:51:21 -0400, "Theodore Ts'o" <tytso@mit.edu> wrote:
> @@ -2560,45 +2522,50 @@ static int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
>  		invalid_block = ~0;
> 
>  	BUG_ON(create == 0);
> -	BUG_ON(bh_result->b_size != inode->i_sb->s_blocksize);
> +	BUG_ON(bh->b_size != inode->i_sb->s_blocksize);
> +
> +	map.m_lblk = iblock;
> +	map.m_len = 1;
> 
>  	/*
>  	 * first, we need to know whether the block is allocated already
>  	 * preallocated blocks are unmapped but should treated
>  	 * the same as allocated blocks.
>  	 */
> -	ret = ext4_get_blocks(NULL, inode, iblock, 1,  bh_result, 0);
> -	if ((ret == 0) && !buffer_delay(bh_result)) {
> -		/* the block isn't (pre)allocated yet, let's reserve space */
> -		/*
> -		 * XXX: __block_prepare_write() unmaps passed block,
> -		 * is it OK?
> -		 */
> -		ret = ext4_da_reserve_space(inode, iblock);
> -		if (ret)
> -			/* not enough space to reserve */
> -			return ret;
> -
> -		map_bh(bh_result, inode->i_sb, invalid_block);
> -		set_buffer_new(bh_result);
> -		set_buffer_delay(bh_result);
> -	} else if (ret > 0) {
> -		bh_result->b_size = (ret << inode->i_blkbits);
> -		if (buffer_unwritten(bh_result)) {
> -			/* A delayed write to unwritten bh should
> -			 * be marked new and mapped.  Mapped ensures
> -			 * that we don't do get_block multiple times
> -			 * when we write to the same offset and new
> -			 * ensures that we do proper zero out for
> -			 * partial write.
> +	ret = ext4_map_blocks(NULL, inode, &map, 0);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0) {
> +		if (!buffer_delay(bh)) {


bh flags are not set here. This check should be based on map.m_flags.




> +			/*
> +			 * XXX: __block_prepare_write() unmaps passed
> +			 * block, is it OK?
>  			 */
> -			set_buffer_new(bh_result);
> -			set_buffer_mapped(bh_result);
> +			ret = ext4_da_reserve_space(inode, iblock);
> +			if (ret)
> +				return ret; /* not enough space to reserve */
> +
> +			map_bh(bh, inode->i_sb, invalid_block);
> +			set_buffer_new(bh);
> +			set_buffer_delay(bh);
>  		}
> -		ret = 0;
> +		return 0;
>  	}
> 

Only thing i am worried about is we were modifying bh_flags in all
possible confusing ways. We may want to make sure we get the update
correct. I am still going through the patch after applying it to the
tree. So may take more time to look at the full changeset.

-aneesh