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/[email protected]
[2] https://lore.kernel.org/r/[email protected]
Signed-off-by: Gao Xiang <[email protected]>
---
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
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/[email protected]
> [2] https://lore.kernel.org/r/[email protected]
> Signed-off-by: Gao Xiang <[email protected]>
> ---
> 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
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/[email protected]
> > [2] https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Gao Xiang <[email protected]>
> > ---
> > 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
>
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
>
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/[email protected]
> [2] https://lore.kernel.org/r/[email protected]
> Signed-off-by: Gao Xiang <[email protected]>
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);
>
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/[email protected]
> > [2] https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Gao Xiang <[email protected]>
>
> 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);
> >
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/[email protected]
[2] https://lore.kernel.org/r/[email protected]
Cc: Theodore Ts'o <[email protected]>
Cc: Andreas Dilger <[email protected]>
Cc: Ritesh Harjani <[email protected]>
Cc: Chao Yu <[email protected]>
Signed-off-by: Gao Xiang <[email protected]>
---
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
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/[email protected]
> [2] https://lore.kernel.org/r/[email protected]
> Cc: Theodore Ts'o <[email protected]>
> Cc: Andreas Dilger <[email protected]>
> Cc: Ritesh Harjani <[email protected]>
> Cc: Chao Yu <[email protected]>
> Signed-off-by: Gao Xiang <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,
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/[email protected]
> > [2] https://lore.kernel.org/r/[email protected]
> > Cc: Theodore Ts'o <[email protected]>
> > Cc: Andreas Dilger <[email protected]>
> > Cc: Ritesh Harjani <[email protected]>
> > Cc: Chao Yu <[email protected]>
> > Signed-off-by: Gao Xiang <[email protected]>
>
> Reviewed-by: Chao Yu <[email protected]>
Thanks, applied.
- Ted