Message-ID: <4ACE7C17.106@rs.jp.nec.com>
Date:	Fri, 09 Oct 2009 08:56:07 +0900
From:	Akira Fujita <a-fujita@...jp.nec.com>
To:	Theodore Tso <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/2]ext4: Fix exchanged block count in EXT4_IOC_MOVE_EXT
 failure

Hi Ted,

If my patches do not seem to have a problem,
would you add them to the ext4 patch queue?

http://marc.info/?l=linux-ext4&m=125447192609335&w=2
http://marc.info/?l=linux-ext4&m=125447192709338&w=2

Regards,
Akira Fujita

Akira Fujita wrote:
> ext4: Fix block count in EXT4_IOC_MOVE_EXT failure
> 
> From: Akira Fujita <a-fujita@...jp.nec.com>
> 
> The halfway exchanged block count should be returned to the user-space,
> if EXT4_IOC_MOVE_EXT is failed for some reasons
> in the environment where block size is not same as page size.
> But current EXT4_IOC_MOVE_EXT returns
> page-aligned block count to the user-space.
> The patch fixes this issue.
> 
> Signed-off-by: Akira Fujita <a-fujita@...jp.nec.com>
> ---
>  fs/ext4/move_extent.c |  139 ++++++++++++++++++++++++++-----------------------
>  1 files changed, 73 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 25b6b14..83f8c9e 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -661,6 +661,7 @@ mext_calc_swap_extents(struct ext4_extent *tmp_dext,
>   * @donor_inode:	donor inode
>   * @from:		block offset of orig_inode
>   * @count:		block count to be replaced
> + * @err:		pointer to save return value
>   *
>   * Replace original inode extents and donor inode extents page by page.
>   * We implement this replacement in the following three steps:
> @@ -671,19 +672,18 @@ mext_calc_swap_extents(struct ext4_extent *tmp_dext,
>   * 3. Change the block information of donor inode to point at the saved
>   *    original inode blocks in the dummy extents.
>   *
> - * Return 0 on success, or a negative error value on failure.
> + * Return replaced block count.
>   */
>  static int
>  mext_replace_branches(handle_t *handle, struct inode *orig_inode,
>  			   struct inode *donor_inode, ext4_lblk_t from,
> -			   ext4_lblk_t count)
> +			   ext4_lblk_t count, int *err)
>  {
>  	struct ext4_ext_path *orig_path = NULL;
>  	struct ext4_ext_path *donor_path = NULL;
>  	struct ext4_extent *oext, *dext;
>  	struct ext4_extent tmp_dext, tmp_oext;
>  	ext4_lblk_t orig_off = from, donor_off = from;
> -	int err = 0;
>  	int depth;
>  	int replaced_count = 0;
>  	int dext_alen;
> @@ -691,13 +691,13 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
>  	mext_double_down_write(orig_inode, donor_inode);
> 
>  	/* Get the original extent for the block "orig_off" */
> -	err = get_ext_path(orig_inode, orig_off, &orig_path);
> -	if (err)
> +	*err = get_ext_path(orig_inode, orig_off, &orig_path);
> +	if (*err)
>  		goto out;
> 
>  	/* Get the donor extent for the head */
> -	err = get_ext_path(donor_inode, donor_off, &donor_path);
> -	if (err)
> +	*err = get_ext_path(donor_inode, donor_off, &donor_path);
> +	if (*err)
>  		goto out;
>  	depth = ext_depth(orig_inode);
>  	oext = orig_path[depth].p_ext;
> @@ -707,9 +707,9 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
>  	dext = donor_path[depth].p_ext;
>  	tmp_dext = *dext;
> 
> -	err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,
> +	*err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,
>  				      donor_off, count);
> -	if (err)
> +	if (*err)
>  		goto out;
> 
>  	/* Loop for the donor extents */
> @@ -718,7 +718,7 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
>  		if (!dext) {
>  			ext4_error(donor_inode->i_sb, __func__,
>  				   "The extent for donor must be found");
> -			err = -EIO;
> +			*err = -EIO;
>  			goto out;
>  		} else if (donor_off != le32_to_cpu(tmp_dext.ee_block)) {
>  			ext4_error(donor_inode->i_sb, __func__,
> @@ -726,20 +726,20 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
>  				"extent(%u) should be equal",
>  				donor_off,
>  				le32_to_cpu(tmp_dext.ee_block));
> -			err = -EIO;
> +			*err = -EIO;
>  			goto out;
>  		}
> 
>  		/* Set donor extent to orig extent */
> -		err = mext_leaf_block(handle, orig_inode,
> +		*err = mext_leaf_block(handle, orig_inode,
>  					   orig_path, &tmp_dext, &orig_off);
> -		if (err < 0)
> +		if (*err)
>  			goto out;
> 
>  		/* Set orig extent to donor extent */
> -		err = mext_leaf_block(handle, donor_inode,
> +		*err = mext_leaf_block(handle, donor_inode,
>  					   donor_path, &tmp_oext, &donor_off);
> -		if (err < 0)
> +		if (*err)
>  			goto out;
> 
>  		dext_alen = ext4_ext_get_actual_len(&tmp_dext);
> @@ -753,35 +753,25 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
> 
>  		if (orig_path)
>  			ext4_ext_drop_refs(orig_path);
> -		err = get_ext_path(orig_inode, orig_off, &orig_path);
> -		if (err)
> +		*err = get_ext_path(orig_inode, orig_off, &orig_path);
> +		if (*err)
>  			goto out;
>  		depth = ext_depth(orig_inode);
>  		oext = orig_path[depth].p_ext;
> -		if (le32_to_cpu(oext->ee_block) +
> -				ext4_ext_get_actual_len(oext) <= orig_off) {
> -			err = 0;
> -			goto out;
> -		}
>  		tmp_oext = *oext;
> 
>  		if (donor_path)
>  			ext4_ext_drop_refs(donor_path);
> -		err = get_ext_path(donor_inode, donor_off, &donor_path);
> -		if (err)
> +		*err = get_ext_path(donor_inode, donor_off, &donor_path);
> +		if (*err)
>  			goto out;
>  		depth = ext_depth(donor_inode);
>  		dext = donor_path[depth].p_ext;
> -		if (le32_to_cpu(dext->ee_block) +
> -				ext4_ext_get_actual_len(dext) <= donor_off) {
> -			err = 0;
> -			goto out;
> -		}
>  		tmp_dext = *dext;
> 
> -		err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,
> +		*err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,
>  					   donor_off, count - replaced_count);
> -		if (err)
> +		if (*err)
>  			goto out;
>  	}
> 
> @@ -796,7 +786,7 @@ out:
>  	}
> 
>  	mext_double_up_write(orig_inode, donor_inode);
> -	return err;
> +	return replaced_count;
>  }
> 
>  /**
> @@ -808,16 +798,17 @@ out:
>   * @data_offset_in_page:	block index where data swapping starts
>   * @block_len_in_page:		the number of blocks to be swapped
>   * @uninit:			orig extent is uninitialized or not
> + * @err:			pointer to save return value
>   *
>   * Save the data in original inode blocks and replace original inode extents
>   * with donor inode extents by calling mext_replace_branches().
> - * Finally, write out the saved data in new original inode blocks. Return 0
> - * on success, or a negative error value on failure.
> + * Finally, write out the saved data in new original inode blocks. Return
> + * replaced block count.
>   */
>  static int
>  move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  		  pgoff_t orig_page_offset, int data_offset_in_page,
> -		  int block_len_in_page, int uninit)
> +		  int block_len_in_page, int uninit, int *err)
>  {
>  	struct inode *orig_inode = o_filp->f_dentry->d_inode;
>  	struct address_space *mapping = orig_inode->i_mapping;
> @@ -829,9 +820,11 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	long long offs = orig_page_offset << PAGE_CACHE_SHIFT;
>  	unsigned long blocksize = orig_inode->i_sb->s_blocksize;
>  	unsigned int w_flags = 0;
> -	unsigned int tmp_data_len, data_len;
> +	unsigned int tmp_data_size, data_size, replaced_size;
>  	void *fsdata;
> -	int ret, i, jblocks;
> +	int i, jblocks;
> +	int err2 = 0;
> +	int replaced_count = 0;
>  	int blocks_per_page = PAGE_CACHE_SIZE >> orig_inode->i_blkbits;
> 
>  	/*
> @@ -841,8 +834,8 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	jblocks = ext4_writepage_trans_blocks(orig_inode) * 2;
>  	handle = ext4_journal_start(orig_inode, jblocks);
>  	if (IS_ERR(handle)) {
> -		ret = PTR_ERR(handle);
> -		return ret;
> +		*err = PTR_ERR(handle);
> +		return 0;
>  	}
> 
>  	if (segment_eq(get_fs(), KERNEL_DS))
> @@ -858,9 +851,9 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	 * Just swap data blocks between orig and donor.
>  	 */
>  	if (uninit) {
> -		ret = mext_replace_branches(handle, orig_inode,
> -						 donor_inode, orig_blk_offset,
> -						 block_len_in_page);
> +		replaced_count = mext_replace_branches(handle, orig_inode,
> +						donor_inode, orig_blk_offset,
> +						block_len_in_page, err);
> 
>  		/* Clear the inode cache not to refer to the old data */
>  		ext4_ext_invalidate_cache(orig_inode);
> @@ -870,27 +863,28 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
> 
>  	offs = (long long)orig_blk_offset << orig_inode->i_blkbits;
> 
> -	/* Calculate data_len */
> +	/* Calculate data_size */
>  	if ((orig_blk_offset + block_len_in_page - 1) ==
>  	    ((orig_inode->i_size - 1) >> orig_inode->i_blkbits)) {
>  		/* Replace the last block */
> -		tmp_data_len = orig_inode->i_size & (blocksize - 1);
> +		tmp_data_size = orig_inode->i_size & (blocksize - 1);
>  		/*
> -		 * If data_len equal zero, it shows data_len is multiples of
> +		 * If data_size equal zero, it shows data_size is multiples of
>  		 * blocksize. So we set appropriate value.
>  		 */
> -		if (tmp_data_len == 0)
> -			tmp_data_len = blocksize;
> +		if (tmp_data_size == 0)
> +			tmp_data_size = blocksize;
> 
> -		data_len = tmp_data_len +
> +		data_size = tmp_data_size +
>  			((block_len_in_page - 1) << orig_inode->i_blkbits);
> -	} else {
> -		data_len = block_len_in_page << orig_inode->i_blkbits;
> -	}
> +	} else
> +		data_size = block_len_in_page << orig_inode->i_blkbits;
> +
> +	replaced_size = data_size;
> 
> -	ret = a_ops->write_begin(o_filp, mapping, offs, data_len, w_flags,
> +	*err = a_ops->write_begin(o_filp, mapping, offs, data_size, w_flags,
>  				 &page, &fsdata);
> -	if (unlikely(ret < 0))
> +	if (unlikely(*err < 0))
>  		goto out;
> 
>  	if (!PageUptodate(page)) {
> @@ -911,10 +905,17 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	/* Release old bh and drop refs */
>  	try_to_release_page(page, 0);
> 
> -	ret = mext_replace_branches(handle, orig_inode, donor_inode,
> -					 orig_blk_offset, block_len_in_page);
> -	if (ret < 0)
> -		goto out;
> +	replaced_count = mext_replace_branches(handle, orig_inode, donor_inode,
> +					orig_blk_offset, block_len_in_page,
> +					&err2);
> +	if (err2) {
> +		if (replaced_count) {
> +			block_len_in_page = replaced_count;
> +			replaced_size =
> +				block_len_in_page << orig_inode->i_blkbits;
> +		} else
> +			goto out;
> +	}
> 
>  	/* Clear the inode cache not to refer to the old data */
>  	ext4_ext_invalidate_cache(orig_inode);
> @@ -928,16 +929,16 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  		bh = bh->b_this_page;
> 
>  	for (i = 0; i < block_len_in_page; i++) {
> -		ret = ext4_get_block(orig_inode,
> +		*err = ext4_get_block(orig_inode,
>  				(sector_t)(orig_blk_offset + i), bh, 0);
> -		if (ret < 0)
> +		if (*err < 0)
>  			goto out;
> 
>  		if (bh->b_this_page != NULL)
>  			bh = bh->b_this_page;
>  	}
> 
> -	ret = a_ops->write_end(o_filp, mapping, offs, data_len, data_len,
> +	*err = a_ops->write_end(o_filp, mapping, offs, data_size, replaced_size,
>  			       page, fsdata);
>  	page = NULL;
> 
> @@ -951,7 +952,10 @@ out:
>  out2:
>  	ext4_journal_stop(handle);
> 
> -	return ret < 0 ? ret : 0;
> +	if (err2)
> +		*err = err2;
> +
> +	return replaced_count;
>  }
> 
>  /**
> @@ -1367,15 +1371,17 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>  		while (orig_page_offset <= seq_end_page) {
> 
>  			/* Swap original branches with new branches */
> -			ret1 = move_extent_per_page(o_filp, donor_inode,
> +			block_len_in_page = move_extent_per_page(
> +						o_filp, donor_inode,
>  						orig_page_offset,
>  						data_offset_in_page,
> -						block_len_in_page, uninit);
> -			if (ret1 < 0)
> -				goto out;
> -			orig_page_offset++;
> +						block_len_in_page, uninit,
> +						&ret1);
> +
>  			/* Count how many blocks we have exchanged */
>  			*moved_len += block_len_in_page;
> +			if (ret1 < 0)
> +				goto out;
>  			if (*moved_len > len) {
>  				ext4_error(orig_inode->i_sb, __func__,
>  					"We replaced blocks too much! "
> @@ -1385,6 +1391,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>  				goto out;
>  			}
> 
> +			orig_page_offset++;
>  			data_offset_in_page = 0;
>  			rest_blocks -= block_len_in_page;
>  			if (rest_blocks > blocks_per_page)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Akira Fujita <a-fujita@...jp.nec.com>

The First Fundamental Software Development Group,
Software Development Division,
NEC Software Tohoku, Ltd.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists