This reduces, by one, the number of callers of iov_iter_get_pages().
That's helpful because these calls are being audited and converted over
to use iov_iter_pin_user_pages(), where applicable. And this one here is
already known by the caller to be only for ITER_PIPE, so let's just
simplify it now.
Signed-off-by: John Hubbard <[email protected]>
---
fs/ceph/file.c | 3 +--
include/linux/uio.h | 3 ++-
lib/iov_iter.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index d51c3f2fdca0..d3d7dd957390 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -879,8 +879,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
more = len < iov_iter_count(to);
if (unlikely(iov_iter_is_pipe(to))) {
- ret = iov_iter_get_pages_alloc(to, &pages, len,
- &page_off);
+ ret = pipe_get_pages_alloc(to, &pages, len, &page_off);
if (ret <= 0) {
ceph_osdc_put_request(req);
ret = -ENOMEM;
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 62bcf5e45f2b..76cd47ab3dfd 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -227,7 +227,8 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages,
size_t maxsize, size_t *start);
int iov_iter_npages(const struct iov_iter *i, int maxpages);
-
+ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
+ size_t maxsize, size_t *start);
const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags);
ssize_t iov_iter_pin_user_pages(struct bio *bio, struct iov_iter *i, struct page **pages,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index a4bc1b3a3fda..f571fe3ddbe8 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1396,9 +1396,8 @@ static struct page **get_pages_array(size_t n)
return kvmalloc_array(n, sizeof(struct page *), GFP_KERNEL);
}
-static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
- struct page ***pages, size_t maxsize,
- size_t *start)
+ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
+ size_t maxsize, size_t *start)
{
struct page **p;
unsigned int iter_head, npages;
@@ -1428,6 +1427,7 @@ static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
kvfree(p);
return n;
}
+EXPORT_SYMBOL(pipe_get_pages_alloc);
ssize_t iov_iter_pin_user_pages_alloc(struct bio *bio, struct iov_iter *i,
struct page ***pages, size_t maxsize,
--
2.28.0
On Fri, 2020-08-21 at 21:20 -0700, John Hubbard wrote:
> This reduces, by one, the number of callers of iov_iter_get_pages().
> That's helpful because these calls are being audited and converted over
> to use iov_iter_pin_user_pages(), where applicable. And this one here is
> already known by the caller to be only for ITER_PIPE, so let's just
> simplify it now.
>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> fs/ceph/file.c | 3 +--
> include/linux/uio.h | 3 ++-
> lib/iov_iter.c | 6 +++---
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index d51c3f2fdca0..d3d7dd957390 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -879,8 +879,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
> more = len < iov_iter_count(to);
>
> if (unlikely(iov_iter_is_pipe(to))) {
> - ret = iov_iter_get_pages_alloc(to, &pages, len,
> - &page_off);
> + ret = pipe_get_pages_alloc(to, &pages, len, &page_off);
> if (ret <= 0) {
> ceph_osdc_put_request(req);
> ret = -ENOMEM;
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 62bcf5e45f2b..76cd47ab3dfd 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -227,7 +227,8 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
> ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages,
> size_t maxsize, size_t *start);
> int iov_iter_npages(const struct iov_iter *i, int maxpages);
> -
> +ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
> + size_t maxsize, size_t *start);
> const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags);
>
> ssize_t iov_iter_pin_user_pages(struct bio *bio, struct iov_iter *i, struct page **pages,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index a4bc1b3a3fda..f571fe3ddbe8 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1396,9 +1396,8 @@ static struct page **get_pages_array(size_t n)
> return kvmalloc_array(n, sizeof(struct page *), GFP_KERNEL);
> }
>
> -static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
> - struct page ***pages, size_t maxsize,
> - size_t *start)
> +ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
> + size_t maxsize, size_t *start)
> {
> struct page **p;
> unsigned int iter_head, npages;
> @@ -1428,6 +1427,7 @@ static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
> kvfree(p);
> return n;
> }
> +EXPORT_SYMBOL(pipe_get_pages_alloc);
>
> ssize_t iov_iter_pin_user_pages_alloc(struct bio *bio, struct iov_iter *i,
> struct page ***pages, size_t maxsize,
This looks fine to me. Let me know if you need this merged via the ceph
tree. Thanks!
Acked-by: Jeff Layton <[email protected]>
On 8/24/20 3:53 AM, Jeff Layton wrote:
>
> This looks fine to me. Let me know if you need this merged via the ceph
> tree. Thanks!
>
> Acked-by: Jeff Layton <[email protected]>
>
Yes, please! It will get proper testing that way, and it doesn't have
any prerequisites, despite being part of this series. So it would be
great to go in via the ceph tree.
For the main series here, I'll send a v2 with only patches 1-3, once
enough feedback has happened.
thanks,
--
John Hubbard
NVIDIA
On Mon, 2020-08-24 at 10:54 -0700, John Hubbard wrote:
> On 8/24/20 3:53 AM, Jeff Layton wrote:
> > This looks fine to me. Let me know if you need this merged via the ceph
> > tree. Thanks!
> >
> > Acked-by: Jeff Layton <[email protected]>
> >
>
> Yes, please! It will get proper testing that way, and it doesn't have
> any prerequisites, despite being part of this series. So it would be
> great to go in via the ceph tree.
>
> For the main series here, I'll send a v2 with only patches 1-3, once
> enough feedback has happened.
>
Ok, I'll plan to pick it up providing no one has issues with exporting that symbol.
Thanks!
--
Jeff Layton <[email protected]>
On Mon, Aug 24, 2020 at 02:47:53PM -0400, Jeff Layton wrote:
> Ok, I'll plan to pick it up providing no one has issues with exporting that symbol.
_GPL, perhaps?
On 8/24/20 11:54 AM, Matthew Wilcox wrote:
> On Mon, Aug 24, 2020 at 02:47:53PM -0400, Jeff Layton wrote:
>> Ok, I'll plan to pick it up providing no one has issues with exporting that symbol.
>
> _GPL, perhaps?
>
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
t=1598295688; bh=TGtHrKY9YE9vgbD3P6YUTHn7yhP+gCmFr3Z8XIdh17s=;
h=X-PGP-Universal:Subject:To:CC:References:From:Message-ID:Date:
User-Agent:MIME-Version:In-Reply-To:X-Originating-IP:
X-ClientProxiedBy:Content-Type:Content-Language:
Content-Transfer-Encoding;
b=KWZ+bMZ8RXIxd4CMBL8Dn6hn0ggsojx1vJaaueo+/+Xwe0yKBa+bMZfnn1XUDWvJs
KMZJ22FgEdc+HO/8Mx0JKVQsLfKj74dwj3kGjs1z0KA5Vcnx93pzd/iMXDFhClf3uz
KmyGEdar0p6poBaBOlsapOb59acP6ot16rhi7ZbTch+7ErO/Rupx6VinU1A2Ydc3OP
mBYXZqz35DZ/H5eqhoE84MuOFj8Ti/Wpen637pLLa5dmtXjSMRmYTQXMRygUmQXdaw
g9XLuqaxKRxp2lnuoVdFK0T90Hfu/71T+S8asZZYhH9zHY2Wzhgp1VkR07ZtXmMNqI
W/lB00RAVtj3Q==
I looked at that, and the equivalent iov_iter_get_pages* and related stuff was
just EXPORT_SYMBOL, so I tried to match that. But if it needs to be _GPL then
that's fine too...
thanks,
--
John Hubbard
NVIDIA
This reduces, by one, the number of callers of iov_iter_get_pages().
That's helpful because these calls are being audited and converted over
to use iov_iter_pin_user_pages(), where applicable. And this one here is
already known by the caller to be only for ITER_PIPE, so let's just
simplify it now.
Signed-off-by: John Hubbard <[email protected]>
---
OK, here's a v2 that does EXPORT_SYMBOL_GPL, instead of EXPORT_SYMBOL,
that's the only change from v1. That should help give this patch a
clear bill of passage. :)
thanks,
John Hubbard
NVIDIA
fs/ceph/file.c | 3 +--
include/linux/uio.h | 3 ++-
lib/iov_iter.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index d51c3f2fdca0..d3d7dd957390 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -879,8 +879,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
more = len < iov_iter_count(to);
if (unlikely(iov_iter_is_pipe(to))) {
- ret = iov_iter_get_pages_alloc(to, &pages, len,
- &page_off);
+ ret = pipe_get_pages_alloc(to, &pages, len, &page_off);
if (ret <= 0) {
ceph_osdc_put_request(req);
ret = -ENOMEM;
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 3835a8a8e9ea..270a4dcf5453 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -226,7 +226,8 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages,
size_t maxsize, size_t *start);
int iov_iter_npages(const struct iov_iter *i, int maxpages);
-
+ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
+ size_t maxsize, size_t *start);
const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags);
static inline size_t iov_iter_count(const struct iov_iter *i)
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 5e40786c8f12..6290998df480 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1355,9 +1355,8 @@ static struct page **get_pages_array(size_t n)
return kvmalloc_array(n, sizeof(struct page *), GFP_KERNEL);
}
-static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
- struct page ***pages, size_t maxsize,
- size_t *start)
+ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
+ size_t maxsize, size_t *start)
{
struct page **p;
unsigned int iter_head, npages;
@@ -1387,6 +1386,7 @@ static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
kvfree(p);
return n;
}
+EXPORT_SYMBOL_GPL(pipe_get_pages_alloc);
ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
struct page ***pages, size_t maxsize,
--
2.28.0
On Mon, 2020-08-24 at 18:20 -0700, John Hubbard wrote:
> This reduces, by one, the number of callers of iov_iter_get_pages().
> That's helpful because these calls are being audited and converted over
> to use iov_iter_pin_user_pages(), where applicable. And this one here is
> already known by the caller to be only for ITER_PIPE, so let's just
> simplify it now.
>
> Signed-off-by: John Hubbard <[email protected]>
> ---
>
> OK, here's a v2 that does EXPORT_SYMBOL_GPL, instead of EXPORT_SYMBOL,
> that's the only change from v1. That should help give this patch a
> clear bill of passage. :)
>
> thanks,
> John Hubbard
> NVIDIA
>
> fs/ceph/file.c | 3 +--
> include/linux/uio.h | 3 ++-
> lib/iov_iter.c | 6 +++---
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index d51c3f2fdca0..d3d7dd957390 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -879,8 +879,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
> more = len < iov_iter_count(to);
>
> if (unlikely(iov_iter_is_pipe(to))) {
> - ret = iov_iter_get_pages_alloc(to, &pages, len,
> - &page_off);
> + ret = pipe_get_pages_alloc(to, &pages, len, &page_off);
> if (ret <= 0) {
> ceph_osdc_put_request(req);
> ret = -ENOMEM;
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 3835a8a8e9ea..270a4dcf5453 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -226,7 +226,8 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
> ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages,
> size_t maxsize, size_t *start);
> int iov_iter_npages(const struct iov_iter *i, int maxpages);
> -
> +ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
> + size_t maxsize, size_t *start);
> const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags);
>
> static inline size_t iov_iter_count(const struct iov_iter *i)
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 5e40786c8f12..6290998df480 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1355,9 +1355,8 @@ static struct page **get_pages_array(size_t n)
> return kvmalloc_array(n, sizeof(struct page *), GFP_KERNEL);
> }
>
> -static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
> - struct page ***pages, size_t maxsize,
> - size_t *start)
> +ssize_t pipe_get_pages_alloc(struct iov_iter *i, struct page ***pages,
> + size_t maxsize, size_t *start)
> {
> struct page **p;
> unsigned int iter_head, npages;
> @@ -1387,6 +1386,7 @@ static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
> kvfree(p);
> return n;
> }
> +EXPORT_SYMBOL_GPL(pipe_get_pages_alloc);
>
> ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
> struct page ***pages, size_t maxsize,
Thanks. I've got a v1 of this in the ceph-client/testing branch and it
seems fine so far.
I'd prefer an ack from Al on one or the other though, since I'm not sure
he wants to expose this primitive, and in the past he hasn't been
enamored with EXPORT_SYMBOL_GPL, because its meaning wasn't well
defined. Maybe that's changed since.
As a side note, Al also asked privately why ceph special cases
ITER_PIPE. I wasn't sure either, so I did a bit of git-archaeology. The
change was added here:
---------------------------8<---------------------------
commit
7ce469a53e7106acdaca2e25027941d0f7c12a8e
Author: Yan, Zheng <[email protected]>
Date: Tue Nov 8 21:54:34 2016 +0800
ceph: fix splice read for no Fc capability case
When iov_iter type is ITER_PIPE, copy_page_to_iter() increases
the page's reference and add the page to a pipe_buffer. It also
set the pipe_buffer's ops to page_cache_pipe_buf_ops. The comfirm
callback in page_cache_pipe_buf_ops expects the page is from page
cache and uptodate, otherwise it return error.
For ceph_sync_read() case, pages are not from page cache. So we
can't call copy_page_to_iter() when iov_iter type is ITER_PIPE.
The fix is using iov_iter_get_pages_alloc() to allocate pages
for the pipe. (the code is similar to default_file_splice_read)
Signed-off-by: Yan, Zheng <[email protected]>
---------------------------8<---------------------------
If we don't have Fc (FILE_CACHE) caps then the client's not allowed to
cache data and so we can't use the pagecache. I'm not certain special
casing pipes in ceph, is the best approach to handle that, but the
confirm callback still seems to work the same way today.
Cheers,
--
Jeff Layton <[email protected]>