[PATCH 0/3] btrfs: fixes and cleanups setting/clearing properties

[PATCH 0/3] btrfs: fixes and cleanups setting/clearing properties

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Some fixes and cleanups to setting and clearing the compression xattr,
either directly through the xattr API (first patch) or through the
fileattr API (all 3 patches). Details in the change logs.

Filipe Manana (3):
  btrfs: validate properties before setting them
  btrfs: don't over reserve metadata space for property in btrfs_fileattr_set()
  btrfs: fix transaction abort logic in btrfs_fileattr_set()

 fs/btrfs/ioctl.c | 22 ++++++++++++++--------
 fs/btrfs/props.c | 16 +++++++++++++---
 2 files changed, 27 insertions(+), 11 deletions(-)

-- 
2.47.2

[PATCH 1/3] btrfs: validate properties before setting them

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We set the xattr and then attempt to apply the property. If the apply
fails we then attempt to delete the xattr to avoid an inconsistency.
However we don't verify if the deletion succeed, so if it fails we
leave an inconsistency between the state in the btree and the in-memory
inode.

So address this by validating first if we can apply the property, then
set the xattr, then apply the property, and this last step should not
fail since the validation succeeded before - assert that it does not fail
but leave code to attempt to delete the xattr if it happens, and then
abort the transaction only if the xattr delete failed.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/props.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index adc956432d2f..bb77d46376d4 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -127,14 +127,24 @@ int btrfs_set_prop(struct btrfs_trans_handle *trans, struct btrfs_inode *inode,
 		return ret;
 	}
 
+	ret = handler->validate(inode, value, value_len);
+	if (ret)
+		return ret;
 	ret = btrfs_setxattr(trans, &inode->vfs_inode, handler->xattr_name, value,
 			     value_len, flags);
 	if (ret)
 		return ret;
 	ret = handler->apply(inode, value, value_len);
-	if (ret) {
-		btrfs_setxattr(trans, &inode->vfs_inode, handler->xattr_name, NULL,
-			       0, flags);
+	/* We validated before, so it should not fail here. */
+	ASSERT(ret == 0);
+	if (unlikely(ret)) {
+		int ret2;
+
+		/* Try to delete xattr, if not possible abort transaction. */
+		ret2 = btrfs_setxattr(trans, &inode->vfs_inode, handler->xattr_name,
+				      NULL, 0, flags);
+		if (unlikely(ret2))
+			btrfs_abort_transaction(trans, ret2);
 		return ret;
 	}
 
-- 
2.47.2

[PATCH 2/3] btrfs: don't over reserve metadata space for property in btrfs_fileattr_set()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We are using 2 units for properties but we only set one property.
Fix this by using the correct amount: 1 unit.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9d47d16394fc..4b49b7e08425 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -392,9 +392,9 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 
 	/*
 	 * 1 for inode item
-	 * 2 for properties
+	 * 1 for property
 	 */
-	trans = btrfs_start_transaction(root, 3);
+	trans = btrfs_start_transaction(root, 2);
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-- 
2.47.2

[PATCH 3/3] btrfs: fix transaction abort logic in btrfs_fileattr_set()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no need to abort the transaction if we failed to set or delete a
property, as we haven't done any change. However we need to abort if we
set a property or delete a property and then fail to update the inode
item, as that would leave the inode's state in subvolume tree
inconsistent.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ioctl.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4b49b7e08425..c691dd48102e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -289,6 +289,7 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 	int ret;
 	const char *comp = NULL;
 	u32 inode_flags;
+	bool prop_set = false;
 
 	if (btrfs_root_readonly(root))
 		return -EROFS;
@@ -401,16 +402,15 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 	if (comp) {
 		ret = btrfs_set_prop(trans, inode, "btrfs.compression",
 				     comp, strlen(comp), 0);
-		if (unlikely(ret)) {
-			btrfs_abort_transaction(trans, ret);
+		if (ret)
 			goto out_end_trans;
-		}
+		prop_set = true;
 	} else {
 		ret = btrfs_set_prop(trans, inode, "btrfs.compression", NULL, 0, 0);
-		if (unlikely(ret && ret != -ENODATA)) {
-			btrfs_abort_transaction(trans, ret);
+		prop_set = (ret == 0);
+		/* If ret == -ENODATA ignore and proceed to update inode item. */
+		if (ret && ret != -ENODATA)
 			goto out_end_trans;
-		}
 	}
 
 update_flags:
@@ -420,6 +420,12 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 	inode_inc_iversion(&inode->vfs_inode);
 	inode_set_ctime_current(&inode->vfs_inode);
 	ret = btrfs_update_inode(trans, inode);
+	/*
+	 * If we set a property or deleted one, we must abort if we fail to
+	 * update the inode, to avoid persisting an inconsistent state.
+	 */
+	if (unlikely(ret && prop_set))
+		btrfs_abort_transaction(trans, ret);
 
  out_end_trans:
 	btrfs_end_transaction(trans);
-- 
2.47.2

Re: [PATCH 0/3] btrfs: fixes and cleanups setting/clearing properties

[Qu Wenruo]

在 2026/6/8 20:49, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Some fixes and cleanups to setting and clearing the compression xattr,
> either directly through the xattr API (first patch) or through the
> fileattr API (all 3 patches). Details in the change logs.
> 
> Filipe Manana (3):
>    btrfs: validate properties before setting them
>    btrfs: don't over reserve metadata space for property in btrfs_fileattr_set()
>    btrfs: fix transaction abort logic in btrfs_fileattr_set()

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
>   fs/btrfs/ioctl.c | 22 ++++++++++++++--------
>   fs/btrfs/props.c | 16 +++++++++++++---
>   2 files changed, 27 insertions(+), 11 deletions(-)
>