Re: [PATCHSET 0/2] Turn single segment imports into ITER_UBUF

From: Jens Axboe <axboe@kernel.dk>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org, torvalds@linux-foundation.org,
	brauner@kernel.org
Subject: Re: [PATCHSET 0/2] Turn single segment imports into ITER_UBUF
Date: Mon, 27 Mar 2023 12:59:09 -0600	[thread overview]
Message-ID: <c975dbcf-1332-5bb5-3375-04280407a897@kernel.dk> (raw)
In-Reply-To: <65c20342-b6ed-59c8-3aef-1d6f6d8bfdf2@kernel.dk>

On 3/27/23 12:52?PM, Jens Axboe wrote:
> On 3/27/23 12:42?PM, Al Viro wrote:
>> On Mon, Mar 27, 2023 at 12:01:08PM -0600, Jens Axboe wrote:
>>> On 3/24/23 10:46?PM, Al Viro wrote:
>>>> On Fri, Mar 24, 2023 at 02:44:41PM -0600, Jens Axboe wrote:
>>>>> Hi,
>>>>>
>>>>> We've been doing a few conversions of ITER_IOVEC to ITER_UBUF in select
>>>>> spots, as the latter is cheaper to iterate and hence saves some cycles.
>>>>> I recently experimented [1] with io_uring converting single segment READV
>>>>> and WRITEV into non-vectored variants, as we can save some cycles through
>>>>> that as well.
>>>>>
>>>>> But there's really no reason why we can't just do this further down,
>>>>> enabling it for everyone. It's quite common to use vectored reads or
>>>>> writes even with a single segment, unfortunately, even for cases where
>>>>> there's no specific reason to do so. From a bit of non-scientific
>>>>> testing on a vm on my laptop, I see about 60% of the import_iovec()
>>>>> calls being for a single segment.
>>>>>
>>>>> I initially was worried that we'd have callers assuming an ITER_IOVEC
>>>>> iter after a call import_iovec() or import_single_range(), but an audit
>>>>> of the kernel code actually looks sane in that regard. Of the ones that
>>>>> do call it, I ran the ltp test cases and they all still pass.
>>>>
>>>> Which tree was that audit on?  Mainline?  Some branch in block.git?
>>>
>>> It was just master in -git. But looks like I did miss two spots, I've
>>> updated the series here and will send out a v2:
>>>
>>> https://git.kernel.dk/cgit/linux-block/log/?h=iter-ubuf
>>
>> Just to make sure - head's at 4d0ba2f0250d?
> 
> Correct!
> 
>> One obvious comment (just about the problems you've dealt with in that
>> branch; I'll go over that tree and look for other sources of trouble,
>> will post tonight):
>>
>> all 3 callers of iov_iter_iovec() in there are accompanied by the identical
>> chunks that deal with ITER_UBUF case; it would make more sense to teach
>> iov_iter_iovec() to handle that.  loop_rw_iter() would turn into
>> 	if (!iov_iter_is_bvec(iter)) {
>> 		iovec = iov_iter_iovec(iter);
>> 	} else {
>> 		iovec.iov_base = u64_to_user_ptr(rw->addr);
>> 		iovec.iov_len = rw->len;
>> 	}
>> and process_madvise() and do_loop_readv_writev() patches simply go away.
>>
>> Again, I'm _not_ saying there's no other problems left, just that these are
>> better dealt with that way.
>>
>> Something like
>>
>> static inline struct iovec iov_iter_iovec(const struct iov_iter *iter)
>> {
>> 	if (WARN_ON(!iter->user_backed))
>> 		return (struct iovec) {
>> 			.iov_base = NULL,
>> 			.iov_len = 0
>> 		};
>> 	else if (iov_iter_is_ubuf(iter))
>> 		return (struct iovec) {
>> 			.iov_base = iter->ubuf + iter->iov_offset,
>> 			.iov_len = iter->count
>> 		}; 
>> 	else
>> 		return (struct iovec) {
>> 			.iov_base = iter->iov->iov_base + iter->iov_offset,
>> 			.iov_len = min(iter->count,
>> 				       iter->iov->iov_len - iter->iov_offset),
>> 		};
>> }
>>
>> and no need to duplicate that logics in all callers.  Or get rid of
>> those elses, seeing that each alternative is a plain return - matter
>> of taste...
> 
> That's a great idea. Two questions - do we want to make that
> WARN_ON_ONCE()? And then do we want to include a WARN_ON_ONCE for a
> non-supported type? Doesn't seem like high risk as they've all been used
> with ITER_IOVEC until now, though.

Scratch that last one, user_backed should double as that as well. At
least currently, where ITER_UBUF and ITER_IOVEC are the only two
iterators that hold user backed memory.

-- 
Jens Axboe