Re: [PATCH 2/7] iov_iter: Add a general purpose iteration function

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Aug 23, 2022 at 03:12:21PM +0100, David Howells wrote:

>  	size_t __maybe_unused off = 0;				\
>  	len = n;						\
>  	base = __p + i->iov_offset;				\
> -	len -= (STEP);						\
> -	i->iov_offset += len;					\
> -	n = len;						\
> +	do {							\
> +		len -= (STEP);					\
> +		i->iov_offset += len;				\
> +		n = len;					\
> +	} while (0);						\
>  }

*blink*

What is that supposed to change?

>  /* covers iovec and kvec alike */
> @@ -1611,6 +1613,64 @@ ssize_t extract_iter_to_iter(struct iov_iter *orig,
>  }
>  EXPORT_SYMBOL(extract_iter_to_iter);
>  
> +/**
> + * iov_iter_scan - Scan a source iter
> + * @i: The iterator to scan
> + * @bytes: The amount of buffer/data to scan
> + * @scanner: The function to call to process each segment
> + * @priv: Private data to pass to the scanner function
> + *
> + * Scan an iterator, passing each segment to the scanner function.  If the
> + * scanner returns an error at any time, scanning stops and the error is
> + * returned, otherwise the sum of the scanner results is returned.
> + */
> +ssize_t iov_iter_scan(struct iov_iter *i, size_t bytes,
> +		      ssize_t (*scanner)(struct iov_iter *i, const void *p,
> +					 size_t len, size_t off, void *priv),
> +		      void *priv)
> +{
> +	unsigned int gup_flags = 0;
> +	ssize_t ret = 0, scanned = 0;
> +
> +	if (!bytes)
> +		return 0;
> +	if (WARN_ON(iov_iter_is_discard(i)))
> +		return 0;
> +	if (iter_is_iovec(i))
> +		might_fault();
> +
> +	if (iov_iter_rw(i) != WRITE)
> +		gup_flags |= FOLL_WRITE;
> +	if (i->nofault)
> +		gup_flags |= FOLL_NOFAULT;
> +
> +	iterate_and_advance(
> +		i, bytes, base, len, off, ({
> +				struct page *page;
> +				void *q;
> +
> +				ret = get_user_pages_fast((unsigned long)base, 1,
> +							  gup_flags, &page);
> +				if (ret < 0)
> +					break;
> +				q = kmap_local_page(page);
> +				ret = scanner(i, q, len, off, priv);
> +				kunmap_local(q);
> +				put_page(page);
> +				if (ret < 0)
> +					break;
> +				scanned += ret;
> +			}), ({

Huh?  You do realize that the first ("userland") callback of
iterate_and_advance() is expected to have the amount not processed
as value?  That's what this
	len -= (STEP);
is about.  And anything non-zero means "fucking stop already".

How the hell does that thing manage to work?  And what makes you
think that it'll keep boinking an iovec segment again and again
on short operations?  Is that what that mystery do-while had
been supposed to do?

This makes no sense.  Again, I'm not even talking about the
potential usefulness of the primitive in question - it won't work
as posted, period.