[PATCH v3 0/5] Miscellaneous error handling patches

[PATCH v3 0/5] Miscellaneous error handling patches

[Josef Bacik]

Hello,

This series is left overs from a few different series.  The error handling
patches look like they just got missed somehow.  The FS_ERROR helper has been
updated based on the comments from Dave.  I'm attaching this to the open GH
thing that I was looking at to update, but really just has the FS_ERROR helper
patch from that series.  Thanks,

Josef

Josef Bacik (5):
  btrfs: change handle_fs_error in recover_log_trees to aborts
  btrfs: change error handling for btrfs_delete_*_in_log
  btrfs: add a BTRFS_FS_ERROR helper
  btrfs: do not infinite loop in data reclaim if we aborted
  btrfs: fix abort logic in btrfs_replace_file_extents

 fs/btrfs/ctree.h       |  3 +++
 fs/btrfs/disk-io.c     |  8 +++---
 fs/btrfs/extent_io.c   |  2 +-
 fs/btrfs/file.c        |  7 ++---
 fs/btrfs/inode.c       | 22 +++++-----------
 fs/btrfs/scrub.c       |  2 +-
 fs/btrfs/space-info.c  | 27 ++++++++++++++++---
 fs/btrfs/super.c       |  2 +-
 fs/btrfs/transaction.c | 11 ++++----
 fs/btrfs/tree-log.c    | 59 ++++++++++++++++--------------------------
 fs/btrfs/tree-log.h    | 16 ++++++------
 11 files changed, 78 insertions(+), 81 deletions(-)

-- 
2.26.3

[PATCH v3 1/5] btrfs: change handle_fs_error in recover_log_trees to aborts

[Josef Bacik]

During inspection of the return path for replay I noticed that we don't
actually abort the transaction if we get a failure during replay.  This
isn't a problem necessarily, as we properly return the error and will
fail to mount.  However we still leave this dangling transaction that
could conceivably be committed without thinking there was an error.
We were using btrfs_handle_fs_error() here, but that pre-dates the
transaction abort code.  Simply replace the btrfs_handle_fs_error()
calls with transaction aborts, so we still know where exactly things
went wrong, and add a few in some other un-handled error cases.

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-log.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 30590ddd69ac..e0c2d4c7f939 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -6527,8 +6527,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 		ret = btrfs_search_slot(NULL, log_root_tree, &key, path, 0, 0);
 
 		if (ret < 0) {
-			btrfs_handle_fs_error(fs_info, ret,
-				    "Couldn't find tree log root.");
+			btrfs_abort_transaction(trans, ret);
 			goto error;
 		}
 		if (ret > 0) {
@@ -6545,8 +6544,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 		log = btrfs_read_tree_root(log_root_tree, &found_key);
 		if (IS_ERR(log)) {
 			ret = PTR_ERR(log);
-			btrfs_handle_fs_error(fs_info, ret,
-				    "Couldn't read tree log root.");
+			btrfs_abort_transaction(trans, ret);
 			goto error;
 		}
 
@@ -6574,8 +6572,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 
 			if (!ret)
 				goto next;
-			btrfs_handle_fs_error(fs_info, ret,
-				"Couldn't read target root for tree log recovery.");
+			btrfs_abort_transaction(trans, ret);
 			goto error;
 		}
 
@@ -6583,14 +6580,15 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 		ret = btrfs_record_root_in_trans(trans, wc.replay_dest);
 		if (ret)
 			/* The loop needs to continue due to the root refs */
-			btrfs_handle_fs_error(fs_info, ret,
-				"failed to record the log root in transaction");
+			btrfs_abort_transaction(trans, ret);
 		else
 			ret = walk_log_tree(trans, log, &wc);
 
 		if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) {
 			ret = fixup_inode_link_counts(trans, wc.replay_dest,
 						      path);
+			if (ret)
+				btrfs_abort_transaction(trans, ret);
 		}
 
 		if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) {
@@ -6607,6 +6605,8 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 			 * could only happen during mount.
 			 */
 			ret = btrfs_init_root_free_objectid(root);
+			if (ret)
+				btrfs_abort_transaction(trans, ret);
 		}
 
 		wc.replay_dest->log_root = NULL;
-- 
2.26.3

Re: [PATCH v3 1/5] btrfs: change handle_fs_error in recover_log_trees to aborts

[Anand Jain]

On 28/09/2021 02:05, Josef Bacik wrote:
> During inspection of the return path for replay I noticed that we don't
> actually abort the transaction if we get a failure during replay.  This
> isn't a problem necessarily, as we properly return the error and will
> fail to mount.  However we still leave this dangling transaction that
> could conceivably be committed without thinking there was an error.
> We were using btrfs_handle_fs_error() here, but that pre-dates the
> transaction abort code.  Simply replace the btrfs_handle_fs_error()
> calls with transaction aborts, so we still know where exactly things
> went wrong, and add a few in some other un-handled error cases.
> 
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

  After this patch, failure from walk_log_tree() still won't call 
btrfs_abort_transaction(), is it intentional?

  Maybe it is time to enhance btrfs_abort_transaction() with an argument 
to pass those kernel-error-messages in the 3rd argument of 
btrfs_handle_fs_error() in btrfs_recover_log_trees().

  After this patch, it will continue to print function line numbers, but 
those kernel-error messages are more informative if printed.

Thanks, Anand

> ---
>   fs/btrfs/tree-log.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 30590ddd69ac..e0c2d4c7f939 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6527,8 +6527,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   		ret = btrfs_search_slot(NULL, log_root_tree, &key, path, 0, 0);
>   
>   		if (ret < 0) {
> -			btrfs_handle_fs_error(fs_info, ret,
> -				    "Couldn't find tree log root.");
> +			btrfs_abort_transaction(trans, ret);
>   			goto error;
>   		}
>   		if (ret > 0) {
> @@ -6545,8 +6544,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   		log = btrfs_read_tree_root(log_root_tree, &found_key);
>   		if (IS_ERR(log)) {
>   			ret = PTR_ERR(log);
> -			btrfs_handle_fs_error(fs_info, ret,
> -				    "Couldn't read tree log root.");
> +			btrfs_abort_transaction(trans, ret);
>   			goto error;
>   		}
>   
> @@ -6574,8 +6572,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   
>   			if (!ret)
>   				goto next;
> -			btrfs_handle_fs_error(fs_info, ret,
> -				"Couldn't read target root for tree log recovery.");
> +			btrfs_abort_transaction(trans, ret);
>   			goto error;
>   		}
>   
> @@ -6583,14 +6580,15 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   		ret = btrfs_record_root_in_trans(trans, wc.replay_dest);
>   		if (ret)
>   			/* The loop needs to continue due to the root refs */
> -			btrfs_handle_fs_error(fs_info, ret,
> -				"failed to record the log root in transaction");
> +			btrfs_abort_transaction(trans, ret);
>   		else
>   			ret = walk_log_tree(trans, log, &wc);
>   
>   		if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) {
>   			ret = fixup_inode_link_counts(trans, wc.replay_dest,
>   						      path);
> +			if (ret)
> +				btrfs_abort_transaction(trans, ret);
>   		}
>   
>   		if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) {
> @@ -6607,6 +6605,8 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   			 * could only happen during mount.
>   			 */
>   			ret = btrfs_init_root_free_objectid(root);
> +			if (ret)
> +				btrfs_abort_transaction(trans, ret);
>   		}
>   
>   		wc.replay_dest->log_root = NULL;
> 

[PATCH v3 2/5] btrfs: change error handling for btrfs_delete_*_in_log

[Josef Bacik]

Currently we will abort the transaction if we get a random error (like
-EIO) while trying to remove the directory entries from the root log
during rename.

However the log sync stuff doesn't actually honor the transaction abort
flag, it'll happily commit the log even if we've aborted the transaction
for reasons related to the log, which is a pretty bad problem.

Fix this by making these functions void, as we don't actually care if
this fails.  Instead mark the log as requiring a full commit on error.
This will keep us from committing this bad log, and if we fsync we'll
force a full transaction commit and have a consistent file system.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c    | 16 +++-------------
 fs/btrfs/tree-log.c | 41 ++++++++++++++---------------------------
 fs/btrfs/tree-log.h | 16 ++++++++--------
 3 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a643be30c18a..03a843b9659b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4108,19 +4108,9 @@ static int __btrfs_unlink_inode(struct btrfs_trans_handle *trans,
 		goto err;
 	}
 
-	ret = btrfs_del_inode_ref_in_log(trans, root, name, name_len, inode,
-			dir_ino);
-	if (ret != 0 && ret != -ENOENT) {
-		btrfs_abort_transaction(trans, ret);
-		goto err;
-	}
-
-	ret = btrfs_del_dir_entries_in_log(trans, root, name, name_len, dir,
-			index);
-	if (ret == -ENOENT)
-		ret = 0;
-	else if (ret)
-		btrfs_abort_transaction(trans, ret);
+	btrfs_del_inode_ref_in_log(trans, root, name, name_len, inode,
+				   dir_ino);
+	btrfs_del_dir_entries_in_log(trans, root, name, name_len, dir, index);
 
 	/*
 	 * If we have a pending delayed iput we could end up with the final iput
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index e0c2d4c7f939..720723611875 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3500,10 +3500,10 @@ static bool inode_logged(struct btrfs_trans_handle *trans,
  * This optimizations allows us to avoid relogging the entire inode
  * or the entire directory.
  */
-int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
-				 struct btrfs_root *root,
-				 const char *name, int name_len,
-				 struct btrfs_inode *dir, u64 index)
+void btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
+				  struct btrfs_root *root,
+				  const char *name, int name_len,
+				  struct btrfs_inode *dir, u64 index)
 {
 	struct btrfs_root *log;
 	struct btrfs_dir_item *di;
@@ -3513,11 +3513,11 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
 	u64 dir_ino = btrfs_ino(dir);
 
 	if (!inode_logged(trans, dir))
-		return 0;
+		return;
 
 	ret = join_running_log_trans(root);
 	if (ret)
-		return 0;
+		return;
 
 	mutex_lock(&dir->log_mutex);
 
@@ -3565,49 +3565,36 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
 	btrfs_free_path(path);
 out_unlock:
 	mutex_unlock(&dir->log_mutex);
-	if (err == -ENOSPC) {
+	if (err < 0 && err != -ENOENT)
 		btrfs_set_log_full_commit(trans);
-		err = 0;
-	} else if (err < 0 && err != -ENOENT) {
-		/* ENOENT can be returned if the entry hasn't been fsynced yet */
-		btrfs_abort_transaction(trans, err);
-	}
-
 	btrfs_end_log_trans(root);
-
-	return err;
 }
 
 /* see comments for btrfs_del_dir_entries_in_log */
-int btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
-			       struct btrfs_root *root,
-			       const char *name, int name_len,
-			       struct btrfs_inode *inode, u64 dirid)
+void btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
+				struct btrfs_root *root,
+				const char *name, int name_len,
+				struct btrfs_inode *inode, u64 dirid)
 {
 	struct btrfs_root *log;
 	u64 index;
 	int ret;
 
 	if (!inode_logged(trans, inode))
-		return 0;
+		return;
 
 	ret = join_running_log_trans(root);
 	if (ret)
-		return 0;
+		return;
 	log = root->log_root;
 	mutex_lock(&inode->log_mutex);
 
 	ret = btrfs_del_inode_ref(trans, log, name, name_len, btrfs_ino(inode),
 				  dirid, &index);
 	mutex_unlock(&inode->log_mutex);
-	if (ret == -ENOSPC) {
+	if (ret < 0 && ret != -ENOENT)
 		btrfs_set_log_full_commit(trans);
-		ret = 0;
-	} else if (ret < 0 && ret != -ENOENT)
-		btrfs_abort_transaction(trans, ret);
 	btrfs_end_log_trans(root);
-
-	return ret;
 }
 
 /*
diff --git a/fs/btrfs/tree-log.h b/fs/btrfs/tree-log.h
index 3ce6bdb76009..f6811c3df38a 100644
--- a/fs/btrfs/tree-log.h
+++ b/fs/btrfs/tree-log.h
@@ -70,14 +70,14 @@ int btrfs_recover_log_trees(struct btrfs_root *tree_root);
 int btrfs_log_dentry_safe(struct btrfs_trans_handle *trans,
 			  struct dentry *dentry,
 			  struct btrfs_log_ctx *ctx);
-int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
-				 struct btrfs_root *root,
-				 const char *name, int name_len,
-				 struct btrfs_inode *dir, u64 index);
-int btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
-			       struct btrfs_root *root,
-			       const char *name, int name_len,
-			       struct btrfs_inode *inode, u64 dirid);
+void btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
+				  struct btrfs_root *root,
+				  const char *name, int name_len,
+				  struct btrfs_inode *dir, u64 index);
+void btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
+				struct btrfs_root *root,
+				const char *name, int name_len,
+				struct btrfs_inode *inode, u64 dirid);
 void btrfs_end_log_trans(struct btrfs_root *root);
 void btrfs_pin_log_trans(struct btrfs_root *root);
 void btrfs_record_unlink_dir(struct btrfs_trans_handle *trans,
-- 
2.26.3

Re: [PATCH v3 2/5] btrfs: change error handling for btrfs_delete_*_in_log

[Filipe Manana]

On Mon, Sep 27, 2021 at 7:06 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Currently we will abort the transaction if we get a random error (like
> -EIO) while trying to remove the directory entries from the root log
> during rename.
>
> However the log sync stuff doesn't actually honor the transaction abort
> flag, it'll happily commit the log even if we've aborted the transaction
> for reasons related to the log, which is a pretty bad problem.

I'm not seeing how that would happen.
Since your commit 165ea85f14831f ("btrfs: do not write supers if we
have an fs error"), log syncing actually checks if a transaction was
aborted, and skips the log commit.
We're also aborting the transaction while holding the log pinned.
So I don't see how we can end up committing an inconsistent log if any
of the calls to remove dirents from the log fails.

Maybe this patch was prepared before that other patch?

Either way it seems the change log needs to be updated, because log
syncing checks for transaction aborts / fs error state.

>
> Fix this by making these functions void, as we don't actually care if
> this fails.  Instead mark the log as requiring a full commit on error.

This makes sense, there's really no need to abort a transaction,
forcing the next log commit attempt to fallback to a transaction
commit is more than enough.

> This will keep us from committing this bad log, and if we fsync we'll
> force a full transaction commit and have a consistent file system.

Codewise it looks good, I just don't think the part of committing a
bad log can happen after that other commit.

Thanks.

>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/inode.c    | 16 +++-------------
>  fs/btrfs/tree-log.c | 41 ++++++++++++++---------------------------
>  fs/btrfs/tree-log.h | 16 ++++++++--------
>  3 files changed, 25 insertions(+), 48 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a643be30c18a..03a843b9659b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4108,19 +4108,9 @@ static int __btrfs_unlink_inode(struct btrfs_trans_handle *trans,
>                 goto err;
>         }
>
> -       ret = btrfs_del_inode_ref_in_log(trans, root, name, name_len, inode,
> -                       dir_ino);
> -       if (ret != 0 && ret != -ENOENT) {
> -               btrfs_abort_transaction(trans, ret);
> -               goto err;
> -       }
> -
> -       ret = btrfs_del_dir_entries_in_log(trans, root, name, name_len, dir,
> -                       index);
> -       if (ret == -ENOENT)
> -               ret = 0;
> -       else if (ret)
> -               btrfs_abort_transaction(trans, ret);
> +       btrfs_del_inode_ref_in_log(trans, root, name, name_len, inode,
> +                                  dir_ino);
> +       btrfs_del_dir_entries_in_log(trans, root, name, name_len, dir, index);
>
>         /*
>          * If we have a pending delayed iput we could end up with the final iput
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index e0c2d4c7f939..720723611875 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3500,10 +3500,10 @@ static bool inode_logged(struct btrfs_trans_handle *trans,
>   * This optimizations allows us to avoid relogging the entire inode
>   * or the entire directory.
>   */
> -int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
> -                                struct btrfs_root *root,
> -                                const char *name, int name_len,
> -                                struct btrfs_inode *dir, u64 index)
> +void btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
> +                                 struct btrfs_root *root,
> +                                 const char *name, int name_len,
> +                                 struct btrfs_inode *dir, u64 index)
>  {
>         struct btrfs_root *log;
>         struct btrfs_dir_item *di;
> @@ -3513,11 +3513,11 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
>         u64 dir_ino = btrfs_ino(dir);
>
>         if (!inode_logged(trans, dir))
> -               return 0;
> +               return;
>
>         ret = join_running_log_trans(root);
>         if (ret)
> -               return 0;
> +               return;
>
>         mutex_lock(&dir->log_mutex);
>
> @@ -3565,49 +3565,36 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
>         btrfs_free_path(path);
>  out_unlock:
>         mutex_unlock(&dir->log_mutex);
> -       if (err == -ENOSPC) {
> +       if (err < 0 && err != -ENOENT)
>                 btrfs_set_log_full_commit(trans);
> -               err = 0;
> -       } else if (err < 0 && err != -ENOENT) {
> -               /* ENOENT can be returned if the entry hasn't been fsynced yet */
> -               btrfs_abort_transaction(trans, err);
> -       }
> -
>         btrfs_end_log_trans(root);
> -
> -       return err;
>  }
>
>  /* see comments for btrfs_del_dir_entries_in_log */
> -int btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
> -                              struct btrfs_root *root,
> -                              const char *name, int name_len,
> -                              struct btrfs_inode *inode, u64 dirid)
> +void btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
> +                               struct btrfs_root *root,
> +                               const char *name, int name_len,
> +                               struct btrfs_inode *inode, u64 dirid)
>  {
>         struct btrfs_root *log;
>         u64 index;
>         int ret;
>
>         if (!inode_logged(trans, inode))
> -               return 0;
> +               return;
>
>         ret = join_running_log_trans(root);
>         if (ret)
> -               return 0;
> +               return;
>         log = root->log_root;
>         mutex_lock(&inode->log_mutex);
>
>         ret = btrfs_del_inode_ref(trans, log, name, name_len, btrfs_ino(inode),
>                                   dirid, &index);
>         mutex_unlock(&inode->log_mutex);
> -       if (ret == -ENOSPC) {
> +       if (ret < 0 && ret != -ENOENT)
>                 btrfs_set_log_full_commit(trans);
> -               ret = 0;
> -       } else if (ret < 0 && ret != -ENOENT)
> -               btrfs_abort_transaction(trans, ret);
>         btrfs_end_log_trans(root);
> -
> -       return ret;
>  }
>
>  /*
> diff --git a/fs/btrfs/tree-log.h b/fs/btrfs/tree-log.h
> index 3ce6bdb76009..f6811c3df38a 100644
> --- a/fs/btrfs/tree-log.h
> +++ b/fs/btrfs/tree-log.h
> @@ -70,14 +70,14 @@ int btrfs_recover_log_trees(struct btrfs_root *tree_root);
>  int btrfs_log_dentry_safe(struct btrfs_trans_handle *trans,
>                           struct dentry *dentry,
>                           struct btrfs_log_ctx *ctx);
> -int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
> -                                struct btrfs_root *root,
> -                                const char *name, int name_len,
> -                                struct btrfs_inode *dir, u64 index);
> -int btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
> -                              struct btrfs_root *root,
> -                              const char *name, int name_len,
> -                              struct btrfs_inode *inode, u64 dirid);
> +void btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
> +                                 struct btrfs_root *root,
> +                                 const char *name, int name_len,
> +                                 struct btrfs_inode *dir, u64 index);
> +void btrfs_del_inode_ref_in_log(struct btrfs_trans_handle *trans,
> +                               struct btrfs_root *root,
> +                               const char *name, int name_len,
> +                               struct btrfs_inode *inode, u64 dirid);
>  void btrfs_end_log_trans(struct btrfs_root *root);
>  void btrfs_pin_log_trans(struct btrfs_root *root);
>  void btrfs_record_unlink_dir(struct btrfs_trans_handle *trans,
> --
> 2.26.3
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH v3 3/5] btrfs: add a BTRFS_FS_ERROR helper

[Josef Bacik]

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       |  3 +++
 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, 19 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 522ded06fd85..53f15decb523 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3576,6 +3576,9 @@ do {								\
 			  (errno), fmt, ##args);		\
 } while (0)
 
+#define BTRFS_FS_ERROR(fs_info)	(unlikely(test_bit(BTRFS_FS_STATE_ERROR, \
+						   &(fs_info)->fs_state)))
+
 __printf(5, 6)
 __cold
 void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 37637539c5ab..1ae30b29f2b5 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1954,8 +1954,7 @@ static int transaction_kthread(void *arg)
 		wake_up_process(fs_info->cleaner_kthread);
 		mutex_unlock(&fs_info->transaction_kthread_mutex);
 
-		if (unlikely(test_bit(BTRFS_FS_STATE_ERROR,
-				      &fs_info->fs_state)))
+		if (BTRFS_FS_ERROR(fs_info))
 			btrfs_cleanup_transaction(fs_info);
 		if (!kthread_should_stop() &&
 				(!btrfs_transaction_blocked(fs_info) ||
@@ -4232,7 +4231,7 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
 		drop_ref = true;
 	spin_unlock(&fs_info->fs_roots_radix_lock);
 
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	if (BTRFS_FS_ERROR(fs_info)) {
 		ASSERT(root->log_root == NULL);
 		if (root->reloc_root) {
 			btrfs_put_root(root->reloc_root);
@@ -4383,8 +4382,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_FS_ERROR(fs_info))
 		btrfs_error_commit_super(fs_info);
 
 	kthread_stop(fs_info->transaction_kthread);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c56973f7daae..f6f004d673a0 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4876,7 +4876,7 @@ int btree_write_cache_pages(struct address_space *mapping,
 	 *   extent io tree. Thus we don't want to submit such wild eb
 	 *   if the fs already has error.
 	 */
