ext4: mark FC as ineligible using an handle

PATCH Superseded Luis Henriques (SUSE) 2 participants 6 messages Sep 19, 2024 – Sep 20, 2024 ext4 file system

lore

Share & tools

lore

Luis Henriques (SUSE)

author

2024-09-19 09:39 UTC #0 lore raw

cover ext4: mark FC as ineligible using an handle

Hi!

The following patches were suggested by Jan Kara[1] some time ago. Basically, they try to address a race that exists in several places, where function ext4_fc_mark_ineligible() is called with a NULL handle: in these cases, marking the filesystem as ineligible may effectively happen _after_ the fast-commit is done.

There are several places where this happen, and the two patches that follow try to address some of them. The first patch fixes the calls that exist in __track_dentry_update().

The second patch _partially_ fixes the call in ext4_xattr_set() -- this is because it may fail to obtain an handle. And, to be honest, I'm not sure what to do in this case: try again to obtain a new handle just for marking the fc, with a lower value for credits? or leave it as-is, with the NULL?

The other two missing callers are the ioctl for filesystem resize and ext4_evict_inode().

[1] https://lore.kernel.org/all/20240724101504.e2t4pvgw6td7rrmm@quack3/

Luis Henriques (SUSE) (2):
ext4: use handle to mark fc as ineligible in __track_dentry_update()
ext4: mark fc as ineligible using an handle in ext4_xattr_set()

 fs/ext4/fast_commit.c | 19 +++++++++++--------
 fs/ext4/xattr.c       |  5 ++++-
 2 files changed, 15 insertions(+), 9 deletions(-)

Luis Henriques (SUSE)

author

2024-09-19 09:39 UTC #1 lore raw

PATCH 1/2 ext4: use handle to mark fc as ineligible in __track_dentry_update()

Calling ext4_fc_mark_ineligible() with a NULL handle is racy and may result in a fast-commit being done before the filesystem is effectively marked as ineligible. This patch fixes the calls to this function in __track_dentry_update() by adding an extra parameter to the callback used in ext4_fc_track_template().

Suggested-by Jan Kara <[email protected]>

Signed-off-by Luis Henriques (SUSE) <[email protected]>

---
 fs/ext4/fast_commit.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

fs/ext4/fast_commit.c +11 -8

index 3926a05eceee..c330efd771d1 100644
@@ -372,7 +372,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
  */
 static int ext4_fc_track_template(
 	handle_t *handle, struct inode *inode,
-	int (*__fc_track_fn)(struct inode *, void *, bool),
+	int (*__fc_track_fn)(handle_t *handle, struct inode *, void *, bool),
 	void *args, int enqueue)
 {
 	bool update = false;

@@ -389,7 +389,7 @@ static int ext4_fc_track_template(
 		ext4_fc_reset_inode(inode);
 		ei->i_sync_tid = tid;
 	}
-	ret = __fc_track_fn(inode, args, update);
+	ret = __fc_track_fn(handle, inode, args, update);
 	mutex_unlock(&ei->i_fc_lock);
 
 	if (!enqueue)

@@ -413,7 +413,8 @@ struct __track_dentry_update_args {
 };
 
 /* __track_fn for directory entry updates. Called with ei->i_fc_lock. */
-static int __track_dentry_update(struct inode *inode, void *arg, bool update)
+static int __track_dentry_update(handle_t *handle, struct inode *inode,
+				 void *arg, bool update)
 {
 	struct ext4_fc_dentry_update *node;
 	struct ext4_inode_info *ei = EXT4_I(inode);

@@ -428,14 +429,14 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
 
 	if (IS_ENCRYPTED(dir)) {
 		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_ENCRYPTED_FILENAME,
-					NULL);
+					handle);
 		mutex_lock(&ei->i_fc_lock);
 		return -EOPNOTSUPP;
 	}
 
 	node = kmem_cache_alloc(ext4_fc_dentry_cachep, GFP_NOFS);
 	if (!node) {
-		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
+		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
 		mutex_lock(&ei->i_fc_lock);
 		return -ENOMEM;
 	}

@@ -447,7 +448,7 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
 		node->fcd_name.name = kmalloc(dentry->d_name.len, GFP_NOFS);
 		if (!node->fcd_name.name) {
 			kmem_cache_free(ext4_fc_dentry_cachep, node);
-			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
+			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
 			mutex_lock(&ei->i_fc_lock);
 			return -ENOMEM;
 		}

@@ -569,7 +570,8 @@ void ext4_fc_track_create(handle_t *handle, struct dentry *dentry)
 }
 
 /* __track_fn for inode tracking */
