| Message ID | 1499912818-16882-1-git-send-email-ashish.samant@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Miklos, Is this version ok? Thanks, Ashish On 07/12/2017 07:26 PM, Ashish Samant wrote: > 'Commit 8fba54aebbdf ("fuse: direct-io: don't dirty ITER_BVEC pages")' > fixes the ITER_BVEC page deadlock for direct io in fuse by checking in > fuse_direct_io(), whether the page is a bvec page or not, before locking > it. However, this check is missed when the "async_dio" mount option is > enabled. In this case, set_page_dirty_lock() is called from the req->end > callback in request_end(), when the fuse thread is returning from > userspace to respond to the read request. This will cause the same > deadlock because the bvec condition is not checked in this path. > > Here is the stack of the deadlocked thread, while returning from userspace: > > [13706.656686] INFO: task glusterfs:3006 blocked for more than 120 seconds. > [13706.657808] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables > this message. > [13706.658788] glusterfs D ffffffff816c80f0 0 3006 1 > 0x00000080 > [13706.658797] ffff8800d6713a58 0000000000000086 ffff8800d9ad7000 > ffff8800d9ad5400 > [13706.658799] ffff88011ffd5cc0 ffff8800d6710008 ffff88011fd176c0 > 7fffffffffffffff > [13706.658801] 0000000000000002 ffffffff816c80f0 ffff8800d6713a78 > ffffffff816c790e > [13706.658803] Call Trace: > [13706.658809] [<ffffffff816c80f0>] ? bit_wait_io_timeout+0x80/0x80 > [13706.658811] [<ffffffff816c790e>] schedule+0x3e/0x90 > [13706.658813] [<ffffffff816ca7e5>] schedule_timeout+0x1b5/0x210 > [13706.658816] [<ffffffff81073ffb>] ? gup_pud_range+0x1db/0x1f0 > [13706.658817] [<ffffffff810668fe>] ? kvm_clock_read+0x1e/0x20 > [13706.658819] [<ffffffff81066909>] ? kvm_clock_get_cycles+0x9/0x10 > [13706.658822] [<ffffffff810f5792>] ? ktime_get+0x52/0xc0 > [13706.658824] [<ffffffff816c6f04>] io_schedule_timeout+0xa4/0x110 > [13706.658826] [<ffffffff816c8126>] bit_wait_io+0x36/0x50 > [13706.658828] [<ffffffff816c7d06>] __wait_on_bit_lock+0x76/0xb0 > [13706.658831] [<ffffffffa0545636>] ? lock_request+0x46/0x70 [fuse] > [13706.658834] [<ffffffff8118800a>] __lock_page+0xaa/0xb0 > [13706.658836] [<ffffffff810c8500>] ? wake_atomic_t_function+0x40/0x40 > [13706.658838] [<ffffffff81194d08>] set_page_dirty_lock+0x58/0x60 > [13706.658841] [<ffffffffa054d968>] fuse_release_user_pages+0x58/0x70 [fuse] > [13706.658844] [<ffffffffa0551430>] ? fuse_aio_complete+0x190/0x190 [fuse] > [13706.658847] [<ffffffffa0551459>] fuse_aio_complete_req+0x29/0x90 [fuse] > [13706.658849] [<ffffffffa05471e9>] request_end+0xd9/0x190 [fuse] > [13706.658852] [<ffffffffa0549126>] fuse_dev_do_write+0x336/0x490 [fuse] > [13706.658854] [<ffffffffa054963e>] fuse_dev_write+0x6e/0xa0 [fuse] > [13706.658857] [<ffffffff812a9ef3>] ? security_file_permission+0x23/0x90 > [13706.658859] [<ffffffff81205300>] do_iter_readv_writev+0x60/0x90 > [13706.658862] [<ffffffffa05495d0>] ? fuse_dev_splice_write+0x350/0x350 > [fuse] > [13706.658863] [<ffffffff812062a1>] do_readv_writev+0x171/0x1f0 > [13706.658866] [<ffffffff810b3d00>] ? try_to_wake_up+0x210/0x210 > [13706.658868] [<ffffffff81206361>] vfs_writev+0x41/0x50 > [13706.658870] [<ffffffff81206496>] SyS_writev+0x56/0xf0 > [13706.658872] [<ffffffff810257a1>] ? syscall_trace_leave+0xf1/0x160 > [13706.658874] [<ffffffff816cbb2e>] system_call_fastpath+0x12/0x71 > > Fix this by making should_dirty a fuse_io_priv parametero that it can be > checked in fuse_aio_complete_req(). > > Reported-by: Tiger Yang <tiger.yang@oracle.com> > Signed-off-by: Ashish Samant <ashish.samant@oracle.com> > --- > fs/fuse/file.c | 8 +++++--- > fs/fuse/fuse_i.h | 2 ++ > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 3ee4fdc..e865ac7 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -608,8 +608,9 @@ static void fuse_aio_complete_req(struct fuse_conn *fc, struct fuse_req *req) > { > struct fuse_io_priv *io = req->io; > ssize_t pos = -1; > + bool should_dirty = io->should_dirty && !io->write; > > - fuse_release_user_pages(req, !io->write); > + fuse_release_user_pages(req, should_dirty); > > if (io->write) { > if (req->misc.write.in.size != req->misc.write.out.size) > @@ -1316,7 +1317,6 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, > loff_t *ppos, int flags) > { > int write = flags & FUSE_DIO_WRITE; > - bool should_dirty = !write && iter_is_iovec(iter); > int cuse = flags & FUSE_DIO_CUSE; > struct file *file = io->file; > struct inode *inode = file->f_mapping->host; > @@ -1346,6 +1346,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, > inode_unlock(inode); > } > > + io->should_dirty = !write && iter_is_iovec(iter); > while (count) { > size_t nres; > fl_owner_t owner = current->files; > @@ -1360,7 +1361,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, > nres = fuse_send_read(req, io, pos, nbytes, owner); > > if (!io->async) > - fuse_release_user_pages(req, should_dirty); > + fuse_release_user_pages(req, io->should_dirty); > if (req->out.h.error) { > err = req->out.h.error; > break; > @@ -2872,6 +2873,7 @@ static inline loff_t fuse_round_up(loff_t off) > io->size = 0; > io->offset = offset; > io->write = (iov_iter_rw(iter) == WRITE); > + io->should_dirty = true; > io->err = 0; > io->file = file; > /* > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index 1bd7ffd..2ef205e 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -249,6 +249,7 @@ struct fuse_io_priv { > size_t size; > __u64 offset; > bool write; > + bool should_dirty; > int err; > struct kiocb *iocb; > struct file *file; > @@ -261,6 +262,7 @@ struct fuse_io_priv { > .refcnt = KREF_INIT(1), \ > .async = 0, \ > .file = f, \ > + .should_dirty = true, \ > } > > /**
On Thu, Jul 27, 2017 at 1:06 AM, Ashish Samant <ashish.samant@oracle.com> wrote: > Hi Miklos, > > Is this version ok? Hi, Thanks for the patch. Cleaned up variant pushed to for-next branch in my fuse tree. Please check if it's still OK. Thanks, Miklos
On 08/03/2017 08:59 AM, Miklos Szeredi wrote: > On Thu, Jul 27, 2017 at 1:06 AM, Ashish Samant <ashish.samant@oracle.com> wrote: >> Hi Miklos, >> >> Is this version ok? > Hi, > > Thanks for the patch. Cleaned up variant pushed to for-next branch in > my fuse tree. Please check if it's still OK. Hi Miklos, Yes, the patch in your tree looks ok. Thanks, Ashish > > Thanks, > Miklos
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 3ee4fdc..e865ac7 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -608,8 +608,9 @@ static void fuse_aio_complete_req(struct fuse_conn *fc, struct fuse_req *req) { struct fuse_io_priv *io = req->io; ssize_t pos = -1; + bool should_dirty = io->should_dirty && !io->write; - fuse_release_user_pages(req, !io->write); + fuse_release_user_pages(req, should_dirty); if (io->write) { if (req->misc.write.in.size != req->misc.write.out.size) @@ -1316,7 +1317,6 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, loff_t *ppos, int flags) { int write = flags & FUSE_DIO_WRITE; - bool should_dirty = !write && iter_is_iovec(iter); int cuse = flags & FUSE_DIO_CUSE; struct file *file = io->file; struct inode *inode = file->f_mapping->host; @@ -1346,6 +1346,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, inode_unlock(inode); } + io->should_dirty = !write && iter_is_iovec(iter); while (count) { size_t nres; fl_owner_t owner = current->files; @@ -1360,7 +1361,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, nres = fuse_send_read(req, io, pos, nbytes, owner); if (!io->async) - fuse_release_user_pages(req, should_dirty); + fuse_release_user_pages(req, io->should_dirty); if (req->out.h.error) { err = req->out.h.error; break; @@ -2872,6 +2873,7 @@ static inline loff_t fuse_round_up(loff_t off) io->size = 0; io->offset = offset; io->write = (iov_iter_rw(iter) == WRITE); + io->should_dirty = true; io->err = 0; io->file = file; /* diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 1bd7ffd..2ef205e 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -249,6 +249,7 @@ struct fuse_io_priv { size_t size; __u64 offset; bool write; + bool should_dirty; int err; struct kiocb *iocb; struct file *file; @@ -261,6 +262,7 @@ struct fuse_io_priv { .refcnt = KREF_INIT(1), \ .async = 0, \ .file = f, \ + .should_dirty = true, \ } /**
'Commit 8fba54aebbdf ("fuse: direct-io: don't dirty ITER_BVEC pages")' fixes the ITER_BVEC page deadlock for direct io in fuse by checking in fuse_direct_io(), whether the page is a bvec page or not, before locking it. However, this check is missed when the "async_dio" mount option is enabled. In this case, set_page_dirty_lock() is called from the req->end callback in request_end(), when the fuse thread is returning from userspace to respond to the read request. This will cause the same deadlock because the bvec condition is not checked in this path. Here is the stack of the deadlocked thread, while returning from userspace: [13706.656686] INFO: task glusterfs:3006 blocked for more than 120 seconds. [13706.657808] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [13706.658788] glusterfs D ffffffff816c80f0 0 3006 1 0x00000080 [13706.658797] ffff8800d6713a58 0000000000000086 ffff8800d9ad7000 ffff8800d9ad5400 [13706.658799] ffff88011ffd5cc0 ffff8800d6710008 ffff88011fd176c0 7fffffffffffffff [13706.658801] 0000000000000002 ffffffff816c80f0 ffff8800d6713a78 ffffffff816c790e [13706.658803] Call Trace: [13706.658809] [<ffffffff816c80f0>] ? bit_wait_io_timeout+0x80/0x80 [13706.658811] [<ffffffff816c790e>] schedule+0x3e/0x90 [13706.658813] [<ffffffff816ca7e5>] schedule_timeout+0x1b5/0x210 [13706.658816] [<ffffffff81073ffb>] ? gup_pud_range+0x1db/0x1f0 [13706.658817] [<ffffffff810668fe>] ? kvm_clock_read+0x1e/0x20 [13706.658819] [<ffffffff81066909>] ? kvm_clock_get_cycles+0x9/0x10 [13706.658822] [<ffffffff810f5792>] ? ktime_get+0x52/0xc0 [13706.658824] [<ffffffff816c6f04>] io_schedule_timeout+0xa4/0x110 [13706.658826] [<ffffffff816c8126>] bit_wait_io+0x36/0x50 [13706.658828] [<ffffffff816c7d06>] __wait_on_bit_lock+0x76/0xb0 [13706.658831] [<ffffffffa0545636>] ? lock_request+0x46/0x70 [fuse] [13706.658834] [<ffffffff8118800a>] __lock_page+0xaa/0xb0 [13706.658836] [<ffffffff810c8500>] ? wake_atomic_t_function+0x40/0x40 [13706.658838] [<ffffffff81194d08>] set_page_dirty_lock+0x58/0x60 [13706.658841] [<ffffffffa054d968>] fuse_release_user_pages+0x58/0x70 [fuse] [13706.658844] [<ffffffffa0551430>] ? fuse_aio_complete+0x190/0x190 [fuse] [13706.658847] [<ffffffffa0551459>] fuse_aio_complete_req+0x29/0x90 [fuse] [13706.658849] [<ffffffffa05471e9>] request_end+0xd9/0x190 [fuse] [13706.658852] [<ffffffffa0549126>] fuse_dev_do_write+0x336/0x490 [fuse] [13706.658854] [<ffffffffa054963e>] fuse_dev_write+0x6e/0xa0 [fuse] [13706.658857] [<ffffffff812a9ef3>] ? security_file_permission+0x23/0x90 [13706.658859] [<ffffffff81205300>] do_iter_readv_writev+0x60/0x90 [13706.658862] [<ffffffffa05495d0>] ? fuse_dev_splice_write+0x350/0x350 [fuse] [13706.658863] [<ffffffff812062a1>] do_readv_writev+0x171/0x1f0 [13706.658866] [<ffffffff810b3d00>] ? try_to_wake_up+0x210/0x210 [13706.658868] [<ffffffff81206361>] vfs_writev+0x41/0x50 [13706.658870] [<ffffffff81206496>] SyS_writev+0x56/0xf0 [13706.658872] [<ffffffff810257a1>] ? syscall_trace_leave+0xf1/0x160 [13706.658874] [<ffffffff816cbb2e>] system_call_fastpath+0x12/0x71 Fix this by making should_dirty a fuse_io_priv parametero that it can be checked in fuse_aio_complete_req(). Reported-by: Tiger Yang <tiger.yang@oracle.com> Signed-off-by: Ashish Samant <ashish.samant@oracle.com> --- fs/fuse/file.c | 8 +++++--- fs/fuse/fuse_i.h | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-)