* [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing @ 2022-10-31 11:43 fdmanana 2022-10-31 11:43 ` [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb fdmanana ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: fdmanana @ 2022-10-31 11:43 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> This fixes direct IO writes with nowait and dsync not getting synced after the writes complete (except when we fallback to blocking context). The first patch is the fix, while the second one only updates a comment that is now stale after 6.1-rc1. Patch only applies to current misc-next, because a function prototype declaration was recently moved out of ctree.h into btrfs_inode.h. I left a version that applies cleanly against 6.0.6 and 6.1-rc at: https://git.kernel.org/pub/scm/linux/kernel/git/fdmanana/linux.git/log/?h=async_dio_fsync_fix_6.1_6.0 Filipe Manana (2): btrfs: fix lost file sync on direct IO write with nowait and dsync iocb btrfs: update stale comment for nowait direct IO writes fs/btrfs/btrfs_inode.h | 5 ++++- fs/btrfs/file.c | 26 ++++++++++++++++++-------- fs/btrfs/inode.c | 14 +++++++++++--- 3 files changed, 33 insertions(+), 12 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb 2022-10-31 11:43 [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing fdmanana @ 2022-10-31 11:43 ` fdmanana 2022-11-01 0:49 ` Wang Yugui 2022-10-31 11:43 ` [PATCH 2/2] btrfs: update stale comment for nowait direct IO writes fdmanana 2022-10-31 16:26 ` [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing David Sterba 2 siblings, 1 reply; 8+ messages in thread From: fdmanana @ 2022-10-31 11:43 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> When doing a direct IO write using a iocb with nowait and dsync set, we end up not syncing the file once the write completes. This is because we tell iomap to not call generic_write_sync(), which would result in calling btrfs_sync_file(), in order to avoid a deadlock since iomap can call it while we are holding the inode's lock and btrfs_sync_file() needs to acquire the inode's lock. The deadlock happens only if the write happens synchronously, when iomap_dio_rw() calls iomap_dio_complete() before it returns. Instead we do the sync ourselves at btrfs_do_write_iter(). For a nowait write however we can end up not doing the sync ourselves at at btrfs_do_write_iter() because the write could have been queued, and therefore we get -EIOCBQUEUED returned from iomap in such case. That makes us skip the sync call at btrfs_do_write_iter(), as we don't do it for any error returned from btrfs_direct_write(). We can't simply do the call even if -EIOCBQUEUED is returned, since that would block the task waiting for IO, both for the data since there are bios still in progress as well as potentially blocking when joining a log transaction and when syncing the log (writing log trees, super blocks, etc). So let iomap do the sync call itself and in order to avoid deadlocks for the case of synchronous writes (without nowait), use __iomap_dio_rw() and have ourselves call iomap_dio_complete() after unlocking the inode. A test case will later be sent for fstests, after this is fixed in Linus' tree. Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes") Reported-by: Марк Коренберг <socketpair@gmail.com> Link: https://lore.kernel.org/linux-btrfs/CAEmTpZGRKbzc16fWPvxbr6AfFsQoLmz-Lcg-7OgJOZDboJ+SGQ@mail.gmail.com/ CC: stable@vger.kernel.org # 6.0+ Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/btrfs_inode.h | 5 ++++- fs/btrfs/file.c | 22 ++++++++++++++++------ fs/btrfs/inode.c | 14 +++++++++++--- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index 79a9f06c2434..d21c30bf7053 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -526,7 +526,10 @@ ssize_t btrfs_encoded_read(struct kiocb *iocb, struct iov_iter *iter, ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from, const struct btrfs_ioctl_encoded_io_args *encoded); -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before); +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, + size_t done_before); +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, + size_t done_before); extern const struct dentry_operations btrfs_dentry_operations; diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 838b3c0ea329..6e2889bc73d8 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1453,6 +1453,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) loff_t endbyte; ssize_t err; unsigned int ilock_flags = 0; + struct iomap_dio *dio; if (iocb->ki_flags & IOCB_NOWAIT) ilock_flags |= BTRFS_ILOCK_TRY; @@ -1513,11 +1514,22 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) * So here we disable page faults in the iov_iter and then retry if we * got -EFAULT, faulting in the pages before the retry. */ -again: from->nofault = true; - err = btrfs_dio_rw(iocb, from, written); + dio = btrfs_dio_write(iocb, from, written); from->nofault = false; + /* + * iomap_dio_complete() will call btrfs_sync_file() if we have a dsync + * iocb, and that needs to lock the inode. So unlock it before calling + * iomap_dio_complete() to avoid a deadlock. + */ + btrfs_inode_unlock(inode, ilock_flags); + + if (IS_ERR_OR_NULL(dio)) + err = PTR_ERR_OR_ZERO(dio); + else + err = iomap_dio_complete(dio); + /* No increment (+=) because iomap returns a cumulative value. */ if (err > 0) written = err; @@ -1543,12 +1555,10 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) } else { fault_in_iov_iter_readable(from, left); prev_left = left; - goto again; + goto relock; } } - btrfs_inode_unlock(inode, ilock_flags); - /* * If 'err' is -ENOTBLK or we have not written all data, then it means * we must fallback to buffered IO. @@ -3752,7 +3762,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) */ pagefault_disable(); to->nofault = true; - ret = btrfs_dio_rw(iocb, to, read); + ret = btrfs_dio_read(iocb, to, read); to->nofault = false; pagefault_enable(); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index fd26282edace..b1e7bfd5f525 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8176,13 +8176,21 @@ static const struct iomap_dio_ops btrfs_dio_ops = { .bio_set = &btrfs_dio_bioset, }; -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) { struct btrfs_dio_data data; return iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, - IOMAP_DIO_PARTIAL | IOMAP_DIO_NOSYNC, - &data, done_before); + IOMAP_DIO_PARTIAL, &data, done_before); +} + +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, + size_t done_before) +{ + struct btrfs_dio_data data; + + return __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, + IOMAP_DIO_PARTIAL, &data, done_before); } static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, -- 2.35.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb 2022-10-31 11:43 ` [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb fdmanana @ 2022-11-01 0:49 ` Wang Yugui 2022-11-01 1:04 ` Wang Yugui 2022-11-01 7:48 ` Filipe Manana 0 siblings, 2 replies; 8+ messages in thread From: Wang Yugui @ 2022-11-01 0:49 UTC (permalink / raw) To: fdmanana; +Cc: linux-btrfs Hi, > From: Filipe Manana <fdmanana@suse.com> > > When doing a direct IO write using a iocb with nowait and dsync set, we > end up not syncing the file once the write completes. > > This is because we tell iomap to not call generic_write_sync(), which > would result in calling btrfs_sync_file(), in order to avoid a deadlock > since iomap can call it while we are holding the inode's lock and > btrfs_sync_file() needs to acquire the inode's lock. The deadlock happens > only if the write happens synchronously, when iomap_dio_rw() calls > iomap_dio_complete() before it returns. Instead we do the sync ourselves > at btrfs_do_write_iter(). > > For a nowait write however we can end up not doing the sync ourselves at > at btrfs_do_write_iter() because the write could have been queued, and > therefore we get -EIOCBQUEUED returned from iomap in such case. That makes > us skip the sync call at btrfs_do_write_iter(), as we don't do it for > any error returned from btrfs_direct_write(). We can't simply do the call > even if -EIOCBQUEUED is returned, since that would block the task waiting > for IO, both for the data since there are bios still in progress as well > as potentially blocking when joining a log transaction and when syncing > the log (writing log trees, super blocks, etc). > > So let iomap do the sync call itself and in order to avoid deadlocks for > the case of synchronous writes (without nowait), use __iomap_dio_rw() and > have ourselves call iomap_dio_complete() after unlocking the inode. > > A test case will later be sent for fstests, after this is fixed in Linus' > tree. > > Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes") > Reported-by: Марк Коренберг <socketpair@gmail.com> > Link: https://lore.kernel.org/linux-btrfs/CAEmTpZGRKbzc16fWPvxbr6AfFsQoLmz-Lcg-7OgJOZDboJ+SGQ@mail.gmail.com/ > CC: stable@vger.kernel.org # 6.0+ The test script provided by Марк Коренберг <socketpair@gmail.com> show a normal result on 6.1.0-rc3+ with this patch. but the test script provided by Марк Коренберг show that there is a problem in 5.15.y too. we need a different fix for 5.15.y? Best Regards Wang Yugui (wangyugui@e16-tech.com) 2022/11/01 > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/btrfs_inode.h | 5 ++++- > fs/btrfs/file.c | 22 ++++++++++++++++------ > fs/btrfs/inode.c | 14 +++++++++++--- > 3 files changed, 31 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > index 79a9f06c2434..d21c30bf7053 100644 > --- a/fs/btrfs/btrfs_inode.h > +++ b/fs/btrfs/btrfs_inode.h > @@ -526,7 +526,10 @@ ssize_t btrfs_encoded_read(struct kiocb *iocb, struct iov_iter *iter, > ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from, > const struct btrfs_ioctl_encoded_io_args *encoded); > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before); > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, > + size_t done_before); > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > + size_t done_before); > > extern const struct dentry_operations btrfs_dentry_operations; > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index 838b3c0ea329..6e2889bc73d8 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -1453,6 +1453,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > loff_t endbyte; > ssize_t err; > unsigned int ilock_flags = 0; > + struct iomap_dio *dio; > > if (iocb->ki_flags & IOCB_NOWAIT) > ilock_flags |= BTRFS_ILOCK_TRY; > @@ -1513,11 +1514,22 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > * So here we disable page faults in the iov_iter and then retry if we > * got -EFAULT, faulting in the pages before the retry. > */ > -again: > from->nofault = true; > - err = btrfs_dio_rw(iocb, from, written); > + dio = btrfs_dio_write(iocb, from, written); > from->nofault = false; > > + /* > + * iomap_dio_complete() will call btrfs_sync_file() if we have a dsync > + * iocb, and that needs to lock the inode. So unlock it before calling > + * iomap_dio_complete() to avoid a deadlock. > + */ > + btrfs_inode_unlock(inode, ilock_flags); > + > + if (IS_ERR_OR_NULL(dio)) > + err = PTR_ERR_OR_ZERO(dio); > + else > + err = iomap_dio_complete(dio); > + > /* No increment (+=) because iomap returns a cumulative value. */ > if (err > 0) > written = err; > @@ -1543,12 +1555,10 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > } else { > fault_in_iov_iter_readable(from, left); > prev_left = left; > - goto again; > + goto relock; > } > } > > - btrfs_inode_unlock(inode, ilock_flags); > - > /* > * If 'err' is -ENOTBLK or we have not written all data, then it means > * we must fallback to buffered IO. > @@ -3752,7 +3762,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) > */ > pagefault_disable(); > to->nofault = true; > - ret = btrfs_dio_rw(iocb, to, read); > + ret = btrfs_dio_read(iocb, to, read); > to->nofault = false; > pagefault_enable(); > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index fd26282edace..b1e7bfd5f525 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -8176,13 +8176,21 @@ static const struct iomap_dio_ops btrfs_dio_ops = { > .bio_set = &btrfs_dio_bioset, > }; > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > { > struct btrfs_dio_data data; > > return iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > - IOMAP_DIO_PARTIAL | IOMAP_DIO_NOSYNC, > - &data, done_before); > + IOMAP_DIO_PARTIAL, &data, done_before); > +} > + > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > + size_t done_before) > +{ > + struct btrfs_dio_data data; > + > + return __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > + IOMAP_DIO_PARTIAL, &data, done_before); > } > > static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > -- > 2.35.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb 2022-11-01 0:49 ` Wang Yugui @ 2022-11-01 1:04 ` Wang Yugui 2022-11-01 7:53 ` Filipe Manana 2022-11-01 7:48 ` Filipe Manana 1 sibling, 1 reply; 8+ messages in thread From: Wang Yugui @ 2022-11-01 1:04 UTC (permalink / raw) To: fdmanana, linux-btrfs Hi, > Hi, > > > From: Filipe Manana <fdmanana@suse.com> > > > > When doing a direct IO write using a iocb with nowait and dsync set, we > > end up not syncing the file once the write completes. > > > > This is because we tell iomap to not call generic_write_sync(), which > > would result in calling btrfs_sync_file(), in order to avoid a deadlock > > since iomap can call it while we are holding the inode's lock and > > btrfs_sync_file() needs to acquire the inode's lock. The deadlock happens > > only if the write happens synchronously, when iomap_dio_rw() calls > > iomap_dio_complete() before it returns. Instead we do the sync ourselves > > at btrfs_do_write_iter(). > > > > For a nowait write however we can end up not doing the sync ourselves at > > at btrfs_do_write_iter() because the write could have been queued, and > > therefore we get -EIOCBQUEUED returned from iomap in such case. That makes > > us skip the sync call at btrfs_do_write_iter(), as we don't do it for > > any error returned from btrfs_direct_write(). We can't simply do the call > > even if -EIOCBQUEUED is returned, since that would block the task waiting > > for IO, both for the data since there are bios still in progress as well > > as potentially blocking when joining a log transaction and when syncing > > the log (writing log trees, super blocks, etc). > > > > So let iomap do the sync call itself and in order to avoid deadlocks for > > the case of synchronous writes (without nowait), use __iomap_dio_rw() and > > have ourselves call iomap_dio_complete() after unlocking the inode. > > > > A test case will later be sent for fstests, after this is fixed in Linus' > > tree. > > > > Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes") > > Reported-by: Марк Коренберг <socketpair@gmail.com> > > Link: https://lore.kernel.org/linux-btrfs/CAEmTpZGRKbzc16fWPvxbr6AfFsQoLmz-Lcg-7OgJOZDboJ+SGQ@mail.gmail.com/ > > CC: stable@vger.kernel.org # 6.0+ > > The test script provided by Марк Коренберг <socketpair@gmail.com> > show a normal result on 6.1.0-rc3+ with this patch. > > but the test script provided by Марк Коренберг show that > there is a problem in 5.15.y too. we need a different fix for 5.15.y? a dirty fix will let the test script provided by Марк Коренбе show a normal result on 5.15.y. but I don't know whether this dirty fix is right. diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index bd05a8f..42b2d20 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -2109,7 +2109,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb, btrfs_set_inode_last_sub_trans(inode); - if (num_written > 0) + if (num_written > 0 || sync) num_written = generic_write_sync(iocb, num_written); if (sync) Best Regards Wang Yugui (wangyugui@e16-tech.com) 2022/11/01 > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/btrfs_inode.h | 5 ++++- > > fs/btrfs/file.c | 22 ++++++++++++++++------ > > fs/btrfs/inode.c | 14 +++++++++++--- > > 3 files changed, 31 insertions(+), 10 deletions(-) > > > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > > index 79a9f06c2434..d21c30bf7053 100644 > > --- a/fs/btrfs/btrfs_inode.h > > +++ b/fs/btrfs/btrfs_inode.h > > @@ -526,7 +526,10 @@ ssize_t btrfs_encoded_read(struct kiocb *iocb, struct iov_iter *iter, > > ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from, > > const struct btrfs_ioctl_encoded_io_args *encoded); > > > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before); > > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, > > + size_t done_before); > > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > > + size_t done_before); > > > > extern const struct dentry_operations btrfs_dentry_operations; > > > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > > index 838b3c0ea329..6e2889bc73d8 100644 > > --- a/fs/btrfs/file.c > > +++ b/fs/btrfs/file.c > > @@ -1453,6 +1453,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > loff_t endbyte; > > ssize_t err; > > unsigned int ilock_flags = 0; > > + struct iomap_dio *dio; > > > > if (iocb->ki_flags & IOCB_NOWAIT) > > ilock_flags |= BTRFS_ILOCK_TRY; > > @@ -1513,11 +1514,22 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > * So here we disable page faults in the iov_iter and then retry if we > > * got -EFAULT, faulting in the pages before the retry. > > */ > > -again: > > from->nofault = true; > > - err = btrfs_dio_rw(iocb, from, written); > > + dio = btrfs_dio_write(iocb, from, written); > > from->nofault = false; > > > > + /* > > + * iomap_dio_complete() will call btrfs_sync_file() if we have a dsync > > + * iocb, and that needs to lock the inode. So unlock it before calling > > + * iomap_dio_complete() to avoid a deadlock. > > + */ > > + btrfs_inode_unlock(inode, ilock_flags); > > + > > + if (IS_ERR_OR_NULL(dio)) > > + err = PTR_ERR_OR_ZERO(dio); > > + else > > + err = iomap_dio_complete(dio); > > + > > /* No increment (+=) because iomap returns a cumulative value. */ > > if (err > 0) > > written = err; > > @@ -1543,12 +1555,10 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > } else { > > fault_in_iov_iter_readable(from, left); > > prev_left = left; > > - goto again; > > + goto relock; > > } > > } > > > > - btrfs_inode_unlock(inode, ilock_flags); > > - > > /* > > * If 'err' is -ENOTBLK or we have not written all data, then it means > > * we must fallback to buffered IO. > > @@ -3752,7 +3762,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) > > */ > > pagefault_disable(); > > to->nofault = true; > > - ret = btrfs_dio_rw(iocb, to, read); > > + ret = btrfs_dio_read(iocb, to, read); > > to->nofault = false; > > pagefault_enable(); > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index fd26282edace..b1e7bfd5f525 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -8176,13 +8176,21 @@ static const struct iomap_dio_ops btrfs_dio_ops = { > > .bio_set = &btrfs_dio_bioset, > > }; > > > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > > { > > struct btrfs_dio_data data; > > > > return iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > > - IOMAP_DIO_PARTIAL | IOMAP_DIO_NOSYNC, > > - &data, done_before); > > + IOMAP_DIO_PARTIAL, &data, done_before); > > +} > > + > > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > > + size_t done_before) > > +{ > > + struct btrfs_dio_data data; > > + > > + return __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > > + IOMAP_DIO_PARTIAL, &data, done_before); > > } > > > > static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > -- > > 2.35.1 > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb 2022-11-01 1:04 ` Wang Yugui @ 2022-11-01 7:53 ` Filipe Manana 0 siblings, 0 replies; 8+ messages in thread From: Filipe Manana @ 2022-11-01 7:53 UTC (permalink / raw) To: Wang Yugui; +Cc: linux-btrfs On Tue, Nov 1, 2022 at 1:31 AM Wang Yugui <wangyugui@e16-tech.com> wrote: > > Hi, > > > Hi, > > > > > From: Filipe Manana <fdmanana@suse.com> > > > > > > When doing a direct IO write using a iocb with nowait and dsync set, we > > > end up not syncing the file once the write completes. > > > > > > This is because we tell iomap to not call generic_write_sync(), which > > > would result in calling btrfs_sync_file(), in order to avoid a deadlock > > > since iomap can call it while we are holding the inode's lock and > > > btrfs_sync_file() needs to acquire the inode's lock. The deadlock happens > > > only if the write happens synchronously, when iomap_dio_rw() calls > > > iomap_dio_complete() before it returns. Instead we do the sync ourselves > > > at btrfs_do_write_iter(). > > > > > > For a nowait write however we can end up not doing the sync ourselves at > > > at btrfs_do_write_iter() because the write could have been queued, and > > > therefore we get -EIOCBQUEUED returned from iomap in such case. That makes > > > us skip the sync call at btrfs_do_write_iter(), as we don't do it for > > > any error returned from btrfs_direct_write(). We can't simply do the call > > > even if -EIOCBQUEUED is returned, since that would block the task waiting > > > for IO, both for the data since there are bios still in progress as well > > > as potentially blocking when joining a log transaction and when syncing > > > the log (writing log trees, super blocks, etc). > > > > > > So let iomap do the sync call itself and in order to avoid deadlocks for > > > the case of synchronous writes (without nowait), use __iomap_dio_rw() and > > > have ourselves call iomap_dio_complete() after unlocking the inode. > > > > > > A test case will later be sent for fstests, after this is fixed in Linus' > > > tree. > > > > > > Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes") > > > Reported-by: Марк Коренберг <socketpair@gmail.com> > > > Link: https://lore.kernel.org/linux-btrfs/CAEmTpZGRKbzc16fWPvxbr6AfFsQoLmz-Lcg-7OgJOZDboJ+SGQ@mail.gmail.com/ > > > CC: stable@vger.kernel.org # 6.0+ > > > > The test script provided by Марк Коренберг <socketpair@gmail.com> > > show a normal result on 6.1.0-rc3+ with this patch. > > > > but the test script provided by Марк Коренберг show that > > there is a problem in 5.15.y too. we need a different fix for 5.15.y? > > a dirty fix will let the test script provided by Марк Коренбе > show a normal result on 5.15.y. but I don't know whether this dirty fix > is right. > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index bd05a8f..42b2d20 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -2109,7 +2109,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb, > > btrfs_set_inode_last_sub_trans(inode); > > - if (num_written > 0) > + if (num_written > 0 || sync) > num_written = generic_write_sync(iocb, num_written); Nop. If it were that simple, I would have done similar for 6.0, 6.1 and misc-next. If we are in a nowait context we shouldn't block, and generic_write_sync() will trigger fsync which can block for many reasons - one of them is blocking for IO to complete. The call to generic_write_sync() must happen when the bios complete in case of async IO. What you did will work, since fsync will wait for the ordered extent to complete, but it is wrong because when we are in nowait context we shouldn't block. Thanks. > > if (sync) > > Best Regards > Wang Yugui (wangyugui@e16-tech.com) > 2022/11/01 > > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > > --- > > > fs/btrfs/btrfs_inode.h | 5 ++++- > > > fs/btrfs/file.c | 22 ++++++++++++++++------ > > > fs/btrfs/inode.c | 14 +++++++++++--- > > > 3 files changed, 31 insertions(+), 10 deletions(-) > > > > > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > > > index 79a9f06c2434..d21c30bf7053 100644 > > > --- a/fs/btrfs/btrfs_inode.h > > > +++ b/fs/btrfs/btrfs_inode.h > > > @@ -526,7 +526,10 @@ ssize_t btrfs_encoded_read(struct kiocb *iocb, struct iov_iter *iter, > > > ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from, > > > const struct btrfs_ioctl_encoded_io_args *encoded); > > > > > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before); > > > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, > > > + size_t done_before); > > > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > > > + size_t done_before); > > > > > > extern const struct dentry_operations btrfs_dentry_operations; > > > > > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > > > index 838b3c0ea329..6e2889bc73d8 100644 > > > --- a/fs/btrfs/file.c > > > +++ b/fs/btrfs/file.c > > > @@ -1453,6 +1453,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > > loff_t endbyte; > > > ssize_t err; > > > unsigned int ilock_flags = 0; > > > + struct iomap_dio *dio; > > > > > > if (iocb->ki_flags & IOCB_NOWAIT) > > > ilock_flags |= BTRFS_ILOCK_TRY; > > > @@ -1513,11 +1514,22 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > > * So here we disable page faults in the iov_iter and then retry if we > > > * got -EFAULT, faulting in the pages before the retry. > > > */ > > > -again: > > > from->nofault = true; > > > - err = btrfs_dio_rw(iocb, from, written); > > > + dio = btrfs_dio_write(iocb, from, written); > > > from->nofault = false; > > > > > > + /* > > > + * iomap_dio_complete() will call btrfs_sync_file() if we have a dsync > > > + * iocb, and that needs to lock the inode. So unlock it before calling > > > + * iomap_dio_complete() to avoid a deadlock. > > > + */ > > > + btrfs_inode_unlock(inode, ilock_flags); > > > + > > > + if (IS_ERR_OR_NULL(dio)) > > > + err = PTR_ERR_OR_ZERO(dio); > > > + else > > > + err = iomap_dio_complete(dio); > > > + > > > /* No increment (+=) because iomap returns a cumulative value. */ > > > if (err > 0) > > > written = err; > > > @@ -1543,12 +1555,10 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > > } else { > > > fault_in_iov_iter_readable(from, left); > > > prev_left = left; > > > - goto again; > > > + goto relock; > > > } > > > } > > > > > > - btrfs_inode_unlock(inode, ilock_flags); > > > - > > > /* > > > * If 'err' is -ENOTBLK or we have not written all data, then it means > > > * we must fallback to buffered IO. > > > @@ -3752,7 +3762,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) > > > */ > > > pagefault_disable(); > > > to->nofault = true; > > > - ret = btrfs_dio_rw(iocb, to, read); > > > + ret = btrfs_dio_read(iocb, to, read); > > > to->nofault = false; > > > pagefault_enable(); > > > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > > index fd26282edace..b1e7bfd5f525 100644 > > > --- a/fs/btrfs/inode.c > > > +++ b/fs/btrfs/inode.c > > > @@ -8176,13 +8176,21 @@ static const struct iomap_dio_ops btrfs_dio_ops = { > > > .bio_set = &btrfs_dio_bioset, > > > }; > > > > > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > > > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > > > { > > > struct btrfs_dio_data data; > > > > > > return iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > > > - IOMAP_DIO_PARTIAL | IOMAP_DIO_NOSYNC, > > > - &data, done_before); > > > + IOMAP_DIO_PARTIAL, &data, done_before); > > > +} > > > + > > > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > > > + size_t done_before) > > > +{ > > > + struct btrfs_dio_data data; > > > + > > > + return __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > > > + IOMAP_DIO_PARTIAL, &data, done_before); > > > } > > > > > > static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > > -- > > > 2.35.1 > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb 2022-11-01 0:49 ` Wang Yugui 2022-11-01 1:04 ` Wang Yugui @ 2022-11-01 7:48 ` Filipe Manana 1 sibling, 0 replies; 8+ messages in thread From: Filipe Manana @ 2022-11-01 7:48 UTC (permalink / raw) To: Wang Yugui; +Cc: linux-btrfs On Tue, Nov 1, 2022 at 1:05 AM Wang Yugui <wangyugui@e16-tech.com> wrote: > > Hi, > > > From: Filipe Manana <fdmanana@suse.com> > > > > When doing a direct IO write using a iocb with nowait and dsync set, we > > end up not syncing the file once the write completes. > > > > This is because we tell iomap to not call generic_write_sync(), which > > would result in calling btrfs_sync_file(), in order to avoid a deadlock > > since iomap can call it while we are holding the inode's lock and > > btrfs_sync_file() needs to acquire the inode's lock. The deadlock happens > > only if the write happens synchronously, when iomap_dio_rw() calls > > iomap_dio_complete() before it returns. Instead we do the sync ourselves > > at btrfs_do_write_iter(). > > > > For a nowait write however we can end up not doing the sync ourselves at > > at btrfs_do_write_iter() because the write could have been queued, and > > therefore we get -EIOCBQUEUED returned from iomap in such case. That makes > > us skip the sync call at btrfs_do_write_iter(), as we don't do it for > > any error returned from btrfs_direct_write(). We can't simply do the call > > even if -EIOCBQUEUED is returned, since that would block the task waiting > > for IO, both for the data since there are bios still in progress as well > > as potentially blocking when joining a log transaction and when syncing > > the log (writing log trees, super blocks, etc). > > > > So let iomap do the sync call itself and in order to avoid deadlocks for > > the case of synchronous writes (without nowait), use __iomap_dio_rw() and > > have ourselves call iomap_dio_complete() after unlocking the inode. > > > > A test case will later be sent for fstests, after this is fixed in Linus' > > tree. > > > > Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes") > > Reported-by: Марк Коренберг <socketpair@gmail.com> > > Link: https://lore.kernel.org/linux-btrfs/CAEmTpZGRKbzc16fWPvxbr6AfFsQoLmz-Lcg-7OgJOZDboJ+SGQ@mail.gmail.com/ > > CC: stable@vger.kernel.org # 6.0+ > > The test script provided by Марк Коренберг <socketpair@gmail.com> > show a normal result on 6.1.0-rc3+ with this patch. > > but the test script provided by Марк Коренберг show that > there is a problem in 5.15.y too. we need a different fix for 5.15.y? Yes, I forgot Anand actually backported it to 5.15. It will need a different (but similar) fix due to some cleanups that happened on 5.19 or 6.0. > > Best Regards > Wang Yugui (wangyugui@e16-tech.com) > 2022/11/01 > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/btrfs_inode.h | 5 ++++- > > fs/btrfs/file.c | 22 ++++++++++++++++------ > > fs/btrfs/inode.c | 14 +++++++++++--- > > 3 files changed, 31 insertions(+), 10 deletions(-) > > > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > > index 79a9f06c2434..d21c30bf7053 100644 > > --- a/fs/btrfs/btrfs_inode.h > > +++ b/fs/btrfs/btrfs_inode.h > > @@ -526,7 +526,10 @@ ssize_t btrfs_encoded_read(struct kiocb *iocb, struct iov_iter *iter, > > ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from, > > const struct btrfs_ioctl_encoded_io_args *encoded); > > > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before); > > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, > > + size_t done_before); > > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > > + size_t done_before); > > > > extern const struct dentry_operations btrfs_dentry_operations; > > > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > > index 838b3c0ea329..6e2889bc73d8 100644 > > --- a/fs/btrfs/file.c > > +++ b/fs/btrfs/file.c > > @@ -1453,6 +1453,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > loff_t endbyte; > > ssize_t err; > > unsigned int ilock_flags = 0; > > + struct iomap_dio *dio; > > > > if (iocb->ki_flags & IOCB_NOWAIT) > > ilock_flags |= BTRFS_ILOCK_TRY; > > @@ -1513,11 +1514,22 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > * So here we disable page faults in the iov_iter and then retry if we > > * got -EFAULT, faulting in the pages before the retry. > > */ > > -again: > > from->nofault = true; > > - err = btrfs_dio_rw(iocb, from, written); > > + dio = btrfs_dio_write(iocb, from, written); > > from->nofault = false; > > > > + /* > > + * iomap_dio_complete() will call btrfs_sync_file() if we have a dsync > > + * iocb, and that needs to lock the inode. So unlock it before calling > > + * iomap_dio_complete() to avoid a deadlock. > > + */ > > + btrfs_inode_unlock(inode, ilock_flags); > > + > > + if (IS_ERR_OR_NULL(dio)) > > + err = PTR_ERR_OR_ZERO(dio); > > + else > > + err = iomap_dio_complete(dio); > > + > > /* No increment (+=) because iomap returns a cumulative value. */ > > if (err > 0) > > written = err; > > @@ -1543,12 +1555,10 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > > } else { > > fault_in_iov_iter_readable(from, left); > > prev_left = left; > > - goto again; > > + goto relock; > > } > > } > > > > - btrfs_inode_unlock(inode, ilock_flags); > > - > > /* > > * If 'err' is -ENOTBLK or we have not written all data, then it means > > * we must fallback to buffered IO. > > @@ -3752,7 +3762,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) > > */ > > pagefault_disable(); > > to->nofault = true; > > - ret = btrfs_dio_rw(iocb, to, read); > > + ret = btrfs_dio_read(iocb, to, read); > > to->nofault = false; > > pagefault_enable(); > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index fd26282edace..b1e7bfd5f525 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -8176,13 +8176,21 @@ static const struct iomap_dio_ops btrfs_dio_ops = { > > .bio_set = &btrfs_dio_bioset, > > }; > > > > -ssize_t btrfs_dio_rw(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > > +ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, size_t done_before) > > { > > struct btrfs_dio_data data; > > > > return iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > > - IOMAP_DIO_PARTIAL | IOMAP_DIO_NOSYNC, > > - &data, done_before); > > + IOMAP_DIO_PARTIAL, &data, done_before); > > +} > > + > > +struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter, > > + size_t done_before) > > +{ > > + struct btrfs_dio_data data; > > + > > + return __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops, > > + IOMAP_DIO_PARTIAL, &data, done_before); > > } > > > > static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > -- > > 2.35.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] btrfs: update stale comment for nowait direct IO writes 2022-10-31 11:43 [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing fdmanana 2022-10-31 11:43 ` [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb fdmanana @ 2022-10-31 11:43 ` fdmanana 2022-10-31 16:26 ` [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing David Sterba 2 siblings, 0 replies; 8+ messages in thread From: fdmanana @ 2022-10-31 11:43 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> If when doing a direct IO write we need to fallback to buffered IO, we this comment at btrfs_direct_write() that says we can't directly fallback to buffered IO if we have a NOWAIT iocb, because we have no support for NOWAIT buffered writes. That is not true anymore, as support for NOWAIT buffered writes was added recently in commit 926078b21db9 ("btrfs: enable nowait async buffered writes"). However we still can't fallback to a buffered write in case we have a NOWAIT iocb, because we'll need to flush delalloc and wait for it to complete after doing the buffered write, and that can block for several reasons, the main reason being waiting for IO to complete. So update the comment to mention all that. Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 6e2889bc73d8..b7855f794ba6 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1570,8 +1570,8 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) /* * If we are in a NOWAIT context, then return -EAGAIN to signal the caller * it must retry the operation in a context where blocking is acceptable, - * since we currently don't have NOWAIT semantics support for buffered IO - * and may block there for many reasons (reserving space for example). + * because even if we end up not blocking during the buffered IO attempt + * below, we will block when flushing and waiting for the IO. */ if (iocb->ki_flags & IOCB_NOWAIT) { err = -EAGAIN; -- 2.35.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing 2022-10-31 11:43 [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing fdmanana 2022-10-31 11:43 ` [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb fdmanana 2022-10-31 11:43 ` [PATCH 2/2] btrfs: update stale comment for nowait direct IO writes fdmanana @ 2022-10-31 16:26 ` David Sterba 2 siblings, 0 replies; 8+ messages in thread From: David Sterba @ 2022-10-31 16:26 UTC (permalink / raw) To: fdmanana; +Cc: linux-btrfs On Mon, Oct 31, 2022 at 11:43:54AM +0000, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > This fixes direct IO writes with nowait and dsync not getting synced after > the writes complete (except when we fallback to blocking context). The first > patch is the fix, while the second one only updates a comment that is now > stale after 6.1-rc1. The nowait + dsync combination is proably not a typical one but maybe there are use cases I don't know about. Anyway, thanks for fixing it. > Patch only applies to current misc-next, because a function prototype > declaration was recently moved out of ctree.h into btrfs_inode.h. I left > a version that applies cleanly against 6.0.6 and 6.1-rc at: > > https://git.kernel.org/pub/scm/linux/kernel/git/fdmanana/linux.git/log/?h=async_dio_fsync_fix_6.1_6.0 Great, thanks, that helps. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-11-01 7:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-31 11:43 [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing fdmanana 2022-10-31 11:43 ` [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb fdmanana 2022-11-01 0:49 ` Wang Yugui 2022-11-01 1:04 ` Wang Yugui 2022-11-01 7:53 ` Filipe Manana 2022-11-01 7:48 ` Filipe Manana 2022-10-31 11:43 ` [PATCH 2/2] btrfs: update stale comment for nowait direct IO writes fdmanana 2022-10-31 16:26 ` [PATCH 0/2] btrfs: fix direct IO writes with nowait and dsync iocb not syncing David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.