-static int __track_inode(struct inode *inode, void *arg, bool update)
+static int __track_inode(handle_t *handle, struct inode *inode, void *arg,
+			 bool update)
 {
 	if (update)
 		return -EEXIST;

@@ -607,7 +609,8 @@ struct __track_range_args {
 };
 
 /* __track_fn for tracking data updates */
-static int __track_range(struct inode *inode, void *arg, bool update)
+static int __track_range(handle_t *handle, struct inode *inode, void *arg,
+			 bool update)
 {
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	ext4_lblk_t oldstart;

Jan Kara

reviewer Reviewed-by on v1 1/2

2024-09-19 21:47 UTC #2 lore raw

Re: [PATCH 1/2] ext4: use handle to mark fc as ineligible in __track_dentry_update()

Quoted 8 lines · Luis Henriques (SUSE)

Calling ext4_fc_mark_ineligible() with a NULL handle is racy and may result in a fast-commit being done before the filesystem is effectively marked as ineligible. This patch fixes the calls to this function in __track_dentry_update() by adding an extra parameter to the callback used in ext4_fc_track_template().
Suggested-by Jan Kara <[email protected]>
Signed-off-by Luis Henriques (SUSE) <[email protected]>

This looks good. Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 82 lines

---
 fs/ext4/fast_commit.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

fs/ext4/fast_commit.c +11 -8

index 3926a05eceee..c330efd771d1 100644
@@ -372,7 +372,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
  */
 static int ext4_fc_track_template(
 	handle_t *handle, struct inode *inode,
-	int (*__fc_track_fn)(struct inode *, void *, bool),
+	int (*__fc_track_fn)(handle_t *handle, struct inode *, void *, bool),
 	void *args, int enqueue)
 {
 	bool update = false;

@@ -389,7 +389,7 @@ static int ext4_fc_track_template(
 		ext4_fc_reset_inode(inode);
 		ei->i_sync_tid = tid;
 	}
-	ret = __fc_track_fn(inode, args, update);
+	ret = __fc_track_fn(handle, inode, args, update);
 	mutex_unlock(&ei->i_fc_lock);
 
 	if (!enqueue)

@@ -413,7 +413,8 @@ struct __track_dentry_update_args {
 };
 
 /* __track_fn for directory entry updates. Called with ei->i_fc_lock. */
-static int __track_dentry_update(struct inode *inode, void *arg, bool update)
+static int __track_dentry_update(handle_t *handle, struct inode *inode,
+				 void *arg, bool update)
 {
 	struct ext4_fc_dentry_update *node;
 	struct ext4_inode_info *ei = EXT4_I(inode);

@@ -428,14 +429,14 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
 
 	if (IS_ENCRYPTED(dir)) {
 		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_ENCRYPTED_FILENAME,
-					NULL);
+					handle);
 		mutex_lock(&ei->i_fc_lock);
 		return -EOPNOTSUPP;
 	}
 
 	node = kmem_cache_alloc(ext4_fc_dentry_cachep, GFP_NOFS);
 	if (!node) {
-		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
+		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
 		mutex_lock(&ei->i_fc_lock);
 		return -ENOMEM;
 	}

@@ -447,7 +448,7 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
 		node->fcd_name.name = kmalloc(dentry->d_name.len, GFP_NOFS);
 		if (!node->fcd_name.name) {
 			kmem_cache_free(ext4_fc_dentry_cachep, node);
-			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
+			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
 			mutex_lock(&ei->i_fc_lock);
 			return -ENOMEM;
 		}

@@ -569,7 +570,8 @@ void ext4_fc_track_create(handle_t *handle, struct dentry *dentry)
 }
 
 /* __track_fn for inode tracking */
-static int __track_inode(struct inode *inode, void *arg, bool update)
+static int __track_inode(handle_t *handle, struct inode *inode, void *arg,
+			 bool update)
 {
 	if (update)
 		return -EEXIST;

@@ -607,7 +609,8 @@ struct __track_range_args {
 };
 
 /* __track_fn for tracking data updates */
-static int __track_range(struct inode *inode, void *arg, bool update)
+static int __track_range(handle_t *handle, struct inode *inode, void *arg,
+			 bool update)
 {
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	ext4_lblk_t oldstart;

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Luis Henriques (SUSE)

author

2024-09-19 09:39 UTC #3 lore raw

PATCH 2/2 ext4: mark fc as ineligible using an handle in ext4_xattr_set()

In reply to Luis Henriques (SUSE)

Calling ext4_fc_mark_ineligible() with a NULL handle is racy and may result in a fast-commit being done before the filesystem is effectively marked as ineligible. This patch reduces the risk of this happening in function ext4_xattr_set() by using an handle if one is available.

Suggested-by Jan Kara <[email protected]>

Signed-off-by Luis Henriques (SUSE) <[email protected]>

---
 fs/ext4/xattr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/ext4/xattr.c +4 -1

index 46ce2f21fef9..dbe4d11cd332 100644
@@ -2554,11 +2554,15 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
 	if (IS_ERR(handle)) {
 		error = PTR_ERR(handle);
+		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
+					NULL);
 	} else {
 		int error2;
 
 		error = ext4_xattr_set_handle(handle, inode, name_index, name,
 					      value, value_len, flags);
+		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
+					handle);
 		error2 = ext4_journal_stop(handle);
 		if (error == -ENOSPC &&
 		    ext4_should_retry_alloc(sb, &retries))

@@ -2566,7 +2570,6 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 		if (error == 0)
 			error = error2;
 	}
-	ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR, NULL);
 
 	return error;
 }

Jan Kara

reviewer

2024-09-19 21:47 UTC #4 lore raw

Re: [PATCH 2/2] ext4: mark fc as ineligible using an handle in ext4_xattr_set()

Quoted 7 lines · Luis Henriques (SUSE)

Calling ext4_fc_mark_ineligible() with a NULL handle is racy and may result in a fast-commit being done before the filesystem is effectively marked as ineligible. This patch reduces the risk of this happening in function ext4_xattr_set() by using an handle if one is available.
Suggested-by Jan Kara <[email protected]>
Signed-off-by Luis Henriques (SUSE) <[email protected]>

One comment below:

fs/ext4/xattr.c ext4_xattr_set(struct inode *inode, int name_index, const char *name, · line 2558

Quoted 10 lines

fs/ext4/xattr.c +2 -0

index 46ce2f21fef9..dbe4d11cd332 100644
@@ -2554,11 +2554,15 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
 	if (IS_ERR(handle)) {
 		error = PTR_ERR(handle);
+		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
+					NULL);

So when starting a transaction fails:

a) We have a big problem, the journal is aborted so marking fs ineligible is moot.

b) We don't set anything and bail with error to userspace so again marking fs as ineligible is pointless.

So there's no need to do anything in this case.

Honza

Quoted 18 lines

fs/ext4/xattr.c +2 -1

 	} else {
 		int error2;
 
 		error = ext4_xattr_set_handle(handle, inode, name_index, name,
 					      value, value_len, flags);
+		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
+					handle);
 		error2 = ext4_journal_stop(handle);
 		if (error == -ENOSPC &&
 		    ext4_should_retry_alloc(sb, &retries))

@@ -2566,7 +2570,6 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 		if (error == 0)
 			error = error2;
 	}
-	ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR, NULL);
 
 	return error;
 }

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Luis Henriques

author

2024-09-20 09:35 UTC #5 lore raw

Re: [PATCH 2/2] ext4: mark fc as ineligible using an handle in ext4_xattr_set()

Quoted 24 lines · Jan Kara

Quoted 7 lines · Luis Henriques (SUSE)
Calling ext4_fc_mark_ineligible() with a NULL handle is racy and may result in a fast-commit being done before the filesystem is effectively marked as ineligible. This patch reduces the risk of this happening in function ext4_xattr_set() by using an handle if one is available.
Suggested-by Jan Kara <[email protected]>
Signed-off-by Luis Henriques (SUSE) <[email protected]>
One comment below:
fs/ext4/xattr.c ext4_xattr_set(struct inode *inode, int name_index, const char *name, · line 2558
Quoted 10 lines
fs/ext4/xattr.c +2 -0
index 46ce2f21fef9..dbe4d11cd332 100644
@@ -2554,11 +2554,15 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
 	if (IS_ERR(handle)) {
 		error = PTR_ERR(handle);
+		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
+					NULL);
So when starting a transaction fails:

Quoted 7 lines

a) We have a big problem, the journal is aborted so marking fs ineligible is moot.
b) We don't set anything and bail with error to userspace so again marking fs as ineligible is pointless.
So there's no need to do anything in this case.

Ah! I spent a good amount of time trying to understand if there was a point marking it as ineligible in that case, but couldn't reach a clear conclusion. That's why I decided to leave it there. And, hoping to get some early feedback, that's also why I decided to send these 2 patches first, because fixing ext4_evict_inode() will require a bit more re-work. The fix will have to deal with more error paths, and probably the call to ext4_journal_start() will need to be moved upper in the function.

And, as always, thanks a lot your review, Jan.

Cheers,

Signature · 1 line

Luís

Quoted 23 lines

Honza

Quoted 18 lines

fs/ext4/xattr.c +2 -1

 	} else {
 		int error2;
 
 		error = ext4_xattr_set_handle(handle, inode, name_index, name,
 					      value, value_len, flags);
+		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
+					handle);
 		error2 = ext4_journal_stop(handle);
 		if (error == -ENOSPC &&
 		    ext4_should_retry_alloc(sb, &retries))

@@ -2566,7 +2570,6 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 		if (error == 0)
 			error = error2;
 	}
-	ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR, NULL);
 
 	return error;
 }

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR