Re: RFC: switch iomap to an iterator model

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jul 19, 2021 at 12:34:53PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series replies the existing callback-based iomap_apply to an iter based
> model.  The prime aim here is to simply the DAX reflink support, which

Jan Kara pointed out that recent gcc and clang support a magic attribute
that causes a cleanup function to be called when an automatic variable
goes out of scope.  I've ported the XFS for_each_perag* macros to use
it, but I think this would be roughly (totally untested) what you'd do
for iomap iterators:

/* automatic iteration cleanup via macro hell */
struct iomap_iter_cleanup {
	struct iomap_ops	*ops;
	struct iomap_iter	*iter;
	loff_t			*ret;
};

static inline void iomap_iter_cleanup(struct iomap_iter_cleanup *ic)
{
	struct iomap_iter *iter = ic->iter;
	int ret2 = 0;

	if (!iter->iomap.length || !ic->ops->iomap_end)
		return;

	ret2 = ops->iomap_end(iter->inode, iter->pos,
			iomap_length(iter), 0, iter->flags,
			&iter->iomap);

	if (ret2 && *ic->ret == 0)
		*ic->ret = ret2;

	iter->iomap.length = 0;
}

#define IOMAP_ITER_CLEANUP(pag)	\
	struct iomap_iter_cleanup __iomap_iter_cleanup \
			__attribute__((__cleanup__(iomap_iter_cleanup))) = \
			{ .iter = (iter), .ops = (ops), .ret = &(ret) }

#define for_each_iomap(iter, ops, ret) \
	(ret) = iomap_iter((iter), (ops)); \
	for (IOMAP_ITER_CLEANUP(iter, ops, ret); \
		(ret) > 0; \
		(ret) = iomap_iter((iter), (ops)) \

Then we actually /can/ write our iteration loops in the normal C style:

	struct iomap_iter iter = {
		.inode = ...,
		.pos = 0,
		.length = 32768,
	};
	loff_t ret = 0;

	for_each_iomap(&iter, ops, ret) {
		if (iter.iomap.type != WHAT_I_WANT)
                        break;

		ret = am_i_pissed_off(...);
		if (ret)
			return ret;
	}

	return ret;

and ->iomap_end will always get called.  There are a few sharp edges:

I can't figure out how far back clang and gcc support this attribute.
The gcc docs mention it at least far back as 3.3.6.  clang (afaict) docs
don't reference it directly, but the clang 4 docs claim that it can be
as pedantic as gcc w.r.t. attribute use.  That's more than new enough
for upstream, which requires gcc 4.9 or clang 10.

The /other/ problem is that gcc gets fussy about defining variables
inside the for loop parentheses, which means that any code using it has
to compile with -std=gnu99, which is /not/ the usual c89 that the kernel
uses.  OTOH, it's been 22 years since C99 was ratified, c'mon...

--D