[2/8] btrfs: Move FS error state bit early during write

Commit Message

Goldwyn Rodrigues June 22, 2020, 4:20 p.m. UTC

From: Goldwyn Rodrigues <[e-mail]>

We don't need the inode locked to check for the error bit. Move the
check early.

Signed-off-by: Goldwyn Rodrigues <[e-mail]>
---
 fs/btrfs/file.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Comments

David Sterba June 30, 2020, 8:22 a.m. UTC | #1

On Mon, Jun 22, 2020 at 11:20:11AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <[e-mail]>
> 
> We don't need the inode locked to check for the error bit. Move the
> check early.

This lacks explanation why it's not needed. I've checked history of the
code and it seems the error state flags has been after all other checks
since long, starting in acce952b0263825d. But it's part of a bigger
patch and is not specific about this call site.

If something checks state and changes the location, we need to make sure
the state is not affected by code between the old and new location.

Goldwyn Rodrigues June 30, 2020, 2:36 p.m. UTC | #2

On 10:22 30/06, David Sterba wrote:
> On Mon, Jun 22, 2020 at 11:20:11AM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <[e-mail]>
> > 
> > We don't need the inode locked to check for the error bit. Move the
> > check early.
> 
> This lacks explanation why it's not needed. I've checked history of the
> code and it seems the error state flags has been after all other checks
> since long, starting in acce952b0263825d. But it's part of a bigger
> patch and is not specific about this call site.
> 
> If something checks state and changes the location, we need to make sure
> the state is not affected by code between the old and new location.

This is a filesystem state bit as opposed to a file level state bit. The
bit states that you don't let writes proceed because of a filesystem
error. Why would you need a inode level lock for that? It is better to
return -EROFS early enough rather than take a lock, check for filesystem
failure, release the lock and then return an error.

The other check is in start_transaction, which perhaps is for
writes-in-flight.

Patch

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 01377a7d1d13..9ec30ccbd67a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1932,6 +1932,15 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	size_t count;
 	loff_t oldsize;
 
+	/*
+	 * If BTRFS flips readonly due to some impossible error
+	 * (fs_info->fs_state now has BTRFS_SUPER_FLAG_ERROR),
+	 * although we have opened a file as writable, we have
+	 * to stop this write operation to ensure FS consistency.
+	 */
+	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
+		return -EROFS;
+
 	if (!(iocb->ki_flags & IOCB_DIRECT) &&
 	    (iocb->ki_flags & IOCB_NOWAIT))
 		return -EOPNOTSUPP;
@@ -1971,18 +1980,6 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 		goto out;
 	}
 
-	/*
-	 * If BTRFS flips readonly due to some impossible error
-	 * (fs_info->fs_state now has BTRFS_SUPER_FLAG_ERROR),
-	 * although we have opened a file as writable, we have
-	 * to stop this write operation to ensure FS consistency.
-	 */
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
-		inode_unlock(inode);
-		err = -EROFS;
-		goto out;
-	}
-
 	/*
 	 * We reserve space for updating the inode when we reserve space for the
 	 * extent we are going to write, so we will enospc out there.  We don't