Re: [PATCH V3 1/1] ublk: add io_uring based userspace block driver

[Ziyang Zhang]

On 2022/6/30 20:33, Ming Lei wrote:
> On Thu, Jun 30, 2022 at 07:35:11PM +0800, Ziyang Zhang wrote:
>> On 2022/6/29 00:08, Ming Lei wrote:
>>
>> [...]
>>
>>> +#define UBLK_MAX_PIN_PAGES	32
>>> +
>>> +static inline void ublk_release_pages(struct ublk_queue *ubq, struct page **pages,
>>> +		int nr_pages)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < nr_pages; i++)
>>> +		put_page(pages[i]);
>>> +}
>>> +
>>> +static inline int ublk_pin_user_pages(struct ublk_queue *ubq, u64 start_vm,
>>> +		unsigned int nr_pages, unsigned int gup_flags,
>>> +		struct page **pages)
>>> +{
>>> +	return get_user_pages_fast(start_vm, nr_pages, gup_flags, pages);
>>> +}
>>
>>> +
>>> +static inline unsigned ublk_copy_bv(struct bio_vec *bv, void **bv_addr,
>>> +		void *pg_addr, unsigned int *pg_off,
>>> +		unsigned int *pg_len, bool to_bv)
>>> +{
>>> +	unsigned len = min_t(unsigned, bv->bv_len, *pg_len);
>>> +
>>> +	if (*bv_addr == NULL)
>>> +		*bv_addr = kmap_local_page(bv->bv_page);
>>> +
>>> +	if (to_bv)
>>> +		memcpy(*bv_addr + bv->bv_offset, pg_addr + *pg_off, len);
>>> +	else
>>> +		memcpy(pg_addr + *pg_off, *bv_addr + bv->bv_offset, len);
>>> +
>>> +	bv->bv_offset += len;
>>> +	bv->bv_len -= len;
>>> +	*pg_off += len;
>>> +	*pg_len -= len;
>>> +
>>> +	if (!bv->bv_len) {
>>> +		kunmap_local(*bv_addr);
>>> +		*bv_addr = NULL;
>>> +	}
>>> +
>>> +	return len;
>>> +}
>>> +
>>> +/* copy rq pages to ublksrv vm address pointed by io->addr */
>>> +static int ublk_copy_pages(struct ublk_queue *ubq, struct request *rq, bool to_rq,
>>> +		unsigned int max_bytes)
>>> +{
>>> +	unsigned int gup_flags = to_rq ? 0 : FOLL_WRITE;
>>> +	struct ublk_io *io = &ubq->ios[rq->tag];
>>> +	struct page *pgs[UBLK_MAX_PIN_PAGES];
>>> +	struct req_iterator req_iter;
>>> +	struct bio_vec bv;
>>> +	const unsigned int rq_bytes = min(blk_rq_bytes(rq), max_bytes);
>>> +	unsigned long start = io->addr, left = rq_bytes;
>>> +	unsigned int idx = 0, pg_len = 0, pg_off = 0;
>>> +	int nr_pin = 0;
>>> +	void *pg_addr = NULL;
>>> +	struct page *curr = NULL;
>>> +
>>> +	rq_for_each_segment(bv, rq, req_iter) {
>>> +		unsigned len, bv_off = bv.bv_offset, bv_len = bv.bv_len;
>>> +		void *bv_addr = NULL;
>>> +
>>> +refill:
>>> +		if (pg_len == 0) {
>>> +			unsigned int off = 0;
>>> +
>>> +			if (pg_addr) {
>>> +				kunmap_local(pg_addr);
>>> +				if (!to_rq)
>>> +					set_page_dirty_lock(curr);
>>> +				pg_addr = NULL;
>>> +			}
>>> +
>>> +			/* refill pages */
>>> +			if (idx >= nr_pin) {
>>> +				unsigned int max_pages;
>>> +
>>> +				ublk_release_pages(ubq, pgs, nr_pin);
>>> +
>>> +				off = start & (PAGE_SIZE - 1);
>>> +				max_pages = min_t(unsigned, (off + left +
>>> +						PAGE_SIZE - 1) >> PAGE_SHIFT,
>>> +						UBLK_MAX_PIN_PAGES);
>>> +				nr_pin = ublk_pin_user_pages(ubq, start,
>>> +						max_pages, gup_flags, pgs);
>>> +				if (nr_pin < 0)
>>> +					goto exit;
>>> +				idx = 0;
>>> +			}
>>> +			pg_off = off;
>>> +			pg_len = min(PAGE_SIZE - off, left);
>>> +			off = 0;
>>> +			curr = pgs[idx++];
>>> +			pg_addr = kmap_local_page(curr);
>>> +		}
>>> +
>>> +		len = ublk_copy_bv(&bv, &bv_addr, pg_addr, &pg_off, &pg_len,
>>> +				to_rq);
>>> +		/* either one of the two has been consumed */
>>> +		WARN_ON_ONCE(bv.bv_len && pg_len);
>>> +		start += len;
>>> +		left -= len;
>>> +
>>> +		/* overflow */
>>> +		WARN_ON_ONCE(left > rq_bytes);
>>> +		WARN_ON_ONCE(bv.bv_len > bv_len);
>>> +		if (bv.bv_len)
>>> +			goto refill;
>>> +
>>> +		bv.bv_len = bv_len;
>>> +		bv.bv_offset = bv_off;
>>> +	}
>>> +	if (pg_addr) {
>>> +		kunmap_local(pg_addr);
>>> +		if (!to_rq)
>>> +			set_page_dirty_lock(curr);
>>> +	}
>>> +	ublk_release_pages(ubq, pgs, nr_pin);
>>> +
>>> +exit:
>>> +	return rq_bytes - left;
>>> +}
>>> +
>>
>> Hi Ming, 
>>
>> I note that you pin the user buffer's pages, memcpy() and release them immediately.
>>
>> 1) I think maybe copy_page_from_iter() is another choice for copying user buffer to biovecs
>>    since copy_page_from_iter() do not pin pages(But it may raise page fault).
> 
> copy_page_from_iter/copy_page_to_iter needs the userspage page,
> then copy between the userspace page and bvec_iter pages, what it does
> is just kmap/copy/kunmap.
> 
> Not see it is useful here.


