Re: [RFC][PATCH] iov_iter: Add ITER_MAPPING

[David Howells <dhowells@redhat.com>]

Matthew Wilcox <willy@infradead.org> wrote:

> 
> > +	rcu_read_lock();						\
> > +	for (page = xas_load(&xas); page; page = xas_next(&xas)) {	\
> > +		if (xas_retry(&xas, page))				\
> > +			continue;					\
> > +		if (xa_is_value(page))					\
> > +			break;						\
> 
> Do you also want to check for !page?  That would be a bug in the caller.

Well, I stated that one of the preconditions for using this was that the
caller made sure that segment of the mapping was fully populated, so the check
ought to be unnecessary.

> > +		if (PageCompound(page))					\
> > +			break;						\
> 
> It's perfectly legal to have compound pages in the page cache.  Call
> find_subpage(page, xas.xa_index) unconditionally.

Yeah, I'm just not sure how to deal with them.

> > +		if (page_to_pgoff(page) != xas.xa_index)		\
> > +			break;						\
> 
> ... and you can ditch this if the pages are pinned as find_subpage()
> will bug in this case.

Ok.

> > +		__v.bv_page = page;					\
> > +		offset = (i->mapping_start + skip) & ~PAGE_MASK;	\
> > +		seg = PAGE_SIZE - offset;			\
> > +		__v.bv_offset = offset;				\
> > +		__v.bv_len = min(n, seg);			\
> > +		(void)(STEP);					\
> > +		n -= __v.bv_len;				\
> > +		skip += __v.bv_len;				\
> 
> Do we want STEP to be called with PAGE_SIZE chunks, or if they have a
> THP, can we have it called with larger than a PAGE_SIZE chunk?

It would mean that the STEP function would have to handle multiple pages, some
part(s) of which might need to be ignored and wouldn't be able to simply call
memcpy_from/to_page().

> > +#define iterate_all_kinds(i, n, v, I, B, K, M) {		\
> >  	if (likely(n)) {					\
> >  		size_t skip = i->iov_offset;			\
> >  		if (unlikely(i->type & ITER_BVEC)) {		\
> > @@ -86,6 +119,9 @@
> >  			struct kvec v;				\
> >  			iterate_kvec(i, n, v, kvec, skip, (K))	\
> >  		} else if (unlikely(i->type & ITER_DISCARD)) {	\
> > +		} else if (unlikely(i->type & ITER_MAPPING)) {	\
> > +			struct bio_vec v;			\
> > +			iterate_mapping(i, n, v, skip, (M));	\
> 
> bio_vec?

Yes - as a strictly temporary thing.  I need a struct contains a page pointer,
a start and a length, and constructing a struct bio_vec on the fly here allows
the caller to potentially share code.  For example:

    size_t _copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i)
    {
	char *to = addr;
	if (unlikely(iov_iter_is_pipe(i))) {
		WARN_ON(1);
		return 0;
	}
	iterate_and_advance(i, bytes, v,
		__copy_from_user_inatomic_nocache((to += v.iov_len) - v.iov_len,
					 v.iov_base, v.iov_len),
		memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
				 v.bv_offset, v.bv_len),
ITER_BVEC ^^^^
		memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len),
		memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
				 v.bv_offset, v.bv_len)
ITER_MAPPING ^^^^
	)

	return bytes;
    }

David