From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E03B3FA3741 for ; Tue, 1 Nov 2022 00:50:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229556AbiKAAuJ (ORCPT ); Mon, 31 Oct 2022 20:50:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39536 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229469AbiKAAuH (ORCPT ); Mon, 31 Oct 2022 20:50:07 -0400 Received: from out20-85.mail.aliyun.com (out20-85.mail.aliyun.com [115.124.20.85]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 27EE611C2E for ; Mon, 31 Oct 2022 17:50:04 -0700 (PDT) X-Alimail-AntiSpam: AC=CONTINUE;BC=0.04436315|-1;CH=green;DM=|CONTINUE|false|;DS=CONTINUE|ham_system_inform|0.502372-6.1771e-05-0.497566;FP=0|0|0|0|0|-1|-1|-1;HT=ay29a033018047207;MF=wangyugui@e16-tech.com;NM=1;PH=DS;RN=2;RT=2;SR=0;TI=SMTPD_---.Py67Ojl_1667263792; Received: from 192.168.2.112(mailfrom:wangyugui@e16-tech.com fp:SMTPD_---.Py67Ojl_1667263792) by smtp.aliyun-inc.com; Tue, 01 Nov 2022 08:49:52 +0800 Date: Tue, 01 Nov 2022 08:49:54 +0800 From: Wang Yugui To: fdmanana@kernel.org Subject: Re: [PATCH 1/2] btrfs: fix lost file sync on direct IO write with nowait and dsync iocb Cc: linux-btrfs@vger.kernel.org In-Reply-To: <84cb2eb28538c98cbde50e83a770de476528a4f1.1667215075.git.fdmanana@suse.com> References: <84cb2eb28538c98cbde50e83a770de476528a4f1.1667215075.git.fdmanana@suse.com> Message-Id: <20221101084953.50DF.409509F4@e16-tech.com> MIME-Version: 1.0 Content-Type: text/plain; charset="GB2312" Content-Transfer-Encoding: 8bit X-Mailer: Becky! ver. 2.75.04 [en] Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org Hi, > From: Filipe Manana > > 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: 妙忘把抗 妞抉把快扶忌快把忍 > 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 妙忘把抗 妞抉把快扶忌快把忍 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 > --- > 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