* [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
* [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
* 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
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).