diff mbox series

[v4,2/5] btrfs: change error handling for btrfs_delete_*_in_log

Message ID e26773a5be8c7e52b6379343514c0b7eb46deb0e.1633465964.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series Miscellaneous error handling patches | expand

Commit Message

Josef Bacik Oct. 5, 2021, 8:35 p.m. UTC 
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 since these are simply log tree related errors, we can mark the
trans as needing a full commit.  Then if the error was truly
catastrophic we'll hit it during the normal commit and abort as
appropriate.

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(-)

Comments

Filipe Manana Oct. 6, 2021, 5:17 p.m. UTC | #1 
On Tue, Oct 5, 2021 at 9:37 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 since these are simply log tree related errors, we can mark the
> trans as needing a full commit.  Then if the error was truly
> catastrophic we'll hit it during the normal commit and abort as
> appropriate.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

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

Now it looks good, thanks.

> ---
>  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 11ba673d195e..df716d1bd2f1 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4116,19 +4116,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 7a7fe0d74c35..a99aa57b8886 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
>
David Sterba Oct. 6, 2021, 7:03 p.m. UTC | #2 
On Tue, Oct 05, 2021 at 04:35:24PM -0400, Josef Bacik wrote:
> @@ -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);

There was a minor conflict with recent Filipe's cleanups simplifying the
error and ENOENT values, in this case the 'err != -ENOENT' was dropped,
so I resolved it by keeping the condition after Filipe's changes so the
final result is

	if (err < 0)
		btrfs_set_log_full_commit(trans)
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 11ba673d195e..df716d1bd2f1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4116,19 +4116,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 7a7fe0d74c35..a99aa57b8886 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,