Messages in this thread | | | From | David Howells <> | Subject | Re: [PATCH v7 01/31] iov_iter: Add ITER_XARRAY | Date | Sun, 25 Apr 2021 14:58:02 +0100 |
| |
Al Viro <viro@zeniv.linux.org.uk> wrote:
> > +struct address_space; > > struct pipe_inode_info; > > > > struct kvec { > > What is that chunk for?
Ah, that can go. It used to be ITER_MAPPING.
> > + }), > > + ({ > > + rem = copy_mc_to_page(v.bv_page, v.bv_offset, > > + (from += v.bv_len) - v.bv_len, v.bv_len); > > + if (rem) { > > + curr_addr = (unsigned long) from; > > + bytes = curr_addr - s_addr - rem; > > + rcu_read_unlock(); > > + return bytes; > > + } > > That's broken, same way as kvec and bvec cases are in the same primitive. > Iterator not advanced on failure halfway through.
Okay. I just copied what ITER_BVEC does. Should this be handled in iterate_and_advance() rather than in the code snippets it takes as parameters?
But for the moment, I guess I should just add:
i->iov_offset += bytes;
to all three (kvec, bvec and xarray)?
> > @@ -1246,7 +1349,8 @@ unsigned long iov_iter_alignment(const struct iov_iter *i) > > iterate_all_kinds(i, size, v, > > (res |= (unsigned long)v.iov_base | v.iov_len, 0), > > res |= v.bv_offset | v.bv_len, > > - res |= (unsigned long)v.iov_base | v.iov_len > > + res |= (unsigned long)v.iov_base | v.iov_len, > > + res |= v.bv_offset | v.bv_len > > ) > > return res; > > } > > Hmm... That looks like a really bad overkill - do you need anything beyond > count and iov_offset in that case + perhaps "do we have the very last page"? > IOW, do you need to iterate anything at all here? What am I missing here?
Good point. I wonder, even, if the alignment could just be set to 1. There's no kdoc description on the function that says what the result is meant to represent.
> > @@ -1268,7 +1372,9 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i) > > ... > Very limited use; it shouldn't be called for anything other than IOV_ITER case. Should that just be cut down, then?
> > @@ -1849,7 +2111,12 @@ int iov_iter_for_each_range(struct iov_iter *i, size_t bytes, > > ... > > Would be easier to have that sucker removed first...
I could do that. I'd rather not do that here since it hasn't sat in linux-next, but since nothing uses it, but Linus might permit it.
David
| |