[PATCH v2 0/2] ext4: mark FC as ineligible using an handle

[PATCH v2 0/2] ext4: mark FC as ineligible using an handle

[Luis Henriques (SUSE)]

Hi!

Changes since v1:
* only the second patch has been changed to drop the call to
  ext4_fc_mark_ineligible() in the error path.  Commit text has also been
  adjusted accordingly.

And here's the original cover-letter:

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       |  3 ++-
 2 files changed, 13 insertions(+), 9 deletions(-)

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

[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 <jack@suse.cz>
Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
---
 fs/ext4/fast_commit.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 3926a05eceee..c330efd771d1 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -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;

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

[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 moves the call to this function so that an handle
can be used.  If a transaction fails to start, then there's not point in
trying to mark the filesystem as ineligible, and an error will eventually be
returned to user-space.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
---
 fs/ext4/xattr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 46ce2f21fef9..aea9e3c405f1 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2559,6 +2559,8 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 
 		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 +2568,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;
 }

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

[Jan Kara]

On Mon 23-09-24 11:49:08, Luis Henriques (SUSE) wrote:
> 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 <jack@suse.cz>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/fast_commit.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 3926a05eceee..c330efd771d1 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -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 <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Mon 23-09-24 11:49:09, Luis Henriques (SUSE) wrote:
> 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 moves the call to this function so that an handle
> can be used.  If a transaction fails to start, then there's not point in
> trying to mark the filesystem as ineligible, and an error will eventually be
> returned to user-space.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/xattr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 46ce2f21fef9..aea9e3c405f1 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2559,6 +2559,8 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
>  
>  		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 +2568,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 <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 0/2] ext4: mark FC as ineligible using an handle

[Theodore Ts'o]

On Mon, 23 Sep 2024 11:49:07 +0100, Luis Henriques (SUSE) wrote:
> Changes since v1:
> * only the second patch has been changed to drop the call to
>   ext4_fc_mark_ineligible() in the error path.  Commit text has also been
>   adjusted accordingly.
> 
> And here's the original cover-letter:
> 
> [...]

Applied, thanks!

[1/2] ext4: use handle to mark fc as ineligible in __track_dentry_update()
      commit: faab35a0370fd6e0821c7a8dd213492946fc776f
[2/2] ext4: mark fc as ineligible using an handle in ext4_xattr_set()
      commit: 04e6ce8f06d161399e5afde3df5dcfa9455b4952

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>