-	if (!test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	if (!BTRFS_FS_ERROR(fs_info)) {
 		ret = flush_write_bio(&epd);
 	} else {
 		ret = -EROFS;
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 7ff577005d0f..fdceab28587e 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2013,7 +2013,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	 * have opened a file as writable, we have to stop this write operation
 	 * to ensure consistency.
 	 */
-	if (test_bit(BTRFS_FS_STATE_ERROR, &inode->root->fs_info->fs_state))
+	if (BTRFS_FS_ERROR(inode->root->fs_info))
 		return -EROFS;
 
 	if (!(iocb->ki_flags & IOCB_DIRECT) &&
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 03a843b9659b..26d155e72152 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4368,7 +4368,7 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
 	struct inode *inode;
 	u64 objectid = 0;
 
-	if (!test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
+	if (!BTRFS_FS_ERROR(fs_info))
 		WARN_ON(btrfs_root_refs(&root->root_item) != 0);
 
 	spin_lock(&root->inode_lock);
@@ -9981,7 +9981,7 @@ int btrfs_start_delalloc_snapshot(struct btrfs_root *root, bool in_reclaim_conte
 	};
 	struct btrfs_fs_info *fs_info = root->fs_info;
 
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
+	if (BTRFS_FS_ERROR(fs_info))
 		return -EROFS;
 
 	return start_delalloc_inodes(root, &wbc, true, in_reclaim_context);
@@ -10000,7 +10000,7 @@ int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, long nr,
 	struct list_head splice;
 	int ret;
 
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
+	if (BTRFS_FS_ERROR(fs_info))
 		return -EROFS;
 
 	INIT_LIST_HEAD(&splice);
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index bd3cd7427391..b1c26a90243b 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3955,7 +3955,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,
 	int	ret;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
 
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
+	if (BTRFS_FS_ERROR(fs_info))
 		return -EROFS;
 
 	/* Seed devices of a new filesystem has their own generation. */
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 7f91d62c2225..a1c54a2c787c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2006,7 +2006,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		if (ret)
 			goto restore;
 	} else {
-		if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+		if (BTRFS_FS_ERROR(fs_info)) {
 			btrfs_err(fs_info,
 				"Remounting read-write after error is not allowed");
 			ret = -EINVAL;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 14b9fdc8aaa9..1c3a1189c0bd 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -283,7 +283,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 	spin_lock(&fs_info->trans_lock);
 loop:
 	/* The file system has been taken offline. No new transactions. */
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	if (BTRFS_FS_ERROR(fs_info)) {
 		spin_unlock(&fs_info->trans_lock);
 		return -EROFS;
 	}
@@ -331,7 +331,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 		 */
 		kfree(cur_trans);
 		goto loop;
-	} else if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	} else if (BTRFS_FS_ERROR(fs_info)) {
 		spin_unlock(&fs_info->trans_lock);
 		kfree(cur_trans);
 		return -EROFS;
@@ -579,7 +579,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 	bool do_chunk_alloc = false;
 	int ret;
 
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
+	if (BTRFS_FS_ERROR(fs_info))
 		return ERR_PTR(-EROFS);
 
 	if (current->journal_info) {
@@ -991,8 +991,7 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 	if (throttle)
 		btrfs_run_delayed_iputs(info);
 
-	if (TRANS_ABORTED(trans) ||
-	    test_bit(BTRFS_FS_STATE_ERROR, &info->fs_state)) {
+	if (TRANS_ABORTED(trans) || BTRFS_FS_ERROR(info)) {
 		wake_up_process(info->transaction_kthread);
 		if (TRANS_ABORTED(trans))
 			err = trans->aborted;
@@ -2155,7 +2154,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 		 * abort to prevent writing a new superblock that reflects a
 		 * corrupt state (pointing to trees with unwritten nodes/leafs).
 		 */
-		if (test_bit(BTRFS_FS_STATE_TRANS_ABORTED, &fs_info->fs_state)) {
+		if (BTRFS_FS_ERROR(fs_info)) {
 			ret = -EROFS;
 			goto cleanup_transaction;
 		}
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 720723611875..9e5937685896 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3335,7 +3335,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	 * writing the super here would result in transid mismatches.  If there
 	 * is an error here just bail.
 	 */
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
+	if (BTRFS_FS_ERROR(fs_info)) {
 		ret = -EIO;
 		btrfs_set_log_full_commit(trans);
 		btrfs_abort_transaction(trans, ret);
-- 
2.26.3

Re: [PATCH v3 3/5] btrfs: add a BTRFS_FS_ERROR helper

[Anand Jain]

On 28/09/2021 02:05, Josef Bacik wrote:
> 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
         Nit:                ERROR  ^^^

> FS_STATE_ERROR to use the helper.


> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

LGTM. So additionally, this patch will add compiler optimize unlikely()
at most of the places which are good IMO.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/ctree.h       |  3 +++
>   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, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 522ded06fd85..53f15decb523 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3576,6 +3576,9 @@ do {								\
>   			  (errno), fmt, ##args);		\
>   } while (0)
>   
> +#define BTRFS_FS_ERROR(fs_info)	(unlikely(test_bit(BTRFS_FS_STATE_ERROR, \
> +						   &(fs_info)->fs_state)))
> +
>   __printf(5, 6)
>   __cold
>   void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 37637539c5ab..1ae30b29f2b5 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1954,8 +1954,7 @@ static int transaction_kthread(void *arg)
>   		wake_up_process(fs_info->cleaner_kthread);
>   		mutex_unlock(&fs_info->transaction_kthread_mutex);
>   
> -		if (unlikely(test_bit(BTRFS_FS_STATE_ERROR,
> -				      &fs_info->fs_state)))
> +		if (BTRFS_FS_ERROR(fs_info))
>   			btrfs_cleanup_transaction(fs_info);
>   		if (!kthread_should_stop() &&
>   				(!btrfs_transaction_blocked(fs_info) ||
> @@ -4232,7 +4231,7 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
>   		drop_ref = true;
>   	spin_unlock(&fs_info->fs_roots_radix_lock);
>   
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> +	if (BTRFS_FS_ERROR(fs_info)) {
>   		ASSERT(root->log_root == NULL);
>   		if (root->reloc_root) {
>   			btrfs_put_root(root->reloc_root);
> @@ -4383,8 +4382,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_FS_ERROR(fs_info))
>   		btrfs_error_commit_super(fs_info);
>   
>   	kthread_stop(fs_info->transaction_kthread);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index c56973f7daae..f6f004d673a0 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4876,7 +4876,7 @@ int btree_write_cache_pages(struct address_space *mapping,
>   	 *   extent io tree. Thus we don't want to submit such wild eb
>   	 *   if the fs already has error.
>   	 */
> -	if (!test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> +	if (!BTRFS_FS_ERROR(fs_info)) {
>   		ret = flush_write_bio(&epd);
>   	} else {
>   		ret = -EROFS;
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 7ff577005d0f..fdceab28587e 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2013,7 +2013,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
>   	 * have opened a file as writable, we have to stop this write operation
>   	 * to ensure consistency.
>   	 */
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &inode->root->fs_info->fs_state))
> +	if (BTRFS_FS_ERROR(inode->root->fs_info))
>   		return -EROFS;
>   
>   	if (!(iocb->ki_flags & IOCB_DIRECT) &&
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 03a843b9659b..26d155e72152 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4368,7 +4368,7 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
>   	struct inode *inode;
>   	u64 objectid = 0;
>   
> -	if (!test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> +	if (!BTRFS_FS_ERROR(fs_info))
>   		WARN_ON(btrfs_root_refs(&root->root_item) != 0);
>   
>   	spin_lock(&root->inode_lock);
> @@ -9981,7 +9981,7 @@ int btrfs_start_delalloc_snapshot(struct btrfs_root *root, bool in_reclaim_conte
>   	};
>   	struct btrfs_fs_info *fs_info = root->fs_info;
>   
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> +	if (BTRFS_FS_ERROR(fs_info))
>   		return -EROFS;
>   
>   	return start_delalloc_inodes(root, &wbc, true, in_reclaim_context);
> @@ -10000,7 +10000,7 @@ int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, long nr,
>   	struct list_head splice;
>   	int ret;
>   
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> +	if (BTRFS_FS_ERROR(fs_info))
>   		return -EROFS;
>   
>   	INIT_LIST_HEAD(&splice);
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index bd3cd7427391..b1c26a90243b 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3955,7 +3955,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,
>   	int	ret;
>   	struct btrfs_fs_info *fs_info = sctx->fs_info;
>   
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> +	if (BTRFS_FS_ERROR(fs_info))
>   		return -EROFS;
>   
>   	/* Seed devices of a new filesystem has their own generation. */
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 7f91d62c2225..a1c54a2c787c 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2006,7 +2006,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>   		if (ret)
>   			goto restore;
>   	} else {
> -		if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> +		if (BTRFS_FS_ERROR(fs_info)) {
>   			btrfs_err(fs_info,
>   				"Remounting read-write after error is not allowed");
>   			ret = -EINVAL;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 14b9fdc8aaa9..1c3a1189c0bd 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -283,7 +283,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
>   	spin_lock(&fs_info->trans_lock);
>   loop:
>   	/* The file system has been taken offline. No new transactions. */
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> +	if (BTRFS_FS_ERROR(fs_info)) {
>   		spin_unlock(&fs_info->trans_lock);
>   		return -EROFS;
>   	}
> @@ -331,7 +331,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
>   		 */
>   		kfree(cur_trans);
>   		goto loop;
> -	} else if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> +	} else if (BTRFS_FS_ERROR(fs_info)) {
>   		spin_unlock(&fs_info->trans_lock);
>   		kfree(cur_trans);
>   		return -EROFS;
> @@ -579,7 +579,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
>   	bool do_chunk_alloc = false;
>   	int ret;
>   
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> +	if (BTRFS_FS_ERROR(fs_info))
>   		return ERR_PTR(-EROFS);
>   
>   	if (current->journal_info) {
> @@ -991,8 +991,7 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
>   	if (throttle)
>   		btrfs_run_delayed_iputs(info);
>   
> -	if (TRANS_ABORTED(trans) ||
> -	    test_bit(BTRFS_FS_STATE_ERROR, &info->fs_state)) {
> +	if (TRANS_ABORTED(trans) || BTRFS_FS_ERROR(info)) {
>   		wake_up_process(info->transaction_kthread);
>   		if (TRANS_ABORTED(trans))
>   			err = trans->aborted;
> @@ -2155,7 +2154,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   		 * abort to prevent writing a new superblock that reflects a
>   		 * corrupt state (pointing to trees with unwritten nodes/leafs).
>   		 */
> -		if (test_bit(BTRFS_FS_STATE_TRANS_ABORTED, &fs_info->fs_state)) {
> +		if (BTRFS_FS_ERROR(fs_info)) {
>   			ret = -EROFS;
>   			goto cleanup_transaction;
>   		}
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 720723611875..9e5937685896 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3335,7 +3335,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>   	 * writing the super here would result in transid mismatches.  If there
>   	 * is an error here just bail.
>   	 */
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> +	if (BTRFS_FS_ERROR(fs_info)) {
>   		ret = -EIO;
>   		btrfs_set_log_full_commit(trans);
>   		btrfs_abort_transaction(trans, ret);
> 

[PATCH v3 4/5] btrfs: do not infinite loop in data reclaim if we aborted

[Josef Bacik]

Error injection stressing uncovered a busy loop in our data reclaim
loop.  There are two cases here, one where we loop creating block groups
until space_info->full is set, or in the main loop we will skip erroring
out any tickets if space_info->full == 0.  Unfortunately if we aborted
the transaction then we will never allocate chunks or reclaim any space
and thus never get ->full, and you'll see stack traces like this

watchdog: BUG: soft lockup - CPU#0 stuck for 26s! [kworker/u4:4:139]
CPU: 0 PID: 139 Comm: kworker/u4:4 Tainted: G        W         5.13.0-rc1+ #328
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014
Workqueue: events_unbound btrfs_async_reclaim_data_space
RIP: 0010:btrfs_join_transaction+0x12/0x20
RSP: 0018:ffffb2b780b77de0 EFLAGS: 00000246
RAX: ffffb2b781863d58 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000801 RSI: ffff987952b57400 RDI: ffff987940aa3000
RBP: ffff987954d55000 R08: 0000000000000001 R09: ffff98795539e8f0
R10: 000000000000000f R11: 000000000000000f R12: ffffffffffffffff
R13: ffff987952b574c8 R14: ffff987952b57400 R15: 0000000000000008
FS:  0000000000000000(0000) GS:ffff9879bbc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f0703da4000 CR3: 0000000113398004 CR4: 0000000000370ef0
Call Trace:
 flush_space+0x4a8/0x660
 btrfs_async_reclaim_data_space+0x55/0x130
 process_one_work+0x1e9/0x380
 worker_thread+0x53/0x3e0
 ? process_one_work+0x380/0x380
 kthread+0x118/0x140
 ? __kthread_bind_mask+0x60/0x60
 ret_from_fork+0x1f/0x30

Fix this by checking to see if we have a btrfs fs error in either of the
reclaim loops, and if so fail the tickets and bail.  In addition to
this, fix maybe_fail_all_tickets() to not try to grant tickets if we've
aborted, simply fail everything.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index aa5be0b24987..63423f9729c5 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -885,6 +885,7 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
 {
 	struct reserve_ticket *ticket;
 	u64 tickets_id = space_info->tickets_id;
+	const bool aborted = btrfs_has_fs_error(fs_info);
 
 	trace_btrfs_fail_all_tickets(fs_info, space_info);
 
@@ -898,16 +899,19 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
 		ticket = list_first_entry(&space_info->tickets,
 					  struct reserve_ticket, list);
 
-		if (ticket->steal &&
+		if (!aborted && ticket->steal &&
 		    steal_from_global_rsv(fs_info, space_info, ticket))
 			return true;
 
-		if (btrfs_test_opt(fs_info, ENOSPC_DEBUG))
+		if (!aborted && btrfs_test_opt(fs_info, ENOSPC_DEBUG))
 			btrfs_info(fs_info, "failing ticket with %llu bytes",
 				   ticket->bytes);
 
 		remove_ticket(space_info, ticket);
-		ticket->error = -ENOSPC;
+		if (aborted)
+			ticket->error = -EIO;
+		else
+			ticket->error = -ENOSPC;
 		wake_up(&ticket->wait);
 
 		/*
@@ -916,7 +920,8 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
 		 * here to see if we can make progress with the next ticket in
 		 * the list.
 		 */
-		btrfs_try_granting_tickets(fs_info, space_info);
+		if (!aborted)
+			btrfs_try_granting_tickets(fs_info, space_info);
 	}
 	return (tickets_id != space_info->tickets_id);
 }
@@ -1172,6 +1177,10 @@ static void btrfs_async_reclaim_data_space(struct work_struct *work)
 			spin_unlock(&space_info->lock);
 			return;
 		}
+
+		/* Something happened, fail everything and bail. */
+		if (btrfs_has_fs_error(fs_info))
+			goto aborted_fs;
 		last_tickets_id = space_info->tickets_id;
 		spin_unlock(&space_info->lock);
 	}
@@ -1202,9 +1211,19 @@ static void btrfs_async_reclaim_data_space(struct work_struct *work)
 			} else {
 				flush_state = 0;
 			}
+
+			/* Something happened, fail everything and bail. */
+			if (btrfs_has_fs_error(fs_info))
+				goto aborted_fs;
+
 		}
 		spin_unlock(&space_info->lock);
 	}
+	return;
+aborted_fs:
+	maybe_fail_all_tickets(fs_info, space_info);
+	space_info->flush = 0;
+	spin_unlock(&space_info->lock);
 }
 
 void btrfs_init_async_reclaim_work(struct btrfs_fs_info *fs_info)
-- 
2.26.3

Re: [PATCH v3 4/5] btrfs: do not infinite loop in data reclaim if we aborted

[Filipe Manana]

On Mon, Sep 27, 2021 at 7:07 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Error injection stressing uncovered a busy loop in our data reclaim
> loop.  There are two cases here, one where we loop creating block groups
> until space_info->full is set, or in the main loop we will skip erroring
> out any tickets if space_info->full == 0.  Unfortunately if we aborted
> the transaction then we will never allocate chunks or reclaim any space
> and thus never get ->full, and you'll see stack traces like this
>
> watchdog: BUG: soft lockup - CPU#0 stuck for 26s! [kworker/u4:4:139]
> CPU: 0 PID: 139 Comm: kworker/u4:4 Tainted: G        W         5.13.0-rc1+ #328
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014
> Workqueue: events_unbound btrfs_async_reclaim_data_space
> RIP: 0010:btrfs_join_transaction+0x12/0x20
> RSP: 0018:ffffb2b780b77de0 EFLAGS: 00000246
> RAX: ffffb2b781863d58 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000801 RSI: ffff987952b57400 RDI: ffff987940aa3000
> RBP: ffff987954d55000 R08: 0000000000000001 R09: ffff98795539e8f0
> R10: 000000000000000f R11: 000000000000000f R12: ffffffffffffffff
> R13: ffff987952b574c8 R14: ffff987952b57400 R15: 0000000000000008
> FS:  0000000000000000(0000) GS:ffff9879bbc00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f0703da4000 CR3: 0000000113398004 CR4: 0000000000370ef0
> Call Trace:
>  flush_space+0x4a8/0x660
>  btrfs_async_reclaim_data_space+0x55/0x130
>  process_one_work+0x1e9/0x380
>  worker_thread+0x53/0x3e0
>  ? process_one_work+0x380/0x380
>  kthread+0x118/0x140
>  ? __kthread_bind_mask+0x60/0x60
>  ret_from_fork+0x1f/0x30
>
> Fix this by checking to see if we have a btrfs fs error in either of the
> reclaim loops, and if so fail the tickets and bail.  In addition to
> this, fix maybe_fail_all_tickets() to not try to grant tickets if we've
> aborted, simply fail everything.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/space-info.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index aa5be0b24987..63423f9729c5 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -885,6 +885,7 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
>  {
>         struct reserve_ticket *ticket;
>         u64 tickets_id = space_info->tickets_id;
> +       const bool aborted = btrfs_has_fs_error(fs_info);
>
>         trace_btrfs_fail_all_tickets(fs_info, space_info);
>
> @@ -898,16 +899,19 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
>                 ticket = list_first_entry(&space_info->tickets,
>                                           struct reserve_ticket, list);
>
> -               if (ticket->steal &&
> +               if (!aborted && ticket->steal &&
>                     steal_from_global_rsv(fs_info, space_info, ticket))
>                         return true;
>
> -               if (btrfs_test_opt(fs_info, ENOSPC_DEBUG))
> +               if (!aborted && btrfs_test_opt(fs_info, ENOSPC_DEBUG))
>                         btrfs_info(fs_info, "failing ticket with %llu bytes",
>                                    ticket->bytes);
>
>                 remove_ticket(space_info, ticket);
> -               ticket->error = -ENOSPC;
> +               if (aborted)
> +                       ticket->error = -EIO;
> +               else
> +                       ticket->error = -ENOSPC;
>                 wake_up(&ticket->wait);
>
>                 /*
> @@ -916,7 +920,8 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
>                  * here to see if we can make progress with the next ticket in
>                  * the list.
>                  */
> -               btrfs_try_granting_tickets(fs_info, space_info);
> +               if (!aborted)
> +                       btrfs_try_granting_tickets(fs_info, space_info);
>         }
>         return (tickets_id != space_info->tickets_id);
>  }
> @@ -1172,6 +1177,10 @@ static void btrfs_async_reclaim_data_space(struct work_struct *work)
>                         spin_unlock(&space_info->lock);
>                         return;
>                 }
> +
> +               /* Something happened, fail everything and bail. */
> +               if (btrfs_has_fs_error(fs_info))
> +                       goto aborted_fs;
>                 last_tickets_id = space_info->tickets_id;
>                 spin_unlock(&space_info->lock);
>         }
> @@ -1202,9 +1211,19 @@ static void btrfs_async_reclaim_data_space(struct work_struct *work)
>                         } else {
>                                 flush_state = 0;
>                         }
> +
> +                       /* Something happened, fail everything and bail. */
> +                       if (btrfs_has_fs_error(fs_info))
> +                               goto aborted_fs;
> +
>                 }
>                 spin_unlock(&space_info->lock);
>         }
> +       return;
> +aborted_fs:
> +       maybe_fail_all_tickets(fs_info, space_info);
> +       space_info->flush = 0;
> +       spin_unlock(&space_info->lock);
>  }
>
>  void btrfs_init_async_reclaim_work(struct btrfs_fs_info *fs_info)
> --
> 2.26.3
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v3 4/5] btrfs: do not infinite loop in data reclaim if we aborted

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4490 bytes --]

Hi Josef,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.15-rc3]
[cannot apply to kdave/for-next next-20210922]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Josef-Bacik/Miscellaneous-error-handling-patches/20210929-185151
base:    5816b3e6577eaa676ceb00a848f0fd65fe2adc29
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4d626ae95cb373b954751bcdadacf6b0f92f3a6c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Josef-Bacik/Miscellaneous-error-handling-patches/20210929-185151
        git checkout 4d626ae95cb373b954751bcdadacf6b0f92f3a6c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/space-info.c: In function 'maybe_fail_all_tickets':
>> fs/btrfs/space-info.c:888:30: error: implicit declaration of function 'btrfs_has_fs_error'; did you mean 'btrfs_handle_fs_error'? [-Werror=implicit-function-declaration]
     888 |         const bool aborted = btrfs_has_fs_error(fs_info);
         |                              ^~~~~~~~~~~~~~~~~~
         |                              btrfs_handle_fs_error
   cc1: all warnings being treated as errors


vim +888 fs/btrfs/space-info.c

   867	
   868	/*
   869	 * maybe_fail_all_tickets - we've exhausted our flushing, start failing tickets
   870	 * @fs_info - fs_info for this fs
   871	 * @space_info - the space info we were flushing
   872	 *
   873	 * We call this when we've exhausted our flushing ability and haven't made
   874	 * progress in satisfying tickets.  The reservation code handles tickets in
   875	 * order, so if there is a large ticket first and then smaller ones we could
   876	 * very well satisfy the smaller tickets.  This will attempt to wake up any
   877	 * tickets in the list to catch this case.
   878	 *
   879	 * This function returns true if it was able to make progress by clearing out
   880	 * other tickets, or if it stumbles across a ticket that was smaller than the
   881	 * first ticket.
   882	 */
   883	static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
   884					   struct btrfs_space_info *space_info)
   885	{
   886		struct reserve_ticket *ticket;
   887		u64 tickets_id = space_info->tickets_id;
 > 888		const bool aborted = btrfs_has_fs_error(fs_info);
   889	
   890		trace_btrfs_fail_all_tickets(fs_info, space_info);
   891	
   892		if (btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
   893			btrfs_info(fs_info, "cannot satisfy tickets, dumping space info");
   894			__btrfs_dump_space_info(fs_info, space_info);
   895		}
   896	
   897		while (!list_empty(&space_info->tickets) &&
   898		       tickets_id == space_info->tickets_id) {
   899			ticket = list_first_entry(&space_info->tickets,
   900						  struct reserve_ticket, list);
   901	
   902			if (!aborted && ticket->steal &&
   903			    steal_from_global_rsv(fs_info, space_info, ticket))
   904				return true;
   905	
   906			if (!aborted && btrfs_test_opt(fs_info, ENOSPC_DEBUG))
   907				btrfs_info(fs_info, "failing ticket with %llu bytes",
   908					   ticket->bytes);
   909	
   910			remove_ticket(space_info, ticket);
   911			if (aborted)
   912				ticket->error = -EIO;
   913			else
   914				ticket->error = -ENOSPC;
   915			wake_up(&ticket->wait);
   916	
   917			/*
   918			 * We're just throwing tickets away, so more flushing may not
   919			 * trip over btrfs_try_granting_tickets, so we need to call it
   920			 * here to see if we can make progress with the next ticket in
   921			 * the list.
   922			 */
   923			if (!aborted)
   924				btrfs_try_granting_tickets(fs_info, space_info);
   925		}
   926		return (tickets_id != space_info->tickets_id);
   927	}
   928	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60767 bytes --]

