diff mbox

[04/15] Btrfs: add ref_count and free function for btrfs_bio

Message ID 1421152488-30548-5-git-send-email-zhaolei@cn.fujitsu.com (mailing list archive)
State Superseded
Headers show

Commit Message

Zhaolei Jan. 13, 2015, 12:34 p.m. UTC
From: Zhao Lei <zhaolei@cn.fujitsu.com>

1: ref_count is simple than current RBIO_HOLD_BBIO_MAP_BIT flag
   to keep btrfs_bio's memory in raid56 recovery implement.
2: free function for bbio will make code clean and flexible, plus
   forced data type checking in compile.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/extent-tree.c |  2 +-
 fs/btrfs/extent_io.c   |  2 +-
 fs/btrfs/raid56.c      | 39 +++++++++------------------------------
 fs/btrfs/reada.c       |  4 ++--
 fs/btrfs/scrub.c       | 12 ++++++------
 fs/btrfs/volumes.c     | 43 ++++++++++++++++++++++++++++++++++++-------
 fs/btrfs/volumes.h     | 10 +++-------
 7 files changed, 58 insertions(+), 54 deletions(-)

Comments

David Sterba Jan. 15, 2015, 12:52 p.m. UTC | #1
The cleanups look good in general, some minor nitpicks below.

On Tue, Jan 13, 2015 at 08:34:37PM +0800, Zhaolei wrote:
> -		kfree(bbio);
> +		put_btrfs_bio(bbio);

Please rename it to btrfs_put_bbio, this is more consistent with other
*_put_* helpers and 'bbio' distinguishes btrfs_bio from regular 'bio'.

