On Thu, 2021-04-22 at 14:51 +0100, David Howells wrote:
> Jeff Layton <jlayton@xxxxxxxxxx> 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.
>
I suppose.
> > > @@ -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).
>
Ahh thanks. I misread that bit. That makes sense. Sucks about having to
BUG() there, but I'm not sure what else you can do.
> > > +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 like "populate" better.
> > 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@xxxxxxxxxx>
>
> Thanks,
> David
>
Cheers,
--
Jeff Layton <jlayton@xxxxxxxxxx>