Re: [PATCH v3 4/5] btrfs: do not infinite loop in data reclaim if we aborted

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4597 bytes --]

Hi Josef,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.15-rc3]
[cannot apply to kdave/for-next next-20210922]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Josef-Bacik/Miscellaneous-error-handling-patches/20210929-185151
base:    5816b3e6577eaa676ceb00a848f0fd65fe2adc29
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4d626ae95cb373b954751bcdadacf6b0f92f3a6c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Josef-Bacik/Miscellaneous-error-handling-patches/20210929-185151
        git checkout 4d626ae95cb373b954751bcdadacf6b0f92f3a6c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/space-info.c: In function 'maybe_fail_all_tickets':
>> fs/btrfs/space-info.c:888:30: error: implicit declaration of function 'btrfs_has_fs_error'; did you mean 'btrfs_handle_fs_error'? [-Werror=implicit-function-declaration]
     888 |         const bool aborted = btrfs_has_fs_error(fs_info);
         |                              ^~~~~~~~~~~~~~~~~~
         |                              btrfs_handle_fs_error
   cc1: all warnings being treated as errors


vim +888 fs/btrfs/space-info.c

   867	
   868	/*
   869	 * maybe_fail_all_tickets - we've exhausted our flushing, start failing tickets
   870	 * @fs_info - fs_info for this fs
   871	 * @space_info - the space info we were flushing
   872	 *
   873	 * We call this when we've exhausted our flushing ability and haven't made
   874	 * progress in satisfying tickets.  The reservation code handles tickets in
   875	 * order, so if there is a large ticket first and then smaller ones we could
   876	 * very well satisfy the smaller tickets.  This will attempt to wake up any
   877	 * tickets in the list to catch this case.
   878	 *
   879	 * This function returns true if it was able to make progress by clearing out
   880	 * other tickets, or if it stumbles across a ticket that was smaller than the
   881	 * first ticket.
   882	 */
   883	static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
   884					   struct btrfs_space_info *space_info)
   885	{
   886		struct reserve_ticket *ticket;
   887		u64 tickets_id = space_info->tickets_id;
 > 888		const bool aborted = btrfs_has_fs_error(fs_info);
   889	
   890		trace_btrfs_fail_all_tickets(fs_info, space_info);
   891	
   892		if (btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
   893			btrfs_info(fs_info, "cannot satisfy tickets, dumping space info");
   894			__btrfs_dump_space_info(fs_info, space_info);
   895		}
   896	
   897		while (!list_empty(&space_info->tickets) &&
   898		       tickets_id == space_info->tickets_id) {
   899			ticket = list_first_entry(&space_info->tickets,
   900						  struct reserve_ticket, list);
   901	
   902			if (!aborted && ticket->steal &&
   903			    steal_from_global_rsv(fs_info, space_info, ticket))
   904				return true;
   905	
   906			if (!aborted && btrfs_test_opt(fs_info, ENOSPC_DEBUG))
   907				btrfs_info(fs_info, "failing ticket with %llu bytes",
   908					   ticket->bytes);
   909	
   910			remove_ticket(space_info, ticket);
   911			if (aborted)
   912				ticket->error = -EIO;
   913			else
   914				ticket->error = -ENOSPC;
   915			wake_up(&ticket->wait);
   916	
   917			/*
   918			 * We're just throwing tickets away, so more flushing may not
   919			 * trip over btrfs_try_granting_tickets, so we need to call it
   920			 * here to see if we can make progress with the next ticket in
   921			 * the list.
   922			 */
   923			if (!aborted)
   924				btrfs_try_granting_tickets(fs_info, space_info);
   925		}
   926		return (tickets_id != space_info->tickets_id);
   927	}
   928	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 60767 bytes --]

[PATCH v3 5/5] btrfs: fix abort logic in btrfs_replace_file_extents

[Josef Bacik]

Error injection testing uncovered a case where we'd end up with a
corrupt file system with a missing extent in the middle of a file.  This
occurs because the if statement to decide if we should abort is wrong.
The only way we would abort in this case is if we got a ret !=
-EOPNOTSUPP and we called from the file clone code.  However the
prealloc code uses this path too.  Instead we need to abort if there is
an error, and the only error we _don't_ abort on is -EOPNOTSUPP and only
if we came from the clone file code.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/file.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fdceab28587e..f9a1498cf030 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2710,8 +2710,9 @@ int btrfs_replace_file_extents(struct btrfs_inode *inode,
 			 * returned by __btrfs_drop_extents() without having
 			 * changed anything in the file.
 			 */
-			if (extent_info && !extent_info->is_new_extent &&
-			    ret && ret != -EOPNOTSUPP)
+			if (ret &&
+			    (ret != -EOPNOTSUPP ||
+			     (extent_info && extent_info->is_new_extent)))
 				btrfs_abort_transaction(trans, ret);
 			break;
 		}
-- 
2.26.3

Re: [PATCH v3 5/5] btrfs: fix abort logic in btrfs_replace_file_extents

[Filipe Manana]

