Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Dec 2022 11:35:19 +0100
From:   Jan Kara <jack@suse.cz>
To:     "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc:     Jan Kara <jack@suse.cz>, Ted Tso <tytso@mit.edu>,
        linux-ext4@vger.kernel.org, Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 4/9] ext4: Drop pointless IO submission from
 ext4_bio_write_page()
Message-ID: <20221201103519.n32kes6llulr2mcx@quack3>
References: <20221130162435.2324-1-jack@suse.cz>
 <20221130163608.29034-4-jack@suse.cz>
 <20221201070655.cugep2fdrtntp67y@riteshh-domain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221201070655.cugep2fdrtntp67y@riteshh-domain>
Precedence: bulk

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@gmail.com>

Thanks for review!

								Honza
> 
> 
> 
> >
> > Signed-off-by: Jan Kara <jack@suse.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@suse.com>
SUSE Labs, CR