Re: [PATCH 2/6] iov_iter: new iov_iter_pin_pages*(), for FOLL_PIN pages

John Hubbard
2022-02-28T22:49:20Z

On 2/27/22 13:57, Jens Axboe wrote:
>> +ssize_t iov_iter_pin_pages(struct iov_iter *i,
>> + struct page **pages, size_t maxsize, unsigned int maxpages,
>> + size_t *start)
>> +{
>> + size_t len;
>> + int n, res;
>> +
>> + if (maxsize > i->count)
>> + maxsize = i->count;
>> + if (!maxsize)
>> + return 0;
>> +
>> + WARN_ON_ONCE(!iter_is_iovec(i));
>> +
>> + if (likely(iter_is_iovec(i))) {
>> + unsigned int gup_flags = 0;
>> + unsigned long addr;
>> +
>> + if (iov_iter_rw(i) != WRITE)
>> + gup_flags |= FOLL_WRITE;
>> + if (i->nofault)
>> + gup_flags |= FOLL_NOFAULT;
>> +
>> + addr = first_iovec_segment(i, &len, start, maxsize, maxpages);
>> + n = DIV_ROUND_UP(len, PAGE_SIZE);
>> + res = pin_user_pages_fast(addr, n, gup_flags, pages);
>> + if (unlikely(res <= 0))
>> + return res;
>> + return (res == n ? len : res * PAGE_SIZE) - *start;
>
> Trying to be clever like that just makes the code a lot less readable. I
> should not have to reason about a return value. Same in the other
> function.
>

Here is a differential patch on top of this one, and only showing one of
the two routines. How does this direction look to you?

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index e64e8e4edd0c..8e96f1e9ebc6 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1588,7 +1588,17 @@ ssize_t iov_iter_pin_pages(struct iov_iter *i,
res = pin_user_pages_fast(addr, n, gup_flags, pages);
if (unlikely(res <= 0))
return res;
- return (res == n ? len : res * PAGE_SIZE) - *start;
+
+ /* Cap len at the number of pages that were actually pinned: */
+ if (res < n)
+ len = res * PAGE_SIZE;
+
+ /*
+ * The return value is the amount pinned in bytes that the
+ * caller will actually use. So, reduce it by the offset into
+ * the first page:
+ */
+ return len - *start;
}

return -EFAULT;

thanks,
--
John Hubbard
NVIDIA