Re: [PATCH v6 01/30] iov_iter: Add ITER_XARRAY

[David Howells <dhowells@redhat.com>]

Jeff Layton <jlayton@kernel.org> wrote:

> As a general note, iov_iter.c could really do with some (verbose)
> comments explaining things. A kerneldoc header that explains the
> arguments to iterate_all_kinds would sure make this easier to review.

Definitely.  But that really requires a separate patch.

> > @@ -1126,7 +1199,12 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll)
> >  		return;
> >  	}
> >  	unroll -= i->iov_offset;
> > -	if (iov_iter_is_bvec(i)) {
> > +	if (iov_iter_is_xarray(i)) {
> > +		BUG(); /* We should never go beyond the start of the specified
> > +			* range since we might then be straying into pages that
> > +			* aren't pinned.
> > +			*/
> 
> It's not needed now, but there are a lot of calls to iov_iter_revert in
> the kernel, and going backward doesn't necessarily mean we'd be straying
> into an unpinned range. xarray_start never changes; would it not be ok
> to allow reverting as long as you don't move to a lower offset than that
> point?

This is handled starting a couple of lines above the start of the hunk:

	if (unroll <= i->iov_offset) {
		i->iov_offset -= unroll;
		return;
	}

As long as the amount you want to unroll by doesn't exceed the amount you've
consumed of the iterator, it will allow you to do it.  The BUG is there to
catch someone attempting to over-revert (and there's no way to return an
error).

> > +static ssize_t iter_xarray_copy_pages(struct page **pages, struct xarray *xa,
> > +				       pgoff_t index, unsigned int nr_pages)
> 
> nit: This could use a different name -- I was expecting to see page
> _contents_ copied here, but it's just populating the page array with
> pointers.

Fair point.  Um...  how about iter_xarray_populate_pages() or
iter_xarray_list_pages()?

> I think you've planned to remove iov_iter_for_each_range as well? I'll
> assume that this is going away. It might be nice to post the latest
> version of this patch with that change, just for posterity.

I'll put that in a separate patch.

> In any case, this all looks reasonable to me, modulo a few nits and a
> general dearth of comments.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Thanks,
David