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@xxxxxx/ Thanks again for your quick review!! -ritesh
