[1/8] btrfs: bioset allocations will never fail, adapt our helpers
diff mbox

Message ID 559a7bd7acc20ab9eb967dd205e41a4a3f2cdb97.1496422340.git.dsterba@suse.com
State New
Headers show

Commit Message

David Sterba June 2, 2017, 4:58 p.m. UTC
Christoph pointed out that bio allocations backed by a bioset will never
fail.  As we always use a bioset for all bio allocations, we can skip
the error handling.  This patch adjusts our low-level helpers, the
cascaded changes to all callers will come next.

CC: Christoph Hellwig <hch@lst.de>
CC: Liu Bo <bo.li.liu@oracle.com>
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 50 +++++++++++++++++++-------------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

Comments

Christoph Hellwig June 3, 2017, 5:10 a.m. UTC | #1
On Fri, Jun 02, 2017 at 06:58:30PM +0200, David Sterba wrote:
> Christoph pointed out that bio allocations backed by a bioset will never
> fail.  As we always use a bioset for all bio allocations, we can skip
> the error handling.  This patch adjusts our low-level helpers, the
> cascaded changes to all callers will come next.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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
Anand Jain June 6, 2017, 10:21 a.m. UTC | #2
On 06/03/17 00:58, David Sterba wrote:
> Christoph pointed out that bio allocations backed by a bioset will never
> fail.

David,

  Looks like this feature comes when  __GFP_DIRECT_RECLAIM is
  set and we aren't, such as [1]. Any idea why? Looks like I am
  missing something ?