>  
>  static void btrfs_end_bio(struct bio *bio, int err)
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index fb0e8c3..db195f0 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -295,6 +295,7 @@ typedef void (btrfs_bio_end_io_t) (struct btrfs_bio *bio, int err);
>  #define BTRFS_BIO_ORIG_BIO_SUBMITTED	(1 << 0)
>  
>  struct btrfs_bio {
> +	atomic_t ref_count;

	atomic_t refs;

>  	atomic_t stripes_pending;
>  	struct btrfs_fs_info *fs_info;
>  	bio_end_io_t *end_io;
> @@ -394,13 +395,8 @@ struct btrfs_balance_control {
>  
>  int btrfs_account_dev_extents_size(struct btrfs_device *device, u64 start,
>  				   u64 end, u64 *length);
> -
> -#define btrfs_bio_size(total_stripes, real_stripes)		\
> -	(sizeof(struct btrfs_bio) +				\
> -	 (sizeof(struct btrfs_bio_stripe) * (total_stripes)) +	\
> -	 (sizeof(int) * (real_stripes)) +			\
> -	 (sizeof(u64) * (real_stripes)))
> -
> +void get_btrfs_bio(struct btrfs_bio *bbio);

	btrfs_get_bbio

> +void put_btrfs_bio(struct btrfs_bio *bbio);
>  int btrfs_map_block(struct btrfs_fs_info *fs_info, int rw,
>  		    u64 logical, u64 *length,
>  		    struct btrfs_bio **bbio_ret, int mirror_num);
--
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
Zhaolei Jan. 16, 2015, 1:38 a.m. UTC | #2
Hi, David Sterba

* From: David Sterba [mailto:dsterba@suse.cz]
> The cleanups look good in general, some minor nitpicks below.
> 
> On Tue, Jan 13, 2015 at 08:34:37PM +0800, Zhaolei wrote:
> > -		kfree(bbio);
> > +		put_btrfs_bio(bbio);
> 
> Please rename it to btrfs_put_bbio, this is more consistent with other
> *_put_* helpers and 'bbio' distinguishes btrfs_bio from regular 'bio'.
> 
Good suggestion, I like these unified-format name.

> >
> >  static void btrfs_end_bio(struct bio *bio, int err)
> > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> > index fb0e8c3..db195f0 100644
> > --- a/fs/btrfs/volumes.h
> > +++ b/fs/btrfs/volumes.h
> > @@ -295,6 +295,7 @@ typedef void (btrfs_bio_end_io_t) (struct btrfs_bio *bio, int err);
> >  #define BTRFS_BIO_ORIG_BIO_SUBMITTED	(1 << 0)
> >
> >  struct btrfs_bio {
> > +	atomic_t ref_count;
> 
> 	atomic_t refs;
> 
Ok.

> >  	atomic_t stripes_pending;
> >  	struct btrfs_fs_info *fs_info;
> >  	bio_end_io_t *end_io;
> > @@ -394,13 +395,8 @@ struct btrfs_balance_control {
> >
> >  int btrfs_account_dev_extents_size(struct btrfs_device *device, u64 start,
> >  				   u64 end, u64 *length);
> > -
> > -#define btrfs_bio_size(total_stripes, real_stripes)		\
> > -	(sizeof(struct btrfs_bio) +				\
> > -	 (sizeof(struct btrfs_bio_stripe) * (total_stripes)) +	\
> > -	 (sizeof(int) * (real_stripes)) +			\
> > -	 (sizeof(u64) * (real_stripes)))
> > -
> > +void get_btrfs_bio(struct btrfs_bio *bbio);
> 
> 	btrfs_get_bbio
> 
Thanks for your suggestion, I'll include above changes in v2.

Thanks
Zhaolei

> > +void put_btrfs_bio(struct btrfs_bio *bbio);
> >  int btrfs_map_block(struct btrfs_fs_info *fs_info, int rw,
> >  		    u64 logical, u64 *length,
> >  		    struct btrfs_bio **bbio_ret, int mirror_num);


--
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
diff mbox

Patch

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 1511658..da8fe79 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1925,7 +1925,7 @@  int btrfs_discard_extent(struct btrfs_root *root, u64 bytenr,
 			 */
 			ret = 0;
 		}
-		kfree(bbio);
+		put_btrfs_bio(bbio);
 	}
 
 	if (actual_bytes)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4ebabd2..4f959e5 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2057,7 +2057,7 @@  int repair_io_failure(struct inode *inode, u64 start, u64 length, u64 logical,
 	sector = bbio->stripes[mirror_num-1].physical >> 9;
 	bio->bi_iter.bi_sector = sector;
 	dev = bbio->stripes[mirror_num-1].dev;
-	kfree(bbio);
+	put_btrfs_bio(bbio);
 	if (!dev || !dev->bdev || !dev->writeable) {
 		bio_put(bio);
 		return -EIO;
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index e301d33..b1dadf7 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -58,15 +58,6 @@ 
  */
 #define RBIO_CACHE_READY_BIT	3
 
-/*
- * bbio and raid_map is managed by the caller, so we shouldn't free
- * them here. And besides that, all rbios with this flag should not
- * be cached, because we need raid_map to check the rbios' stripe
- * is the same or not, but it is very likely that the caller has
- * free raid_map, so don't cache those rbios.
- */
-#define RBIO_HOLD_BBIO_MAP_BIT	4
-
 #define RBIO_CACHE_SIZE 1024
 
 enum btrfs_rbio_ops {
@@ -834,19 +825,6 @@  done_nolock:
 		remove_rbio_from_cache(rbio);
 }
 
-static inline void
-__free_bbio(struct btrfs_bio *bbio, int need)
-{
-	if (need)
-		kfree(bbio);
-}
-
-static inline void free_bbio(struct btrfs_raid_bio *rbio)
-{
-	__free_bbio(rbio->bbio,
-		    !test_bit(RBIO_HOLD_BBIO_MAP_BIT, &rbio->flags));
-}
-
 static void __free_raid_bio(struct btrfs_raid_bio *rbio)
 {
 	int i;
@@ -866,8 +844,7 @@  static void __free_raid_bio(struct btrfs_raid_bio *rbio)
 		}
 	}
 
-	free_bbio(rbio);
-
+	put_btrfs_bio(rbio->bbio);
 	kfree(rbio);
 }
 
@@ -1774,7 +1751,7 @@  int raid56_parity_write(struct btrfs_root *root, struct bio *bio,
 
 	rbio = alloc_rbio(root, bbio, stripe_len);
 	if (IS_ERR(rbio)) {
-		__free_bbio(bbio, 1);
+		put_btrfs_bio(bbio);
 		return PTR_ERR(rbio);
 	}
 	bio_list_add(&rbio->bio_list, bio);
@@ -1990,8 +1967,7 @@  cleanup:
 
 cleanup_io:
 	if (rbio->operation == BTRFS_RBIO_READ_REBUILD) {
-		if (err == 0 &&
-		    !test_bit(RBIO_HOLD_BBIO_MAP_BIT, &rbio->flags))
+		if (err == 0)
 			cache_rbio_pages(rbio);
 		else
 			clear_bit(RBIO_CACHE_READY_BIT, &rbio->flags);
@@ -2153,7 +2129,8 @@  int raid56_parity_recover(struct btrfs_root *root, struct bio *bio,
 
 	rbio = alloc_rbio(root, bbio, stripe_len);
 	if (IS_ERR(rbio)) {
-		__free_bbio(bbio, generic_io);
+		if (generic_io)
+			put_btrfs_bio(bbio);
 		return PTR_ERR(rbio);
 	}
 
@@ -2164,7 +2141,8 @@  int raid56_parity_recover(struct btrfs_root *root, struct bio *bio,
 	rbio->faila = find_logical_bio_stripe(rbio, bio);
 	if (rbio->faila == -1) {
 		BUG();
-		__free_bbio(bbio, generic_io);
+		if (generic_io)
+			put_btrfs_bio(bbio);
 		kfree(rbio);
 		return -EIO;
 	}
@@ -2173,9 +2151,10 @@  int raid56_parity_recover(struct btrfs_root *root, struct bio *bio,
 		btrfs_bio_counter_inc_noblocked(root->fs_info);
 		rbio->generic_bio_cnt = 1;
 	} else {
-		set_bit(RBIO_HOLD_BBIO_MAP_BIT, &rbio->flags);
+		get_btrfs_bio(bbio);
 	}
 
+
 	/*
 	 * reconstruct from the q stripe if they are
 	 * asking for mirror 3
diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index b63ae20..b346227 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -463,7 +463,7 @@  static struct reada_extent *reada_find_extent(struct btrfs_root *root,
 	spin_unlock(&fs_info->reada_lock);
 	btrfs_dev_replace_unlock(&fs_info->dev_replace);
 
-	kfree(bbio);
+	put_btrfs_bio(bbio);
 	return re;
 
 error:
@@ -488,7 +488,7 @@  error:
 		kref_put(&zone->refcnt, reada_zone_release);
 		spin_unlock(&fs_info->reada_lock);
 	}
-	kfree(bbio);
+	put_btrfs_bio(bbio);
 	kfree(re);
 	return re_exist;
 }
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 8f32520..c4b7876 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -847,7 +847,7 @@  static inline void scrub_get_recover(struct scrub_recover *recover)
 static inline void scrub_put_recover(struct scrub_recover *recover)
 {
 	if (atomic_dec_and_test(&recover->refs)) {
-		kfree(recover->bbio);
+		put_btrfs_bio(recover->bbio);
 		kfree(recover);
 	}
 }
@@ -1365,13 +1365,13 @@  static int scrub_setup_recheck_block(struct scrub_ctx *sctx,
 		ret = btrfs_map_sblock(fs_info, REQ_GET_READ_MIRRORS, logical,
 				       &mapped_length, &bbio, 0, 1);
 		if (ret || !bbio || mapped_length < sublen) {
-			kfree(bbio);
+			put_btrfs_bio(bbio);
 			return -EIO;
 		}
 
 		recover = kzalloc(sizeof(struct scrub_recover), GFP_NOFS);
 		if (!recover) {
-			kfree(bbio);
+			put_btrfs_bio(bbio);
 			return -ENOMEM;
 		}
 
@@ -2740,7 +2740,7 @@  static void scrub_parity_check_and_repair(struct scrub_parity *sparity)
 rbio_out:
 	bio_put(bio);
 bbio_out:
-	kfree(bbio);
+	put_btrfs_bio(bbio);
 	bitmap_or(sparity->ebitmap, sparity->ebitmap, sparity->dbitmap,
 		  sparity->nsectors);
 	spin_lock(&sctx->stat_lock);
@@ -3871,14 +3871,14 @@  static void scrub_remap_extent(struct btrfs_fs_info *fs_info,
 			      &mapped_length, &bbio, 0);
 	if (ret || !bbio || mapped_length < extent_len ||
 	    !bbio->stripes[0].dev->bdev) {
-		kfree(bbio);
+		put_btrfs_bio(bbio);
 		return;
 	}
 
 	*extent_physical = bbio->stripes[0].physical;
 	*extent_mirror_num = bbio->mirror_num;
 	*extent_dev = bbio->stripes[0].dev;
-	kfree(bbio);
+	put_btrfs_bio(bbio);
 }
 
 static int scrub_setup_wr_ctx(struct scrub_ctx *sctx,
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b364e14..8671400 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4901,6 +4901,37 @@  static void sort_parity_stripes(struct btrfs_bio *bbio, int num_stripes)
 	}
 }
 
+static struct btrfs_bio *alloc_btrfs_bio(int total_stripes, int real_stripes)
+{
+	struct btrfs_bio *bbio = kzalloc(
+		sizeof(struct btrfs_bio) +
+		sizeof(struct btrfs_bio_stripe) * (total_stripes) +
+		sizeof(int) * (real_stripes) +
+		sizeof(u64) * (real_stripes),
+		GFP_NOFS);
+	if (!bbio)
+		return NULL;
+
+	atomic_set(&bbio->error, 0);
+	atomic_set(&bbio->ref_count, 1);
+
+	return bbio;
+}
+
+void get_btrfs_bio(struct btrfs_bio *bbio)
+{
+	WARN_ON(!atomic_read(&bbio->ref_count));
+	atomic_inc(&bbio->ref_count);
+}
+
+void put_btrfs_bio(struct btrfs_bio *bbio)
+{
+	if (!bbio)
+		return;
+	if (atomic_dec_and_test(&bbio->ref_count))
+		kfree(bbio);
+}
+
 static int __btrfs_map_block(struct btrfs_fs_info *fs_info, int rw,
 			     u64 logical, u64 *length,
 			     struct btrfs_bio **bbio_ret,
@@ -5052,7 +5083,7 @@  static int __btrfs_map_block(struct btrfs_fs_info *fs_info, int rw,
 			 * is not left of the left cursor
 			 */
 			ret = -EIO;
-			kfree(tmp_bbio);
+			put_btrfs_bio(tmp_bbio);
 			goto out;
 		}
 
@@ -5087,11 +5118,11 @@  static int __btrfs_map_block(struct btrfs_fs_info *fs_info, int rw,
 		} else {
 			WARN_ON(1);
 			ret = -EIO;
-			kfree(tmp_bbio);
+			put_btrfs_bio(tmp_bbio);
 			goto out;
 		}
 
-		kfree(tmp_bbio);
+		put_btrfs_bio(tmp_bbio);
 	} else if (mirror_num > map->num_stripes) {
 		mirror_num = 0;
 	}
@@ -5213,13 +5244,11 @@  static int __btrfs_map_block(struct btrfs_fs_info *fs_info, int rw,
 		tgtdev_indexes = num_stripes;
 	}
 
-	bbio = kzalloc(btrfs_bio_size(num_alloc_stripes, tgtdev_indexes),
-		       GFP_NOFS);
+	bbio = alloc_btrfs_bio(num_alloc_stripes, tgtdev_indexes);
 	if (!bbio) {
 		ret = -ENOMEM;
 		goto out;
 	}
-	atomic_set(&bbio->error, 0);
 	if (dev_replace_is_ongoing)
 		bbio->tgtdev_map = (int *)(bbio->stripes + num_alloc_stripes);
 
@@ -5558,7 +5587,7 @@  static inline void btrfs_end_bbio(struct btrfs_bio *bbio, struct bio *bio, int e
 		bio_endio_nodec(bio, err);
 	else
 		bio_endio(bio, err);
-	kfree(bbio);
+	put_btrfs_bio(bbio);
 }
 
 static void btrfs_end_bio(struct bio *bio, int err)
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index fb0e8c3..db195f0 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -295,6 +295,7 @@  typedef void (btrfs_bio_end_io_t) (struct btrfs_bio *bio, int err);
 #define BTRFS_BIO_ORIG_BIO_SUBMITTED	(1 << 0)
 
 struct btrfs_bio {
+	atomic_t ref_count;
 	atomic_t stripes_pending;
 	struct btrfs_fs_info *fs_info;
 	bio_end_io_t *end_io;
@@ -394,13 +395,8 @@  struct btrfs_balance_control {
 
 int btrfs_account_dev_extents_size(struct btrfs_device *device, u64 start,
 				   u64 end, u64 *length);
-
-#define btrfs_bio_size(total_stripes, real_stripes)		\
-	(sizeof(struct btrfs_bio) +				\
-	 (sizeof(struct btrfs_bio_stripe) * (total_stripes)) +	\
-	 (sizeof(int) * (real_stripes)) +			\
-	 (sizeof(u64) * (real_stripes)))
-
+void get_btrfs_bio(struct btrfs_bio *bbio);
+void put_btrfs_bio(struct btrfs_bio *bbio);
 int btrfs_map_block(struct btrfs_fs_info *fs_info, int rw,
 		    u64 logical, u64 *length,
 		    struct btrfs_bio **bbio_ret, int mirror_num);