public inbox for linux-btrfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] btrfs: avoid full commit due to race when logging dentry deletion
@ 2022-03-04 18:10 fdmanana
  2022-03-05 23:19 ` Zygo Blaxell
  2022-03-07 12:39 ` David Sterba
  0 siblings, 2 replies; 4+ messages in thread
From: fdmanana @ 2022-03-04 18:10 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

During a rename, when logging that a directory entry was deleted, we may
race with another task that is logging the directory. Even though the
directory is locked at the VFS level, its logging can be triggered when
other task is logging some other inode that had, or still has, a dentry
in the directory (because its last_unlink_trans matches the current
transaction).

The chances are slim, and if the race happens, recording the deletion
through insert_dir_log_key() can fail with -EEXIST and result in marking
the log for a full transaction commit, which will make the next fsync
fallback to a transaction commit. The opposite can also happen, we log the
key before the other task attempts to insert the same key, in which case
it fails with -EEXIST and fallsback to a transaction commit or trigger an
assertion at process_dir_items_leaf() due to the unexpected -EEXIST error.

So make that code that records a dentry deletion to be inside a critical
section delimited by the directory's log mutex.

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

David, this can be optionally squashed into the following patch:

   "btrfs: avoid logging all directory changes during renames"

(misc-next only)

 fs/btrfs/tree-log.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 4f61b38bb186..571dae8ad65e 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -7015,6 +7015,17 @@ void btrfs_log_new_name(struct btrfs_trans_handle *trans,
 			goto out;
 		}
 
+		/*
+		 * Other concurrent task might be logging the old directory,
+		 * as it can be triggered when logging other inode that had or
+		 * still has a dentry in the old directory. So take the old
+		 * directory's log_mutex to prevent getting an -EEXIST when
+		 * logging a key to record the deletion, or having that other
+		 * task logging the old directory get an -EEXIST if it attempts
+		 * to log the same key after we just did it. In both cases that
+		 * would result in falling back to a transaction commit.
+		 */
+		mutex_lock(&old_dir->log_mutex);
 		ret = del_logged_dentry(trans, log, path, btrfs_ino(old_dir),
 					old_dentry->d_name.name,
 					old_dentry->d_name.len, old_dir_index);
@@ -7028,6 +7039,7 @@ void btrfs_log_new_name(struct btrfs_trans_handle *trans,
 						 btrfs_ino(old_dir),
 						 old_dir_index, old_dir_index);
 		}
+		mutex_unlock(&old_dir->log_mutex);
 
 		btrfs_free_path(path);
 		if (ret < 0)
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-07 12:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-04 18:10 [PATCH] btrfs: avoid full commit due to race when logging dentry deletion fdmanana
2022-03-05 23:19 ` Zygo Blaxell
2022-03-07 11:05   ` Filipe Manana
2022-03-07 12:39 ` David Sterba

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox