linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ext4: bio_alloc never fails
@ 2019-10-30  4:26 Gao Xiang
  2019-10-30 10:13 ` Ritesh Harjani
  2019-10-31  2:01 ` [PATCH] ext4: bio_alloc " Chao Yu
  0 siblings, 2 replies; 11+ messages in thread
From: Gao Xiang @ 2019-10-30  4:26 UTC (permalink / raw)
  To: Theodore Ts'o, Andreas Dilger; +Cc: linux-ext4, linux-kernel, Gao Xiang

Similar to [1] [2], it seems a trivial cleanup since
bio_alloc can handle memory allocation as mentioned in
fs/direct-io.c (also see fs/block_dev.c, fs/buffer.c, ..)

[1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
[2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 fs/ext4/page-io.c  | 11 +++--------
 fs/ext4/readpage.c |  2 --
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 12ceadef32c5..f1f7b6601780 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -358,14 +358,12 @@ void ext4_io_submit_init(struct ext4_io_submit *io,
 	io->io_end = NULL;
 }
 
-static int io_submit_init_bio(struct ext4_io_submit *io,
-			      struct buffer_head *bh)
+static void io_submit_init_bio(struct ext4_io_submit *io,
+			       struct buffer_head *bh)
 {
 	struct bio *bio;
 
 	bio = bio_alloc(GFP_NOIO, BIO_MAX_PAGES);
-	if (!bio)
-		return -ENOMEM;
 	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
 	bio_set_dev(bio, bh->b_bdev);
 	bio->bi_end_io = ext4_end_bio;
@@ -373,7 +371,6 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
 	io->io_bio = bio;
 	io->io_next_block = bh->b_blocknr;
 	wbc_init_bio(io->io_wbc, bio);
-	return 0;
 }
 
 static int io_submit_add_bh(struct ext4_io_submit *io,
@@ -388,9 +385,7 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
 		ext4_io_submit(io);
 	}
 	if (io->io_bio == NULL) {
-		ret = io_submit_init_bio(io, bh);
-		if (ret)
-			return ret;
+		io_submit_init_bio(io, bh);
 		io->io_bio->bi_write_hint = inode->i_write_hint;
 	}
 	ret = bio_add_page(io->io_bio, page, bh->b_size, bh_offset(bh));
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index a30b203fa461..bfeb77b93f48 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -362,8 +362,6 @@ int ext4_mpage_readpages(struct address_space *mapping,
 
 			bio = bio_alloc(GFP_KERNEL,
 				min_t(int, nr_pages, BIO_MAX_PAGES));
-			if (!bio)
-				goto set_error_page;
 			ctx = get_bio_post_read_ctx(inode, bio, page->index);
 			if (IS_ERR(ctx)) {
 				bio_put(bio);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] ext4: bio_alloc never fails
  2019-10-30  4:26 [PATCH] ext4: bio_alloc never fails Gao Xiang
@ 2019-10-30 10:13 ` Ritesh Harjani
  2019-10-30 10:40   ` Gao Xiang
  2019-10-30 15:04   ` Theodore Y. Ts'o
  2019-10-31  2:01 ` [PATCH] ext4: bio_alloc " Chao Yu
  1 sibling, 2 replies; 11+ messages in thread
From: Ritesh Harjani @ 2019-10-30 10:13 UTC (permalink / raw)
  To: Gao Xiang, Theodore Ts'o, Andreas Dilger
  Cc: linux-ext4, linux-kernel, linux-fsdevel



On 10/30/19 9:56 AM, Gao Xiang wrote:
> Similar to [1] [2], it seems a trivial cleanup since
> bio_alloc can handle memory allocation as mentioned in
> fs/direct-io.c (also see fs/block_dev.c, fs/buffer.c, ..)
> 

AFAIU, the reason is that, bio_alloc with __GFP_DIRECT_RECLAIM
flags guarantees bio allocation under some given restrictions,
as stated in fs/direct-io.c
So here it is ok to not check for NULL value from bio_alloc.

I think we can update above info too in your commit msg.

> [1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
> [2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>   fs/ext4/page-io.c  | 11 +++--------
>   fs/ext4/readpage.c |  2 --
>   2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 12ceadef32c5..f1f7b6601780 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -358,14 +358,12 @@ void ext4_io_submit_init(struct ext4_io_submit *io,
>   	io->io_end = NULL;
>   }
> 
> -static int io_submit_init_bio(struct ext4_io_submit *io,
> -			      struct buffer_head *bh)
> +static void io_submit_init_bio(struct ext4_io_submit *io,
> +			       struct buffer_head *bh)
>   {
>   	struct bio *bio;
> 
>   	bio = bio_alloc(GFP_NOIO, BIO_MAX_PAGES);
> -	if (!bio)
> -		return -ENOMEM;
>   	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
>   	bio_set_dev(bio, bh->b_bdev);
>   	bio->bi_end_io = ext4_end_bio;
> @@ -373,7 +371,6 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
>   	io->io_bio = bio;
>   	io->io_next_block = bh->b_blocknr;
>   	wbc_init_bio(io->io_wbc, bio);
> -	return 0;
>   }
> 
>   static int io_submit_add_bh(struct ext4_io_submit *io,
> @@ -388,9 +385,7 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
>   		ext4_io_submit(io);
>   	}
>   	if (io->io_bio == NULL) {
> -		ret = io_submit_init_bio(io, bh);
> -		if (ret)
> -			return ret;
> +		io_submit_init_bio(io, bh);
>   		io->io_bio->bi_write_hint = inode->i_write_hint;
>   	}
>   	ret = bio_add_page(io->io_bio, page, bh->b_size, bh_offset(bh));


Also we can further simplify it like below. Please check.

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index f1f7b6601780..a3a2edeb3bbf 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -373,7 +373,7 @@ static void io_submit_init_bio(struct ext4_io_submit 
*io,
  	wbc_init_bio(io->io_wbc, bio);
  }

-static int io_submit_add_bh(struct ext4_io_submit *io,
+static void io_submit_add_bh(struct ext4_io_submit *io,
  			    struct inode *inode,
  			    struct page *page,
  			    struct buffer_head *bh)
@@ -393,7 +393,6 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
  		goto submit_and_retry;
  	wbc_account_cgroup_owner(io->io_wbc, page, bh->b_size);
  	io->io_next_block++;
-	return 0;
  }

  int ext4_bio_write_page(struct ext4_io_submit *io,
@@ -495,30 +494,23 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
  	do {
  		if (!buffer_async_write(bh))
  			continue;
-		ret = io_submit_add_bh(io, inode, bounce_page ?: page, bh);
-		if (ret) {
-			/*
-			 * We only get here on ENOMEM.  Not much else
-			 * we can do but mark the page as dirty, and
-			 * better luck next time.
-			 */
-			break;
-		}
+		io_submit_add_bh(io, inode, bounce_page ?: page, bh);
  		nr_submitted++;
  		clear_buffer_dirty(bh);
  	} while ((bh = bh->b_this_page) != head);

-	/* Error stopped previous loop? Clean up buffers... */
-	if (ret) {
-	out:
-		fscrypt_free_bounce_page(bounce_page);
-		printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
-		redirty_page_for_writepage(wbc, page);
-		do {
-			clear_buffer_async_write(bh);
-			bh = bh->b_this_page;
-		} while (bh != head);
-	}
+	goto unlock;
+
+out:
+	fscrypt_free_bounce_page(bounce_page);
+	printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
+	redirty_page_for_writepage(wbc, page);
+	do {
+		clear_buffer_async_write(bh);
+		bh = bh->b_this_page;
+	} while (bh != head);
+
+unlock:
  	unlock_page(page);
  	/* Nothing submitted - we have to end page writeback */
  	if (!nr_submitted)


-ritesh


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] ext4: bio_alloc never fails
  2019-10-30 10:13 ` Ritesh Harjani
@ 2019-10-30 10:40   ` Gao Xiang
  2019-10-30 15:04   ` Theodore Y. Ts'o
  1 sibling, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2019-10-30 10:40 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: Theodore Ts'o, Andreas Dilger, linux-ext4, linux-kernel,
	linux-fsdevel

Hi Ritech,

On Wed, Oct 30, 2019 at 03:43:10PM +0530, Ritesh Harjani wrote:
> 
> 
> On 10/30/19 9:56 AM, Gao Xiang wrote:
> > Similar to [1] [2], it seems a trivial cleanup since
> > bio_alloc can handle memory allocation as mentioned in
> > fs/direct-io.c (also see fs/block_dev.c, fs/buffer.c, ..)
> > 
> 
> AFAIU, the reason is that, bio_alloc with __GFP_DIRECT_RECLAIM
> flags guarantees bio allocation under some given restrictions,
> as stated in fs/direct-io.c
> So here it is ok to not check for NULL value from bio_alloc.
> 
> I think we can update above info too in your commit msg.

Ok, I will update commit msg as you suggested.

> 
> > [1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
> > [2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > ---
> >   fs/ext4/page-io.c  | 11 +++--------
> >   fs/ext4/readpage.c |  2 --
> >   2 files changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> > index 12ceadef32c5..f1f7b6601780 100644
> > --- a/fs/ext4/page-io.c
> > +++ b/fs/ext4/page-io.c
> > @@ -358,14 +358,12 @@ void ext4_io_submit_init(struct ext4_io_submit *io,
> >   	io->io_end = NULL;
> >   }
> > 
> > -static int io_submit_init_bio(struct ext4_io_submit *io,
> > -			      struct buffer_head *bh)
> > +static void io_submit_init_bio(struct ext4_io_submit *io,
> > +			       struct buffer_head *bh)
> >   {
> >   	struct bio *bio;
> > 
> >   	bio = bio_alloc(GFP_NOIO, BIO_MAX_PAGES);
> > -	if (!bio)
> > -		return -ENOMEM;
> >   	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
> >   	bio_set_dev(bio, bh->b_bdev);
> >   	bio->bi_end_io = ext4_end_bio;
> > @@ -373,7 +371,6 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
> >   	io->io_bio = bio;
> >   	io->io_next_block = bh->b_blocknr;
> >   	wbc_init_bio(io->io_wbc, bio);
> > -	return 0;
> >   }
> > 
> >   static int io_submit_add_bh(struct ext4_io_submit *io,
> > @@ -388,9 +385,7 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
> >   		ext4_io_submit(io);
> >   	}
> >   	if (io->io_bio == NULL) {
> > -		ret = io_submit_init_bio(io, bh);
> > -		if (ret)
> > -			return ret;
> > +		io_submit_init_bio(io, bh);
> >   		io->io_bio->bi_write_hint = inode->i_write_hint;
> >   	}
> >   	ret = bio_add_page(io->io_bio, page, bh->b_size, bh_offset(bh));
> 
> 
> Also we can further simplify it like below. Please check.

Got it, let me update the patch later. :-)
Thanks for your suggestion. I will wait for a while and
see if other opinions raise up...

Thanks,
Gao Xiang

> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index f1f7b6601780..a3a2edeb3bbf 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -373,7 +373,7 @@ static void io_submit_init_bio(struct ext4_io_submit
> *io,
>  	wbc_init_bio(io->io_wbc, bio);
>  }
> 
> -static int io_submit_add_bh(struct ext4_io_submit *io,
> +static void io_submit_add_bh(struct ext4_io_submit *io,
>  			    struct inode *inode,
>  			    struct page *page,
>  			    struct buffer_head *bh)
> @@ -393,7 +393,6 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
>  		goto submit_and_retry;
>  	wbc_account_cgroup_owner(io->io_wbc, page, bh->b_size);
>  	io->io_next_block++;
> -	return 0;
>  }
> 
>  int ext4_bio_write_page(struct ext4_io_submit *io,
> @@ -495,30 +494,23 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  	do {
>  		if (!buffer_async_write(bh))
>  			continue;
> -		ret = io_submit_add_bh(io, inode, bounce_page ?: page, bh);
> -		if (ret) {
> -			/*
> -			 * We only get here on ENOMEM.  Not much else
> -			 * we can do but mark the page as dirty, and
> -			 * better luck next time.
> -			 */
> -			break;
> -		}
> +		io_submit_add_bh(io, inode, bounce_page ?: page, bh);
>  		nr_submitted++;
>  		clear_buffer_dirty(bh);
>  	} while ((bh = bh->b_this_page) != head);
> 
> -	/* Error stopped previous loop? Clean up buffers... */
> -	if (ret) {
> -	out:
> -		fscrypt_free_bounce_page(bounce_page);
> -		printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
> -		redirty_page_for_writepage(wbc, page);
> -		do {
> -			clear_buffer_async_write(bh);
> -			bh = bh->b_this_page;
> -		} while (bh != head);
> -	}
> +	goto unlock;
> +
> +out:
> +	fscrypt_free_bounce_page(bounce_page);
> +	printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
> +	redirty_page_for_writepage(wbc, page);
> +	do {
> +		clear_buffer_async_write(bh);
> +		bh = bh->b_this_page;
> +	} while (bh != head);
> +
> +unlock:
>  	unlock_page(page);
>  	/* Nothing submitted - we have to end page writeback */
>  	if (!nr_submitted)
> 
> 
> -ritesh
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ext4: bio_alloc never fails
  2019-10-30 10:13 ` Ritesh Harjani
  2019-10-30 10:40   ` Gao Xiang
@ 2019-10-30 15:04   ` Theodore Y. Ts'o
  2019-10-30 16:12     ` Gao Xiang
  1 sibling, 1 reply; 11+ messages in thread
From: Theodore Y. Ts'o @ 2019-10-30 15:04 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: Gao Xiang, Andreas Dilger, linux-ext4, linux-kernel, linux-fsdevel

On Wed, Oct 30, 2019 at 03:43:10PM +0530, Ritesh Harjani wrote:
> 
> 
> On 10/30/19 9:56 AM, Gao Xiang wrote:
> > Similar to [1] [2], it seems a trivial cleanup since
> > bio_alloc can handle memory allocation as mentioned in
> > fs/direct-io.c (also see fs/block_dev.c, fs/buffer.c, ..)
> > 
> 
> AFAIU, the reason is that, bio_alloc with __GFP_DIRECT_RECLAIM
> flags guarantees bio allocation under some given restrictions,
> as stated in fs/direct-io.c
> So here it is ok to not check for NULL value from bio_alloc.
> 
> I think we can update above info too in your commit msg.

Please also add a short comment in the code itself, so it's clear why
it's OK to skip the error check, and reference the comments for
bio_alloc_bioset().  This is the fairly subtle bit which makes this
change not obvious:

 *   When @bs is not NULL, if %__GFP_DIRECT_RECLAIM is set then bio_alloc will
 *   always be able to allocate a bio. This is due to the mempool guarantees.
 *   To make this work, callers must never allocate more than 1 bio at a time
 *   from this pool. Callers that need to allocate more than 1 bio must always
 *   submit the previously allocated bio for IO before attempting to allocate
 *   a new one. Failure to do so can cause deadlocks under memory pressure.
 *
 *   Note that when running under generic_make_request() (i.e. any block
 *   driver), bios are not submitted until after you return - see the code in
 *   generic_make_request() that converts recursion into iteration, to prevent
 *   stack overflows.
 *
 *   This would normally mean allocating multiple bios under
 *   generic_make_request() would be susceptible to deadlocks, but we have
 *   deadlock avoidance code that resubmits any blocked bios from a rescuer
 *   thread.

Otherwise, someone else may not understand why it's safe to not check
the error return then submit cleanup patch to add the error checking
back.  :-)

					- Ted
					

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ext4: bio_alloc never fails
  2019-10-30 15:04   ` Theodore Y. Ts'o
@ 2019-10-30 16:12     ` Gao Xiang
  2019-10-31  9:23       ` [PATCH v2] ext4: bio_alloc with __GFP_DIRECT_RECLAIM " Gao Xiang
  0 siblings, 1 reply; 11+ messages in thread
From: Gao Xiang @ 2019-10-30 16:12 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Ritesh Harjani, Gao Xiang, Andreas Dilger, linux-ext4,
	linux-kernel, linux-fsdevel

Hi Ted,

On Wed, Oct 30, 2019 at 11:04:37AM -0400, Theodore Y. Ts'o wrote:
> On Wed, Oct 30, 2019 at 03:43:10PM +0530, Ritesh Harjani wrote:
> > 
> > 
> > On 10/30/19 9:56 AM, Gao Xiang wrote:
> > > Similar to [1] [2], it seems a trivial cleanup since
> > > bio_alloc can handle memory allocation as mentioned in
> > > fs/direct-io.c (also see fs/block_dev.c, fs/buffer.c, ..)
> > > 
> > 
> > AFAIU, the reason is that, bio_alloc with __GFP_DIRECT_RECLAIM
> > flags guarantees bio allocation under some given restrictions,
> > as stated in fs/direct-io.c
> > So here it is ok to not check for NULL value from bio_alloc.
> > 
> > I think we can update above info too in your commit msg.
> 
> Please also add a short comment in the code itself, so it's clear why
> it's OK to skip the error check, and reference the comments for
> bio_alloc_bioset().  This is the fairly subtle bit which makes this
> change not obvious:

OK, I will add short comments in code then, and tidy up later since
it's not urgent (but I'm surprised that so many in-kernel code handles
that, those also makes me misleaded before, but I think mempool back
maybe better since the total efficient path is shorter compared with
error handling path)... and I'd like to know the f2fs side as well :-)

> 
>  *   When @bs is not NULL, if %__GFP_DIRECT_RECLAIM is set then bio_alloc will
>  *   always be able to allocate a bio. This is due to the mempool guarantees.
>  *   To make this work, callers must never allocate more than 1 bio at a time
>  *   from this pool. Callers that need to allocate more than 1 bio must always
>  *   submit the previously allocated bio for IO before attempting to allocate
>  *   a new one. Failure to do so can cause deadlocks under memory pressure.
>  *
>  *   Note that when running under generic_make_request() (i.e. any block
>  *   driver), bios are not submitted until after you return - see the code in
>  *   generic_make_request() that converts recursion into iteration, to prevent
>  *   stack overflows.
>  *
>  *   This would normally mean allocating multiple bios under
>  *   generic_make_request() would be susceptible to deadlocks, but we have
>  *   deadlock avoidance code that resubmits any blocked bios from a rescuer
>  *   thread.
> 
> Otherwise, someone else may not understand why it's safe to not check
> the error return then submit cleanup patch to add the error checking
> back.  :-)

Got it.

Thanks,
Gao Xiang

> 
> 					- Ted
> 					

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ext4: bio_alloc never fails
  2019-10-30  4:26 [PATCH] ext4: bio_alloc never fails Gao Xiang
  2019-10-30 10:13 ` Ritesh Harjani
@ 2019-10-31  2:01 ` Chao Yu
  2019-10-31  9:10   ` Gao Xiang
  1 sibling, 1 reply; 11+ messages in thread
From: Chao Yu @ 2019-10-31  2:01 UTC (permalink / raw)
  To: Gao Xiang, Theodore Ts'o, Andreas Dilger; +Cc: linux-ext4, linux-kernel

On 2019/10/30 12:26, Gao Xiang wrote:
> Similar to [1] [2], it seems a trivial cleanup since
> bio_alloc can handle memory allocation as mentioned in
> fs/direct-io.c (also see fs/block_dev.c, fs/buffer.c, ..)
> 
> [1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
> [2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

I notice that there is still similar code in mpage.c

static struct bio *
mpage_alloc(struct block_device *bdev,
		sector_t first_sector, int nr_vecs,
		gfp_t gfp_flags)
{
	struct bio *bio;

	/* Restrict the given (page cache) mask for slab allocations */
	gfp_flags &= GFP_KERNEL;
	bio = bio_alloc(gfp_flags, nr_vecs);

	if (bio == NULL && (current->flags & PF_MEMALLOC)) {
		while (!bio && (nr_vecs /= 2))
			bio = bio_alloc(gfp_flags, nr_vecs);
	}

	if (bio) {
		bio_set_dev(bio, bdev);
		bio->bi_iter.bi_sector = first_sector;
	}
	return bio;
}

Should we clean up them as well? however, I doubt we should get rid of loop in
mempool allocation to relief the memory pressure on those uncritical path, for
critical path like we should never fail, it would be fine looping in bio_alloc().

Thanks,

> ---
>  fs/ext4/page-io.c  | 11 +++--------
>  fs/ext4/readpage.c |  2 --
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 12ceadef32c5..f1f7b6601780 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -358,14 +358,12 @@ void ext4_io_submit_init(struct ext4_io_submit *io,
>  	io->io_end = NULL;
>  }
>  
> -static int io_submit_init_bio(struct ext4_io_submit *io,
> -			      struct buffer_head *bh)
> +static void io_submit_init_bio(struct ext4_io_submit *io,
> +			       struct buffer_head *bh)
>  {
>  	struct bio *bio;
>  
>  	bio = bio_alloc(GFP_NOIO, BIO_MAX_PAGES);
> -	if (!bio)
> -		return -ENOMEM;
>  	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
>  	bio_set_dev(bio, bh->b_bdev);
>  	bio->bi_end_io = ext4_end_bio;
> @@ -373,7 +371,6 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
>  	io->io_bio = bio;
>  	io->io_next_block = bh->b_blocknr;
>  	wbc_init_bio(io->io_wbc, bio);
> -	return 0;
>  }
>  
>  static int io_submit_add_bh(struct ext4_io_submit *io,
> @@ -388,9 +385,7 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
>  		ext4_io_submit(io);
>  	}
>  	if (io->io_bio == NULL) {
> -		ret = io_submit_init_bio(io, bh);
> -		if (ret)
> -			return ret;
> +		io_submit_init_bio(io, bh);
>  		io->io_bio->bi_write_hint = inode->i_write_hint;
>  	}
>  	ret = bio_add_page(io->io_bio, page, bh->b_size, bh_offset(bh));
> diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> index a30b203fa461..bfeb77b93f48 100644
> --- a/fs/ext4/readpage.c
> +++ b/fs/ext4/readpage.c
> @@ -362,8 +362,6 @@ int ext4_mpage_readpages(struct address_space *mapping,
>  
>  			bio = bio_alloc(GFP_KERNEL,
>  				min_t(int, nr_pages, BIO_MAX_PAGES));
> -			if (!bio)
> -				goto set_error_page;
>  			ctx = get_bio_post_read_ctx(inode, bio, page->index);
>  			if (IS_ERR(ctx)) {
>  				bio_put(bio);
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ext4: bio_alloc never fails
  2019-10-31  2:01 ` [PATCH] ext4: bio_alloc " Chao Yu
@ 2019-10-31  9:10   ` Gao Xiang
  0 siblings, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2019-10-31  9:10 UTC (permalink / raw)
  To: Chao Yu; +Cc: Theodore Ts'o, Andreas Dilger, linux-ext4, linux-kernel

Hi Chao,

On Thu, Oct 31, 2019 at 10:01:20AM +0800, Chao Yu wrote:
> On 2019/10/30 12:26, Gao Xiang wrote:
> > Similar to [1] [2], it seems a trivial cleanup since
> > bio_alloc can handle memory allocation as mentioned in
> > fs/direct-io.c (also see fs/block_dev.c, fs/buffer.c, ..)
> > 
> > [1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
> > [2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> 
> I notice that there is still similar code in mpage.c
> 
> static struct bio *
> mpage_alloc(struct block_device *bdev,
> 		sector_t first_sector, int nr_vecs,
> 		gfp_t gfp_flags)
> {
> 	struct bio *bio;
> 
> 	/* Restrict the given (page cache) mask for slab allocations */
> 	gfp_flags &= GFP_KERNEL;
> 	bio = bio_alloc(gfp_flags, nr_vecs);
> 
> 	if (bio == NULL && (current->flags & PF_MEMALLOC)) {
> 		while (!bio && (nr_vecs /= 2))
> 			bio = bio_alloc(gfp_flags, nr_vecs);
> 	}
> 
> 	if (bio) {
> 		bio_set_dev(bio, bdev);
> 		bio->bi_iter.bi_sector = first_sector;
> 	}
> 	return bio;
> }
> 
> Should we clean up them as well? however, I doubt we should get rid of loop in
> mempool allocation to relief the memory pressure on those uncritical path, for
> critical path like we should never fail, it would be fine looping in bio_alloc().

Thanks for your suggestion.

For mpage.c, it seems another story since those gfp_flags are
actually derived from specific inodes (via mapping_gfp_constraint
or readahead_gfp_mask), so I think leaving such paths for mpage.c
could be necessary. Just my personal thought.

Thanks,
Gao Xiang


> 
> Thanks,
> 
> > ---
> >  fs/ext4/page-io.c  | 11 +++--------
> >  fs/ext4/readpage.c |  2 --
> >  2 files changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> > index 12ceadef32c5..f1f7b6601780 100644
> > --- a/fs/ext4/page-io.c
> > +++ b/fs/ext4/page-io.c
> > @@ -358,14 +358,12 @@ void ext4_io_submit_init(struct ext4_io_submit *io,
> >  	io->io_end = NULL;
> >  }
> >  
> > -static int io_submit_init_bio(struct ext4_io_submit *io,
> > -			      struct buffer_head *bh)
> > +static void io_submit_init_bio(struct ext4_io_submit *io,
> > +			       struct buffer_head *bh)
> >  {
> >  	struct bio *bio;
> >  
> >  	bio = bio_alloc(GFP_NOIO, BIO_MAX_PAGES);
> > -	if (!bio)
> > -		return -ENOMEM;
> >  	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
> >  	bio_set_dev(bio, bh->b_bdev);
> >  	bio->bi_end_io = ext4_end_bio;
> > @@ -373,7 +371,6 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
> >  	io->io_bio = bio;
> >  	io->io_next_block = bh->b_blocknr;
> >  	wbc_init_bio(io->io_wbc, bio);
> > -	return 0;
> >  }
> >  
> >  static int io_submit_add_bh(struct ext4_io_submit *io,
> > @@ -388,9 +385,7 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
> >  		ext4_io_submit(io);
> >  	}
> >  	if (io->io_bio == NULL) {
> > -		ret = io_submit_init_bio(io, bh);
> > -		if (ret)
> > -			return ret;
> > +		io_submit_init_bio(io, bh);
> >  		io->io_bio->bi_write_hint = inode->i_write_hint;
> >  	}
> >  	ret = bio_add_page(io->io_bio, page, bh->b_size, bh_offset(bh));
> > diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> > index a30b203fa461..bfeb77b93f48 100644
> > --- a/fs/ext4/readpage.c
> > +++ b/fs/ext4/readpage.c
> > @@ -362,8 +362,6 @@ int ext4_mpage_readpages(struct address_space *mapping,
> >  
> >  			bio = bio_alloc(GFP_KERNEL,
> >  				min_t(int, nr_pages, BIO_MAX_PAGES));
> > -			if (!bio)
> > -				goto set_error_page;
> >  			ctx = get_bio_post_read_ctx(inode, bio, page->index);
> >  			if (IS_ERR(ctx)) {
> >  				bio_put(bio);
> > 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] ext4: bio_alloc with __GFP_DIRECT_RECLAIM never fails
  2019-10-30 16:12     ` Gao Xiang
@ 2019-10-31  9:23       ` Gao Xiang
  2019-10-31  9:29         ` Chao Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Gao Xiang @ 2019-10-31  9:23 UTC (permalink / raw)
  To: Theodore Ts'o, Andreas Dilger
  Cc: linux-ext4, linux-kernel, Gao Xiang, Ritesh Harjani, Chao Yu

Similar to [1] [2], bio_alloc with __GFP_DIRECT_RECLAIM flags
guarantees bio allocation under some given restrictions, as
stated in block/bio.c and fs/direct-io.c So here it's ok to
not check for NULL value from bio_alloc().

[1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
[2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Ritesh Harjani <riteshh@linux.ibm.com>
Cc: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

changes since v1:
 - fix commit message and simplify logic as suggested by Ritesh;
 - add short messages ahead of bio_alloc suggested by Ted.

 fs/ext4/page-io.c  | 57 ++++++++++++++++++----------------------------
 fs/ext4/readpage.c |  6 +++--
 2 files changed, 26 insertions(+), 37 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 12ceadef32c5..c92504c1b1ca 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -358,14 +358,16 @@ void ext4_io_submit_init(struct ext4_io_submit *io,
 	io->io_end = NULL;
 }
 
-static int io_submit_init_bio(struct ext4_io_submit *io,
-			      struct buffer_head *bh)
+static void io_submit_init_bio(struct ext4_io_submit *io,
+			       struct buffer_head *bh)
 {
 	struct bio *bio;
 
+	/*
+	 * bio_alloc will _always_ be able to allocate a bio if
+	 * __GFP_DIRECT_RECLAIM is set, see comments for bio_alloc_bioset().
+	 */
 	bio = bio_alloc(GFP_NOIO, BIO_MAX_PAGES);
-	if (!bio)
-		return -ENOMEM;
 	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
 	bio_set_dev(bio, bh->b_bdev);
 	bio->bi_end_io = ext4_end_bio;
@@ -373,13 +375,12 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
 	io->io_bio = bio;
 	io->io_next_block = bh->b_blocknr;
 	wbc_init_bio(io->io_wbc, bio);
-	return 0;
 }
 
-static int io_submit_add_bh(struct ext4_io_submit *io,
-			    struct inode *inode,
-			    struct page *page,
-			    struct buffer_head *bh)
+static void io_submit_add_bh(struct ext4_io_submit *io,
+			     struct inode *inode,
+			     struct page *page,
+			     struct buffer_head *bh)
 {
 	int ret;
 
@@ -388,9 +389,7 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
 		ext4_io_submit(io);
 	}
 	if (io->io_bio == NULL) {
-		ret = io_submit_init_bio(io, bh);
-		if (ret)
-			return ret;
+		io_submit_init_bio(io, bh);
 		io->io_bio->bi_write_hint = inode->i_write_hint;
 	}
 	ret = bio_add_page(io->io_bio, page, bh->b_size, bh_offset(bh));
@@ -398,7 +397,6 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
 		goto submit_and_retry;
 	wbc_account_cgroup_owner(io->io_wbc, page, bh->b_size);
 	io->io_next_block++;
-	return 0;
 }
 
 int ext4_bio_write_page(struct ext4_io_submit *io,
@@ -491,8 +489,14 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 				gfp_flags |= __GFP_NOFAIL;
 				goto retry_encrypt;
 			}
-			bounce_page = NULL;
-			goto out;
+
+			printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
+			redirty_page_for_writepage(wbc, page);
+			do {
+				clear_buffer_async_write(bh);
+				bh = bh->b_this_page;
+			} while (bh != head);
+			goto unlock;
 		}
 	}
 
@@ -500,30 +504,13 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	do {
 		if (!buffer_async_write(bh))
 			continue;
-		ret = io_submit_add_bh(io, inode, bounce_page ?: page, bh);
-		if (ret) {
-			/*
-			 * We only get here on ENOMEM.  Not much else
-			 * we can do but mark the page as dirty, and
-			 * better luck next time.
-			 */
-			break;
-		}
+		io_submit_add_bh(io, inode,
+				 bounce_page ? bounce_page : page, bh);
 		nr_submitted++;
 		clear_buffer_dirty(bh);
 	} while ((bh = bh->b_this_page) != head);
 
-	/* Error stopped previous loop? Clean up buffers... */
-	if (ret) {
-	out:
-		fscrypt_free_bounce_page(bounce_page);
-		printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
-		redirty_page_for_writepage(wbc, page);
-		do {
-			clear_buffer_async_write(bh);
-			bh = bh->b_this_page;
-		} while (bh != head);
-	}
+unlock:
 	unlock_page(page);
 	/* Nothing submitted - we have to end page writeback */
 	if (!nr_submitted)
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index a30b203fa461..fef7755300c3 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -360,10 +360,12 @@ int ext4_mpage_readpages(struct address_space *mapping,
 		if (bio == NULL) {
 			struct bio_post_read_ctx *ctx;
 
+			/*
+			 * bio_alloc will _always_ be able to allocate a bio if
+			 * __GFP_DIRECT_RECLAIM is set, see bio_alloc_bioset().
+			 */
 			bio = bio_alloc(GFP_KERNEL,
 				min_t(int, nr_pages, BIO_MAX_PAGES));
-			if (!bio)
-				goto set_error_page;
 			ctx = get_bio_post_read_ctx(inode, bio, page->index);
 			if (IS_ERR(ctx)) {
 				bio_put(bio);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] ext4: bio_alloc with __GFP_DIRECT_RECLAIM never fails
  2019-10-31  9:23       ` [PATCH v2] ext4: bio_alloc with __GFP_DIRECT_RECLAIM " Gao Xiang
@ 2019-10-31  9:29         ` Chao Yu
  2019-11-15  3:19           ` Theodore Y. Ts'o
  0 siblings, 1 reply; 11+ messages in thread
From: Chao Yu @ 2019-10-31  9:29 UTC (permalink / raw)
  To: Gao Xiang, Theodore Ts'o, Andreas Dilger
  Cc: linux-ext4, linux-kernel, Ritesh Harjani

On 2019/10/31 17:23, Gao Xiang wrote:
> Similar to [1] [2], bio_alloc with __GFP_DIRECT_RECLAIM flags
> guarantees bio allocation under some given restrictions, as
> stated in block/bio.c and fs/direct-io.c So here it's ok to
> not check for NULL value from bio_alloc().
> 
> [1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
> [2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Cc: Ritesh Harjani <riteshh@linux.ibm.com>
> Cc: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] ext4: bio_alloc with __GFP_DIRECT_RECLAIM never fails
  2019-10-31  9:29         ` Chao Yu
@ 2019-11-15  3:19           ` Theodore Y. Ts'o
  2019-11-15  3:24             ` Gao Xiang
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Y. Ts'o @ 2019-11-15  3:19 UTC (permalink / raw)
  To: Chao Yu
  Cc: Gao Xiang, Andreas Dilger, linux-ext4, linux-kernel, Ritesh Harjani

On Thu, Oct 31, 2019 at 05:29:58PM +0800, Chao Yu wrote:
> On 2019/10/31 17:23, Gao Xiang wrote:
> > Similar to [1] [2], bio_alloc with __GFP_DIRECT_RECLAIM flags
> > guarantees bio allocation under some given restrictions, as
> > stated in block/bio.c and fs/direct-io.c So here it's ok to
> > not check for NULL value from bio_alloc().
> > 
> > [1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
> > [2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > Cc: Ritesh Harjani <riteshh@linux.ibm.com>
> > Cc: Chao Yu <yuchao0@huawei.com>
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] ext4: bio_alloc with __GFP_DIRECT_RECLAIM never fails
  2019-11-15  3:19           ` Theodore Y. Ts'o
@ 2019-11-15  3:24             ` Gao Xiang
  0 siblings, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2019-11-15  3:24 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Chao Yu, Andreas Dilger, linux-ext4, linux-kernel, Ritesh Harjani

Hi Ted,

On Thu, Nov 14, 2019 at 10:19:53PM -0500, Theodore Y. Ts'o wrote:
> On Thu, Oct 31, 2019 at 05:29:58PM +0800, Chao Yu wrote:
> > On 2019/10/31 17:23, Gao Xiang wrote:
> > > Similar to [1] [2], bio_alloc with __GFP_DIRECT_RECLAIM flags
> > > guarantees bio allocation under some given restrictions, as
> > > stated in block/bio.c and fs/direct-io.c So here it's ok to
> > > not check for NULL value from bio_alloc().
> > > 
> > > [1] https://lore.kernel.org/r/20191030035518.65477-1-gaoxiang25@huawei.com
> > > [2] https://lore.kernel.org/r/20190830162812.GA10694@infradead.org
> > > Cc: Theodore Ts'o <tytso@mit.edu>
> > > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > > Cc: Ritesh Harjani <riteshh@linux.ibm.com>
> > > Cc: Chao Yu <yuchao0@huawei.com>
> > > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > 
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks, applied.
> 
> 					- Ted

Thanks for considering this. Have a nice day.

Thanks,
Gao Xiang


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-11-15  3:21 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-30  4:26 [PATCH] ext4: bio_alloc never fails Gao Xiang
2019-10-30 10:13 ` Ritesh Harjani
2019-10-30 10:40   ` Gao Xiang
2019-10-30 15:04   ` Theodore Y. Ts'o
2019-10-30 16:12     ` Gao Xiang
2019-10-31  9:23       ` [PATCH v2] ext4: bio_alloc with __GFP_DIRECT_RECLAIM " Gao Xiang
2019-10-31  9:29         ` Chao Yu
2019-11-15  3:19           ` Theodore Y. Ts'o
2019-11-15  3:24             ` Gao Xiang
2019-10-31  2:01 ` [PATCH] ext4: bio_alloc " Chao Yu
2019-10-31  9:10   ` Gao Xiang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).