Re: [PATCH V6 12/17] block: ublk_drv: cleanup ublk_copy_user_pages

[Bernd Schubert]

On 3/30/23 13:36, Ming Lei wrote:
> Clean up ublk_copy_user_pages() by using iov iter, and code
> gets simplified a lot and becomes much more readable than before.
> 
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
>   drivers/block/ublk_drv.c | 112 +++++++++++++++++----------------------
>   1 file changed, 49 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index fdccbf5fdaa1..cca0e95a89d8 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -419,49 +419,39 @@ static const struct block_device_operations ub_fops = {
>   
>   #define UBLK_MAX_PIN_PAGES	32
>   
> -struct ublk_map_data {
> -	const struct request *rq;
> -	unsigned long	ubuf;
> -	unsigned int	len;
> -};
> -
>   struct ublk_io_iter {
>   	struct page *pages[UBLK_MAX_PIN_PAGES];
> -	unsigned pg_off;	/* offset in the 1st page in pages */
> -	int nr_pages;		/* how many page pointers in pages */
>   	struct bio *bio;
>   	struct bvec_iter iter;
>   };
>   
> -static inline unsigned ublk_copy_io_pages(struct ublk_io_iter *data,
> -		unsigned max_bytes, bool to_vm)
> +/* return how many pages are copied */
> +static void ublk_copy_io_pages(struct ublk_io_iter *data,
> +		size_t total, size_t pg_off, int dir)
>   {
> -	const unsigned total = min_t(unsigned, max_bytes,
> -			PAGE_SIZE - data->pg_off +
> -			((data->nr_pages - 1) << PAGE_SHIFT));
>   	unsigned done = 0;
>   	unsigned pg_idx = 0;
>   
>   	while (done < total) {
>   		struct bio_vec bv = bio_iter_iovec(data->bio, data->iter);
> -		const unsigned int bytes = min3(bv.bv_len, total - done,
> -				(unsigned)(PAGE_SIZE - data->pg_off));
> +		unsigned int bytes = min3(bv.bv_len, (unsigned)total - done,
> +				(unsigned)(PAGE_SIZE - pg_off));
>   		void *bv_buf = bvec_kmap_local(&bv);
>   		void *pg_buf = kmap_local_page(data->pages[pg_idx]);
>   
> -		if (to_vm)
> -			memcpy(pg_buf + data->pg_off, bv_buf, bytes);
> +		if (dir == ITER_DEST)
> +			memcpy(pg_buf + pg_off, bv_buf, bytes);
>   		else
> -			memcpy(bv_buf, pg_buf + data->pg_off, bytes);
> +			memcpy(bv_buf, pg_buf + pg_off, bytes);
>   
>   		kunmap_local(pg_buf);
>   		kunmap_local(bv_buf);
>   
>   		/* advance page array */
> -		data->pg_off += bytes;
> -		if (data->pg_off == PAGE_SIZE) {
> +		pg_off += bytes;
> +		if (pg_off == PAGE_SIZE) {
>   			pg_idx += 1;
> -			data->pg_off = 0;
> +			pg_off = 0;
>   		}
>   
>   		done += bytes;
> @@ -475,41 +465,40 @@ static inline unsigned ublk_copy_io_pages(struct ublk_io_iter *data,
>   			data->iter = data->bio->bi_iter;
>   		}
>   	}
> -
> -	return done;
>   }
>   
> -static int ublk_copy_user_pages(struct ublk_map_data *data, bool to_vm)
> +/*
> + * Copy data between request pages and io_iter, and 'offset'
> + * is the start point of linear offset of request.
> + */
> +static size_t ublk_copy_user_pages(const struct request *req,
> +		struct iov_iter *uiter, int dir)
>   {
> -	const unsigned int gup_flags = to_vm ? FOLL_WRITE : 0;
> -	const unsigned long start_vm = data->ubuf;
> -	unsigned int done = 0;
>   	struct ublk_io_iter iter = {
> -		.pg_off	= start_vm & (PAGE_SIZE - 1),
> -		.bio	= data->rq->bio,
> -		.iter	= data->rq->bio->bi_iter,
> +		.bio	= req->bio,
> +		.iter	= req->bio->bi_iter,
>   	};
> -	const unsigned int nr_pages = round_up(data->len +
> -			(start_vm & (PAGE_SIZE - 1)), PAGE_SIZE) >> PAGE_SHIFT;
> -
> -	while (done < nr_pages) {
> -		const unsigned to_pin = min_t(unsigned, UBLK_MAX_PIN_PAGES,
> -				nr_pages - done);
> -		unsigned i, len;
> -
> -		iter.nr_pages = get_user_pages_fast(start_vm +
> -				(done << PAGE_SHIFT), to_pin, gup_flags,
> -				iter.pages);
> -		if (iter.nr_pages <= 0)
> -			return done == 0 ? iter.nr_pages : done;
> -		len = ublk_copy_io_pages(&iter, data->len, to_vm);
> -		for (i = 0; i < iter.nr_pages; i++) {
> -			if (to_vm)
> +	size_t done = 0;
> +
> +	while (iov_iter_count(uiter) && iter.bio) {
> +		unsigned nr_pages;
> +		size_t len, off;
> +		int i;
> +
> +		len = iov_iter_get_pages2(uiter, iter.pages,
> +				iov_iter_count(uiter),
> +				UBLK_MAX_PIN_PAGES, &off);
> +		if (len <= 0)
> +			return done;
> +
> +		ublk_copy_io_pages(&iter, len, off, dir);
> +		nr_pages = DIV_ROUND_UP(len + off, PAGE_SIZE);
> +		for (i = 0; i < nr_pages; i++) {
> +			if (dir == ITER_DEST)
>   				set_page_dirty(iter.pages[i]);
>   			put_page(iter.pages[i]);
>   		}
> -		data->len -= len;
> -		done += iter.nr_pages;
> +		done += len;
>   	}
>   
>   	return done;
> @@ -536,15 +525,14 @@ static int ublk_map_io(const struct ublk_queue *ubq, const struct request *req,
>   	 * context is pretty fast, see ublk_pin_user_pages
>   	 */
>   	if (ublk_need_map_req(req)) {
> -		struct ublk_map_data data = {
> -			.rq	=	req,
> -			.ubuf	=	io->addr,
> -			.len	=	rq_bytes,
> -		};
> +		struct iov_iter iter;
> +		struct iovec iov;
> +		const int dir = ITER_DEST;

Maybe a comment here that this means "copy to daemon"?

>   
> -		ublk_copy_user_pages(&data, true);
> +		import_single_range(dir, u64_to_user_ptr(io->addr), rq_bytes,
> +				&iov, &iter);
>   
> -		return rq_bytes - data.len;
> +		return ublk_copy_user_pages(req, &iter, dir);
>   	}
>   	return rq_bytes;
>   }
> @@ -556,17 +544,15 @@ static int ublk_unmap_io(const struct ublk_queue *ubq,
>   	const unsigned int rq_bytes = blk_rq_bytes(req);
>   
>   	if (ublk_need_unmap_req(req)) {
> -		struct ublk_map_data data = {
> -			.rq	=	req,
> -			.ubuf	=	io->addr,
> -			.len	=	io->res,
> -		};
> +		struct iov_iter iter;
> +		struct iovec iov;
> +		const int dir = ITER_SOURCE;

And here "from daemon"?
>   
>   		WARN_ON_ONCE(io->res > rq_bytes);
>   
> -		ublk_copy_user_pages(&data, false);
> -
> -		return io->res - data.len;
> +		import_single_range(dir, u64_to_user_ptr(io->addr), io->res,
> +				&iov, &iter);
> +		return ublk_copy_user_pages(req, &iter, dir);
>   	}
>   	return rq_bytes;
>   }