[RESEND,4/9] Btrfs: fix put dio bio twice when we submit dio bio fail
diff mbox

Message ID 1404382933-26672-4-git-send-email-miaox@cn.fujitsu.com
State Superseded
Headers show

Commit Message

Miao Xie July 3, 2014, 10:22 a.m. UTC
The caller of btrfs_submit_direct_hook() will put the original dio bio
when btrfs_submit_direct_hook() return a error number, so we needn't
put the original bio in btrfs_submit_direct_hook().

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/inode.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Satoru Takeuchi July 4, 2014, 1:58 a.m. UTC | #1
Hi Miao,

(2014/07/03 19:22), Miao Xie wrote:
> The caller of btrfs_submit_direct_hook() will put the original dio bio
> when btrfs_submit_direct_hook() return a error number, so we needn't
> put the original bio in btrfs_submit_direct_hook().
> 
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>

Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

Here is the review result, CMIIW.

call trace:
  btrfs_submit_direct
    -> btrfs_submit_direct_hook

fs/btrfs/inode.c:
===============================================================================
static int btrfs_submit_direct_hook(int rw, struct btrfs_dio_private *dip,
				    int skip_sum)
{
	...
	struct bio *orig_bio = dip->orig_bio;
	...
	map_length = orig_bio->bi_iter.bi_size;
	ret = btrfs_map_block(root->fs_info, rw, start_sector << 9,
			      &map_length, NULL, 0);
	if (ret) {
		bio_put(orig_bio); # (1)
		return -EIO;
	}
	...
}
...
static void btrfs_submit_direct(int rw, struct bio *dio_bio,
				struct inode *inode, loff_t file_offset)
{
	dip = kmalloc(sizeof(*dip) + sum_len, GFP_NOFS);
	if (!dip) {
		ret = -ENOMEM;
		goto free_io_bio; # (2)
	}
	...
	ret = btrfs_submit_direct_hook(rw, dip, skip_sum);
	if (!ret)
		return;

free_io_bio:
	bio_put(io_bio); # (3)
	...
}
===============================================================================

If btrfs_map_block() fails in btrfs_submit_direct_hook(), it put
orig_bio at (1) and return -EIO. Then caller, btrfs_submit_direct()
free the same bio at (3). Since (3) is also used for other error
handling (2), I consider your way, removing (1), is better.

Thanks,
Satoru

> ---
>   fs/btrfs/inode.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a616fa4..15902eb 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7325,10 +7325,8 @@ static int btrfs_submit_direct_hook(int rw, struct btrfs_dio_private *dip,
>   	map_length = orig_bio->bi_iter.bi_size;
>   	ret = btrfs_map_block(root->fs_info, rw, start_sector << 9,
>   			      &map_length, NULL, 0);
> -	if (ret) {
> -		bio_put(orig_bio);
> +	if (ret)
>   		return -EIO;
> -	}
>   
>   	if (map_length >= orig_bio->bi_iter.bi_size) {
>   		bio = orig_bio;
> @@ -7345,6 +7343,7 @@ static int btrfs_submit_direct_hook(int rw, struct btrfs_dio_private *dip,
>   	bio = btrfs_dio_bio_alloc(orig_bio->bi_bdev, start_sector, GFP_NOFS);
>   	if (!bio)
>   		return -ENOMEM;
> +
>   	bio->bi_private = dip;
>   	bio->bi_end_io = btrfs_end_dio_bio;
>   	atomic_inc(&dip->pending_bios);
> 

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

Patch
diff mbox

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a616fa4..15902eb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7325,10 +7325,8 @@  static int btrfs_submit_direct_hook(int rw, struct btrfs_dio_private *dip,
 	map_length = orig_bio->bi_iter.bi_size;
 	ret = btrfs_map_block(root->fs_info, rw, start_sector << 9,
 			      &map_length, NULL, 0);
-	if (ret) {
-		bio_put(orig_bio);
+	if (ret)
 		return -EIO;
-	}
 
 	if (map_length >= orig_bio->bi_iter.bi_size) {
 		bio = orig_bio;
@@ -7345,6 +7343,7 @@  static int btrfs_submit_direct_hook(int rw, struct btrfs_dio_private *dip,
 	bio = btrfs_dio_bio_alloc(orig_bio->bi_bdev, start_sector, GFP_NOFS);
 	if (!bio)
 		return -ENOMEM;
+
 	bio->bi_private = dip;
 	bio->bi_end_io = btrfs_end_dio_bio;
 	atomic_inc(&dip->pending_bios);