diff mbox series

[6/8] btrfs: simplify update of last_dir_index_offset when logging a directory

Message ID ff77f41924e197d99e62ef323f03467c87ef43a0.1673361215.git.fdmanana@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: some log tree fixes and cleanups | expand

Commit Message

Filipe Manana Jan. 10, 2023, 2:56 p.m. UTC 
From: Filipe Manana <fdmanana@suse.com>

When logging a directory, we always set the inode's last_dir_index_offset
to the offset of the last dir index item we found. This is using an extra
field in the log context structure, and it makes more sense to update it
only after we insert dir index items, and we could directly update the
inode's last_dir_index_offset field instead.

So make this simpler by updating the inode's last_dir_index_offset only
when we actually insert dir index keys in the log tree, and getting rid
of the last_dir_item_offset field in the log context structure.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 23 +++++++++++++++++------
 fs/btrfs/tree-log.h |  2 --
 2 files changed, 17 insertions(+), 8 deletions(-)

Comments

Filipe Manana Jan. 31, 2023, 5:42 p.m. UTC | #1 
On Tue, Jan 10, 2023 at 3:20 PM <fdmanana@kernel.org> wrote:
>
> From: Filipe Manana <fdmanana@suse.com>
>
> When logging a directory, we always set the inode's last_dir_index_offset
> to the offset of the last dir index item we found. This is using an extra
> field in the log context structure, and it makes more sense to update it
> only after we insert dir index items, and we could directly update the
> inode's last_dir_index_offset field instead.
>
> So make this simpler by updating the inode's last_dir_index_offset only
> when we actually insert dir index keys in the log tree, and getting rid
> of the last_dir_item_offset field in the log context structure.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reported-by: David Arendt <admin@prnet.org>
Link: https://lore.kernel.org/linux-btrfs/ae169fc6-f504-28f0-a098-6fa6a4dfb612@leemhuis.info/
Reported-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Link: https://lore.kernel.org/linux-btrfs/Y8voyTXdnPDz8xwY@mail.gmail.com/
Reported-by: Hunter Wardlaw <wardlawhunter@gmail.com>
Link: https://bugzilla.suse.com/show_bug.cgi?id=1207231
CC: stable@vger.kernel.org # 6.1+

David, can you please add the following tags to the patch?
It happens to fix an issue those users reported, all on 6.1 only.

Thanks.

> ---
>  fs/btrfs/tree-log.c | 23 +++++++++++++++++------
>  fs/btrfs/tree-log.h |  2 --
>  2 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index d43261545264..58599189bd18 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3576,17 +3576,19 @@ static noinline int insert_dir_log_key(struct btrfs_trans_handle *trans,
>  }
>
>  static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
> -                                struct btrfs_root *log,
> +                                struct btrfs_inode *inode,
>                                  struct extent_buffer *src,
>                                  struct btrfs_path *dst_path,
>                                  int start_slot,
>                                  int count)
>  {
> +       struct btrfs_root *log = inode->root->log_root;
>         char *ins_data = NULL;
>         struct btrfs_item_batch batch;
>         struct extent_buffer *dst;
>         unsigned long src_offset;
>         unsigned long dst_offset;
> +       u64 last_index;
>         struct btrfs_key key;
>         u32 item_size;
>         int ret;
> @@ -3644,6 +3646,19 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
>         src_offset = btrfs_item_ptr_offset(src, start_slot + count - 1);
>         copy_extent_buffer(dst, src, dst_offset, src_offset, batch.total_data_size);
>         btrfs_release_path(dst_path);
> +
> +       last_index = batch.keys[count - 1].offset;
> +       ASSERT(last_index > inode->last_dir_index_offset);
> +
> +       /*
> +        * If for some unexpected reason the last item's index is not greater
> +        * than the last index we logged, warn and return an error to fallback
> +        * to a transaction commit.
> +        */
> +       if (WARN_ON(last_index <= inode->last_dir_index_offset))
> +               ret = -EUCLEAN;
> +       else
> +               inode->last_dir_index_offset = last_index;
>  out:
>         kfree(ins_data);
>
> @@ -3693,7 +3708,6 @@ static int process_dir_items_leaf(struct btrfs_trans_handle *trans,
>                 }
>
>                 di = btrfs_item_ptr(src, i, struct btrfs_dir_item);
> -               ctx->last_dir_item_offset = key.offset;
>
>                 /*
>                  * Skip ranges of items that consist only of dir item keys created
> @@ -3756,7 +3770,7 @@ static int process_dir_items_leaf(struct btrfs_trans_handle *trans,
>         if (batch_size > 0) {
>                 int ret;
>
> -               ret = flush_dir_items_batch(trans, log, src, dst_path,
> +               ret = flush_dir_items_batch(trans, inode, src, dst_path,
>                                             batch_start, batch_size);
>                 if (ret < 0)
>                         return ret;
> @@ -4044,7 +4058,6 @@ static noinline int log_directory_changes(struct btrfs_trans_handle *trans,
>
>         min_key = BTRFS_DIR_START_INDEX;
>         max_key = 0;
> -       ctx->last_dir_item_offset = inode->last_dir_index_offset;
>
>         while (1) {
>                 ret = log_dir_items(trans, inode, path, dst_path,
> @@ -4056,8 +4069,6 @@ static noinline int log_directory_changes(struct btrfs_trans_handle *trans,
>                 min_key = max_key + 1;
>         }
>
> -       inode->last_dir_index_offset = ctx->last_dir_item_offset;
> -
>         return 0;
>  }
>
> diff --git a/fs/btrfs/tree-log.h b/fs/btrfs/tree-log.h
> index 85b43075ac58..85cd24cb0540 100644
> --- a/fs/btrfs/tree-log.h
> +++ b/fs/btrfs/tree-log.h
> @@ -24,8 +24,6 @@ struct btrfs_log_ctx {
>         bool logging_new_delayed_dentries;
>         /* Indicate if the inode being logged was logged before. */
>         bool logged_before;
> -       /* Tracks the last logged dir item/index key offset. */
> -       u64 last_dir_item_offset;
>         struct inode *inode;
>         struct list_head list;
>         /* Only used for fast fsyncs. */
> --
> 2.35.1
>
David Sterba Feb. 6, 2023, 6:40 p.m. UTC | #2 
On Tue, Jan 31, 2023 at 05:42:12PM +0000, Filipe Manana wrote:
> On Tue, Jan 10, 2023 at 3:20 PM <fdmanana@kernel.org> wrote:
> >
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > When logging a directory, we always set the inode's last_dir_index_offset
> > to the offset of the last dir index item we found. This is using an extra
> > field in the log context structure, and it makes more sense to update it
> > only after we insert dir index items, and we could directly update the
> > inode's last_dir_index_offset field instead.
> >
> > So make this simpler by updating the inode's last_dir_index_offset only
> > when we actually insert dir index keys in the log tree, and getting rid
> > of the last_dir_item_offset field in the log context structure.
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> 
> Reported-by: David Arendt <admin@prnet.org>
> Link: https://lore.kernel.org/linux-btrfs/ae169fc6-f504-28f0-a098-6fa6a4dfb612@leemhuis.info/
> Reported-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> Link: https://lore.kernel.org/linux-btrfs/Y8voyTXdnPDz8xwY@mail.gmail.com/
> Reported-by: Hunter Wardlaw <wardlawhunter@gmail.com>
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1207231
> CC: stable@vger.kernel.org # 6.1+
> 
> David, can you please add the following tags to the patch?
> It happens to fix an issue those users reported, all on 6.1 only.

Updated and added to pull request queue, thanks.
diff mbox series

Patch

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index d43261545264..58599189bd18 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3576,17 +3576,19 @@  static noinline int insert_dir_log_key(struct btrfs_trans_handle *trans,
 }
 
 static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
-				 struct btrfs_root *log,
+				 struct btrfs_inode *inode,
 				 struct extent_buffer *src,
 				 struct btrfs_path *dst_path,
 				 int start_slot,
 				 int count)
 {
+	struct btrfs_root *log = inode->root->log_root;
 	char *ins_data = NULL;
 	struct btrfs_item_batch batch;
 	struct extent_buffer *dst;
 	unsigned long src_offset;
 	unsigned long dst_offset;
+	u64 last_index;
 	struct btrfs_key key;
 	u32 item_size;
 	int ret;
@@ -3644,6 +3646,19 @@  static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
 	src_offset = btrfs_item_ptr_offset(src, start_slot + count - 1);
 	copy_extent_buffer(dst, src, dst_offset, src_offset, batch.total_data_size);
 	btrfs_release_path(dst_path);
+
+	last_index = batch.keys[count - 1].offset;
+	ASSERT(last_index > inode->last_dir_index_offset);
+
+	/*
+	 * If for some unexpected reason the last item's index is not greater
+	 * than the last index we logged, warn and return an error to fallback
+	 * to a transaction commit.
+	 */
+	if (WARN_ON(last_index <= inode->last_dir_index_offset))
+		ret = -EUCLEAN;
+	else
+		inode->last_dir_index_offset = last_index;
 out:
 	kfree(ins_data);
 
@@ -3693,7 +3708,6 @@  static int process_dir_items_leaf(struct btrfs_trans_handle *trans,
 		}
 
 		di = btrfs_item_ptr(src, i, struct btrfs_dir_item);
-		ctx->last_dir_item_offset = key.offset;
 
 		/*
 		 * Skip ranges of items that consist only of dir item keys created
@@ -3756,7 +3770,7 @@  static int process_dir_items_leaf(struct btrfs_trans_handle *trans,
 	if (batch_size > 0) {
 		int ret;
 
-		ret = flush_dir_items_batch(trans, log, src, dst_path,
+		ret = flush_dir_items_batch(trans, inode, src, dst_path,
 					    batch_start, batch_size);
 		if (ret < 0)
 			return ret;
@@ -4044,7 +4058,6 @@  static noinline int log_directory_changes(struct btrfs_trans_handle *trans,
 
 	min_key = BTRFS_DIR_START_INDEX;
 	max_key = 0;
-	ctx->last_dir_item_offset = inode->last_dir_index_offset;
 
 	while (1) {
 		ret = log_dir_items(trans, inode, path, dst_path,
@@ -4056,8 +4069,6 @@  static noinline int log_directory_changes(struct btrfs_trans_handle *trans,
 		min_key = max_key + 1;
 	}
 
-	inode->last_dir_index_offset = ctx->last_dir_item_offset;
-
 	return 0;
 }
 
diff --git a/fs/btrfs/tree-log.h b/fs/btrfs/tree-log.h
index 85b43075ac58..85cd24cb0540 100644
--- a/fs/btrfs/tree-log.h
+++ b/fs/btrfs/tree-log.h
@@ -24,8 +24,6 @@  struct btrfs_log_ctx {
 	bool logging_new_delayed_dentries;
 	/* Indicate if the inode being logged was logged before. */
 	bool logged_before;
-	/* Tracks the last logged dir item/index key offset. */
-	u64 last_dir_item_offset;
 	struct inode *inode;
 	struct list_head list;
 	/* Only used for fast fsyncs. */