On 5/25/21 11:34 AM, David Sterba wrote:

On Thu, May 20, 2021 at 11:21:32AM -0400, Josef Bacik wrote:

quoted

We have a few flags that are inconsistently used to describe the fs in
different states of failure.  As of

	btrfs: always abort the transaction if we abort a trans handle

we will always set BTRFS_FS_STATE_ERROR if we abort, so we don't have to
check both ABORTED and ERROR to see if things have gone wrong.  Add a
helper to check BTRFS_FS_STATE_HELPER and then convert all checkers of
FS_STATE_ERROR to use the helper.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
  fs/btrfs/ctree.h       |  5 +++++
  fs/btrfs/disk-io.c     |  8 +++-----
  fs/btrfs/extent_io.c   |  2 +-
  fs/btrfs/file.c        |  2 +-
  fs/btrfs/inode.c       |  6 +++---
  fs/btrfs/scrub.c       |  2 +-
  fs/btrfs/super.c       |  2 +-
  fs/btrfs/transaction.c | 11 +++++------
  fs/btrfs/tree-log.c    |  2 +-
  9 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 938d8ebf4cf3..3c22c3308667 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h

@@ -3531,6 +3531,11 @@ do {								\
  			  (errno), fmt, ##args);		\
  } while (0)
  
+static inline bool btrfs_has_fs_error(struct btrfs_fs_info *fs_info)
+{
+	return test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state);

This could also get the unlikely() annotattion, similar to what
TRANS_ABORTED does. Which means this inline would have to be a macro eg.
FS_ERROR or similar.

Yup I'll do that.

quoted

@@ -4372,8 +4371,7 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
  			btrfs_err(fs_info, "commit super ret %d", ret);
  	}
  
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state) ||
-	    test_bit(BTRFS_FS_STATE_TRANS_ABORTED, &fs_info->fs_state))
+	if (btrfs_has_fs_error(fs_info))
  		btrfs_error_commit_super(fs_info);

This is not equivalent change, previously it also checks for
STATE_TRANS_ABORTED, this was added in af7227338135 ("Btrfs: clean up
resources during umount after trans is aborted") . So it should be kept
in place or there may be a bigger problem when the filesystem and
transaction error bits get out of sync, I haven't checked.

We had this because sometimes an empty transaction would be aborted and we 
wouldn't set FS_STATE_ERROR.  With the patch

   btrfs: always abort the transaction if we abort a trans handle

we no longer have the case where we have TRANS_ABORTED but not FS_STATE_ERROR 
set.  Now TRANS_ABORTED simply keeps us from printing multiple transaction abort 
messages, while FS_STATE_ERROR tells us there's been a problem.  Thanks,

Josef