| Message ID | 20221130163608.29034-4-jack@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: Stop using ext4_writepage() for writeout of ordered data | expand |
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 But this patch looks good to me. Feel free to add: Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > 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 >
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 > >
On 22/12/01 11:35AM, Jan Kara wrote: > 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. Aah! right. I didn't see the ret == -ENOMEM compare there. Thanks! -ritesh
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))
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. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/page-io.c | 2 -- 1 file changed, 2 deletions(-)