On Wed, Jan 08, 2025 at 11:07:22PM -0800, Christoph Hellwig wrote: > On Fri, Dec 13, 2024 at 09:36:07AM -0500, Brian Foster wrote: > > Note that the semantics for operations that use incremental advances > > is slightly different than traditional operations. Operations that > > advance the iter directly are expected to return success or failure > > (i.e. 0 or negative error code) in iter.processed rather than the > > number of bytes processed. > > While the uses of the incremental advance later look nice, this bit > is pretty ugly. I wonder if we could just move overy everything to > the incremental advance model, even if it isn't all that incremental, > that is always call iomap_iter_advance from the processing loop and > eventually remove the call in iomap_iter() entirely? > Yeah, I agree this is a wart. Another option I thought about was creating a new flag to declare which iteration mode a particular operation uses, if for nothing else but to improve clarity. FWIW my first pass at finding a solution here was actually with intent to convert everything over, but then I got lost in the weeds of all the various operations and gave up. I didn't want to spend forever changing every op over for something before it was shown to work or be useful, so my thought process was more to try and see this through for zero range and if that pans out, follow up with changing everything else over as a later step. That said, this was early on before I had the idea fleshed out and I don't recall all the details. I'll make a note to do another audit pass at this and see how feasible it is. I suppose my immediate question on that is: suppose the folio batch thing just doesn't pan out for whatever reason.. would we think this is a worthwhile iteration cleanup on its own? > > @@ -36,7 +36,7 @@ static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count) > > return -EIO; > > iter->pos += count; > > iter->len -= count; > > - if (!iter->len || (!count && !stale)) > > + if (!iter->len || (!count && !stale && iomap_length(iter))) > > This probably warrantd a comment even with the existing code, but really > needs one now. > Ack. > > + * @iter_spos: The original start pos for the current iomap. Used for > > + * incremental iter advance. > > Maybe spell out the usage as iter_start_pos in the field name as spos > reads a little weird? > Ack. Brian
