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@xxxxxx/ 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);