No, I don't agree.
copy_page_from_iter(): copy data from an iovec to kernel pages(such as bio's bv pages).
It finally calls raw_copy_from_user().

Here the src(iovec, here it is from user) is actually generated 
from a single void __user *ubuf, not a userspace page.

In copy_page_from_iter() I only find kmap/kunmap for the dest(kernel pages)
but it is unnecessary to kmap/kunmap the src iovec(from user) 
and please check the exception table usage in this routine.
I think raw_copy_from_user() inside copy_page_from_iter() should handle page faults.

You may find blk_rq_map_user() and bio_copy_from_iter() use copy_page_from_iter()
to copy from  void __user *ubuf to bio's bv pages. 

> 
>>
>> 2) Or will you design some mechanism such as LRU to manage these pinned pages? 
>>    For example pin those pages frequently required for a long time and release
>>    those pages not used for a long time.
>>    I remember you have talked about this LRU on pinned pages?
> 
> I'd explain it a bit.
> 
> When I worked on v1/v2, 'perf report' shows that get_user_pages_fast()
> as one of top samples. Turns out it is a bug, which is fixed in
> 
> https://github.com/ming1/linux/commit/3c9fd476951759858cc548dee4cedc074194d0b0
> 
> After the issue is fixed, not see get_user_pages_fast() being hot spot
> any more. I actually implemented one patch which pins all pages in
> the ubd device whole lifetime, but not see obvious improvement, so I gave
> up the idea.
> 
> In the test VM on my laptop, single job ubd/null randwrite can reach 700K iops.
> 
>>
>> Which one do you think is better? copy_page_from_iter() or pin pages with LRU?
>> Maybe it depends on the user's workload?
> 
> So far in the enablement stage, I think the current approach is just fine,
> but we still can improve it in future.
> 
> 
> Thanks,
> Ming