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

["Ritesh Harjani (IBM)" <ritesh.list@gmail.com>]

On 23/01/30 09:02AM, Christoph Hellwig wrote:
> On Mon, Jan 30, 2023 at 09:44:11PM +0530, Ritesh Harjani (IBM) wrote:
> > The problem is that commit[1] moved iop creation later i.e. after checking for
> > whether the folio is uptodate. And if the folio is uptodate, it simply
> > returns and doesn't allocate a iop.
> > Now what can happen is that during __iomap_write_begin() for bs < ps,
> > there can be a folio which is marked uptodate but does not have a iomap_page
> > structure allocated.
> > (I think one of the reason it can happen is due to memory pressure, we
> > can end up freeing folio->private resource).
> >
> > 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.
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.

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/


Thanks again for your quick review!!
-ritesh