diff mbox series

[2/2] btrfs: Make btrfs_direct_write atomic with respect to inode_lock

Message ID 49ff9bfb8ef20e7a9c6e26fd54bc9f4508c9ccb4.1608053602.git.rgoldwyn@suse.com (mailing list archive)
State New, archived
Headers show
Series Fix locking for btrfs direct writes | expand

Commit Message

Goldwyn Rodrigues Dec. 15, 2020, 6:06 p.m. UTC 
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.

fa54fc76db94 ("btrfs: push inode locking and unlocking into buffered/direct write")
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/file.c | 69 ++++++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 29 deletions(-)

Comments

Darrick J. Wong Dec. 15, 2020, 10:13 p.m. UTC | #1 
On Tue, Dec 15, 2020 at 12:06:36PM -0600, Goldwyn Rodrigues wrote:
> 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.

Er... can you grab the inode lock before deciding which of the IO
path(s) you're going to take?  Then you'd always have an atomic write
even if fallback happens.

(Also vaguely wondering why this needs even more slicing and dicing of
the iomap directio functions...)

--D

> 
> fa54fc76db94 ("btrfs: push inode locking and unlocking into buffered/direct write")
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/file.c | 69 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 0e41459b8de6..9fc768b951f1 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;
> -
> -	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;
> +	lockdep_assert_held(&inode->i_rwsem);
>  
> -	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)
>  {
> @@ -1927,10 +1939,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 +1954,19 @@ 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 {
> -		written = iomap_dio_complete(dio);
> +		written = __iomap_dio_complete(dio);
> +		kfree(dio);
>  	}
>  
>  	if (written < 0 || !iov_iter_count(from)) {
> @@ -1969,7 +1976,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 +1997,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)
> +		generic_write_sync(iocb, written);
> +
>  	return written ? written : err;
>  }
>  
> -- 
> 2.29.2
>
Goldwyn Rodrigues Dec. 16, 2020, 9:07 p.m. UTC | #2 
On 14:13 15/12, Darrick J. Wong wrote:
> On Tue, Dec 15, 2020 at 12:06:36PM -0600, Goldwyn Rodrigues wrote:
> > 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.
> 
> Er... can you grab the inode lock before deciding which of the IO
> path(s) you're going to take?  Then you'd always have an atomic write
> even if fallback happens.

No, since this is a fallback option which also works if the I/O is
incomplete.

> 
> (Also vaguely wondering why this needs even more slicing and dicing of
> the iomap directio functions...)

I would most likely go with Dave's method of storing the flag in the
function and calling iomap dio functions without IOCB_DSYNC flag. This
way we don't have to change iomap.
diff mbox series

Patch

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 0e41459b8de6..9fc768b951f1 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;
-
-	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;
+	lockdep_assert_held(&inode->i_rwsem);
 
-	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)
 {
@@ -1927,10 +1939,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 +1954,19 @@  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 {
-		written = iomap_dio_complete(dio);
+		written = __iomap_dio_complete(dio);
+		kfree(dio);
 	}
 
 	if (written < 0 || !iov_iter_count(from)) {
@@ -1969,7 +1976,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 +1997,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)
+		generic_write_sync(iocb, written);
+
 	return written ? written : err;
 }