Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/2] iomap: Add iomap_iter
Date: Tue, 11 Aug 2020 22:32:30 +0100	[thread overview]
Message-ID: <20200811213230.GX17456@casper.infradead.org> (raw)
In-Reply-To: <20200811210127.GG6107@magnolia>

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.


  reply	other threads:[~2020-08-11 21:32 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-28 17:32 [RFC 0/2] Avoid indirect function calls in iomap Matthew Wilcox (Oracle)
2020-07-28 17:32 ` [PATCH 1/2] iomap: Add iomap_iter Matthew Wilcox (Oracle)
2020-08-11 21:01   ` Darrick J. Wong
2020-08-11 21:32     ` Matthew Wilcox [this message]
2020-07-28 17:32 ` [PATCH 2/2] iomap: Convert readahead to iomap_iter Matthew Wilcox (Oracle)
2020-08-11 20:56   ` Darrick J. Wong
2020-08-11 22:31     ` Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200811213230.GX17456@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=darrick.wong@oracle.com \
    --cc=hch@infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --subject='Re: [PATCH 1/2] iomap: Add iomap_iter' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).