linux-kernel - Re: [PATCH] ext4: bio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2716559d-95ac-399b-8105-38834f5ed660@huawei.com>
Date:   Thu, 31 Oct 2019 10:01:20 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     Gao Xiang <gaoxiang25@...wei.com>, Theodore Ts'o <tytso@....edu>,
        "Andreas Dilger" <adilger.kernel@...ger.ca>
CC:     <linux-ext4@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ext4: bio_alloc never fails

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@...wei.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);
>