Re: [PATCH 1/2] iomap: Add iomap_iter

[Matthew Wilcox <willy@infradead.org>]

On Tue, Aug 11, 2020 at 02:01:27PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 28, 2020 at 06:32:14PM +0100, Matthew Wilcox (Oracle) wrote:
> > The iomap_iter provides a convenient way to package up and maintain
> > all the arguments to the various mapping and operation functions.
> > iomi_advance() moves the iterator forward to the next chunk of the file.
> > This approach has only one callback to the filesystem -- the iomap_next_t
> > instead of the separate iomap_begin / iomap_end calls.  This slightly
> > complicates the filesystems, but is more efficient.  The next function
> 
> How much more efficient?  I've wondered since the start of

I haven't done any performance measurements.  Not entirely sure what
I'd do to measure it, to be honest.  I suppose I should also note the
decreased stack depth here, although I do mention that in the next patch.

> > +/**
> > + * struct iomap_iter - Iterate through a range of a file.
> > + * @inode: Set at the start of the iteration and should not change.
> > + * @pos: The current file position we are operating on.  It is updated by
> > + *	calls to iomap_iter().  Treat as read-only in the body.
> > + * @len: The remaining length of the file segment we're operating on.
> > + *	It is updated at the same time as @pos.
> > + * @ret: What we want our declaring function to return.  It is initialised
> > + *	to zero and is the cumulative number of bytes processed so far.
> > + *	It is updated at the same time as @pos.
> > + * @copied: The number of bytes operated on by the body in the most
> > + *	recent iteration.  If no bytes were operated on, it may be set to
> > + *	a negative errno.  0 is treated as -EIO.
> > + * @flags: Zero or more of the iomap_begin flags above.
> > + * @iomap: ...
> > + * @srcma:s ...
> 
> ...? :)

I ran out of tuits.  If this approach makes people happy, I can finish it
off.

> > + */
> > +struct iomap_iter {
> > +	struct inode *inode;
> > +	loff_t pos;
> > +	u64 len;
> 
> Thanks for leaving this u64 :)
> 
> > +	loff_t ret;
> > +	ssize_t copied;
> 
> Is this going to be sufficient for SEEK_{HOLE,DATA} when it wants to
> jump ahead by more than 2GB?

That's a good point.  loff_t, I guess.  Even though the current users
of iomap don't support extents larger than 2GB ;-)

> > +	unsigned flags;
> > +	struct iomap iomap;
> > +	struct iomap srcmap;
> > +};
> > +
> > +#define IOMAP_ITER(name, _inode, _pos, _len, _flags)			\
> > +	struct iomap_iter name = {					\
> > +		.inode = _inode,					\
> > +		.pos = _pos,						\
> > +		.len = _len,						\
> > +		.flags = _flags,					\
> > +	}
> > +
> > +typedef int (*iomap_next_t)(const struct iomap_iter *,
> > +		struct iomap *iomap, struct iomap *srcmap);
> 
> I muttered in my other reply how these probably should be called
> iomap_iter_advance_t since they actually do the upper level work of
> advancing the iterator to the next mapping.

nono, the iterator is const!  They don't move the iterator at all,
they just get the next iomap/srcmap.