On Mon, Sep 27, 2021 at 7:07 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Error injection testing uncovered a case where we'd end up with a
> corrupt file system with a missing extent in the middle of a file.  This
> occurs because the if statement to decide if we should abort is wrong.
> The only way we would abort in this case is if we got a ret !=
> -EOPNOTSUPP and we called from the file clone code.  However the
> prealloc code uses this path too.  Instead we need to abort if there is
> an error, and the only error we _don't_ abort on is -EOPNOTSUPP and only
> if we came from the clone file code.

Looks good, but the comment above the conditional in the code needs to
be updated, to reflect the new logic as mentioned in the changelog.

Thanks.

>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/file.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index fdceab28587e..f9a1498cf030 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2710,8 +2710,9 @@ int btrfs_replace_file_extents(struct btrfs_inode *inode,
>                          * returned by __btrfs_drop_extents() without having
>                          * changed anything in the file.
>                          */
> -                       if (extent_info && !extent_info->is_new_extent &&
> -                           ret && ret != -EOPNOTSUPP)
> +                       if (ret &&
> +                           (ret != -EOPNOTSUPP ||
> +                            (extent_info && extent_info->is_new_extent)))
>                                 btrfs_abort_transaction(trans, ret);
>                         break;
>                 }
> --
> 2.26.3
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”