Hi Linus et al.,
here's an update of my gfs2 mmap + page fault fixes (against -rc2).
From my point of view,
* these two are ready and need to be looked at by Al:
iov_iter: Introduce fault_in_iov_iter helper
iov_iter: Introduce noio flag to disable page faults
* these two need to be reviewed by Christoph at least:
iomap: Fix iomap_dio_rw return value for user copies
iomap: Support restarting direct I/O requests after user copy failures
Thanks a lot,
Andreas
Andreas Gruenbacher (7):
iov_iter: Introduce fault_in_iov_iter helper
gfs2: Add wrapper for iomap_file_buffered_write
gfs2: Fix mmap + page fault deadlocks for buffered I/O
iomap: Fix iomap_dio_rw return value for user copies
iomap: Support restarting direct I/O requests after user copy failures
iov_iter: Introduce noio flag to disable page faults
gfs2: Fix mmap + page fault deadlocks for direct I/O
fs/gfs2/file.c | 77 ++++++++++++++++++++++++++++++++++++++++----
fs/iomap/direct-io.c | 13 ++++++--
include/linux/mm.h | 3 ++
include/linux/uio.h | 2 ++
lib/iov_iter.c | 62 ++++++++++++++++++++++++++++++++---
mm/gup.c | 68 ++++++++++++++++++++++++++++++++++++++
6 files changed, 211 insertions(+), 14 deletions(-)
--
2.26.3
Add a wrapper around iomap_file_buffered_write. We'll add code for when
the operation needs to be retried here later.
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
fs/gfs2/file.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 84ec053d43b4..55ec1cadc9e6 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -876,6 +876,18 @@ static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
return written ? written : ret;
}
+static ssize_t gfs2_file_buffered_write(struct kiocb *iocb, struct iov_iter *from)
+{
+ struct file *file = iocb->ki_filp;
+ struct inode *inode = file_inode(file);
+ ssize_t ret;
+
+ current->backing_dev_info = inode_to_bdi(inode);
+ ret = iomap_file_buffered_write(iocb, from, &gfs2_iomap_ops);
+ current->backing_dev_info = NULL;
+ return ret;
+}
+
/**
* gfs2_file_write_iter - Perform a write to a file
* @iocb: The io context
@@ -927,9 +939,7 @@ static ssize_t gfs2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
goto out_unlock;
iocb->ki_flags |= IOCB_DSYNC;
- current->backing_dev_info = inode_to_bdi(inode);
- buffered = iomap_file_buffered_write(iocb, from, &gfs2_iomap_ops);
- current->backing_dev_info = NULL;
+ buffered = gfs2_file_buffered_write(iocb, from);
if (unlikely(buffered <= 0)) {
if (!ret)
ret = buffered;
@@ -951,9 +961,7 @@ static ssize_t gfs2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (!ret || ret2 > 0)
ret += ret2;
} else {
- current->backing_dev_info = inode_to_bdi(inode);
- ret = iomap_file_buffered_write(iocb, from, &gfs2_iomap_ops);
- current->backing_dev_info = NULL;
+ ret = gfs2_file_buffered_write(iocb, from);
if (likely(ret > 0)) {
iocb->ki_pos += ret;
ret = generic_write_sync(iocb, ret);
--
2.26.3
Introduce a new noio flag to indicate to get_user_pages to use the
FOLL_FAST_ONLY flag. This will cause get_user_pages to fail when it
would otherwise fault in a page.
Currently, the noio flag is only checked in iov_iter_get_pages and
iov_iter_get_pages_alloc. This is enough for iomaop_dio_rw, but it
may make sense to check for this flag in other contexts as well.
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
include/linux/uio.h | 1 +
lib/iov_iter.c | 20 +++++++++++++++-----
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 152b3605e86c..8de6354ade14 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -29,6 +29,7 @@ enum iter_type {
struct iov_iter {
u8 iter_type;
+ bool noio;
bool data_source;
size_t iov_offset;
size_t count;
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 7221665f7ac4..a20426cedf60 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -509,6 +509,7 @@ void iov_iter_init(struct iov_iter *i, unsigned int direction,
WARN_ON(direction & ~(READ | WRITE));
*i = (struct iov_iter) {
.iter_type = ITER_IOVEC,
+ .noio = false,
.data_source = direction,
.iov = iov,
.nr_segs = nr_segs,
@@ -1519,13 +1520,17 @@ ssize_t iov_iter_get_pages(struct iov_iter *i,
return 0;
if (likely(iter_is_iovec(i))) {
+ unsigned int gup_flags = 0;
unsigned long addr;
+ if (iov_iter_rw(i) != WRITE)
+ gup_flags |= FOLL_WRITE;
+ if (i->noio)
+ gup_flags |= FOLL_FAST_ONLY;
+
addr = first_iovec_segment(i, &len, start, maxsize, maxpages);
n = DIV_ROUND_UP(len, PAGE_SIZE);
- res = get_user_pages_fast(addr, n,
- iov_iter_rw(i) != WRITE ? FOLL_WRITE : 0,
- pages);
+ res = get_user_pages_fast(addr, n, gup_flags, pages);
if (unlikely(res <= 0))
return res;
return (res == n ? len : res * PAGE_SIZE) - *start;
@@ -1641,15 +1646,20 @@ ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
return 0;
if (likely(iter_is_iovec(i))) {
+ unsigned int gup_flags = 0;
unsigned long addr;
+ if (iov_iter_rw(i) != WRITE)
+ gup_flags |= FOLL_WRITE;
+ if (i->noio)
+ gup_flags |= FOLL_FAST_ONLY;
+
addr = first_iovec_segment(i, &len, start, maxsize, ~0U);
n = DIV_ROUND_UP(len, PAGE_SIZE);
p = get_pages_array(n);
if (!p)
return -ENOMEM;
- res = get_user_pages_fast(addr, n,
- iov_iter_rw(i) != WRITE ? FOLL_WRITE : 0, p);
+ res = get_user_pages_fast(addr, n, gup_flags, p);
if (unlikely(res <= 0)) {
kvfree(p);
*pages = ZERO_SIZE_PTR;
--
2.26.3
In __iomap_dio_rw, when iomap_apply returns an -EFAULT error, complete the
request synchronously and reset the iterator to the start position. This
allows callers to deal with the failure and retry the operation.
In gfs2, we need to disable page faults while we're holding glocks to prevent
deadlocks. This patch is the minimum solution I could find to make
iomap_dio_rw work with page faults disabled. It's still expensive because any
I/O that was carried out before hitting -EFAULT needs to be retried.
A possible improvement would be to add an IOMAP_DIO_FAULT_RETRY or similar flag
that would allow iomap_dio_rw to return a short result when hitting -EFAULT.
Callers could then retry only the rest of the request after dealing with the
page fault.
Asynchronous requests turn into synchronous requests up to the point of the
page fault in any case, but they could be retried asynchronously after dealing
with the page fault. To make that work, the completion notification would have
to include the bytes read or written before the page fault(s) as well, and we'd
need an additional iomap_dio_rw argument for that.
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
fs/iomap/direct-io.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index cc0b4bc8861b..b0a494211bb4 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -561,6 +561,15 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
ret = iomap_apply(inode, pos, count, iomap_flags, ops, dio,
iomap_dio_actor);
if (ret <= 0) {
+ if (ret == -EFAULT) {
+ /*
+ * To allow retrying the request, fail
+ * synchronously and reset the iterator.
+ */
+ wait_for_completion = true;
+ iov_iter_revert(dio->submit.iter, dio->size);
+ }
+
/* magic error code to fall back to buffered I/O */
if (ret == -ENOTBLK) {
wait_for_completion = true;
--
2.26.3
Also disable page faults during direct I/O requests and implement the same kind
of retry logic as in the buffered I/O case.
Direct I/O requests differ from buffered I/O requests in that they use
bio_iov_iter_get_pages for grabbing page references and faulting in pages
instead of triggering real page faults. Those manual page faults can be
disabled with the iocb->noio flag.
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
fs/gfs2/file.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index f66ac7f56f6d..7986f3be69d2 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -782,21 +782,41 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to,
struct file *file = iocb->ki_filp;
struct gfs2_inode *ip = GFS2_I(file->f_mapping->host);
size_t count = iov_iter_count(to);
+ size_t written = 0;
ssize_t ret;
+ /*
+ * In this function, we disable page faults when we're holding the
+ * inode glock while doing I/O. If a page fault occurs, we drop the
+ * inode glock, fault in the pages manually, and then we retry. Other
+ * than in gfs2_file_read_iter, iomap_dio_rw can trigger implicit as
+ * well as manual page faults, and we need to disable both kinds
+ * separately.
+ */
+
if (!count)
return 0; /* skip atime */
gfs2_holder_init(ip->i_gl, LM_ST_DEFERRED, 0, gh);
+retry:
ret = gfs2_glock_nq(gh);
if (ret)
goto out_uninit;
+ pagefault_disable();
+ to->noio = true;
ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL, 0);
+ to->noio = false;
+ pagefault_enable();
+
gfs2_glock_dq(gh);
+ if (ret > 0)
+ written += ret;
+ if (unlikely(ret == -EFAULT) && fault_in_iov_iter(to))
+ goto retry;
out_uninit:
gfs2_holder_uninit(gh);
- return ret;
+ return written ? written : ret;
}
static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from,
@@ -809,6 +829,12 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from,
loff_t offset = iocb->ki_pos;
ssize_t ret;
+ /*
+ * In this function, we disable page faults when we're holding the
+ * inode glock while doing I/O. If a page fault occurs, we drop the
+ * inode glock, fault in the pages manually, and then we retry.
+ */
+
/*
* Deferred lock, even if its a write, since we do no allocation on
* this path. All we need to change is the atime, and this lock mode
@@ -818,6 +844,7 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from,
* VFS does.
*/
gfs2_holder_init(ip->i_gl, LM_ST_DEFERRED, 0, gh);
+retry:
ret = gfs2_glock_nq(gh);
if (ret)
goto out_uninit;
@@ -826,11 +853,16 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from,
if (offset + len > i_size_read(&ip->i_inode))
goto out;
+ from->noio = true;
ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL, 0);
+ from->noio = false;
+
if (ret == -ENOTBLK)
ret = 0;
out:
gfs2_glock_dq(gh);
+ if (unlikely(ret == -EFAULT) && fault_in_iov_iter(from))
+ goto retry;
out_uninit:
gfs2_holder_uninit(gh);
return ret;
--
2.26.3
On Fri 23-07-21 22:58:40, Andreas Gruenbacher wrote:
> Also disable page faults during direct I/O requests and implement the same kind
> of retry logic as in the buffered I/O case.
>
> Direct I/O requests differ from buffered I/O requests in that they use
> bio_iov_iter_get_pages for grabbing page references and faulting in pages
> instead of triggering real page faults. Those manual page faults can be
> disabled with the iocb->noio flag.
>
> Signed-off-by: Andreas Gruenbacher <[email protected]>
> ---
> fs/gfs2/file.c | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index f66ac7f56f6d..7986f3be69d2 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -782,21 +782,41 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to,
> struct file *file = iocb->ki_filp;
> struct gfs2_inode *ip = GFS2_I(file->f_mapping->host);
> size_t count = iov_iter_count(to);
> + size_t written = 0;
> ssize_t ret;
>
> + /*
> + * In this function, we disable page faults when we're holding the
> + * inode glock while doing I/O. If a page fault occurs, we drop the
> + * inode glock, fault in the pages manually, and then we retry. Other
> + * than in gfs2_file_read_iter, iomap_dio_rw can trigger implicit as
> + * well as manual page faults, and we need to disable both kinds
> + * separately.
> + */
> +
> if (!count)
> return 0; /* skip atime */
>
> gfs2_holder_init(ip->i_gl, LM_ST_DEFERRED, 0, gh);
> +retry:
> ret = gfs2_glock_nq(gh);
> if (ret)
> goto out_uninit;
>
> + pagefault_disable();
Is there any use in pagefault_disable() here? iomap_dio_rw() should not
trigger any page faults anyway, should it?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri 23-07-21 22:58:38, Andreas Gruenbacher wrote:
> In __iomap_dio_rw, when iomap_apply returns an -EFAULT error, complete the
> request synchronously and reset the iterator to the start position. This
> allows callers to deal with the failure and retry the operation.
>
> In gfs2, we need to disable page faults while we're holding glocks to prevent
> deadlocks. This patch is the minimum solution I could find to make
> iomap_dio_rw work with page faults disabled. It's still expensive because any
> I/O that was carried out before hitting -EFAULT needs to be retried.
>
> A possible improvement would be to add an IOMAP_DIO_FAULT_RETRY or similar flag
> that would allow iomap_dio_rw to return a short result when hitting -EFAULT.
> Callers could then retry only the rest of the request after dealing with the
> page fault.
>
> Asynchronous requests turn into synchronous requests up to the point of the
> page fault in any case, but they could be retried asynchronously after dealing
> with the page fault. To make that work, the completion notification would have
> to include the bytes read or written before the page fault(s) as well, and we'd
> need an additional iomap_dio_rw argument for that.
>
> Signed-off-by: Andreas Gruenbacher <[email protected]>
> ---
> fs/iomap/direct-io.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index cc0b4bc8861b..b0a494211bb4 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -561,6 +561,15 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> ret = iomap_apply(inode, pos, count, iomap_flags, ops, dio,
> iomap_dio_actor);
> if (ret <= 0) {
> + if (ret == -EFAULT) {
> + /*
> + * To allow retrying the request, fail
> + * synchronously and reset the iterator.
> + */
> + wait_for_completion = true;
> + iov_iter_revert(dio->submit.iter, dio->size);
> + }
> +
Hum, OK, but this means that if userspace submits large enough write, GFS2
will livelock trying to complete it? While other filesystems can just
submit multiple smaller bios constructed in iomap_apply() (paging in
different parts of the buffer) and thus complete the write?
Honza
> /* magic error code to fall back to buffered I/O */
> if (ret == -ENOTBLK) {
> wait_for_completion = true;
> --
> 2.26.3
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon 26-07-21 19:50:23, Andreas Gr?nbacher wrote:
> Jan Kara <[email protected]> schrieb am Mo., 26. Juli 2021, 19:10:
>
> > On Fri 23-07-21 22:58:40, Andreas Gruenbacher wrote:
> > > Also disable page faults during direct I/O requests and implement the
> > same kind
> > > of retry logic as in the buffered I/O case.
> > >
> > > Direct I/O requests differ from buffered I/O requests in that they use
> > > bio_iov_iter_get_pages for grabbing page references and faulting in pages
> > > instead of triggering real page faults. Those manual page faults can be
> > > disabled with the iocb->noio flag.
> > >
> > > Signed-off-by: Andreas Gruenbacher <[email protected]>
> > > ---
> > > fs/gfs2/file.c | 34 +++++++++++++++++++++++++++++++++-
> > > 1 file changed, 33 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> > > index f66ac7f56f6d..7986f3be69d2 100644
> > > --- a/fs/gfs2/file.c
> > > +++ b/fs/gfs2/file.c
> > > @@ -782,21 +782,41 @@ static ssize_t gfs2_file_direct_read(struct kiocb
> > *iocb, struct iov_iter *to,
> > > struct file *file = iocb->ki_filp;
> > > struct gfs2_inode *ip = GFS2_I(file->f_mapping->host);
> > > size_t count = iov_iter_count(to);
> > > + size_t written = 0;
> > > ssize_t ret;
> > >
> > > + /*
> > > + * In this function, we disable page faults when we're holding the
> > > + * inode glock while doing I/O. If a page fault occurs, we drop
> > the
> > > + * inode glock, fault in the pages manually, and then we retry.
> > Other
> > > + * than in gfs2_file_read_iter, iomap_dio_rw can trigger implicit
> > as
> > > + * well as manual page faults, and we need to disable both kinds
> > > + * separately.
> > > + */
> > > +
> > > if (!count)
> > > return 0; /* skip atime */
> > >
> > > gfs2_holder_init(ip->i_gl, LM_ST_DEFERRED, 0, gh);
> > > +retry:
> > > ret = gfs2_glock_nq(gh);
> > > if (ret)
> > > goto out_uninit;
> > >
> > > + pagefault_disable();
> >
> > Is there any use in pagefault_disable() here? iomap_dio_rw() should not
> > trigger any page faults anyway, should it?
> >
>
> It can trigger physical page faults when reading from holes.
Aha, good point. Maybe even worth a comment at this site? Thanks for
explanation!
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon 26-07-21 19:45:22, Andreas Gr?nbacher wrote:
> Jan Kara <[email protected]> schrieb am Mo., 26. Juli 2021, 19:21:
>
> > On Fri 23-07-21 22:58:38, Andreas Gruenbacher wrote:
> > > In __iomap_dio_rw, when iomap_apply returns an -EFAULT error, complete
> > the
> > > request synchronously and reset the iterator to the start position. This
> > > allows callers to deal with the failure and retry the operation.
> > >
> > > In gfs2, we need to disable page faults while we're holding glocks to
> > prevent
> > > deadlocks. This patch is the minimum solution I could find to make
> > > iomap_dio_rw work with page faults disabled. It's still expensive
> > because any
> > > I/O that was carried out before hitting -EFAULT needs to be retried.
> > >
> > > A possible improvement would be to add an IOMAP_DIO_FAULT_RETRY or
> > similar flag
> > > that would allow iomap_dio_rw to return a short result when hitting
> > -EFAULT.
> > > Callers could then retry only the rest of the request after dealing with
> > the
> > > page fault.
> > >
> > > Asynchronous requests turn into synchronous requests up to the point of
> > the
> > > page fault in any case, but they could be retried asynchronously after
> > dealing
> > > with the page fault. To make that work, the completion notification
> > would have
> > > to include the bytes read or written before the page fault(s) as well,
> > and we'd
> > > need an additional iomap_dio_rw argument for that.
> > >
> > > Signed-off-by: Andreas Gruenbacher <[email protected]>
> > > ---
> > > fs/iomap/direct-io.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > > index cc0b4bc8861b..b0a494211bb4 100644
> > > --- a/fs/iomap/direct-io.c
> > > +++ b/fs/iomap/direct-io.c
> > > @@ -561,6 +561,15 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter
> > *iter,
> > > ret = iomap_apply(inode, pos, count, iomap_flags, ops, dio,
> > > iomap_dio_actor);
> > > if (ret <= 0) {
> > > + if (ret == -EFAULT) {
> > > + /*
> > > + * To allow retrying the request, fail
> > > + * synchronously and reset the iterator.
> > > + */
> > > + wait_for_completion = true;
> > > + iov_iter_revert(dio->submit.iter,
> > dio->size);
> > > + }
> > > +
> >
> > Hum, OK, but this means that if userspace submits large enough write, GFS2
> > will livelock trying to complete it? While other filesystems can just
> > submit multiple smaller bios constructed in iomap_apply() (paging in
> > different parts of the buffer) and thus complete the write?
> >
>
> No. First, this affects reads but not writes. We cannot just blindly repeat
> writes; when a page fault occurs in the middle of a write, the result will
> be a short write. For reads, the plan is to ads a flag to allow
> iomap_dio_rw to return a partial result when a page fault occurs.
> (Currently, it fails the entire request.) Then we can handle the page fault
> and complete the rest of the request.
>
> The changes needed for that are simple on the iomap side, but we need to go
> through some gymnastics for handling the page fault without giving up the
> glock in the non-contended case. There will still be the potential for
> losing the lock and having to re-acquire it, in which case we'll actually
> have to repeat the entire read.
I've missed you've already sent out v4 (I'm catching up after vacation so
my mailbox is a bit of a mess). What's in there addresses my objection.
I'm sorry for the noise.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR