This is v2. Changes since v1 are:
* Incorporated feedback from Al Viro and Jan Kara: this approach now
pins both bvecs (ITER_BVEC) and user pages (user_backed_iter()) with
FOLL_PIN.
* Incorporated David Hildenbrand's feedback: Rewrote pin_user_pages()
documentation and added a WARN_ON_ONCE() to somewhat enforce the rule
that this new function is only intended for use on file-backed pages.
* Added a tiny new patch to fix up the release_pages() number of pages
argument, so as to avoid a lot of impedance-matching checks in
subsequent patches.
v1 is here:
https://lore.kernel.org/all/[email protected]/
Original cover letter still applies, here it is for convenience:
This converts the iomap core and bio_release_pages() to
pin_user_pages_fast(), also referred to as FOLL_PIN here.
The conversion is temporarily guarded by
CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO. In the future (not part of this
series), when we are certain that all filesystems have converted their
Direct IO paths to FOLL_PIN, then we can do the final step, which is to
get rid of CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO and search-and-replace
the dio_w_*() functions with their final names (see bvec.h changes).
I'd like to get this part committed at some point, because it seems to
work well already. And this will help get the remaining items, below,
converted.
Status: although many filesystems have been converted, some remain to be
investigated. These include (you can recreate this list by grepping for
iov_iter_get_pages):
cephfs
cifs
9P
RDS
net/core: datagram.c, skmsg.c
net/tls
fs/splice.c
Testing: this passes some light LTP and xfstest runs and fio and a few
other things like that, on my local x86_64 test machine, both with and
without CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO being set.
Conflicts: Logan, the iov_iter parts of this will conflict with your
[PATCH v9 2/8] iov_iter: introduce iov_iter_get_pages_[alloc_]flags(),
but I think it's easy to resolve.
John Hubbard (7):
mm: change release_pages() to use unsigned long for npages
mm/gup: introduce pin_user_page()
block: add dio_w_*() wrappers for pin, unpin user pages
iov_iter: new iov_iter_pin_pages*() routines
block, bio, fs: convert most filesystems to pin_user_pages_fast()
NFS: direct-io: convert to FOLL_PIN pages
fuse: convert direct IO paths to use FOLL_PIN
block/Kconfig | 24 +++++++++++++
block/bio.c | 27 +++++++-------
block/blk-map.c | 7 ++--
fs/direct-io.c | 40 ++++++++++-----------
fs/fuse/dev.c | 11 ++++--
fs/fuse/file.c | 32 +++++++++++------
fs/fuse/fuse_i.h | 1 +
fs/iomap/direct-io.c | 2 +-
fs/nfs/direct.c | 22 ++++++------
include/linux/bvec.h | 37 +++++++++++++++++++
include/linux/mm.h | 3 +-
include/linux/uio.h | 4 +++
lib/iov_iter.c | 86 ++++++++++++++++++++++++++++++++++++++++----
mm/gup.c | 50 ++++++++++++++++++++++++++
mm/swap.c | 6 ++--
15 files changed, 282 insertions(+), 70 deletions(-)
base-commit: dcf8e5633e2e69ad60b730ab5905608b756a032f
--
2.37.2
Convert the NFS Direct IO layer to use pin_user_pages_fast() and
unpin_user_page(), instead of get_user_pages_fast() and put_page().
The user of pin_user_pages_fast() depends upon:
1) CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO, and
2) User-space-backed pages: user_backed_iter(i) == true
Signed-off-by: John Hubbard <[email protected]>
---
fs/nfs/direct.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 1707f46b1335..71b794f39ee2 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -142,11 +142,13 @@ int nfs_swap_rw(struct kiocb *iocb, struct iov_iter *iter)
return 0;
}
-static void nfs_direct_release_pages(struct page **pages, unsigned int npages)
+static void nfs_direct_release_pages(struct iov_iter *iter, struct page **pages,
+ unsigned int npages)
{
- unsigned int i;
- for (i = 0; i < npages; i++)
- put_page(pages[i]);
+ if (user_backed_iter(iter) || iov_iter_is_bvec(iter))
+ dio_w_unpin_user_pages(pages, npages);
+ else
+ release_pages(pages, npages);
}
void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
@@ -332,11 +334,11 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
size_t pgbase;
unsigned npages, i;
- result = iov_iter_get_pages_alloc2(iter, &pagevec,
+ result = dio_w_iov_iter_pin_pages_alloc(iter, &pagevec,
rsize, &pgbase);
if (result < 0)
break;
-
+
bytes = result;
npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;
for (i = 0; i < npages; i++) {
@@ -362,7 +364,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
pos += req_len;
dreq->bytes_left -= req_len;
}
- nfs_direct_release_pages(pagevec, npages);
+ nfs_direct_release_pages(iter, pagevec, npages);
kvfree(pagevec);
if (result < 0)
break;
@@ -791,8 +793,8 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
size_t pgbase;
unsigned npages, i;
- result = iov_iter_get_pages_alloc2(iter, &pagevec,
- wsize, &pgbase);
+ result = dio_w_iov_iter_pin_pages_alloc(iter, &pagevec,
+ wsize, &pgbase);
if (result < 0)
break;
@@ -829,7 +831,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
pos += req_len;
dreq->bytes_left -= req_len;
}
- nfs_direct_release_pages(pagevec, npages);
+ nfs_direct_release_pages(iter, pagevec, npages);
kvfree(pagevec);
if (result < 0)
break;
--
2.37.2
Convert the fuse filesystem to use pin_user_pages_fast() and
unpin_user_page(), instead of get_user_pages_fast() and put_page().
The user of pin_user_pages_fast() depends upon:
1) CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO, and
2) User-space-backed pages or ITER_BVEC pages.
Signed-off-by: John Hubbard <[email protected]>
---
fs/fuse/dev.c | 11 +++++++++--
fs/fuse/file.c | 32 +++++++++++++++++++++-----------
fs/fuse/fuse_i.h | 1 +
3 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 51897427a534..5de98a7a45b1 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -675,7 +675,12 @@ static void fuse_copy_finish(struct fuse_copy_state *cs)
flush_dcache_page(cs->pg);
set_page_dirty_lock(cs->pg);
}
- put_page(cs->pg);
+ if (!cs->pipebufs &&
+ (user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter)))
+ dio_w_unpin_user_page(cs->pg);
+
+ else
+ put_page(cs->pg);
}
cs->pg = NULL;
}
@@ -730,7 +735,9 @@ static int fuse_copy_fill(struct fuse_copy_state *cs)
}
} else {
size_t off;
- err = iov_iter_get_pages2(cs->iter, &page, PAGE_SIZE, 1, &off);
+
+ err = dio_w_iov_iter_pin_pages(cs->iter, &page, PAGE_SIZE, 1,
+ &off);
if (err < 0)
return err;
BUG_ON(!err);
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 1a3afd469e3a..01da38928d0b 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -625,14 +625,19 @@ void fuse_read_args_fill(struct fuse_io_args *ia, struct file *file, loff_t pos,
}
static void fuse_release_user_pages(struct fuse_args_pages *ap,
- bool should_dirty)
+ bool should_dirty, bool is_user_or_bvec)
{
unsigned int i;
- for (i = 0; i < ap->num_pages; i++) {
- if (should_dirty)
- set_page_dirty_lock(ap->pages[i]);
- put_page(ap->pages[i]);
+ if (is_user_or_bvec) {
+ dio_w_unpin_user_pages_dirty_lock(ap->pages, ap->num_pages,
+ should_dirty);
+ } else {
+ for (i = 0; i < ap->num_pages; i++) {
+ if (should_dirty)
+ set_page_dirty_lock(ap->pages[i]);
+ put_page(ap->pages[i]);
+ }
}
}
@@ -733,7 +738,7 @@ static void fuse_aio_complete_req(struct fuse_mount *fm, struct fuse_args *args,
struct fuse_io_priv *io = ia->io;
ssize_t pos = -1;
- fuse_release_user_pages(&ia->ap, io->should_dirty);
+ fuse_release_user_pages(&ia->ap, io->should_dirty, io->is_user_or_bvec);
if (err) {
/* Nothing */
@@ -1414,10 +1419,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
while (nbytes < *nbytesp && ap->num_pages < max_pages) {
unsigned npages;
size_t start;
- ret = iov_iter_get_pages2(ii, &ap->pages[ap->num_pages],
- *nbytesp - nbytes,
- max_pages - ap->num_pages,
- &start);
+ ret = dio_w_iov_iter_pin_pages(ii, &ap->pages[ap->num_pages],
+ *nbytesp - nbytes,
+ max_pages - ap->num_pages,
+ &start);
if (ret < 0)
break;
@@ -1483,6 +1488,10 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
fl_owner_t owner = current->files;
size_t nbytes = min(count, nmax);
+ /* For use in fuse_release_user_pages(): */
+ io->is_user_or_bvec = user_backed_iter(iter) ||
+ iov_iter_is_bvec(iter);
+
err = fuse_get_user_pages(&ia->ap, iter, &nbytes, write,
max_pages);
if (err && !nbytes)
@@ -1498,7 +1507,8 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
}
if (!io->async || nres < 0) {
- fuse_release_user_pages(&ia->ap, io->should_dirty);
+ fuse_release_user_pages(&ia->ap, io->should_dirty,
+ io->is_user_or_bvec);
fuse_io_free(ia);
}
ia = NULL;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 488b460e046f..6ee7f72e29eb 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -290,6 +290,7 @@ struct fuse_io_priv {
struct kiocb *iocb;
struct completion *done;
bool blocking;
+ bool is_user_or_bvec;
};
#define FUSE_IO_PRIV_SYNC(i) \
--
2.37.2
Provide two new wrapper routines that are intended for user space pages
only:
iov_iter_pin_pages()
iov_iter_pin_pages_alloc()
Internally, these routines call pin_user_pages_fast(), instead of
get_user_pages_fast(), for user_backed_iter(i) and iov_iter_bvec(i)
cases.
As always, callers must use unpin_user_pages() or a suitable FOLL_PIN
variant, to release the pages, if they actually were acquired via
pin_user_pages_fast().
This is a prerequisite to converting bio/block layers over to use
pin_user_pages_fast().
Signed-off-by: John Hubbard <[email protected]>
---
include/linux/uio.h | 4 +++
lib/iov_iter.c | 86 +++++++++++++++++++++++++++++++++++++++++----
2 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 5896af36199c..e26908e443d1 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -251,6 +251,10 @@ ssize_t iov_iter_get_pages2(struct iov_iter *i, struct page **pages,
size_t maxsize, unsigned maxpages, size_t *start);
ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i, struct page ***pages,
size_t maxsize, size_t *start);
+ssize_t iov_iter_pin_pages(struct iov_iter *i, struct page **pages,
+ size_t maxsize, unsigned int maxpages, size_t *start);
+ssize_t iov_iter_pin_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);
void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state);
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 4b7fce72e3e5..c63ce0eadfcb 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1425,9 +1425,31 @@ static struct page *first_bvec_segment(const struct iov_iter *i,
return page;
}
+enum pages_alloc_internal_flags {
+ USE_FOLL_GET,
+ MAYBE_USE_FOLL_PIN
+};
+
+/*
+ * Pins pages, either via get_page(), or via pin_user_page*(). The caller is
+ * responsible for tracking which pinning mechanism was used here, and releasing
+ * pages via the appropriate call: put_page() or unpin_user_page().
+ *
+ * The way to figure that out is:
+ *
+ * a) If how_to_pin == FOLL_GET, then this routine will always pin via
+ * get_page().
+ *
+ * b) If how_to_pin == MAYBE_USE_FOLL_PIN, then this routine will pin via
+ * pin_user_page*() for either user_backed_iter(i) cases, or
+ * iov_iter_is_bvec(i) cases. However, for the other cases (pipe,
+ * xarray), pages will be pinned via get_page().
+ */
static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
struct page ***pages, size_t maxsize,
- unsigned int maxpages, size_t *start)
+ unsigned int maxpages, size_t *start,
+ enum pages_alloc_internal_flags how_to_pin)
+
{
unsigned int n;
@@ -1454,7 +1476,12 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
n = want_pages_array(pages, maxsize, *start, maxpages);
if (!n)
return -ENOMEM;
- res = get_user_pages_fast(addr, n, gup_flags, *pages);
+
+ if (how_to_pin == MAYBE_USE_FOLL_PIN)
+ res = pin_user_pages_fast(addr, n, gup_flags, *pages);
+ else
+ res = get_user_pages_fast(addr, n, gup_flags, *pages);
+
if (unlikely(res <= 0))
return res;
maxsize = min_t(size_t, maxsize, res * PAGE_SIZE - *start);
@@ -1470,8 +1497,13 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
if (!n)
return -ENOMEM;
p = *pages;
- for (int k = 0; k < n; k++)
- get_page(p[k] = page + k);
+ for (int k = 0; k < n; k++) {
+ p[k] = page + k;
+ if (how_to_pin == MAYBE_USE_FOLL_PIN)
+ pin_user_page(p[k]);
+ else
+ get_page(p[k]);
+ }
maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start);
i->count -= maxsize;
i->iov_offset += maxsize;
@@ -1497,10 +1529,29 @@ ssize_t iov_iter_get_pages2(struct iov_iter *i,
return 0;
BUG_ON(!pages);
- return __iov_iter_get_pages_alloc(i, &pages, maxsize, maxpages, start);
+ return __iov_iter_get_pages_alloc(i, &pages, maxsize, maxpages, start,
+ USE_FOLL_GET);
}
EXPORT_SYMBOL(iov_iter_get_pages2);
+/*
+ * A FOLL_PIN variant that calls pin_user_pages_fast() instead of
+ * get_user_pages_fast().
+ */
+ssize_t iov_iter_pin_pages(struct iov_iter *i,
+ struct page **pages, size_t maxsize, unsigned int maxpages,
+ size_t *start)
+{
+ if (!maxpages)
+ return 0;
+ if (WARN_ON_ONCE(!pages))
+ return -EINVAL;
+
+ return __iov_iter_get_pages_alloc(i, &pages, maxsize, maxpages, start,
+ MAYBE_USE_FOLL_PIN);
+}
+EXPORT_SYMBOL(iov_iter_pin_pages);
+
ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i,
struct page ***pages, size_t maxsize,
size_t *start)
@@ -1509,7 +1560,8 @@ ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i,
*pages = NULL;
- len = __iov_iter_get_pages_alloc(i, pages, maxsize, ~0U, start);
+ len = __iov_iter_get_pages_alloc(i, pages, maxsize, ~0U, start,
+ USE_FOLL_GET);
if (len <= 0) {
kvfree(*pages);
*pages = NULL;
@@ -1518,6 +1570,28 @@ ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i,
}
EXPORT_SYMBOL(iov_iter_get_pages_alloc2);
+/*
+ * A FOLL_PIN variant that calls pin_user_pages_fast() instead of
+ * get_user_pages_fast().
+ */
+ssize_t iov_iter_pin_pages_alloc(struct iov_iter *i,
+ struct page ***pages, size_t maxsize,
+ size_t *start)
+{
+ ssize_t len;
+
+ *pages = NULL;
+
+ len = __iov_iter_get_pages_alloc(i, pages, maxsize, ~0U, start,
+ MAYBE_USE_FOLL_PIN);
+ if (len <= 0) {
+ kvfree(*pages);
+ *pages = NULL;
+ }
+ return len;
+}
+EXPORT_SYMBOL(iov_iter_pin_pages_alloc);
+
size_t csum_and_copy_from_iter(void *addr, size_t bytes, __wsum *csum,
struct iov_iter *i)
{
--
2.37.2
On Wed, 31 Aug 2022 at 06:19, John Hubbard <[email protected]> wrote:
>
> Convert the fuse filesystem to use pin_user_pages_fast() and
> unpin_user_page(), instead of get_user_pages_fast() and put_page().
>
> The user of pin_user_pages_fast() depends upon:
>
> 1) CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO, and
>
> 2) User-space-backed pages or ITER_BVEC pages.
>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> fs/fuse/dev.c | 11 +++++++++--
> fs/fuse/file.c | 32 +++++++++++++++++++++-----------
> fs/fuse/fuse_i.h | 1 +
> 3 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 51897427a534..5de98a7a45b1 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -675,7 +675,12 @@ static void fuse_copy_finish(struct fuse_copy_state *cs)
> flush_dcache_page(cs->pg);
> set_page_dirty_lock(cs->pg);
> }
> - put_page(cs->pg);
> + if (!cs->pipebufs &&
> + (user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter)))
> + dio_w_unpin_user_page(cs->pg);
> +
> + else
> + put_page(cs->pg);
Why not move the logic into a helper and pass a "bool pinned" argument?
> }
> cs->pg = NULL;
> }
> @@ -730,7 +735,9 @@ static int fuse_copy_fill(struct fuse_copy_state *cs)
> }
> } else {
> size_t off;
> - err = iov_iter_get_pages2(cs->iter, &page, PAGE_SIZE, 1, &off);
> +
> + err = dio_w_iov_iter_pin_pages(cs->iter, &page, PAGE_SIZE, 1,
> + &off);
> if (err < 0)
> return err;
> BUG_ON(!err);
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 1a3afd469e3a..01da38928d0b 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -625,14 +625,19 @@ void fuse_read_args_fill(struct fuse_io_args *ia, struct file *file, loff_t pos,
> }
>
> static void fuse_release_user_pages(struct fuse_args_pages *ap,
> - bool should_dirty)
> + bool should_dirty, bool is_user_or_bvec)
> {
> unsigned int i;
>
> - for (i = 0; i < ap->num_pages; i++) {
> - if (should_dirty)
> - set_page_dirty_lock(ap->pages[i]);
> - put_page(ap->pages[i]);
> + if (is_user_or_bvec) {
> + dio_w_unpin_user_pages_dirty_lock(ap->pages, ap->num_pages,
> + should_dirty);
> + } else {
> + for (i = 0; i < ap->num_pages; i++) {
> + if (should_dirty)
> + set_page_dirty_lock(ap->pages[i]);
> + put_page(ap->pages[i]);
> + }
Same here.
> }
> }
>
> @@ -733,7 +738,7 @@ static void fuse_aio_complete_req(struct fuse_mount *fm, struct fuse_args *args,
> struct fuse_io_priv *io = ia->io;
> ssize_t pos = -1;
>
> - fuse_release_user_pages(&ia->ap, io->should_dirty);
> + fuse_release_user_pages(&ia->ap, io->should_dirty, io->is_user_or_bvec);
>
> if (err) {
> /* Nothing */
> @@ -1414,10 +1419,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
> while (nbytes < *nbytesp && ap->num_pages < max_pages) {
> unsigned npages;
> size_t start;
> - ret = iov_iter_get_pages2(ii, &ap->pages[ap->num_pages],
> - *nbytesp - nbytes,
> - max_pages - ap->num_pages,
> - &start);
> + ret = dio_w_iov_iter_pin_pages(ii, &ap->pages[ap->num_pages],
> + *nbytesp - nbytes,
> + max_pages - ap->num_pages,
> + &start);
> if (ret < 0)
> break;
>
> @@ -1483,6 +1488,10 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
> fl_owner_t owner = current->files;
> size_t nbytes = min(count, nmax);
>
> + /* For use in fuse_release_user_pages(): */
> + io->is_user_or_bvec = user_backed_iter(iter) ||
> + iov_iter_is_bvec(iter);
> +
How about io->is_pinned? And a iov_iter_is_pinned() helper?
Thanks,
Miklos
On Tue, Aug 30, 2022 at 09:18:40PM -0700, John Hubbard wrote:
> Provide two new wrapper routines that are intended for user space pages
> only:
>
> iov_iter_pin_pages()
> iov_iter_pin_pages_alloc()
>
> Internally, these routines call pin_user_pages_fast(), instead of
> get_user_pages_fast(), for user_backed_iter(i) and iov_iter_bvec(i)
> cases.
>
> As always, callers must use unpin_user_pages() or a suitable FOLL_PIN
> variant, to release the pages, if they actually were acquired via
> pin_user_pages_fast().
>
> This is a prerequisite to converting bio/block layers over to use
> pin_user_pages_fast().
What of ITER_PIPE (splice from O_DIRECT fd to a to pipe, for filesystem
that uses generic_file_splice_read())?
On 8/31/22 03:37, Miklos Szeredi wrote:
Hi Miklos,
Thanks for looking at this, I'll accept all of these suggestions.
> On Wed, 31 Aug 2022 at 06:19, John Hubbard <[email protected]> wrote:
>>
>> Convert the fuse filesystem to use pin_user_pages_fast() and
>> unpin_user_page(), instead of get_user_pages_fast() and put_page().
>>
>> The user of pin_user_pages_fast() depends upon:
>>
>> 1) CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO, and
>>
>> 2) User-space-backed pages or ITER_BVEC pages.
>>
>> Signed-off-by: John Hubbard <[email protected]>
>> ---
>> fs/fuse/dev.c | 11 +++++++++--
>> fs/fuse/file.c | 32 +++++++++++++++++++++-----------
>> fs/fuse/fuse_i.h | 1 +
>> 3 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index 51897427a534..5de98a7a45b1 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -675,7 +675,12 @@ static void fuse_copy_finish(struct fuse_copy_state *cs)
>> flush_dcache_page(cs->pg);
>> set_page_dirty_lock(cs->pg);
>> }
>> - put_page(cs->pg);
>> + if (!cs->pipebufs &&
>> + (user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter)))
>> + dio_w_unpin_user_page(cs->pg);
>> +
>> + else
>> + put_page(cs->pg);
>
> Why not move the logic into a helper and pass a "bool pinned" argument?
OK, will do.
It's not yet clear from the discussion in the other thread with Jan and Al [1],
if I'll end up keeping this check:
user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter)
...but if it stays, then the helper is a good idea.
>
>> }
>> cs->pg = NULL;
>> }
>> @@ -730,7 +735,9 @@ static int fuse_copy_fill(struct fuse_copy_state *cs)
>> }
>> } else {
>> size_t off;
>> - err = iov_iter_get_pages2(cs->iter, &page, PAGE_SIZE, 1, &off);
>> +
>> + err = dio_w_iov_iter_pin_pages(cs->iter, &page, PAGE_SIZE, 1,
>> + &off);
>> if (err < 0)
>> return err;
>> BUG_ON(!err);
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 1a3afd469e3a..01da38928d0b 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -625,14 +625,19 @@ void fuse_read_args_fill(struct fuse_io_args *ia, struct file *file, loff_t pos,
>> }
>>
>> static void fuse_release_user_pages(struct fuse_args_pages *ap,
>> - bool should_dirty)
>> + bool should_dirty, bool is_user_or_bvec)
>> {
>> unsigned int i;
>>
>> - for (i = 0; i < ap->num_pages; i++) {
>> - if (should_dirty)
>> - set_page_dirty_lock(ap->pages[i]);
>> - put_page(ap->pages[i]);
>> + if (is_user_or_bvec) {
>> + dio_w_unpin_user_pages_dirty_lock(ap->pages, ap->num_pages,
>> + should_dirty);
>> + } else {
>> + for (i = 0; i < ap->num_pages; i++) {
>> + if (should_dirty)
>> + set_page_dirty_lock(ap->pages[i]);
>> + put_page(ap->pages[i]);
>> + }
>
> Same here.
Yes. Definitely belongs in a helper function. I was thinking, "don't
go that far, because the code will eventually get deleted anyway", but
you are right. :)
>
>> }
>> }
>>
>> @@ -733,7 +738,7 @@ static void fuse_aio_complete_req(struct fuse_mount *fm, struct fuse_args *args,
>> struct fuse_io_priv *io = ia->io;
>> ssize_t pos = -1;
>>
>> - fuse_release_user_pages(&ia->ap, io->should_dirty);
>> + fuse_release_user_pages(&ia->ap, io->should_dirty, io->is_user_or_bvec);
>>
>> if (err) {
>> /* Nothing */
>> @@ -1414,10 +1419,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
>> while (nbytes < *nbytesp && ap->num_pages < max_pages) {
>> unsigned npages;
>> size_t start;
>> - ret = iov_iter_get_pages2(ii, &ap->pages[ap->num_pages],
>> - *nbytesp - nbytes,
>> - max_pages - ap->num_pages,
>> - &start);
>> + ret = dio_w_iov_iter_pin_pages(ii, &ap->pages[ap->num_pages],
>> + *nbytesp - nbytes,
>> + max_pages - ap->num_pages,
>> + &start);
>> if (ret < 0)
>> break;
>>
>> @@ -1483,6 +1488,10 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
>> fl_owner_t owner = current->files;
>> size_t nbytes = min(count, nmax);
>>
>> + /* For use in fuse_release_user_pages(): */
>> + io->is_user_or_bvec = user_backed_iter(iter) ||
>> + iov_iter_is_bvec(iter);
>> +
>
> How about io->is_pinned? And a iov_iter_is_pinned() helper?
Agreed, is_pinned is a better name, and the helper (if we end up needing
that logic) also sounds good.
[1] https://lore.kernel.org/r/20220831094349.boln4jjajkdtykx3@quack3
thanks,
--
John Hubbard
NVIDIA
On 8/31/22 17:42, Al Viro wrote:
> On Tue, Aug 30, 2022 at 09:18:40PM -0700, John Hubbard wrote:
>> Provide two new wrapper routines that are intended for user space pages
>> only:
>>
>> iov_iter_pin_pages()
>> iov_iter_pin_pages_alloc()
>>
>> Internally, these routines call pin_user_pages_fast(), instead of
>> get_user_pages_fast(), for user_backed_iter(i) and iov_iter_bvec(i)
>> cases.
>>
>> As always, callers must use unpin_user_pages() or a suitable FOLL_PIN
>> variant, to release the pages, if they actually were acquired via
>> pin_user_pages_fast().
>>
>> This is a prerequisite to converting bio/block layers over to use
>> pin_user_pages_fast().
>
> What of ITER_PIPE (splice from O_DIRECT fd to a to pipe, for filesystem
> that uses generic_file_splice_read())?
Yes. And it turns out that I sent this v2 just a little too early: it
does not include Jan Kara's latest idea [1] of including ITER_PIPE and
ITER_XARRAY. That should fix this up.
[1] https://lore.kernel.org/r/20220831094349.boln4jjajkdtykx3@quack3
thanks,
--
John Hubbard
NVIDIA
On Tue, Aug 30, 2022 at 09:18:36PM -0700, John Hubbard wrote:
> The conversion is temporarily guarded by
> CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO. In the future (not part of this
> series), when we are certain that all filesystems have converted their
> Direct IO paths to FOLL_PIN, then we can do the final step, which is to
> get rid of CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO and search-and-replace
> the dio_w_*() functions with their final names (see bvec.h changes).
What is the the point of these wrappers? We should be able to
convert one caller at a time in an entirely safe way.
I'd it one step back. For BVECS we never need a get or pin. The
block layer already does this, an the other callers should as well.
For KVEC the same is true. For PIPE and xarray as you pointed out
we can probably just do the pin, it is not like these are performance
paths.
So, I'd suggest to:
- factor out the user backed and bvec cases from
__iov_iter_get_pages_alloc into helper just to keep
__iov_iter_get_pages_alloc readable.
- for the pin case don't use the existing bvec helper at all, but
copy the logic for the block layer for not pinning.
On Tue, Aug 30, 2022 at 09:18:42PM -0700, John Hubbard wrote:
static void nfs_direct_release_pages(struct iov_iter *iter, struct page **pages,
unsigned int npages)
> {
> - unsigned int i;
> - for (i = 0; i < npages; i++)
> - put_page(pages[i]);
> + if (user_backed_iter(iter) || iov_iter_is_bvec(iter))
> + dio_w_unpin_user_pages(pages, npages);
> + else
> + release_pages(pages, npages);
Instead of having this magic in all kinds of places, we need a helper
that takes the page array, npages and iter->type and does the right
thing.
On 9/5/22 23:36, Christoph Hellwig wrote:
> On Tue, Aug 30, 2022 at 09:18:36PM -0700, John Hubbard wrote:
>> The conversion is temporarily guarded by
>> CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO. In the future (not part of this
>> series), when we are certain that all filesystems have converted their
>> Direct IO paths to FOLL_PIN, then we can do the final step, which is to
>> get rid of CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO and search-and-replace
>> the dio_w_*() functions with their final names (see bvec.h changes).
>
> What is the the point of these wrappers? We should be able to
> convert one caller at a time in an entirely safe way.
I would be delighted if that were somehow possible. Every time I think
it's possible, it has fallen apart. The fact that bio_release_pages()
will need to switch over from put_page() to unpin_user_page(), combined
with the fact that there are a lot of callers that submit bios, has
led me to the current approach.
What did you have in mind?
thanks,
--
John Hubbard
NVIDIA
On 9/5/22 23:49, Christoph Hellwig wrote:
> On Tue, Aug 30, 2022 at 09:18:42PM -0700, John Hubbard wrote:
> static void nfs_direct_release_pages(struct iov_iter *iter, struct page **pages,
> unsigned int npages)
>> {
>> - unsigned int i;
>> - for (i = 0; i < npages; i++)
>> - put_page(pages[i]);
>> + if (user_backed_iter(iter) || iov_iter_is_bvec(iter))
>> + dio_w_unpin_user_pages(pages, npages);
>> + else
>> + release_pages(pages, npages);
>
> Instead of having this magic in all kinds of places, we need a helper
> that takes the page array, npages and iter->type and does the right
> thing.
Yes, Miklos asked for much the same thing, too. :)
thanks,
--
John Hubbard
NVIDIA
On Tue, Sep 06, 2022 at 12:10:54AM -0700, John Hubbard wrote:
> I would be delighted if that were somehow possible. Every time I think
> it's possible, it has fallen apart. The fact that bio_release_pages()
> will need to switch over from put_page() to unpin_user_page(), combined
> with the fact that there are a lot of callers that submit bios, has
> led me to the current approach.
We can (temporarily) pass the gup flag to bio_release_pages or even
better add a new bio_unpin_pages helper that undoes the pin side.
That is: don't try to reuse the old APIs, but ad new ones, just like
we do on the lower layers.
On 9/6/22 00:22, Christoph Hellwig wrote:
> On Tue, Sep 06, 2022 at 12:10:54AM -0700, John Hubbard wrote:
>> I would be delighted if that were somehow possible. Every time I think
>> it's possible, it has fallen apart. The fact that bio_release_pages()
>> will need to switch over from put_page() to unpin_user_page(), combined
>> with the fact that there are a lot of callers that submit bios, has
>> led me to the current approach.
>
> We can (temporarily) pass the gup flag to bio_release_pages or even
> better add a new bio_unpin_pages helper that undoes the pin side.
> That is: don't try to reuse the old APIs, but ad new ones, just like
> we do on the lower layers.
OK...so, to confirm: the idea is to convert these callsites (below) to
call a new bio_unpin_pages() routine that does unpin_user_page().
$ git grep -nw bio_release_pages
block/bio.c:1474: bio_release_pages(bio, true);
block/bio.c:1490: bio_release_pages(bio, false);
block/blk-map.c:308: bio_release_pages(bio, false);
block/blk-map.c:610: bio_release_pages(bio, bio_data_dir(bio) == READ);
block/fops.c:99: bio_release_pages(&bio, should_dirty);
block/fops.c:165: bio_release_pages(bio, false);
block/fops.c:289: bio_release_pages(bio, false);
fs/direct-io.c:510: bio_release_pages(bio, should_dirty);
fs/iomap/direct-io.c:185: bio_release_pages(bio, false);
fs/zonefs/super.c:793: bio_release_pages(bio, false);
And these can probably be done in groups, not as one big patch--your
other email also asked to break them up into block, iomap, and legacy.
That would be nice, I really hate the ugly dio_w*() wrappers, thanks
for this help. :)
thanks,
--
John Hubbard
NVIDIA
On 9/5/22 23:47, Christoph Hellwig wrote:
> I'd it one step back. For BVECS we never need a get or pin. The
> block layer already does this, an the other callers should as well.
> For KVEC the same is true. For PIPE and xarray as you pointed out
> we can probably just do the pin, it is not like these are performance
> paths.
>
> So, I'd suggest to:
>
> - factor out the user backed and bvec cases from
> __iov_iter_get_pages_alloc into helper just to keep
> __iov_iter_get_pages_alloc readable.
OK, that part is clear.
> - for the pin case don't use the existing bvec helper at all, but
> copy the logic for the block layer for not pinning.
I'm almost, but not quite sure I get the idea above. Overall, what
happens to bvec pages? Leave the get_page() pin in place for FOLL_GET
(or USE_FOLL_GET), I suppose, but do...what, for FOLL_PIN callers?
thanks,
--
John Hubbard
NVIDIA
On Tue, Sep 06, 2022 at 12:37:00AM -0700, John Hubbard wrote:
> On 9/6/22 00:22, Christoph Hellwig wrote:
> > On Tue, Sep 06, 2022 at 12:10:54AM -0700, John Hubbard wrote:
> >> I would be delighted if that were somehow possible. Every time I think
> >> it's possible, it has fallen apart. The fact that bio_release_pages()
> >> will need to switch over from put_page() to unpin_user_page(), combined
> >> with the fact that there are a lot of callers that submit bios, has
> >> led me to the current approach.
> >
> > We can (temporarily) pass the gup flag to bio_release_pages or even
> > better add a new bio_unpin_pages helper that undoes the pin side.
> > That is: don't try to reuse the old APIs, but ad new ones, just like
> > we do on the lower layers.
>
> OK...so, to confirm: the idea is to convert these callsites (below) to
> call a new bio_unpin_pages() routine that does unpin_user_page().
Yeah. And to stay symmetric also a new bio_iov_iter_pin_pages for
the pin side.
On Tue, Sep 06, 2022 at 12:44:28AM -0700, John Hubbard wrote:
> OK, that part is clear.
>
> > - for the pin case don't use the existing bvec helper at all, but
> > copy the logic for the block layer for not pinning.
>
> I'm almost, but not quite sure I get the idea above. Overall, what
> happens to bvec pages? Leave the get_page() pin in place for FOLL_GET
> (or USE_FOLL_GET), I suppose, but do...what, for FOLL_PIN callers?
Do not change anyhing for FOLL_GET callers, as they are on the way out
anyway.
For FOLL_PIN callers, never pin bvec and kvec pages: For file systems
not acquiring a reference is obviously safe, and the other callers will
need an audit, but I can't think of why it woul ever be unsafe.
On 9/6/22 00:48, Christoph Hellwig wrote:
> On Tue, Sep 06, 2022 at 12:44:28AM -0700, John Hubbard wrote:
>> OK, that part is clear.
>>
>>> - for the pin case don't use the existing bvec helper at all, but
>>> copy the logic for the block layer for not pinning.
>>
>> I'm almost, but not quite sure I get the idea above. Overall, what
>> happens to bvec pages? Leave the get_page() pin in place for FOLL_GET
>> (or USE_FOLL_GET), I suppose, but do...what, for FOLL_PIN callers?
>
> Do not change anyhing for FOLL_GET callers, as they are on the way out
> anyway.
>
OK, got it.
> For FOLL_PIN callers, never pin bvec and kvec pages: For file systems
> not acquiring a reference is obviously safe, and the other callers will
> need an audit, but I can't think of why it woul ever be unsafe.
In order to do that, one would need to be confident that such bvec and kvec
pages do not get passed down to bio_release_pages() (or the new
bio_unpin_pages()). Also, I'm missing a key point, because today bvec pages get
a get_page() reference from __iov_iter_get_pages_alloc(). If I just skip
that, then the get/put calls are unbalanced...
I can just hear Al Viro repeating his points about splice() and vmsplice(),
heh. :)
thanks,
--
John Hubbard
NVIDIA
On Tue 06-09-22 00:48:49, Christoph Hellwig wrote:
> On Tue, Sep 06, 2022 at 12:44:28AM -0700, John Hubbard wrote:
> > OK, that part is clear.
> >
> > > - for the pin case don't use the existing bvec helper at all, but
> > > copy the logic for the block layer for not pinning.
> >
> > I'm almost, but not quite sure I get the idea above. Overall, what
> > happens to bvec pages? Leave the get_page() pin in place for FOLL_GET
> > (or USE_FOLL_GET), I suppose, but do...what, for FOLL_PIN callers?
>
> Do not change anyhing for FOLL_GET callers, as they are on the way out
> anyway.
>
> For FOLL_PIN callers, never pin bvec and kvec pages: For file systems
> not acquiring a reference is obviously safe, and the other callers will
> need an audit, but I can't think of why it woul ever be unsafe.
Are you sure about "For file systems not acquiring a reference is obviously
safe"? I can see places e.g. in orangefs, afs, etc. which create bvec iters
from pagecache pages. And then we have iter_file_splice_write() which
creates bvec from pipe pages (which can also be pagecache pages if
vmsplice() is used). So perhaps there are no lifetime issues even without
acquiring a reference (but looking at the code I would not say it is
obvious) but I definitely don't see how it would be safe to not get a pin
to signal to filesystem backing the pagecache page that there is DMA
happening to/from the page.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Sep 06, 2022 at 12:21:06PM +0200, Jan Kara wrote:
> > For FOLL_PIN callers, never pin bvec and kvec pages: For file systems
> > not acquiring a reference is obviously safe, and the other callers will
> > need an audit, but I can't think of why it woul ever be unsafe.
>
> Are you sure about "For file systems not acquiring a reference is obviously
> safe"? I can see places e.g. in orangefs, afs, etc. which create bvec iters
> from pagecache pages. And then we have iter_file_splice_write() which
> creates bvec from pipe pages (which can also be pagecache pages if
> vmsplice() is used). So perhaps there are no lifetime issues even without
> acquiring a reference (but looking at the code I would not say it is
> obvious) but I definitely don't see how it would be safe to not get a pin
> to signal to filesystem backing the pagecache page that there is DMA
> happening to/from the page.
I mean in the context of iov_iter_get_pages callers, that is direct
I/O. Direct callers of iov_iter_bvec which then pass that iov to
->read_iter / ->write_iter will need to hold references (those are
the references that the callers of iov_iter_get_pages rely on!).
On Tue, Sep 06, 2022 at 12:58:59AM -0700, John Hubbard wrote:
> > For FOLL_PIN callers, never pin bvec and kvec pages: For file systems
> > not acquiring a reference is obviously safe, and the other callers will
> > need an audit, but I can't think of why it woul ever be unsafe.
>
> In order to do that, one would need to be confident that such bvec and kvec
> pages do not get passed down to bio_release_pages() (or the new
> bio_unpin_pages()). Also, I'm missing a key point, because today bvec pages get
> a get_page() reference from __iov_iter_get_pages_alloc(). If I just skip
> that, then the get/put calls are unbalanced...
Except that for the most relevant callers (bdev and iomap) we never
end up in __iov_iter_get_pages_alloc. What I suggest is that, with
the move to the pin helper, we codify that logic.
For callers of the bio_iov_iter_pin_pages helper, the corresponding
bio_unpin_pages helper will encapsulate that logic. For direct calls
to iov_iter_pin_pages, we should add a iov_iter_unpin_pages helper that
also encapsulates the logic. The beauty is that this means the caller
itself does not have to care about any of thise, and the logic is in
one (well two due to the block special case that reuses the bio_vec
array for the pages space) place.
> I can just hear Al Viro repeating his points about splice() and vmsplice(),
> heh. :)
splice does take these references before calling into the file system
(see my reply to Jan).
On Wed, Sep 07, 2022 at 01:45:26AM -0700, Christoph Hellwig wrote:
> On Tue, Sep 06, 2022 at 12:21:06PM +0200, Jan Kara wrote:
> > > For FOLL_PIN callers, never pin bvec and kvec pages: For file systems
> > > not acquiring a reference is obviously safe, and the other callers will
> > > need an audit, but I can't think of why it woul ever be unsafe.
> >
> > Are you sure about "For file systems not acquiring a reference is obviously
> > safe"? I can see places e.g. in orangefs, afs, etc. which create bvec iters
> > from pagecache pages. And then we have iter_file_splice_write() which
> > creates bvec from pipe pages (which can also be pagecache pages if
> > vmsplice() is used). So perhaps there are no lifetime issues even without
> > acquiring a reference (but looking at the code I would not say it is
> > obvious) but I definitely don't see how it would be safe to not get a pin
> > to signal to filesystem backing the pagecache page that there is DMA
> > happening to/from the page.
>
> I mean in the context of iov_iter_get_pages callers, that is direct
> I/O. Direct callers of iov_iter_bvec which then pass that iov to
> ->read_iter / ->write_iter will need to hold references (those are
> the references that the callers of iov_iter_get_pages rely on!).
Unless I'm misreading Jan, the question is whether they should get or
pin. AFAICS, anyone who passes the sucker to ->read_iter() (or ->recvmsg(),
or does direct copy_to_iter()/zero_iter(), etc.) is falling under
=================================================================================
CASE 5: Pinning in order to write to the data within the page
-------------------------------------------------------------
Even though neither DMA nor Direct IO is involved, just a simple case of "pin,
write to a page's data, unpin" can cause a problem. Case 5 may be considered a
superset of Case 1, plus Case 2, plus anything that invokes that pattern. In
other words, if the code is neither Case 1 nor Case 2, it may still require
FOLL_PIN, for patterns like this:
Correct (uses FOLL_PIN calls):
pin_user_pages()
write to the data within the pages
unpin_user_pages()
INCORRECT (uses FOLL_GET calls):
get_user_pages()
write to the data within the pages
put_page()
=================================================================================
Regarding iter_file_splice_write() case, do we need to pin pages
when we are not going to modify the data in those?
The same goes for afs, AFAICS; I started to type "... and everything that passes
WRITE to iov_iter_bvec()", but...
drivers/vhost/vringh.c:1165: iov_iter_bvec(&iter, READ, iov, ret, translated);
drivers/vhost/vringh.c:1198: iov_iter_bvec(&iter, WRITE, iov, ret, translated);
is backwards - READ is for data destinations, comes with copy_to_iter(); WRITE is
for data sources and it comes with copy_from_iter()...
I'm really tempted to slap
if (WARN_ON(i->data_source))
return 0;
into copy_to_iter() et.al., along with its opposite for copy_from_iter().
And see who comes screaming... Things like
if (unlikely(iov_iter_is_pipe(i) || iov_iter_is_discard(i))) {
WARN_ON(1);
return 0;
}
in e.g. csum_and_copy_from_iter() would be replaced by that, and become
easier to understand...
These two are also getting it wrong, BTW:
drivers/target/target_core_file.c:340: iov_iter_bvec(&iter, READ, bvec, sgl_nents, len);
drivers/target/target_core_file.c:476: iov_iter_bvec(&iter, READ, bvec, nolb, len);
On Wed 14-09-22 04:51:17, Al Viro wrote:
> On Wed, Sep 07, 2022 at 01:45:26AM -0700, Christoph Hellwig wrote:
> > On Tue, Sep 06, 2022 at 12:21:06PM +0200, Jan Kara wrote:
> > > > For FOLL_PIN callers, never pin bvec and kvec pages: For file systems
> > > > not acquiring a reference is obviously safe, and the other callers will
> > > > need an audit, but I can't think of why it woul ever be unsafe.
> > >
> > > Are you sure about "For file systems not acquiring a reference is obviously
> > > safe"? I can see places e.g. in orangefs, afs, etc. which create bvec iters
> > > from pagecache pages. And then we have iter_file_splice_write() which
> > > creates bvec from pipe pages (which can also be pagecache pages if
> > > vmsplice() is used). So perhaps there are no lifetime issues even without
> > > acquiring a reference (but looking at the code I would not say it is
> > > obvious) but I definitely don't see how it would be safe to not get a pin
> > > to signal to filesystem backing the pagecache page that there is DMA
> > > happening to/from the page.
> >
> > I mean in the context of iov_iter_get_pages callers, that is direct
> > I/O. Direct callers of iov_iter_bvec which then pass that iov to
> > ->read_iter / ->write_iter will need to hold references (those are
> > the references that the callers of iov_iter_get_pages rely on!).
>
> Unless I'm misreading Jan, the question is whether they should get or
> pin. AFAICS, anyone who passes the sucker to ->read_iter() (or ->recvmsg(),
> or does direct copy_to_iter()/zero_iter(), etc.) is falling under
> =================================================================================
> CASE 5: Pinning in order to write to the data within the page
> -------------------------------------------------------------
> Even though neither DMA nor Direct IO is involved, just a simple case of "pin,
> write to a page's data, unpin" can cause a problem. Case 5 may be considered a
> superset of Case 1, plus Case 2, plus anything that invokes that pattern. In
> other words, if the code is neither Case 1 nor Case 2, it may still require
> FOLL_PIN, for patterns like this:
>
> Correct (uses FOLL_PIN calls):
> pin_user_pages()
> write to the data within the pages
> unpin_user_pages()
>
> INCORRECT (uses FOLL_GET calls):
> get_user_pages()
> write to the data within the pages
> put_page()
> =================================================================================
Yes, that was my point.
> Regarding iter_file_splice_write() case, do we need to pin pages
> when we are not going to modify the data in those?
Strictly speaking not. So far we are pinning pages even if they serve as
data source because it is simpler not to bother about data access direction
but I'm not really aware of anything that would mandate that.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Sep 14, 2022 at 04:52:33PM +0200, Jan Kara wrote:
> > =================================================================================
> > CASE 5: Pinning in order to write to the data within the page
> > -------------------------------------------------------------
> > Even though neither DMA nor Direct IO is involved, just a simple case of "pin,
> > write to a page's data, unpin" can cause a problem. Case 5 may be considered a
> > superset of Case 1, plus Case 2, plus anything that invokes that pattern. In
> > other words, if the code is neither Case 1 nor Case 2, it may still require
> > FOLL_PIN, for patterns like this:
> >
> > Correct (uses FOLL_PIN calls):
> > pin_user_pages()
> > write to the data within the pages
> > unpin_user_pages()
> >
> > INCORRECT (uses FOLL_GET calls):
> > get_user_pages()
> > write to the data within the pages
> > put_page()
> > =================================================================================
>
> Yes, that was my point.
The thing is, at which point do we pin those pages? pin_user_pages() works by
userland address; by the time we get to any of those we have struct page
references and no idea whether they are still mapped anywhere.
How would that work? What protects the area where you want to avoid running
into pinned pages from previously acceptable page getting pinned? If "they
must have been successfully unmapped" is a part of what you are planning, we
really do have a problem...
On Wed 14-09-22 17:42:40, Al Viro wrote:
> On Wed, Sep 14, 2022 at 04:52:33PM +0200, Jan Kara wrote:
> > > =================================================================================
> > > CASE 5: Pinning in order to write to the data within the page
> > > -------------------------------------------------------------
> > > Even though neither DMA nor Direct IO is involved, just a simple case of "pin,
> > > write to a page's data, unpin" can cause a problem. Case 5 may be considered a
> > > superset of Case 1, plus Case 2, plus anything that invokes that pattern. In
> > > other words, if the code is neither Case 1 nor Case 2, it may still require
> > > FOLL_PIN, for patterns like this:
> > >
> > > Correct (uses FOLL_PIN calls):
> > > pin_user_pages()
> > > write to the data within the pages
> > > unpin_user_pages()
> > >
> > > INCORRECT (uses FOLL_GET calls):
> > > get_user_pages()
> > > write to the data within the pages
> > > put_page()
> > > =================================================================================
> >
> > Yes, that was my point.
>
> The thing is, at which point do we pin those pages? pin_user_pages() works by
> userland address; by the time we get to any of those we have struct page
> references and no idea whether they are still mapped anywhere.
Yes, pin_user_pages() currently works by page address but there's nothing
fundamental about that. Technically, pin is currently just another type of
page reference so we can as well just pin the page when given struct page.
In fact John Hubbart has added such helper in this series.
> How would that work? What protects the area where you want to avoid running
> into pinned pages from previously acceptable page getting pinned? If "they
> must have been successfully unmapped" is a part of what you are planning, we
> really do have a problem...
But this is a very good question. So far the idea was that we lock the
page, unmap (or writeprotect) the page, and then check pincount == 0 and
that is a reliable method for making sure page data is stable (until we
unlock the page & release other locks blocking page faults and writes). But
once suddently ordinary page references can be used to create pins this
does not work anymore. Hrm.
Just brainstorming ideas now: So we'd either need to obtain the pins early
when we still have the virtual address (but I guess that is often not
practical but should work e.g. for normal direct IO path) or we need some
way to "simulate" the page fault when pinning the page, just don't map it
into page tables in the end. This simulated page fault could be perhaps
avoided if rmap walk shows that the page is already mapped somewhere with
suitable permissions.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Sep 15, 2022 at 10:16:25AM +0200, Jan Kara wrote:
> > How would that work? What protects the area where you want to avoid running
> > into pinned pages from previously acceptable page getting pinned? If "they
> > must have been successfully unmapped" is a part of what you are planning, we
> > really do have a problem...
>
> But this is a very good question. So far the idea was that we lock the
> page, unmap (or writeprotect) the page, and then check pincount == 0 and
> that is a reliable method for making sure page data is stable (until we
> unlock the page & release other locks blocking page faults and writes). But
> once suddently ordinary page references can be used to create pins this
> does not work anymore. Hrm.
>
> Just brainstorming ideas now: So we'd either need to obtain the pins early
> when we still have the virtual address (but I guess that is often not
> practical but should work e.g. for normal direct IO path) or we need some
> way to "simulate" the page fault when pinning the page, just don't map it
> into page tables in the end. This simulated page fault could be perhaps
> avoided if rmap walk shows that the page is already mapped somewhere with
> suitable permissions.
OK... I'd done some digging; results so far
* READ vs. WRITE turned out to be an awful way to specify iov_iter
data direction. Local iov_iter branch so far:
get rid of unlikely() on page_copy_sane() calls
csum_and_copy_to_iter(): handle ITER_DISCARD
[s390] copy_oldmem_kernel() - WRITE is "data source", not destination
[fsi] WRITE is "data source", not destination...
[infiniband] READ is "data destination", not source...
[s390] zcore: WRITE is "data source", not destination...
[target] fix iov_iter_bvec() "direction" argument
[vhost] fix 'direction' argument of iov_iter_{init,bvec}()
[xen] fix "direction" argument of iov_iter_kvec()
[trace] READ means "data destination", not source...
iov_iter: saner checks for attempt to copy to/from iterator
use less confusing names for iov_iter direction initializers
those 8 commits in the middle consist of fixes, some of them with more than
one call site affected. Folks keep going "oh, we are going to copy data
into that iterator, must be WRITE". Wrong - WRITE means "as for write(2)",
i.e. the data _source_, not data destination. And the same kind of bugs
goes in the opposite direction, of course.
I think something like ITER_DEST vs. ITER_SOURCE would be less
confusing.
* anything that goes with ITER_SOURCE doesn't need pin.
* ITER_IOVEC/ITER_UBUF need pin for get_pages and for nothing else.
Need to grab reference on get_pages, obviously.
* even more obviously, ITER_DISCARD is irrelevant here.
* ITER_PIPE only modifies anonymous pages that had been allocated
by iov_iter primitives and hadn't been observed by anything outside until
we are done with said ITER_PIPE.
* quite a few instances are similar to e.g. REQ_OP_READ handling in
/dev/loop - we work with ITER_BVEC there and we do modify the page contents,
but the damn thing would better be given to us locked and stay locked until
all involved modifications (be it real IO/decoding/whatever) is complete.
That ought to be safe, unless I'm missing something.
That doesn't cover everything; still going through the list...
On Fri, Sep 16, 2022 at 02:55:53AM +0100, Al Viro wrote:
> * READ vs. WRITE turned out to be an awful way to specify iov_iter
> data direction. Local iov_iter branch so far:
> get rid of unlikely() on page_copy_sane() calls
> csum_and_copy_to_iter(): handle ITER_DISCARD
> [s390] copy_oldmem_kernel() - WRITE is "data source", not destination
> [fsi] WRITE is "data source", not destination...
> [infiniband] READ is "data destination", not source...
> [s390] zcore: WRITE is "data source", not destination...
> [target] fix iov_iter_bvec() "direction" argument
> [vhost] fix 'direction' argument of iov_iter_{init,bvec}()
> [xen] fix "direction" argument of iov_iter_kvec()
> [trace] READ means "data destination", not source...
> iov_iter: saner checks for attempt to copy to/from iterator
> use less confusing names for iov_iter direction initializers
> those 8 commits in the middle consist of fixes, some of them with more than
> one call site affected. Folks keep going "oh, we are going to copy data
> into that iterator, must be WRITE". Wrong - WRITE means "as for write(2)",
> i.e. the data _source_, not data destination. And the same kind of bugs
> goes in the opposite direction, of course.
> I think something like ITER_DEST vs. ITER_SOURCE would be less
> confusing.
>
> * anything that goes with ITER_SOURCE doesn't need pin.
> * ITER_IOVEC/ITER_UBUF need pin for get_pages and for nothing else.
> Need to grab reference on get_pages, obviously.
> * even more obviously, ITER_DISCARD is irrelevant here.
> * ITER_PIPE only modifies anonymous pages that had been allocated
> by iov_iter primitives and hadn't been observed by anything outside until
> we are done with said ITER_PIPE.
> * quite a few instances are similar to e.g. REQ_OP_READ handling in
> /dev/loop - we work with ITER_BVEC there and we do modify the page contents,
> but the damn thing would better be given to us locked and stay locked until
> all involved modifications (be it real IO/decoding/whatever) is complete.
> That ought to be safe, unless I'm missing something.
>
> That doesn't cover everything; still going through the list...
More:
nvme target: nvme read requests end up with somebody allocating and filling
sglist, followed by reading from file into it (using ITER_BVEC). Then the
pages are sent out, presumably. I would be very surprised if it turned out
to be anything other than anon pages allocated by the driver, but I'd like
to see that confirmed by nvme folks. Probably doesn't need pinning.
->read_folio() instances - page locked by caller, not unlocked until we are done.
->readahead() instances - pages are in the segment of page cache that had been
populated and locked by the caller; some are ITER_BVEC (with page(s) extracted
by readahead_page()), some - ITER_XARRAY.
other similar places (some of ->write_begin() instances, after having grabbed
a locked page, etc.)
->issue_read() instances - the call graph is scary (in particular, recursion
prevention there is non-obvious), but unless netfs folks say otherwise, I'd
assume that all pages involved are supposed to be locked by the caller.
swap reads (ending up at __swap_read_unplug()) - pages locked by callers.
in some cases (cifs) pages are privately allocated and not visible to anyone
else.
io_import_fixed() sets ITER_BVEC over pinned pages; see io_pin_pages() for
the place where that's done.
In cifs_send_async_read() we take the pages that will eventually go into
ITER_BVEC iterator from iov_iter_get_pages() - that one wants pinning if
the type of ctx->iter would demand so. The same goes for setup_aio_ctx_iter() -
iov_iter_get_pages() is used to make an ITER_BVEC counterpart of the
iov_iter passed to ->read_iter(), with the same considerations re pinning.
The same goes for ceph __iter_get_bvecs().
Haven't done yet:
drivers/target/target_core_file.c:292: iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
drivers/vhost/vringh.c:1198: iov_iter_bvec(&iter, ITER_DEST, iov, ret, translated);
fs/afs/dir.c:308: iov_iter_xarray(&req->def_iter, ITER_DEST, &dvnode->netfs.inode.i_mapping->i_pages,
net/ceph/messenger_v1.c:52: iov_iter_bvec(&msg.msg_iter, ITER_DEST, &bvec, 1, length);
net/ceph/messenger_v2.c:236: iov_iter_bvec(&con->v2.in_iter, ITER_DEST, &con->v2.in_bvec, 1, bv->bv_len);
net/sunrpc/svcsock.c:263: iov_iter_bvec(&msg.msg_iter, ITER_DEST, bvec, i, buflen);
net/sunrpc/xprtsock.c:376: iov_iter_bvec(&msg->msg_iter, ITER_DEST, bvec, nr, count);
The picture so far looks like we mostly need to take care of pinning when
we obtain the references from iov_iter_get_pages(). What's more, it looks
like ITER_BVEC/ITER_XARRAY/ITER_PIPE we really don't need to pin anything on
get_pages/pin_pages - they are already protected (or, in case of ITER_PIPE,
allocated by iov_iter itself and not reachable by anybody outside).
Might or might not be true for the remaining 7 call sites...
NOTE: all of the above assumes that callers with pre-locked pages are
either synchronous or do not unlock until the completion callbacks.
It does appear to be true; if it is true, I really wonder if we need
to even grab references in iov_iter_pin_pages() for anything other
than ITER_IOVEC/ITER_UBUF. The right primitive might be
if user-backed
pin pages
else
just copy the pointers; any lifetime-related issues are
up to the caller.
advance iterator in either case
On Thu, Sep 15, 2022 at 10:16:25AM +0200, Jan Kara wrote:
> > How would that work? What protects the area where you want to avoid running
> > into pinned pages from previously acceptable page getting pinned? If "they
> > must have been successfully unmapped" is a part of what you are planning, we
> > really do have a problem...
>
> But this is a very good question. So far the idea was that we lock the
> page, unmap (or writeprotect) the page, and then check pincount == 0 and
> that is a reliable method for making sure page data is stable (until we
> unlock the page & release other locks blocking page faults and writes). But
> once suddently ordinary page references can be used to create pins this
> does not work anymore. Hrm.
>
> Just brainstorming ideas now: So we'd either need to obtain the pins early
> when we still have the virtual address (but I guess that is often not
> practical but should work e.g. for normal direct IO path) or we need some
> way to "simulate" the page fault when pinning the page, just don't map it
> into page tables in the end. This simulated page fault could be perhaps
> avoided if rmap walk shows that the page is already mapped somewhere with
> suitable permissions.
OK. As far as I can see, the rules are along the lines of
* creator of ITER_BVEC/ITER_XARRAY is responsible for pages being safe.
That includes
* page known to be locked by caller
* page being privately allocated and not visible to anyone else
* iterator being data source
* page coming from pin_user_pages(), possibly as the result of
iov_iter_pin_pages() on ITER_IOVEC/ITER_UBUF.
* ITER_PIPE pages are always safe
* pages found in ITER_BVEC/ITER_XARRAY are safe, since the iterator
had been created with such.
My preference would be to have iov_iter_get_pages() and friends pin if and
only if we have data-destination iov_iter that is user-backed. For
data-source user-backed we only need FOLL_GET, and for all other flavours
(ITER_BVEC, etc.) we only do get_page(), if we need to grab any references
at all.
What I'd like to have is the understanding of the places where we drop
the references acquired by iov_iter_get_pages(). How do we decide
whether to unpin? E.g. pipe_buffer carries a reference to page and no
way to tell whether it's a pinned one; results of iov_iter_get_pages()
on ITER_IOVEC *can* end up there, but thankfully only from data-source
(== WRITE, aka. ITER_SOURCE) iov_iter. So for those we don't care.
Then there's nfs_request; AFAICS, we do need to pin the references in
those if they are coming from nfs_direct_read_schedule_iovec(), but
not if they come from readpage_async_filler(). How do we deal with
coalescence, etc.? It's been a long time since I really looked at
that code... Christoph, could you give any comments on that one?
Note, BTW, that nfs_request coming from readpage_async_filler() have
pages locked by caller; the ones from nfs_direct_read_schedule_iovec()
do not, and that's where we want them pinned. Resulting page references
end up (after quite a trip through data structures) stuffed into struct
rpc_rqst ->rc_recv_buf.pages[] and when a response arrives from server,
they get picked by xs_read_bvec() and fed to iov_iter_bvec(). In one
case it's safe since the pages are locked; in another - since they would
come from pin_user_pages(). The call chain at the time they are used
has nothing to do with the originator - sunrpc is looking at the arrived
response to READ that matches an rpc_rqst that had been created by sender
of that request and safety is the sender's responsibility.
On 9/21/22 19:22, Al Viro wrote:
> On Thu, Sep 15, 2022 at 10:16:25AM +0200, Jan Kara wrote:
>
>>> How would that work? What protects the area where you want to avoid running
>>> into pinned pages from previously acceptable page getting pinned? If "they
>>> must have been successfully unmapped" is a part of what you are planning, we
>>> really do have a problem...
>>
>> But this is a very good question. So far the idea was that we lock the
>> page, unmap (or writeprotect) the page, and then check pincount == 0 and
>> that is a reliable method for making sure page data is stable (until we
>> unlock the page & release other locks blocking page faults and writes). But
>> once suddently ordinary page references can be used to create pins this
>> does not work anymore. Hrm.
>>
>> Just brainstorming ideas now: So we'd either need to obtain the pins early
>> when we still have the virtual address (but I guess that is often not
>> practical but should work e.g. for normal direct IO path) or we need some
>> way to "simulate" the page fault when pinning the page, just don't map it
>> into page tables in the end. This simulated page fault could be perhaps
>> avoided if rmap walk shows that the page is already mapped somewhere with
>> suitable permissions.
>
> OK. As far as I can see, the rules are along the lines of
> * creator of ITER_BVEC/ITER_XARRAY is responsible for pages being safe.
> That includes
> * page known to be locked by caller
> * page being privately allocated and not visible to anyone else
> * iterator being data source
> * page coming from pin_user_pages(), possibly as the result of
> iov_iter_pin_pages() on ITER_IOVEC/ITER_UBUF.
> * ITER_PIPE pages are always safe
> * pages found in ITER_BVEC/ITER_XARRAY are safe, since the iterator
> had been created with such.
> My preference would be to have iov_iter_get_pages() and friends pin if and
> only if we have data-destination iov_iter that is user-backed. For
> data-source user-backed we only need FOLL_GET, and for all other flavours
> (ITER_BVEC, etc.) we only do get_page(), if we need to grab any references
> at all.
This rule would mostly work, as long as we can relax it in some cases, to
allow pinning of both source and dest pages, instead of just destination
pages, in some cases. In particular, bio_release_pages() has lost all
context about whether it was a read or a write request, as far as I can
tell. And bio_release_pages() is the primary place to unpin pages for
direct IO.
>
> What I'd like to have is the understanding of the places where we drop
> the references acquired by iov_iter_get_pages(). How do we decide
> whether to unpin? E.g. pipe_buffer carries a reference to page and no
> way to tell whether it's a pinned one; results of iov_iter_get_pages()
> on ITER_IOVEC *can* end up there, but thankfully only from data-source
> (== WRITE, aka. ITER_SOURCE) iov_iter. So for those we don't care.
> Then there's nfs_request; AFAICS, we do need to pin the references in
> those if they are coming from nfs_direct_read_schedule_iovec(), but
> not if they come from readpage_async_filler(). How do we deal with
> coalescence, etc.? It's been a long time since I really looked at
> that code... Christoph, could you give any comments on that one?
>
> Note, BTW, that nfs_request coming from readpage_async_filler() have
> pages locked by caller; the ones from nfs_direct_read_schedule_iovec()
> do not, and that's where we want them pinned. Resulting page references
> end up (after quite a trip through data structures) stuffed into struct
> rpc_rqst ->rc_recv_buf.pages[] and when a response arrives from server,
> they get picked by xs_read_bvec() and fed to iov_iter_bvec(). In one
> case it's safe since the pages are locked; in another - since they would
> come from pin_user_pages(). The call chain at the time they are used
> has nothing to do with the originator - sunrpc is looking at the arrived
> response to READ that matches an rpc_rqst that had been created by sender
> of that request and safety is the sender's responsibility.
For NFS Direct, is there any reason it can't be as simple as this
(conceptually, that is--the implementation of iov_iter_pin_pages_alloc()
is not shown here)? Here:
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 1707f46b1335..7dbc705bab83 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -142,13 +142,6 @@ int nfs_swap_rw(struct kiocb *iocb, struct iov_iter *iter)
return 0;
}
-static void nfs_direct_release_pages(struct page **pages, unsigned int npages)
-{
- unsigned int i;
- for (i = 0; i < npages; i++)
- put_page(pages[i]);
-}
-
void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
struct nfs_direct_req *dreq)
{
@@ -332,7 +325,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
size_t pgbase;
unsigned npages, i;
- result = iov_iter_get_pages_alloc2(iter, &pagevec,
+ result = iov_iter_pin_pages_alloc(iter, &pagevec,
rsize, &pgbase);
if (result < 0)
break;
@@ -362,7 +355,16 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
pos += req_len;
dreq->bytes_left -= req_len;
}
- nfs_direct_release_pages(pagevec, npages);
+
+ /*
+ * iov_iter_pin_pages_alloc() calls pin_user_pages_fast() for
+ * the user_backed_iter() case (only).
+ */
+ if (user_backed_iter(iter))
+ unpin_user_pages(pagevec, npages);
+ else
+ release_pages(pagevec, npages);
+
kvfree(pagevec);
if (result < 0)
break;
@@ -829,7 +831,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
pos += req_len;
dreq->bytes_left -= req_len;
}
- nfs_direct_release_pages(pagevec, npages);
+ release_pages(pagevec, npages);
kvfree(pagevec);
if (result < 0)
break;
thanks,
--
John Hubbard
NVIDIA
On Wed 21-09-22 23:09:06, John Hubbard wrote:
> On 9/21/22 19:22, Al Viro wrote:
> > On Thu, Sep 15, 2022 at 10:16:25AM +0200, Jan Kara wrote:
> >
> >>> How would that work? What protects the area where you want to avoid running
> >>> into pinned pages from previously acceptable page getting pinned? If "they
> >>> must have been successfully unmapped" is a part of what you are planning, we
> >>> really do have a problem...
> >>
> >> But this is a very good question. So far the idea was that we lock the
> >> page, unmap (or writeprotect) the page, and then check pincount == 0 and
> >> that is a reliable method for making sure page data is stable (until we
> >> unlock the page & release other locks blocking page faults and writes). But
> >> once suddently ordinary page references can be used to create pins this
> >> does not work anymore. Hrm.
> >>
> >> Just brainstorming ideas now: So we'd either need to obtain the pins early
> >> when we still have the virtual address (but I guess that is often not
> >> practical but should work e.g. for normal direct IO path) or we need some
> >> way to "simulate" the page fault when pinning the page, just don't map it
> >> into page tables in the end. This simulated page fault could be perhaps
> >> avoided if rmap walk shows that the page is already mapped somewhere with
> >> suitable permissions.
> >
> > OK. As far as I can see, the rules are along the lines of
> > * creator of ITER_BVEC/ITER_XARRAY is responsible for pages being safe.
> > That includes
> > * page known to be locked by caller
> > * page being privately allocated and not visible to anyone else
> > * iterator being data source
> > * page coming from pin_user_pages(), possibly as the result of
> > iov_iter_pin_pages() on ITER_IOVEC/ITER_UBUF.
> > * ITER_PIPE pages are always safe
> > * pages found in ITER_BVEC/ITER_XARRAY are safe, since the iterator
> > had been created with such.
> > My preference would be to have iov_iter_get_pages() and friends pin if and
> > only if we have data-destination iov_iter that is user-backed. For
> > data-source user-backed we only need FOLL_GET, and for all other flavours
> > (ITER_BVEC, etc.) we only do get_page(), if we need to grab any references
> > at all.
>
> This rule would mostly work, as long as we can relax it in some cases, to
> allow pinning of both source and dest pages, instead of just destination
> pages, in some cases. In particular, bio_release_pages() has lost all
> context about whether it was a read or a write request, as far as I can
> tell. And bio_release_pages() is the primary place to unpin pages for
> direct IO.
Well, we already do have BIO_NO_PAGE_REF bio flag that gets checked in
bio_release_pages(). I think we can easily spare another bio flag to tell
whether we need to unpin or not. So as long as all the pages in the created
bio need the same treatment, the situation should be simple.
> > What I'd like to have is the understanding of the places where we drop
> > the references acquired by iov_iter_get_pages(). How do we decide
> > whether to unpin? E.g. pipe_buffer carries a reference to page and no
> > way to tell whether it's a pinned one; results of iov_iter_get_pages()
> > on ITER_IOVEC *can* end up there, but thankfully only from data-source
> > (== WRITE, aka. ITER_SOURCE) iov_iter. So for those we don't care.
> > Then there's nfs_request; AFAICS, we do need to pin the references in
> > those if they are coming from nfs_direct_read_schedule_iovec(), but
> > not if they come from readpage_async_filler(). How do we deal with
> > coalescence, etc.? It's been a long time since I really looked at
> > that code... Christoph, could you give any comments on that one?
> >
> > Note, BTW, that nfs_request coming from readpage_async_filler() have
> > pages locked by caller; the ones from nfs_direct_read_schedule_iovec()
> > do not, and that's where we want them pinned. Resulting page references
> > end up (after quite a trip through data structures) stuffed into struct
> > rpc_rqst ->rc_recv_buf.pages[] and when a response arrives from server,
> > they get picked by xs_read_bvec() and fed to iov_iter_bvec(). In one
> > case it's safe since the pages are locked; in another - since they would
> > come from pin_user_pages(). The call chain at the time they are used
> > has nothing to do with the originator - sunrpc is looking at the arrived
> > response to READ that matches an rpc_rqst that had been created by sender
> > of that request and safety is the sender's responsibility.
>
> For NFS Direct, is there any reason it can't be as simple as this
> (conceptually, that is--the implementation of iov_iter_pin_pages_alloc()
> is not shown here)? Here:
>
>
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index 1707f46b1335..7dbc705bab83 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -142,13 +142,6 @@ int nfs_swap_rw(struct kiocb *iocb, struct iov_iter *iter)
> return 0;
> }
>
> -static void nfs_direct_release_pages(struct page **pages, unsigned int npages)
> -{
> - unsigned int i;
> - for (i = 0; i < npages; i++)
> - put_page(pages[i]);
> -}
> -
> void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
> struct nfs_direct_req *dreq)
> {
> @@ -332,7 +325,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
> size_t pgbase;
> unsigned npages, i;
>
> - result = iov_iter_get_pages_alloc2(iter, &pagevec,
> + result = iov_iter_pin_pages_alloc(iter, &pagevec,
> rsize, &pgbase);
> if (result < 0)
> break;
> @@ -362,7 +355,16 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
> pos += req_len;
> dreq->bytes_left -= req_len;
> }
> - nfs_direct_release_pages(pagevec, npages);
> +
> + /*
> + * iov_iter_pin_pages_alloc() calls pin_user_pages_fast() for
> + * the user_backed_iter() case (only).
> + */
> + if (user_backed_iter(iter))
> + unpin_user_pages(pagevec, npages);
> + else
> + release_pages(pagevec, npages);
> +
I don't think this will work. The pin nfs_direct_read_schedule_iovec()
obtains needs to be released once the IO is completed. Not once the IO is
submitted. Notice how nfs_create_request()->__nfs_create_request() gets
another page reference which is released on completion
(nfs_direct_read_completion->nfs_release_request->nfs_page_group_destroy->
nfs_free_request->nfs_clear_request). And we need to stop releasing the
obtained pin in nfs_direct_read_schedule_iovec() (and acquiring another
reference in __nfs_create_request()) and instead propagate it to
nfs_clear_request() where it can get released.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Sep 14, 2022 at 04:51:17AM +0100, Al Viro wrote:
> Unless I'm misreading Jan, the question is whether they should get or
> pin.
And I think the answer is: inside ->read_iter or ->write_iter they
should neither get or pin. The callers of it need to pin the pages
if they are pagecache pages that can potentially be written to through
shared mappings, else a get would be enough. But the method instance
should not have to care and just be able to rely on the caller making
sure they do not go away.
> I'm really tempted to slap
> if (WARN_ON(i->data_source))
> return 0;
> into copy_to_iter() et.al., along with its opposite for copy_from_iter().
Ys, I think that would be useful. And we could use something more
descriptive than READ/WRITE to start with.
On Tue, Sep 20, 2022 at 06:02:11AM +0100, Al Viro wrote:
> nvme target: nvme read requests end up with somebody allocating and filling
> sglist, followed by reading from file into it (using ITER_BVEC). Then the
> pages are sent out, presumably
Yes.
> . I would be very surprised if it turned out
> to be anything other than anon pages allocated by the driver, but I'd like
> to see that confirmed by nvme folks. Probably doesn't need pinning.
They are anon pages allocated by the driver using sgl_alloc().
> drivers/target/target_core_file.c:292: iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
Same as nvme target.
> The picture so far looks like we mostly need to take care of pinning when
> we obtain the references from iov_iter_get_pages(). What's more, it looks
> like ITER_BVEC/ITER_XARRAY/ITER_PIPE we really don't need to pin anything on
> get_pages/pin_pages - they are already protected (or, in case of ITER_PIPE,
> allocated by iov_iter itself and not reachable by anybody outside).
That's what I've been trying to say for a while..
On Thu, Sep 22, 2022 at 07:31:36AM -0700, Christoph Hellwig wrote:
> On Wed, Sep 14, 2022 at 04:51:17AM +0100, Al Viro wrote:
> > Unless I'm misreading Jan, the question is whether they should get or
> > pin.
>
> And I think the answer is: inside ->read_iter or ->write_iter they
> should neither get or pin. The callers of it need to pin the pages
> if they are pagecache pages that can potentially be written to through
> shared mappings, else a get would be enough. But the method instance
> should not have to care and just be able to rely on the caller making
> sure they do not go away.
The interesting part, AFAICS, is where do we _unpin_ them and how do
we keep track which pages (obtained from iov_iter_get_pages et.al.)
need to be unpinned.
> > I'm really tempted to slap
> > if (WARN_ON(i->data_source))
> > return 0;
> > into copy_to_iter() et.al., along with its opposite for copy_from_iter().
>
> Ys, I think that would be useful. And we could use something more
> descriptive than READ/WRITE to start with.
See #work.iov_iter; done, but it took a bit of fixing the places that
create iov_iter instances.
On Thu, Sep 22, 2022 at 03:22:48AM +0100, Al Viro wrote:
> What I'd like to have is the understanding of the places where we drop
> the references acquired by iov_iter_get_pages(). How do we decide
> whether to unpin?
Add a iov_iter_unpin_pages that does the right thing based on the
type. (block will need a modified copy of it as it doesn't keep
the pages array around, but logic will be the same).
> E.g. pipe_buffer carries a reference to page and no
> way to tell whether it's a pinned one; results of iov_iter_get_pages()
> on ITER_IOVEC *can* end up there, but thankfully only from data-source
> (== WRITE, aka. ITER_SOURCE) iov_iter. So for those we don't care.
> Then there's nfs_request; AFAICS, we do need to pin the references in
> those if they are coming from nfs_direct_read_schedule_iovec(), but
> not if they come from readpage_async_filler(). How do we deal with
> coalescence, etc.? It's been a long time since I really looked at
> that code... Christoph, could you give any comments on that one?
I think the above should work, but I'll need to look at the NFS code
in more detail to be sure.
On Thu, Sep 22, 2022 at 04:43:57PM +0200, David Hildenbrand wrote:
> I assume they are not anon pages as in "PageAnon()", but simply not
> pagecache pages, correct?
Yes, sorry. From the page allocator and not added to the page cache.
On Thu, Sep 22, 2022 at 01:29:35PM +0200, Jan Kara wrote:
> > This rule would mostly work, as long as we can relax it in some cases, to
> > allow pinning of both source and dest pages, instead of just destination
> > pages, in some cases. In particular, bio_release_pages() has lost all
> > context about whether it was a read or a write request, as far as I can
> > tell. And bio_release_pages() is the primary place to unpin pages for
> > direct IO.
>
> Well, we already do have BIO_NO_PAGE_REF bio flag that gets checked in
> bio_release_pages(). I think we can easily spare another bio flag to tell
> whether we need to unpin or not. So as long as all the pages in the created
> bio need the same treatment, the situation should be simple.
Yes. Incidentally, the same condition is already checked by the creators
of those bio - see the assorted should_dirty logics.
While we are at it - how much of the rationale around bio_check_pages_dirty()
doing dirtying is still applicable with pinning pages before we stick them
into bio? We do dirty them before submitting bio, then on completion
bio_check_pages_dirty() checks if something has marked them clean while
we'd been doing IO; if all of them are still dirty we just drop the pages
(well, unpin and drop), otherwise we arrange for dirty + unpin + drop
done in process context (via schedule_work()). Can they be marked clean by
anyone while they are pinned? After all, pinning is done to prevent
writeback getting done on them while we are modifying the suckers...
On Thu, Sep 22, 2022 at 07:38:25AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 22, 2022 at 03:22:48AM +0100, Al Viro wrote:
> > What I'd like to have is the understanding of the places where we drop
> > the references acquired by iov_iter_get_pages(). How do we decide
> > whether to unpin?
>
> Add a iov_iter_unpin_pages that does the right thing based on the
> type. (block will need a modified copy of it as it doesn't keep
> the pages array around, but logic will be the same).
Huh? You want to keep the type (+ direction) of iov_iter in any structure
a page reference coming from iov_iter_get_pages might end up in? IDGI...
BTW, speaking of lifetime rules - am I right assuming that fd_execute_rw()
does IO on pages of the scatterlist passed to it? Where are they getting
dropped and what guarantees that IO is complete by that point?
The reason I'm asking is that here you have an ITER_BVEC possibly fed to
__blkdev_direct_IO_async(), with its
if (iov_iter_is_bvec(iter)) {
/*
* Users don't rely on the iterator being in any particular
* state for async I/O returning -EIOCBQUEUED, hence we can
* avoid expensive iov_iter_advance(). Bypass
* bio_iov_iter_get_pages() and set the bvec directly.
*/
bio_iov_bvec_set(bio, iter);
which does *not* grab the page referneces. Sure, bio_release_pages() knows
to leave those alone and doesn't drop anything. However, what is the
mechanism preventing the pages getting freed before the IO completion
in this case?
On 9/22/22 04:29, Jan Kara wrote:
> I don't think this will work. The pin nfs_direct_read_schedule_iovec()
> obtains needs to be released once the IO is completed. Not once the IO is
> submitted. Notice how nfs_create_request()->__nfs_create_request() gets
> another page reference which is released on completion
> (nfs_direct_read_completion->nfs_release_request->nfs_page_group_destroy->
> nfs_free_request->nfs_clear_request). And we need to stop releasing the
> obtained pin in nfs_direct_read_schedule_iovec() (and acquiring another
> reference in __nfs_create_request()) and instead propagate it to
> nfs_clear_request() where it can get released.
>
> Honza
OK, now I see how that is supposed to work, thanks for explaining!
thanks,
--
John Hubbard
NVIDIA
On Thu, Sep 22, 2022 at 09:05:16PM -0700, John Hubbard wrote:
> I certainly hope not. And in fact, we should really just say that that's
> a rule: the whole time the page is pinned, it simply must remain dirty
> and writable, at least with the way things are right now.
Yes, if we can stick to that rule and make sure shared pagecache is
never dirtied through get_user_pags anywhere that will allow us to
fix a lot of mess
> To fix those cases, IIUC, the answer is: you must make the page dirty
> properly, with page_mkwrite(), not just with set_page_dirty_lock(). And
> that has to be done probably a lot earlier, for reasons that I'm still
> vague on. But perhaps right after pinning the page. (Assuming that we
> hold off writeback while the page is pinned.)
I think we need to hold off the writeback for it to work properly.
The big question is, is if there are callers that do expect data
to be written back on mappings that are longterm pinned. RDMA or
vfio would come to mind.
On Fri, Sep 23, 2022 at 05:22:17AM +0100, Al Viro wrote:
> > Add a iov_iter_unpin_pages that does the right thing based on the
> > type. (block will need a modified copy of it as it doesn't keep
> > the pages array around, but logic will be the same).
>
> Huh? You want to keep the type (+ direction) of iov_iter in any structure
> a page reference coming from iov_iter_get_pages might end up in? IDGI...
Why would I? We generall do have or should have the iov_iter around.
And for the common case where we don't (bios) we can carry that
information in the bio as it needs a special unmap helper anyway.
But if you don't want to use the iov_iter for some reason, we'll just
need to condense the information to a flags variable and then pass that.
>
> BTW, speaking of lifetime rules - am I right assuming that fd_execute_rw()
> does IO on pages of the scatterlist passed to it?
Yes.
> Where are they getting
> dropped and what guarantees that IO is complete by that point?
The exact place depens on the exact taaraget frontend of which we have
a few. But it happens from the end_io callback that is triggered
through a call to target_complete_cmd.
> The reason I'm asking is that here you have an ITER_BVEC possibly fed to
> __blkdev_direct_IO_async(), with its
> if (iov_iter_is_bvec(iter)) {
> /*
> * Users don't rely on the iterator being in any particular
> * state for async I/O returning -EIOCBQUEUED, hence we can
> * avoid expensive iov_iter_advance(). Bypass
> * bio_iov_iter_get_pages() and set the bvec directly.
> */
> bio_iov_bvec_set(bio, iter);
> which does *not* grab the page referneces. Sure, bio_release_pages() knows
> to leave those alone and doesn't drop anything. However, what is the
> mechanism preventing the pages getting freed before the IO completion
> in this case?
The contract that callers of bvec iters need to hold their own
references as without that doing I/O do them would be unsafe. It they
did not hold references the pages could go away before even calling
bio_iov_iter_get_pages (or this open coded bio_iov_bvec_set).
On Thu 22-09-22 21:05:16, John Hubbard wrote:
> On 9/22/22 20:19, Al Viro wrote:
> > On Thu, Sep 22, 2022 at 01:29:35PM +0200, Jan Kara wrote:
> >
> >>> This rule would mostly work, as long as we can relax it in some cases, to
> >>> allow pinning of both source and dest pages, instead of just destination
> >>> pages, in some cases. In particular, bio_release_pages() has lost all
> >>> context about whether it was a read or a write request, as far as I can
> >>> tell. And bio_release_pages() is the primary place to unpin pages for
> >>> direct IO.
> >>
> >> Well, we already do have BIO_NO_PAGE_REF bio flag that gets checked in
> >> bio_release_pages(). I think we can easily spare another bio flag to tell
> >> whether we need to unpin or not. So as long as all the pages in the created
> >> bio need the same treatment, the situation should be simple.
> >
> > Yes. Incidentally, the same condition is already checked by the creators
> > of those bio - see the assorted should_dirty logics.
>
> Beautiful!
>
> >
> > While we are at it - how much of the rationale around bio_check_pages_dirty()
> > doing dirtying is still applicable with pinning pages before we stick them
> > into bio? We do dirty them before submitting bio, then on completion
> > bio_check_pages_dirty() checks if something has marked them clean while
> > we'd been doing IO; if all of them are still dirty we just drop the pages
> > (well, unpin and drop), otherwise we arrange for dirty + unpin + drop
> > done in process context (via schedule_work()). Can they be marked clean by
> > anyone while they are pinned? After all, pinning is done to prevent
> > writeback getting done on them while we are modifying the suckers...
>
> I certainly hope not. And in fact, we should really just say that that's
> a rule: the whole time the page is pinned, it simply must remain dirty
> and writable, at least with the way things are right now.
I agree the page should be staying dirty the whole time it is pinned. I
don't think it is feasible to keep it writeable in the page tables because
that would mean you would need to block e.g. munmap() until the pages gets
unpinned and that will almost certainly upset some current userspace.
But keeping page dirty should be enough so that we can get rid of all these
nasty calls to set_page_dirty() from IO completion.
> This reminds me that I'm not exactly sure what the rules for
> FOLL_LONGTERM callers should be, with respect to dirtying. At the
> moment, most, if not all of the code that does "set_page_dirty_lock();
> unpin_user_page()" is wrong.
Right.
> To fix those cases, IIUC, the answer is: you must make the page dirty
> properly, with page_mkwrite(), not just with set_page_dirty_lock(). And
Correct, and GUP (or PUP) actually does that under the hood so I don't
think we need to change anything there.
> that has to be done probably a lot earlier, for reasons that I'm still
> vague on. But perhaps right after pinning the page. (Assuming that we
> hold off writeback while the page is pinned.)
Holding off writeback is not always doable - as Christoph mentions, for
data integrity writeback we'll have to get the data to disk before the page
is unpinned (as for longterm users it can take days for the page to be
unpinned). But we can just writeback the page without clearing the dirty
bit in these cases. We may need to use bounce pages to be able to safely
writeback pinned pages but that's another part of the story...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri, Sep 23, 2022 at 01:44:33AM -0700, Christoph Hellwig wrote:
> Why would I? We generall do have or should have the iov_iter around.
Not for async IO.
> And for the common case where we don't (bios) we can carry that
> information in the bio as it needs a special unmap helper anyway.
*Any* async read_iter is like that.
> > Where are they getting
> > dropped and what guarantees that IO is complete by that point?
>
> The exact place depens on the exact taaraget frontend of which we have
> a few. But it happens from the end_io callback that is triggered
> through a call to target_complete_cmd.
OK...
> > The reason I'm asking is that here you have an ITER_BVEC possibly fed to
> > __blkdev_direct_IO_async(), with its
> > if (iov_iter_is_bvec(iter)) {
> > /*
> > * Users don't rely on the iterator being in any particular
> > * state for async I/O returning -EIOCBQUEUED, hence we can
> > * avoid expensive iov_iter_advance(). Bypass
> > * bio_iov_iter_get_pages() and set the bvec directly.
> > */
> > bio_iov_bvec_set(bio, iter);
> > which does *not* grab the page referneces. Sure, bio_release_pages() knows
> > to leave those alone and doesn't drop anything. However, what is the
> > mechanism preventing the pages getting freed before the IO completion
> > in this case?
>
> The contract that callers of bvec iters need to hold their own
> references as without that doing I/O do them would be unsafe. It they
> did not hold references the pages could go away before even calling
> bio_iov_iter_get_pages (or this open coded bio_iov_bvec_set).
You are mixing two issues here - holding references to pages while using
iov_iter instance is obvious; holding them until async IO is complete, even
though struct iov_iter might be long gone by that point is a different
story.
And originating iov_iter instance really can be long-gone by the time
of IO completion - requirement to keep it around would be very hard to
satisfy. I've no objections to requiring the pages in ITER_BVEC to be
preserved at least until the IO completion by means independent of
whatever ->read_iter/->write_iter does to them, but
* that needs to be spelled out very clearly and
* we need to verify that it is, indeed, the case for all existing
iov_iter_bvec callers, preferably with comments next to non-obvious ones
(something that is followed only by the sync IO is obvious)
That goes not just for bio - if we make get_pages *NOT* grab references
on ITER_BVEC (and I'm all for it), we need to make sure that those
pages won't be retained after the original protection runs out. Which
includes the reference put into struct nfs_request, for example, as well
as whatever ceph transport is doing, etc. Another thing that needs to
be sorted out is __zerocopy_sg_from_iter() and its callers - AFAICS,
all of those are in ->sendmsg() with MSG_ZEROCOPY in flags.
It's a non-trivial amount of code audit - we have about 40 iov_iter_bvec()
callers in the tree, and while many are clearly sync-only... the ones
that are not tend to balloon into interesting analysis of call chains, etc.
Don't get me wrong - that analysis needs to be done, just don't expect
it to be trivial. And it will require quite a bit of cooperation from the
folks familiar with e.g. drivers/target, or with ceph transport layer,
etc.
FWIW, my preference would be along the lines of
Backing memory for any non user-backed iov_iter should be protected
from reuse by creator of iov_iter and that protection should continue
through not just all synchronous operations with iov_iter in question
- it should last until all async operations involving that memory are
finished. That continued protection must not depend upon taking
extra page references, etc. while we are working with iov_iter.
But getting there will take quite a bit of code audit and probably some massage
as well.
On Fri, Sep 23, 2022 at 05:13:42PM +0100, Al Viro wrote:
> You are mixing two issues here - holding references to pages while using
> iov_iter instance is obvious; holding them until async IO is complete, even
> though struct iov_iter might be long gone by that point is a different
> story.
But someone needs to hold a refernce until the I/O is completed, because
the I/O obviously needs the pages. Yes, we could say the callers holds
them and can drop the references right after I/O submission, while
the method needs to grab another reference. But that is more
complicated and is more costly than just holding the damn reference.
> And originating iov_iter instance really can be long-gone by the time
> of IO completion - requirement to keep it around would be very hard to
> satisfy. I've no objections to requiring the pages in ITER_BVEC to be
> preserved at least until the IO completion by means independent of
> whatever ->read_iter/->write_iter does to them, but
> * that needs to be spelled out very clearly and
> * we need to verify that it is, indeed, the case for all existing
> iov_iter_bvec callers, preferably with comments next to non-obvious ones
> (something that is followed only by the sync IO is obvious)
Agreed.
> That goes not just for bio - if we make get_pages *NOT* grab references
> on ITER_BVEC (and I'm all for it), we need to make sure that those
> pages won't be retained after the original protection runs out.
Yes.
On Mon, Sep 26, 2022 at 08:53:43AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 23, 2022 at 05:13:42PM +0100, Al Viro wrote:
> > You are mixing two issues here - holding references to pages while using
> > iov_iter instance is obvious; holding them until async IO is complete, even
> > though struct iov_iter might be long gone by that point is a different
> > story.
>
> But someone needs to hold a refernce until the I/O is completed, because
> the I/O obviously needs the pages. Yes, we could say the callers holds
> them and can drop the references right after I/O submission, while
> the method needs to grab another reference. But that is more
> complicated and is more costly than just holding the damn reference.
Take a look at __nfs_create_request(). And trace the call chains leading
to nfs_clear_request() where the corresponding put_page() happens.
What I'm afraid of is something similar in the bowels of some RDMA driver.
With upper layers shoving page references into sglist using iov_iter_get_pages(),
then passing sglist to some intermediate layer, then *that* getting passed down
into a driver which grabs references for its own use and releases them from
destructor of some private structure. Done via kref_put(). Have that
delayed by, hell - anything, up to and including debugfs shite somewhere
in the same driver, iterating through those private structures, grabbing
a reference to do some pretty-print into kmalloc'ed buffer, then drooping it.
Voila - we have page refs duplicated from ITER_BVEC and occasionally staying
around after the ->ki_complete() of async ->write_iter() that got that
ITER_BVEC.
It's really not a trivial rule change.