All of lore.kernel.org
 help / color / mirror / Atom feed
From: Goldwyn Rodrigues <rgoldwyn@suse.de>
To: linux-btrfs@vger.kernel.org
Cc: Goldwyn Rodrigues <rgoldwyn@suse.com>
Subject: [PATCH v3] btrfs: Make btrfs_direct_write atomic with respect to inode_lock
Date: Fri, 18 Dec 2020 10:07:57 -0600	[thread overview]
Message-ID: <8846c296bcd4d5d3c21c6a98ee467ab5060c6757.1608307404.git.rgoldwyn@suse.com> (raw)

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

btrfs_direct_write() fallsback to buffered write in case btrfs is not
able to perform or complete a direct I/O. During the fallback
inode lock is unlocked and relocked. This does not guarantee the
atomicity of the entire write since the lock can be acquired by another
write between unlock and relock.

__btrfs_buffered_write() is used to perform the direct fallback write,
which performs the write without acquiring the lock or checks.

Before calling iomap_dio_complete(), clar the IOCB_DSYNC flag.
Re-instate after calling iomap_dio_complete()

fa54fc76db94 ("btrfs: push inode locking and unlocking into buffered/direct write")
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Changes since v2:
 - Manipulate iocb->ki_flags before and after calling
 iomap_dio_complete()

Changes since v1:
 - Fix the issue of hang with the direct I/O and sync.
 - lockdep_assert_held() in __btrfs_buffered_write()

---
 fs/btrfs/file.c | 77 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index e65223e3510d..b7fe2f3d179a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1638,11 +1638,11 @@ static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from,
 	return 0;
 }
 
-static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
+static noinline ssize_t __btrfs_buffered_write(struct kiocb *iocb,
 					       struct iov_iter *i)
 {
 	struct file *file = iocb->ki_filp;
-	loff_t pos;
+	loff_t pos = iocb->ki_pos;
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct page **pages = NULL;
@@ -1656,24 +1656,9 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 	bool only_release_metadata = false;
 	bool force_page_uptodate = false;
 	loff_t old_isize = i_size_read(inode);
-	unsigned int ilock_flags = 0;
 
-	if (iocb->ki_flags & IOCB_NOWAIT)
-		ilock_flags |= BTRFS_ILOCK_TRY;
+	lockdep_assert_held(&inode->i_rwsem);
 
-	ret = btrfs_inode_lock(inode, ilock_flags);
-	if (ret < 0)
-		return ret;
-
-	ret = generic_write_checks(iocb, i);
-	if (ret <= 0)
-		goto out;
-
-	ret = btrfs_write_check(iocb, i, ret);
-	if (ret < 0)
-		goto out;
-
-	pos = iocb->ki_pos;
 	nrptrs = min(DIV_ROUND_UP(iov_iter_count(i), PAGE_SIZE),
 			PAGE_SIZE / (sizeof(struct page *)));
 	nrptrs = min(nrptrs, current->nr_dirtied_pause - current->nr_dirtied);
@@ -1877,10 +1862,37 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 		iocb->ki_pos += num_written;
 	}
 out:
-	btrfs_inode_unlock(inode, ilock_flags);
 	return num_written ? num_written : ret;
 }
 
+static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
+					       struct iov_iter *i)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+	unsigned int ilock_flags = 0;
+	ssize_t ret;
+
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		ilock_flags |= BTRFS_ILOCK_TRY;
+
+	ret = btrfs_inode_lock(inode, ilock_flags);
+	if (ret < 0)
+		return ret;
+
+	ret = generic_write_checks(iocb, i);
+	if (ret <= 0)
+		goto out;
+
+	ret = btrfs_write_check(iocb, i, ret);
+	if (ret < 0)
+		goto out;
+
+	ret = __btrfs_buffered_write(iocb, i);
+out:
+	btrfs_inode_unlock(inode, ilock_flags);
+	return ret;
+}
+
 static ssize_t check_direct_IO(struct btrfs_fs_info *fs_info,
 			       const struct iov_iter *iter, loff_t offset)
 {
@@ -1907,6 +1919,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t err;
 	unsigned int ilock_flags = 0;
 	struct iomap_dio *dio = NULL;
+	bool dsync;
 
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		ilock_flags |= BTRFS_ILOCK_TRY;
@@ -1927,10 +1940,8 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	}
 
 	err = btrfs_write_check(iocb, from, err);
-	if (err < 0) {
-		btrfs_inode_unlock(inode, ilock_flags);
+	if (err < 0)
 		goto out;
-	}
 
 	pos = iocb->ki_pos;
 	/*
@@ -1944,22 +1955,30 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 		goto relock;
 	}
 
-	if (check_direct_IO(fs_info, from, pos)) {
-		btrfs_inode_unlock(inode, ilock_flags);
+	if (check_direct_IO(fs_info, from, pos))
 		goto buffered;
-	}
 
 	dio = __iomap_dio_rw(iocb, from, &btrfs_dio_iomap_ops,
 			     &btrfs_dio_ops, is_sync_kiocb(iocb));
 
-	btrfs_inode_unlock(inode, ilock_flags);
 
 	if (IS_ERR_OR_NULL(dio)) {
 		err = PTR_ERR_OR_ZERO(dio);
 		if (err < 0 && err != -ENOTBLK)
 			goto out;
 	} else {
+		/*
+		 * Remove the DSYNC flag because iomap_dio_complete() calls
+		 * generic_write_sync() which may deadlock with btrfs_sync()
+		 * on inode->i_rwsem
+		 */
+		if (iocb->ki_flags & IOCB_DSYNC) {
+			dsync = true;
+			iocb->ki_flags &= ~IOCB_DSYNC;
+		}
 		written = iomap_dio_complete(dio);
+		if (dsync)
+			iocb->ki_flags |= IOCB_DSYNC;
 	}
 
 	if (written < 0 || !iov_iter_count(from)) {
@@ -1969,7 +1988,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 
 buffered:
 	pos = iocb->ki_pos;
-	written_buffered = btrfs_buffered_write(iocb, from);
+	written_buffered = __btrfs_buffered_write(iocb, from);
 	if (written_buffered < 0) {
 		err = written_buffered;
 		goto out;
@@ -1990,6 +2009,10 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	invalidate_mapping_pages(file->f_mapping, pos >> PAGE_SHIFT,
 				 endbyte >> PAGE_SHIFT);
 out:
+	btrfs_inode_unlock(inode, ilock_flags);
+	if (written > 0 && dsync)
+		generic_write_sync(iocb, written);
+
 	return written ? written : err;
 }
 
-- 
2.29.2


             reply	other threads:[~2020-12-18 16:08 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-18 16:07 Goldwyn Rodrigues [this message]
2020-12-19 17:50 ` [PATCH v3] btrfs: Make btrfs_direct_write atomic with respect to inode_lock kernel test robot
2020-12-19 17:50   ` kernel test robot
2020-12-24  3:10 ` [btrfs] e7a8dd2d95: fio.write_iops -98.3% regression kernel test robot
2020-12-24  3:10   ` kernel test robot
2021-01-04 10:10 ` [kbuild] Re: [PATCH v3] btrfs: Make btrfs_direct_write atomic with respect to inode_lock Dan Carpenter
2021-01-04 10:10   ` Dan Carpenter
2021-01-04 10:10   ` Dan Carpenter
2020-12-29 16:03 kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8846c296bcd4d5d3c21c6a98ee467ab5060c6757.1608307404.git.rgoldwyn@suse.com \
    --to=rgoldwyn@suse.de \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=rgoldwyn@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.