linux-ext4 - Re: [PATCH 4/9] ext4: Drop pointless IO submission from ext4_bio_write

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: <20221201103519.n32kes6llulr2mcx@quack3>
Date:   Thu, 1 Dec 2022 11:35:19 +0100
From:   Jan Kara <jack@...e.cz>
To:     "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
Cc:     Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
        linux-ext4@...r.kernel.org, Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 4/9] ext4: Drop pointless IO submission from
 ext4_bio_write_page()

On Thu 01-12-22 12:36:55, Ritesh Harjani (IBM) wrote:
> On 22/11/30 05:35PM, Jan Kara wrote:
> > We submit outstanding IO in ext4_bio_write_page() if we find a buffer we
> > are not going to write. This is however pointless because we already
> > handle submission of previous IO in case we detect newly added buffer
> > head is discontiguous. So just delete the pointless IO submission call.
> 
> Agreed. io_submit_add_bh() is anyway called at the end for submitting buffers.
> And io_submit_add_bh() also has the logic to:
> 1. submit a discontiguous bio
> 2. Also submit a bio if the bio gets full (submit_and_retry label).
> 
> Hence calling ext4_io_submit() early is not required.
> 
> I guess the same will also hold true for at this place.
> https://elixir.bootlin.com/linux/v6.1-rc7/source/fs/ext4/page-io.c#L524

So there the submission is needed because we are OOM and are going to wait
for some memory to free. If we have some bio accumulated, it is pinning
pages in writeback state and memory reclaim can be waiting on them. So if
we don't submit, it is a deadlock possibility or at least asking for
trouble.

> But this patch looks good to me. Feel free to add:
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>

Thanks for review!

								Honza
> 
> 
> 
> >
> > Signed-off-by: Jan Kara <jack@...e.cz>
> > ---
> >  fs/ext4/page-io.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> > index 2bdfb8a046d9..beaec6d81074 100644
> > --- a/fs/ext4/page-io.c
> > +++ b/fs/ext4/page-io.c
> > @@ -489,8 +489,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
> >  					redirty_page_for_writepage(wbc, page);
> >  				keep_towrite = true;
> >  			}
> > -			if (io->io_bio)
> > -				ext4_io_submit(io);
> >  			continue;
> >  		}
> >  		if (buffer_new(bh))
> > --
> > 2.35.3
> >
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR