On Mon, Jan 27, 2025 at 09:34:15PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2025 at 08:34:30AM -0500, Brian Foster wrote:
> > + s64 ret;
> > + bool stale = iter->iomap.flags & IOMAP_F_STALE;
>
> Nit: I find code more redable if variables that initialized at
> declaration time (especially when derived from arguments) are
> before plain variable declarations. Not a big thing here with just
> two of them, but variable counts keep growing over time.
>
Ack. I have a couple pending changes for followon work I wanted to fold
into a v3 anyways, so I'll make that tweak.
> >
> > - if (iter->iomap.length && ops->iomap_end) {
> > + if (!iter->iomap.length) {
> > + trace_iomap_iter(iter, ops, _RET_IP_);
> > + goto begin;
> > + }
>
> This might be a chance to split trace_iomap_iter into two trace points
> for the initial and following iterations? Or maybe we shouldn't bother.
>
Hmm.. not sure I see the value in a tracepoint just for the initial
case, but maybe we should just move trace_iomap_iter() to the top of the
function? We already have post-lookup tracepoints in iomap_iter_done()
to show the mappings, and that would remove the duplication. Hm?
> Otherwise this looks great:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>
Thanks.
Brian