[1]
-----
static int submit_extent_page(int op, int op_flags, struct 
extent_io_tree *tree,
::

  bio = btrfs_bio_alloc(bdev, sector, BIO_MAX_PAGES,
                 GFP_NOFS | __GFP_HIGH);

------

Thanks, Anand


>  As we always use a bioset for all bio allocations, we can skip
> the error handling.  This patch adjusts our low-level helpers, the
> cascaded changes to all callers will come next.
>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Liu Bo <bo.li.liu@oracle.com>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/extent_io.c | 50 +++++++++++++++++++-------------------------------
>  1 file changed, 19 insertions(+), 31 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 5909f8214255..78a787c36a35 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2659,8 +2659,9 @@ static void end_bio_extent_readpage(struct bio *bio)
>  }
>
>  /*
> - * this allocates from the btrfs_bioset.  We're returning a bio right now
> - * but you can call btrfs_io_bio for the appropriate container_of magic
> + * The following helpers allocate a bio. As it's backed by a bioset, it'll
> + * never fail.  We're returning a bio right now but you can call btrfs_io_bio
> + * for the appropriate container_of magic
>   */
>  struct bio *
>  btrfs_bio_alloc(struct block_device *bdev, u64 first_sector, int nr_vecs,
> @@ -2670,22 +2671,12 @@ btrfs_bio_alloc(struct block_device *bdev, u64 first_sector, int nr_vecs,
>  	struct bio *bio;
>
>  	bio = bio_alloc_bioset(gfp_flags, nr_vecs, btrfs_bioset);
> -
> -	if (bio == NULL && (current->flags & PF_MEMALLOC)) {
> -		while (!bio && (nr_vecs /= 2)) {
> -			bio = bio_alloc_bioset(gfp_flags,
> -					       nr_vecs, btrfs_bioset);
> -		}
> -	}
> -
> -	if (bio) {
> -		bio->bi_bdev = bdev;
> -		bio->bi_iter.bi_sector = first_sector;
> -		btrfs_bio = btrfs_io_bio(bio);
> -		btrfs_bio->csum = NULL;
> -		btrfs_bio->csum_allocated = NULL;
> -		btrfs_bio->end_io = NULL;
> -	}
> +	bio->bi_bdev = bdev;
> +	bio->bi_iter.bi_sector = first_sector;
> +	btrfs_bio = btrfs_io_bio(bio);
> +	btrfs_bio->csum = NULL;
> +	btrfs_bio->csum_allocated = NULL;
> +	btrfs_bio->end_io = NULL;
>  	return bio;
>  }
>
> @@ -2694,29 +2685,26 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask)
>  	struct btrfs_io_bio *btrfs_bio;
>  	struct bio *new;
>
> +	/* Bio allocation backed by a bioset does not fail */
>  	new = bio_clone_fast(bio, gfp_mask, btrfs_bioset);
> -	if (new) {
> -		btrfs_bio = btrfs_io_bio(new);
> -		btrfs_bio->csum = NULL;
> -		btrfs_bio->csum_allocated = NULL;
> -		btrfs_bio->end_io = NULL;
> -	}
> +	btrfs_bio = btrfs_io_bio(new);
> +	btrfs_bio->csum = NULL;
> +	btrfs_bio->csum_allocated = NULL;
> +	btrfs_bio->end_io = NULL;
>  	return new;
>  }
>
> -/* this also allocates from the btrfs_bioset */
>  struct bio *btrfs_io_bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
>  {
>  	struct btrfs_io_bio *btrfs_bio;
>  	struct bio *bio;
>
> +	/* Bio allocation backed by a bioset does not fail */
>  	bio = bio_alloc_bioset(gfp_mask, nr_iovecs, btrfs_bioset);
> -	if (bio) {
> -		btrfs_bio = btrfs_io_bio(bio);
> -		btrfs_bio->csum = NULL;
> -		btrfs_bio->csum_allocated = NULL;
> -		btrfs_bio->end_io = NULL;
> -	}
> +	btrfs_bio = btrfs_io_bio(bio);
> +	btrfs_bio->csum = NULL;
> +	btrfs_bio->csum_allocated = NULL;
> +	btrfs_bio->end_io = NULL;
>  	return bio;
>  }
>
>
--
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
Christoph Hellwig June 6, 2017, 10:44 a.m. UTC | #3
On Tue, Jun 06, 2017 at 06:21:17PM +0800, Anand Jain wrote:
>
>
> On 06/03/17 00:58, David Sterba wrote:
>> Christoph pointed out that bio allocations backed by a bioset will never
>> fail.
>
> David,
>
>  Looks like this feature comes when  __GFP_DIRECT_RECLAIM is
>  set and we aren't, such as [1]. Any idea why? Looks like I am
>  missing something ?
>
>
> [1]
> -----
> static int submit_extent_page(int op, int op_flags, struct extent_io_tree 
> *tree,
> ::
>
>  bio = btrfs_bio_alloc(bdev, sector, BIO_MAX_PAGES,
>                 GFP_NOFS | __GFP_HIGH);

#define __GFP_RECLAIM ((__force gfp_t)(___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM))


#define GFP_NOFS        (__GFP_RECLAIM | __GFP_IO)

--
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
David Sterba June 6, 2017, 10:51 a.m. UTC | #4
On Tue, Jun 06, 2017 at 06:21:17PM +0800, Anand Jain wrote:
> On 06/03/17 00:58, David Sterba wrote:
> > Christoph pointed out that bio allocations backed by a bioset will never
> > fail.
> 
>   Looks like this feature comes when  __GFP_DIRECT_RECLAIM is
>   set and we aren't, such as [1]. Any idea why? Looks like I am
>   missing something ?

bio_alloc_bioset:

 406  *   When @bs is not NULL, if %__GFP_DIRECT_RECLAIM is set then bio_alloc will
 407  *   always be able to allocate a bio. This is due to the mempool guarantees.

> [1]
> -----
> static int submit_extent_page(int op, int op_flags, struct 
> extent_io_tree *tree,
> ::
> 
>   bio = btrfs_bio_alloc(bdev, sector, BIO_MAX_PAGES,
>                  GFP_NOFS | __GFP_HIGH);

GFP_NOFS contains __GFP_DIRECT_RECLAIM, so what's the problem? __GFP_HIGH
allows using the reserves, but should not otherwise change the constraints.
And it's use seems unnecessary, as far as I could track in the history.
--
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
Anand Jain June 7, 2017, 6:52 a.m. UTC | #5
> #define __GFP_RECLAIM ((__force gfp_t)(___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM))
> 
> 
> #define GFP_NOFS        (__GFP_RECLAIM | __GFP_IO)

   Ah. Thanks.
--
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
Anand Jain June 7, 2017, 9:10 a.m. UTC | #6
Reviewed-by: Anand Jain <anand.jain@oracle.com>
--
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/extent_io.c b/fs/btrfs/extent_io.c
index 5909f8214255..78a787c36a35 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2659,8 +2659,9 @@  static void end_bio_extent_readpage(struct bio *bio)
 }
 
 /*
- * this allocates from the btrfs_bioset.  We're returning a bio right now
- * but you can call btrfs_io_bio for the appropriate container_of magic
+ * The following helpers allocate a bio. As it's backed by a bioset, it'll
+ * never fail.  We're returning a bio right now but you can call btrfs_io_bio
+ * for the appropriate container_of magic
  */
 struct bio *
 btrfs_bio_alloc(struct block_device *bdev, u64 first_sector, int nr_vecs,
@@ -2670,22 +2671,12 @@  btrfs_bio_alloc(struct block_device *bdev, u64 first_sector, int nr_vecs,
 	struct bio *bio;
 
 	bio = bio_alloc_bioset(gfp_flags, nr_vecs, btrfs_bioset);
-
-	if (bio == NULL && (current->flags & PF_MEMALLOC)) {
-		while (!bio && (nr_vecs /= 2)) {
-			bio = bio_alloc_bioset(gfp_flags,
-					       nr_vecs, btrfs_bioset);
-		}
-	}
-
-	if (bio) {
-		bio->bi_bdev = bdev;
-		bio->bi_iter.bi_sector = first_sector;
-		btrfs_bio = btrfs_io_bio(bio);
-		btrfs_bio->csum = NULL;
-		btrfs_bio->csum_allocated = NULL;
-		btrfs_bio->end_io = NULL;
-	}
+	bio->bi_bdev = bdev;
+	bio->bi_iter.bi_sector = first_sector;
+	btrfs_bio = btrfs_io_bio(bio);
+	btrfs_bio->csum = NULL;
+	btrfs_bio->csum_allocated = NULL;
+	btrfs_bio->end_io = NULL;
 	return bio;
 }
 
@@ -2694,29 +2685,26 @@  struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask)
 	struct btrfs_io_bio *btrfs_bio;
 	struct bio *new;
 
+	/* Bio allocation backed by a bioset does not fail */
 	new = bio_clone_fast(bio, gfp_mask, btrfs_bioset);
-	if (new) {
-		btrfs_bio = btrfs_io_bio(new);
-		btrfs_bio->csum = NULL;
-		btrfs_bio->csum_allocated = NULL;
-		btrfs_bio->end_io = NULL;
-	}
+	btrfs_bio = btrfs_io_bio(new);
+	btrfs_bio->csum = NULL;
+	btrfs_bio->csum_allocated = NULL;
+	btrfs_bio->end_io = NULL;
 	return new;
 }
 
-/* this also allocates from the btrfs_bioset */
 struct bio *btrfs_io_bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
 {
 	struct btrfs_io_bio *btrfs_bio;
 	struct bio *bio;
 
+	/* Bio allocation backed by a bioset does not fail */
 	bio = bio_alloc_bioset(gfp_mask, nr_iovecs, btrfs_bioset);
-	if (bio) {
-		btrfs_bio = btrfs_io_bio(bio);
-		btrfs_bio->csum = NULL;
-		btrfs_bio->csum_allocated = NULL;
-		btrfs_bio->end_io = NULL;
-	}
+	btrfs_bio = btrfs_io_bio(bio);
+	btrfs_bio->csum = NULL;
+	btrfs_bio->csum_allocated = NULL;
+	btrfs_bio->end_io = NULL;
 	return bio;
 }