Dave Chinner <david@xxxxxxxxxxxxx> writes: > On Mon, Feb 27, 2023 at 01:13:30AM +0530, Ritesh Harjani (IBM) wrote: >> Earlier when the folio is uptodate, we only allocate iop at writeback >> time (in iomap_writepage_map()). This is ok until now, but when we are >> going to add support for subpage size dirty bitmap tracking in iop, this >> could cause some performance degradation. The reason is that if we don't >> allocate iop during ->write_begin(), then we will never mark the >> necessary dirty bits in ->write_end() call. And we will have to mark all >> the bits as dirty at the writeback time, that could cause the same write >> amplification and performance problems as it is now (w/o subpage dirty >> bitmap tracking in iop). >> >> However, for all the writes with (pos, len) which completely overlaps >> the given folio, there is no need to allocate an iop during >> ->write_begin(). So skip those cases. >> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> >> --- >> fs/iomap/buffered-io.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c >> index 356193e44cf0..c5b51ab1184e 100644 >> --- a/fs/iomap/buffered-io.c >> +++ b/fs/iomap/buffered-io.c >> @@ -535,11 +535,16 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, >> size_t from = offset_in_folio(folio, pos), to = from + len; >> size_t poff, plen; >> >> + if (pos <= folio_pos(folio) && >> + pos + len >= folio_pos(folio) + folio_size(folio)) >> + return 0; > > This is magic without a comment explaining why it exists. You have > that explanation in the commit message, but that doesn't help anyone > looking at the code: > > /* > * If the write completely overlaps the current folio, then > * entire folio will be dirtied so there is no need for > * sub-folio state tracking structures to be attached to this folio. > */ Sure, got it. I will add a comment which explains this in the code as well. Thanks for the review! -ritesh
