Re: [PATCH v4 3/4] block: add bio_iov_iter_get_all_pages() helper

[Martin Wilck <mwilck@suse.com>]

On Fri, 2018-07-20 at 17:11 +0200, Christoph Hellwig wrote:
> On Fri, Jul 20, 2018 at 03:05:51PM +0200, Martin Wilck wrote:
> > bio_iov_iter_get_pages() only adds pages for the next non-zero
> > segment from the iov_iter to the bio. Some callers prefer to
> > obtain as many pages as would fit into the bio, with proper
> > rollback in case of failure. Add bio_iov_iter_get_all_pages()
> > for this purpose.
> 
> I'd much rather have you fix bio_iov_iter_get_pages.  It only has
> three callers, all beeing slight variations of the same direct I/O
> pattern.  There is no point in diverging in implementation details
> for them.

If that's consensus, I'll do it happily. So far I felt there was
opposition against it.


> > +	do {
> > +		int ret = bio_iov_iter_get_pages(bio, iter);
> > +
> > +		if (unlikely(ret)) {
> > +			struct bio_vec *bvec;
> > +			unsigned short i;
> > +
> > +			bio_for_each_segment_all(bvec, bio, i) {
> > +				if (i >= orig_vcnt) {
> > +					put_page(bvec->bv_page);
> > +					bvec->bv_page = NULL;
> > +					bvec->bv_len = 0;
> > +					bvec->bv_offset = 0;
> > +				}
> > +			}
> 
> I don't think we need any of the zeroing here.   Also for code flow
> purposes I'd rather see a goto for the error handling here.

OK, will do.

I'll wait for some more feedback on the "fix bio_iov_iter_get_pages"
part.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)