diff mbox series

btrfs: avoid unnecessarily logging directories that had no changes

Message ID ffa14771bb6d672a2a74d92625bd024013b3f8ce.1627580467.git.fdmanana@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: avoid unnecessarily logging directories that had no changes | expand

Commit Message

Filipe Manana July 29, 2021, 5:52 p.m. UTC 
From: Filipe Manana <fdmanana@suse.com>

There are several cases where when logging an inode we need to log its
parent directories or logging subdirectories when logging a directory.

There are cases however where we end up logging a directory even if it was
not changed in the current transaction, no dentries added or removed since
the last transaction. While this is harmless from a functional point of
view, it is a waste time as it brings no advantage.

One example where this is triggered is the following:

  $ mkfs.btrfs -f /dev/sdc
  $ mount /dev/sdc /mnt

  $ mkdir /mnt/A
  $ mkdir /mnt/B
  $ mkdir /mnt/C

  $ touch /mnt/A/foo
  $ ln /mnt/A/foo /mnt/B/bar
  $ ln /mnt/A/foo /mnt/C/baz

  $ sync

  $ rm -f /mnt/A/foo
  $ xfs_io -c "fsync" /mnt/B/bar

This last fsync ends up logging directories A, B and C, however we only
need to log directory A, as B and C were not changed since the last
transaction commit.

So fix this by changing need_log_inode(), to return false in case the
given inode is a directory and has a ->last_trans value smaller than the
current transaction's ID.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

David Sterba Aug. 16, 2021, 1:02 p.m. UTC | #1 
On Thu, Jul 29, 2021 at 06:52:46PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There are several cases where when logging an inode we need to log its
> parent directories or logging subdirectories when logging a directory.
> 
> There are cases however where we end up logging a directory even if it was
> not changed in the current transaction, no dentries added or removed since
> the last transaction. While this is harmless from a functional point of
> view, it is a waste time as it brings no advantage.
> 
> One example where this is triggered is the following:
> 
>   $ mkfs.btrfs -f /dev/sdc
>   $ mount /dev/sdc /mnt
> 
>   $ mkdir /mnt/A
>   $ mkdir /mnt/B
>   $ mkdir /mnt/C
> 
>   $ touch /mnt/A/foo
>   $ ln /mnt/A/foo /mnt/B/bar
>   $ ln /mnt/A/foo /mnt/C/baz
> 
>   $ sync
> 
>   $ rm -f /mnt/A/foo
>   $ xfs_io -c "fsync" /mnt/B/bar
> 
> This last fsync ends up logging directories A, B and C, however we only
> need to log directory A, as B and C were not changed since the last
> transaction commit.
> 
> So fix this by changing need_log_inode(), to return false in case the
> given inode is a directory and has a ->last_trans value smaller than the
> current transaction's ID.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.
diff mbox series

Patch

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index d2039743ecf2..b4e820cde461 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -5610,6 +5610,13 @@  static int btrfs_log_inode(struct btrfs_trans_handle *trans,
 static bool need_log_inode(struct btrfs_trans_handle *trans,
 			   struct btrfs_inode *inode)
 {
+	/*
+	 * If a directory was not modified, no dentries added or removed, we can
+	 * and should avoid logging it.
+	 */
+	if (S_ISDIR(inode->vfs_inode.i_mode) && inode->last_trans < trans->transid)
+		return false;
+
 	/*
 	 * If this inode does not have new/updated/deleted xattrs since the last
 	 * time it was logged and is flagged as logged in the current transaction,