Re: [RFCv2 1/3] iomap: Move creation of iomap_page early in __iomap_write_begin

From: Matthew Wilcox <willy@infradead.org>
To: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Aravinda Herle <araherle@in.ibm.com>
Subject: Re: [RFCv2 1/3] iomap: Move creation of iomap_page early in __iomap_write_begin
Date: Mon, 30 Jan 2023 21:00:01 +0000	[thread overview]
Message-ID: <Y9gv0YV9V6gR9l3F@casper.infradead.org> (raw)
In-Reply-To: <20230130202150.pfohy5yg6dtu64ce@rh-tp>

On Tue, Jan 31, 2023 at 01:51:50AM +0530, Ritesh Harjani (IBM) wrote:
> > > Thus the iop structure will only gets allocated at the time of writeback
> > > in iomap_writepage_map(). This I think, was a not problem till now since
> > > we anyway only track uptodate status in iop (no support of tracking
> > > dirty bitmap status which later patches will add), and we also end up
> > > setting all the bits in iomap_page_create(), if the page is uptodate.
> >
> > delayed iop allocation is a feature and not a bug.  We might have to
> > refine the criteria for sub-page dirty tracking, but in general having
> > the iop allocates is a memory and performance overhead and should be
> > avoided as much as possible.  In fact I still have some unfinished
> > work to allocate it even more lazily.
> 
> So, what I meant here was that the commit[1] chaged the behavior/functionality
> without indenting to. I agree it's not a bug.

It didn't change the behaviour or functionality.  It broke your patches,
but it certainly doesn't deserve its own commit reverting it -- because
it's not wrong.

> But when I added dirty bitmap tracking support, I couldn't understand for
> sometime on why were we allocating iop only at the time of writeback.
> And it was due to a small line change which somehow slipped into this commit [1].
> Hence I made this as a seperate patch so that it doesn't slip through again w/o
> getting noticed/review.

It didn't "slip through".  It was intended.

> Thanks for the info on the lazy allocation work. Yes, though it is not a bug, but
> with subpage dirty tracking in iop->state[], if we end up allocating iop only
> at the time of writeback, than that might cause some performance degradation
> compared to, if we allocat iop at ->write_begin() and mark the required dirty
> bit ranges in ->write_end(). Like how we do in this patch series.
> (Ofcourse it is true only for bs < ps use case).
> 
> [1]: https://lore.kernel.org/all/20220623175157.1715274-5-shr@fb.com/

You absolutely can allocate it in iomap_write_begin, but you can avoid
allocating it until writeback time if (pos, len) entirely overlap the
folio.  ie:

	if (pos > folio_pos(folio) ||
	    pos + len < folio_pos(folio) + folio_size(folio))
		iop = iomap_page_create(iter->inode, folio, iter->flags, false);