Re: [PATCH 2/2] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Jan Kara <jack@suse.cz>]

On Fri 22-11-19 05:26:58, Christoph Hellwig wrote:
> > -	/*
> > -	 * Operate on a partial iter trimmed to the extent we were called for.
> > -	 * We'll update the iter in the dio once we're done with this extent.
> > -	 */
> > -	iter = *dio->submit.iter;
> > -	iov_iter_truncate(&iter, length);
> > +	/* Operate on a partial iter trimmed to the extent we were called for */
> > +	iov_iter_truncate(dio->submit.iter, length);
> 
> I think the comment could be kept a little more verbose given that the
> scheme isn't exactly obvious.  Also I'd move the initialization of
> orig_count here to keep it all together.  E.g.
> 
> 	/*
> 	 * Save the original count and trim the iter to just the extent we
> 	 * are operating on right now.  The iter will be re-expanded once
> 	 * we are done.
> 	 */
> 	orig_count = iov_iter_count(dio->submit.iter);
> 	iov_iter_truncate(dio->submit.iter, length);
> 
> >  
> > -	nr_pages = iov_iter_npages(&iter, BIO_MAX_PAGES);
> > -	if (nr_pages <= 0)
> > +	nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
> > +	if (nr_pages <= 0) {
> > +		iov_iter_reexpand(dio->submit.iter, orig_count);
> >  		return nr_pages;
> > +	}
> 
> Can we stick to a single iov_iter_reexpand call?  E.g. turn this into
> 
> 	if (nr_pages <= 0) {
> 		ret = nr_pages;
> 		goto out;
> 	}
> 
> and then have the out label at the very end call iov_iter_reexpand.
> 
> >  			iomap_dio_zero(dio, iomap, pos, fs_block_size - pad);
> >  	}
> > +	/* Undo iter limitation to current extent */
> > +	iov_iter_reexpand(dio->submit.iter, orig_count - copied);
> >  	return copied ? copied : ret;
> 
> In iomap-for-next this is:
> 
> 	if (copied)
> 		return copied;
> 	return ret;
> 
> so please rebase to iomap-for-next for the next spin.

OK, I can see Darrick has already picked up the first patch so I'll just
respin this second one with the updates you've asked for. Thanks for
review!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR