[PATCH 0/5] Error handling fixes

[PATCH 0/5] Error handling fixes

[David Sterba]

A few replacements of btrfs_handle_fs_error() with transaction abort or
other means.

David Sterba (5):
  btrfs: abort transaction if we don't find extref in
    btrfs_del_inode_extref()
  btrfs: only print error message when checking item size in
    print_extent_item()
  btrfs: abort transaction on errors in btrfs_free_chunk()
  btrfs: qgroup: preallocate memory before adding a relation
  btrfs: qgroup: warn about inconsistent qgroups when relation update
    fails

 fs/btrfs/inode-item.c |  4 ++--
 fs/btrfs/ioctl.c      | 25 +++++++++++++++++++------
 fs/btrfs/print-tree.c |  2 +-
 fs/btrfs/qgroup.c     | 25 ++++++++-----------------
 fs/btrfs/qgroup.h     | 11 ++++++++++-
 fs/btrfs/volumes.c    | 15 +++++++++------
 6 files changed, 49 insertions(+), 33 deletions(-)

-- 
2.45.0

[PATCH 1/5] btrfs: abort transaction if we don't find extref in btrfs_del_inode_extref()

[David Sterba]

When an extended ref is deleted we do a sanity check right before
removing the item, if we can't find it then handle the error. This is
done by btrfs_handle_fs_error() but this is from the time before we had
the transaction abort infrastructure, so switch to that. The end result
is the same, the error is reported and switched to read-only. We newly
return the -ENOENT error code as this better represents what happened.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode-item.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 84a94d19b22c..316756ff08ac 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -141,8 +141,8 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 	extref = btrfs_find_name_in_ext_backref(path->nodes[0], path->slots[0],
 						ref_objectid, name);
 	if (!extref) {
-		btrfs_handle_fs_error(root->fs_info, -ENOENT, NULL);
-		ret = -EROFS;
+		btrfs_abort_transaction(trans, -ENOENT);
+		ret = -ENOENT;
 		goto out;
 	}
 
-- 
2.45.0

[PATCH 2/5] btrfs: only print error message when checking item size in print_extent_item()

[David Sterba]

The extent item used to have a v0 that was removed in 6.6. There's a
check for minimum expected size that could lead to
btrfs_handle_fs_error() that would make the filesystem read-only. As we
don't have v0 anymore (and haven't seen any reports in the deprecation
period), handle this in a less intrusive way.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/print-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
index 7e46aa8a0444..ffe89db24bec 100644
--- a/fs/btrfs/print-tree.c
+++ b/fs/btrfs/print-tree.c
@@ -109,7 +109,7 @@ static void print_extent_item(const struct extent_buffer *eb, int slot, int type
 		btrfs_err(eb->fs_info,
 			  "unexpected extent item size, has %u expect >= %zu",
 			  item_size, sizeof(*ei));
-		btrfs_handle_fs_error(eb->fs_info, -EUCLEAN, NULL);
+		return;
 	}
 
 	ei = btrfs_item_ptr(eb, slot, struct btrfs_extent_item);
-- 
2.45.0

[PATCH 3/5] btrfs: abort transaction on errors in btrfs_free_chunk()

[David Sterba]

The errors during removing a chunk item are fatal, we expect to have a
matching item in the chunk map from which the chunk_offset is taken.
Handle that by transaction abort.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ac6056072ee8..fcedc43ef291 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2989,16 +2989,19 @@ static int btrfs_free_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 	if (ret < 0)
 		goto out;
 	else if (ret > 0) { /* Logic error or corruption */
-		btrfs_handle_fs_error(fs_info, -ENOENT,
-				      "Failed lookup while freeing chunk.");
-		ret = -ENOENT;
+		btrfs_err(fs_info, "failed to lookup chunk %llu when freeing",
+			  chunk_offset);
+		btrfs_abort_transaction(trans, -ENOENT);
+		ret = -EUCLEAN;
 		goto out;
 	}
 
 	ret = btrfs_del_item(trans, root, path);
-	if (ret < 0)
-		btrfs_handle_fs_error(fs_info, ret,
-				      "Failed to delete chunk item.");
+	if (ret < 0) {
+		btrfs_err(fs_info, "failed to delete chunk %llu item", chunk_offset);
+		btrfs_abort_transaction(trans, ret);
+		goto out;
+	}
 out:
 	btrfs_free_path(path);
 	return ret;
-- 
2.45.0

[PATCH 4/5] btrfs: qgroup: preallocate memory before adding a relation

[David Sterba]

There's a transaction joined in the qgroup relation add/remove ioctl and
any error will lead to abort/error. We could lift the allocation from
btrfs_add_qgroup_relation() and move it outside of the transaction
context. The relation deletion does not need that.

The ownership of the structure is moved to the add relation handler.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c  | 20 ++++++++++++++++----
 fs/btrfs/qgroup.c | 25 ++++++++-----------------
 fs/btrfs/qgroup.h | 11 ++++++++++-
 3 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d28ebabe3720..31c4aea8b93a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3829,6 +3829,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct btrfs_ioctl_qgroup_assign_args *sa;
+	struct btrfs_qgroup_list *prealloc = NULL;
 	struct btrfs_trans_handle *trans;
 	int ret;
 	int err;
@@ -3849,17 +3850,27 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 		goto drop_write;
 	}
 
+	prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
+	if (!prealloc) {
+		ret = -ENOMEM;
+		goto drop_write;
+	}
+
 	trans = btrfs_join_transaction(root);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		goto out;
 	}
 
-	if (sa->assign) {
-		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst);
-	} else {
+	/*
+	 * Prealloc ownership is moved to the relation handler, there it's used
+	 * or freed on error.
+	 */
+	if (sa->assign)
+		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
+	else
 		ret = btrfs_del_qgroup_relation(trans, sa->src, sa->dst);
-	}
+	prealloc = NULL;
 
 	/* update qgroup status and info */
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
@@ -3873,6 +3884,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 		ret = err;
 
 out:
+	kfree(prealloc);
 	kfree(sa);
 drop_write:
 	mnt_drop_write_file(file);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 3edbe5bb19c6..4ae01c87e418 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -155,16 +155,6 @@ static inline u64 btrfs_qgroup_get_new_refcnt(const struct btrfs_qgroup *qg, u64
 	return qg->new_refcnt - seq;
 }
 
-/*
- * glue structure to represent the relations between qgroups.
- */
-struct btrfs_qgroup_list {
-	struct list_head next_group;
-	struct list_head next_member;
-	struct btrfs_qgroup *group;
-	struct btrfs_qgroup *member;
-};
-
 static int
 qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
 		   int init_flags);
@@ -1568,15 +1558,21 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
-int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst)
+/*
+ * Add relation between @src and @dst qgroup. The @prealloc is allocated by the
+ * callers and transferred here (either used or freed on error).
+ */
+int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
+			      struct btrfs_qgroup_list *prealloc)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_qgroup *parent;
 	struct btrfs_qgroup *member;
 	struct btrfs_qgroup_list *list;
-	struct btrfs_qgroup_list *prealloc = NULL;
 	int ret = 0;
 
+	ASSERT(prealloc);
+
 	/* Check the level of src and dst first */
 	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
 		return -EINVAL;
@@ -1601,11 +1597,6 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 		}
 	}
 
-	prealloc = kzalloc(sizeof(*list), GFP_NOFS);
-	if (!prealloc) {
-		ret = -ENOMEM;
-		goto out;
-	}
 	ret = add_qgroup_relation_item(trans, src, dst);
 	if (ret)
 		goto out;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 95881dcab684..deb479d176a9 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -278,6 +278,14 @@ struct btrfs_qgroup {
 	struct kobject kobj;
 };
 
+/* Glue structure to represent the relations between qgroups. */
+struct btrfs_qgroup_list {
+	struct list_head next_group;
+	struct list_head next_member;
+	struct btrfs_qgroup *group;
+	struct btrfs_qgroup *member;
+};
+
 struct btrfs_squota_delta {
 	/* The fstree root this delta counts against. */
 	u64 root;
@@ -321,7 +329,8 @@ int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
 void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
 int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,
 				     bool interruptible);
-int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst);
+int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
+			      struct btrfs_qgroup_list *prealloc);
 int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 			      u64 dst);
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid);
-- 
2.45.0

[PATCH 5/5] btrfs: qgroup: warn about inconsistent qgroups when relation update fails

[David Sterba]

Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to
update the qgroup status is probably not necessary, this would turn the
filesystem to read-only. For the same reason aborting the transaction is
also not a good option.

The state is left inconsistent and can be fixed by rescan, printing a
warning should be sufficient. Return code reflects the status of
adding/deleting the relation and if the transaction was ended properly.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 31c4aea8b93a..f893a6b711c6 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3877,8 +3877,9 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 	err = btrfs_run_qgroups(trans);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	if (err < 0)
-		btrfs_handle_fs_error(fs_info, err,
-				      "failed to update qgroup status and info");
+		btrfs_warn(fs_info,
+			   "qgroup status update failed after %s relation, marked as inconsistent",
+			   sa->assign ? "adding" : "deleting");
 	err = btrfs_end_transaction(trans);
 	if (err && !ret)
 		ret = err;
-- 
2.45.0

Re: [PATCH 0/5] Error handling fixes

[Qu Wenruo]

在 2024/6/20 02:38, David Sterba 写道:
> A few replacements of btrfs_handle_fs_error() with transaction abort or
> other means.
> 
> David Sterba (5):
>    btrfs: abort transaction if we don't find extref in
>      btrfs_del_inode_extref()
>    btrfs: only print error message when checking item size in
>      print_extent_item()

I guess it's the usual mailing list problem, but I didn't see the first 
two patches, only 3~5 are in the list.

Thanks,
Qu

>    btrfs: abort transaction on errors in btrfs_free_chunk()
>    btrfs: qgroup: preallocate memory before adding a relation
>    btrfs: qgroup: warn about inconsistent qgroups when relation update
>      fails
> 
>   fs/btrfs/inode-item.c |  4 ++--
>   fs/btrfs/ioctl.c      | 25 +++++++++++++++++++------
>   fs/btrfs/print-tree.c |  2 +-
>   fs/btrfs/qgroup.c     | 25 ++++++++-----------------
>   fs/btrfs/qgroup.h     | 11 ++++++++++-
>   fs/btrfs/volumes.c    | 15 +++++++++------
>   6 files changed, 49 insertions(+), 33 deletions(-)
> 

Re: [PATCH 4/5] btrfs: qgroup: preallocate memory before adding a relation

[Qu Wenruo]

在 2024/6/20 02:39, David Sterba 写道:
> There's a transaction joined in the qgroup relation add/remove ioctl and
> any error will lead to abort/error. We could lift the allocation from
> btrfs_add_qgroup_relation() and move it outside of the transaction
> context. The relation deletion does not need that.
>
> The ownership of the structure is moved to the add relation handler.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/ioctl.c  | 20 ++++++++++++++++----
>   fs/btrfs/qgroup.c | 25 ++++++++-----------------
>   fs/btrfs/qgroup.h | 11 ++++++++++-
>   3 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index d28ebabe3720..31c4aea8b93a 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3829,6 +3829,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
>   	struct btrfs_root *root = BTRFS_I(inode)->root;
>   	struct btrfs_ioctl_qgroup_assign_args *sa;
> +	struct btrfs_qgroup_list *prealloc = NULL;
>   	struct btrfs_trans_handle *trans;
>   	int ret;
>   	int err;
> @@ -3849,17 +3850,27 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   		goto drop_write;
>   	}
>
> +	prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
> +	if (!prealloc) {
> +		ret = -ENOMEM;
> +		goto drop_write;
> +	}
> +
>   	trans = btrfs_join_transaction(root);
>   	if (IS_ERR(trans)) {
>   		ret = PTR_ERR(trans);
>   		goto out;
>   	}
>
> -	if (sa->assign) {
> -		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst);
> -	} else {
> +	/*
> +	 * Prealloc ownership is moved to the relation handler, there it's used
> +	 * or freed on error.
> +	 */
> +	if (sa->assign)
> +		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
> +	else
>   		ret = btrfs_del_qgroup_relation(trans, sa->src, sa->dst);
> -	}
> +	prealloc = NULL;

This leads to memory leak, as if we're doing relation deletion, we just
do the preallocation, then reset prealloc to NULL, no way to release the
preallocated memory.

Thanks,
Qu

>
>   	/* update qgroup status and info */
>   	mutex_lock(&fs_info->qgroup_ioctl_lock);
> @@ -3873,6 +3884,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   		ret = err;
>
>   out:
> +	kfree(prealloc);
>   	kfree(sa);
>   drop_write:
>   	mnt_drop_write_file(file);
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 3edbe5bb19c6..4ae01c87e418 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -155,16 +155,6 @@ static inline u64 btrfs_qgroup_get_new_refcnt(const struct btrfs_qgroup *qg, u64
>   	return qg->new_refcnt - seq;
>   }
>
> -/*
> - * glue structure to represent the relations between qgroups.
> - */
> -struct btrfs_qgroup_list {
> -	struct list_head next_group;
> -	struct list_head next_member;
> -	struct btrfs_qgroup *group;
> -	struct btrfs_qgroup *member;
> -};
> -
>   static int
>   qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>   		   int init_flags);
> @@ -1568,15 +1558,21 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>   	return ret;
>   }
>
> -int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst)
> +/*
> + * Add relation between @src and @dst qgroup. The @prealloc is allocated by the
> + * callers and transferred here (either used or freed on error).
> + */
> +int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
> +			      struct btrfs_qgroup_list *prealloc)
>   {
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
>   	struct btrfs_qgroup *parent;
>   	struct btrfs_qgroup *member;
>   	struct btrfs_qgroup_list *list;
> -	struct btrfs_qgroup_list *prealloc = NULL;
>   	int ret = 0;
>
> +	ASSERT(prealloc);
> +
>   	/* Check the level of src and dst first */
>   	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
>   		return -EINVAL;
> @@ -1601,11 +1597,6 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
>   		}
>   	}
>
> -	prealloc = kzalloc(sizeof(*list), GFP_NOFS);
> -	if (!prealloc) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
>   	ret = add_qgroup_relation_item(trans, src, dst);
>   	if (ret)
>   		goto out;
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index 95881dcab684..deb479d176a9 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -278,6 +278,14 @@ struct btrfs_qgroup {
>   	struct kobject kobj;
>   };
>
> +/* Glue structure to represent the relations between qgroups. */
> +struct btrfs_qgroup_list {
> +	struct list_head next_group;
> +	struct list_head next_member;
> +	struct btrfs_qgroup *group;
> +	struct btrfs_qgroup *member;
> +};
> +
>   struct btrfs_squota_delta {
>   	/* The fstree root this delta counts against. */
>   	u64 root;
> @@ -321,7 +329,8 @@ int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
>   void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
>   int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,
>   				     bool interruptible);
> -int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst);
> +int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
> +			      struct btrfs_qgroup_list *prealloc);
>   int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
>   			      u64 dst);
>   int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid);

Re: [PATCH 4/5] btrfs: qgroup: preallocate memory before adding a relation

[David Sterba]

On Thu, Jun 20, 2024 at 11:29:22AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2024/6/20 02:39, David Sterba 写道:
> > There's a transaction joined in the qgroup relation add/remove ioctl and
> > any error will lead to abort/error. We could lift the allocation from
> > btrfs_add_qgroup_relation() and move it outside of the transaction
> > context. The relation deletion does not need that.
> >
> > The ownership of the structure is moved to the add relation handler.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/ioctl.c  | 20 ++++++++++++++++----
> >   fs/btrfs/qgroup.c | 25 ++++++++-----------------
> >   fs/btrfs/qgroup.h | 11 ++++++++++-
> >   3 files changed, 34 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index d28ebabe3720..31c4aea8b93a 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -3829,6 +3829,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
> >   	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
> >   	struct btrfs_root *root = BTRFS_I(inode)->root;
> >   	struct btrfs_ioctl_qgroup_assign_args *sa;
> > +	struct btrfs_qgroup_list *prealloc = NULL;
> >   	struct btrfs_trans_handle *trans;
> >   	int ret;
> >   	int err;
> > @@ -3849,17 +3850,27 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
> >   		goto drop_write;
> >   	}
> >
> > +	prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
> > +	if (!prealloc) {
> > +		ret = -ENOMEM;
> > +		goto drop_write;
> > +	}
> > +
> >   	trans = btrfs_join_transaction(root);
> >   	if (IS_ERR(trans)) {
> >   		ret = PTR_ERR(trans);
> >   		goto out;
> >   	}
> >
> > -	if (sa->assign) {
> > -		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst);
> > -	} else {
> > +	/*
> > +	 * Prealloc ownership is moved to the relation handler, there it's used
> > +	 * or freed on error.
> > +	 */
> > +	if (sa->assign)
> > +		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
> > +	else
> >   		ret = btrfs_del_qgroup_relation(trans, sa->src, sa->dst);
> > -	}
> > +	prealloc = NULL;
> 
> This leads to memory leak, as if we're doing relation deletion, we just
> do the preallocation, then reset prealloc to NULL, no way to release the
> preallocated memory.

Right, I should maybe also add the preallocation only when it's adding
the relation so it does not fail for deletion where it's not needed.

[PATCH v2] btrfs: qgroup: preallocate memory before adding a relation

[David Sterba]

There's a transaction joined in the qgroup relation add/remove ioctl and
any error will lead to abort/error. We could lift the allocation from
btrfs_add_qgroup_relation() and move it outside of the transaction
context. The relation deletion does not need that.

The ownership of the structure is moved to the add relation handler.

Signed-off-by: David Sterba <dsterba@suse.com>
---

v2:
- preallocate only when adding the relation

 fs/btrfs/ioctl.c  | 17 ++++++++++++++++-
 fs/btrfs/qgroup.c | 25 ++++++++-----------------
 fs/btrfs/qgroup.h | 11 ++++++++++-
 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d28ebabe3720..dc7300f2815f 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3829,6 +3829,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct btrfs_ioctl_qgroup_assign_args *sa;
+	struct btrfs_qgroup_list *prealloc = NULL;
 	struct btrfs_trans_handle *trans;
 	int ret;
 	int err;
@@ -3849,14 +3850,27 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 		goto drop_write;
 	}
 
+	if (sa->assign) {
+		prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
+		if (!prealloc) {
+			ret = -ENOMEM;
+			goto drop_write;
+		}
+	}
+
 	trans = btrfs_join_transaction(root);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		goto out;
 	}
 
+	/*
+	 * Prealloc ownership is moved to the relation handler, there it's used
+	 * or freed on error.
+	 */
 	if (sa->assign) {
-		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst);
+		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
+		prealloc = NULL;
 	} else {
 		ret = btrfs_del_qgroup_relation(trans, sa->src, sa->dst);
 	}
@@ -3873,6 +3887,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 		ret = err;
 
 out:
+	kfree(prealloc);
 	kfree(sa);
 drop_write:
 	mnt_drop_write_file(file);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 3edbe5bb19c6..4ae01c87e418 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -155,16 +155,6 @@ static inline u64 btrfs_qgroup_get_new_refcnt(const struct btrfs_qgroup *qg, u64
 	return qg->new_refcnt - seq;
 }
 
-/*
- * glue structure to represent the relations between qgroups.
- */
-struct btrfs_qgroup_list {
-	struct list_head next_group;
-	struct list_head next_member;
-	struct btrfs_qgroup *group;
-	struct btrfs_qgroup *member;
-};
-
 static int
 qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
 		   int init_flags);
@@ -1568,15 +1558,21 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
-int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst)
+/*
+ * Add relation between @src and @dst qgroup. The @prealloc is allocated by the
+ * callers and transferred here (either used or freed on error).
+ */
+int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
+			      struct btrfs_qgroup_list *prealloc)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_qgroup *parent;
 	struct btrfs_qgroup *member;
 	struct btrfs_qgroup_list *list;
-	struct btrfs_qgroup_list *prealloc = NULL;
 	int ret = 0;
 
+	ASSERT(prealloc);
+
 	/* Check the level of src and dst first */
 	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
 		return -EINVAL;
@@ -1601,11 +1597,6 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 		}
 	}
 
-	prealloc = kzalloc(sizeof(*list), GFP_NOFS);
-	if (!prealloc) {
-		ret = -ENOMEM;
-		goto out;
-	}
 	ret = add_qgroup_relation_item(trans, src, dst);
 	if (ret)
 		goto out;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 95881dcab684..deb479d176a9 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -278,6 +278,14 @@ struct btrfs_qgroup {
 	struct kobject kobj;
 };
 
+/* Glue structure to represent the relations between qgroups. */
+struct btrfs_qgroup_list {
+	struct list_head next_group;
+	struct list_head next_member;
+	struct btrfs_qgroup *group;
+	struct btrfs_qgroup *member;
+};
+
 struct btrfs_squota_delta {
 	/* The fstree root this delta counts against. */
 	u64 root;
@@ -321,7 +329,8 @@ int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
 void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
 int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,
 				     bool interruptible);
-int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst);
+int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
+			      struct btrfs_qgroup_list *prealloc);
 int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 			      u64 dst);
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid);
-- 
2.45.0

Re: [PATCH 5/5] btrfs: qgroup: warn about inconsistent qgroups when relation update fails

[Boris Burkov]

On Wed, Jun 19, 2024 at 07:09:20PM +0200, David Sterba wrote:
> Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to
> update the qgroup status is probably not necessary, this would turn the
> filesystem to read-only. For the same reason aborting the transaction is
> also not a good option.
> 
> The state is left inconsistent and can be fixed by rescan, printing a
> warning should be sufficient. Return code reflects the status of
> adding/deleting the relation and if the transaction was ended properly.

A few thoughts/questions about this:

1. Why do we even need to btrfs_run_qgroups() here? Won't we btrfs_run_qgroups
in the transaction that actually commits the qgroup relation items? I'm
guessing some qgroup lookup sees the not-committed items in a way users
care about? Are we expecting such high-scale `qgroup assign` that we
can't just commit this txn and make it simpler?

2. Is this really failing in cases where adding the relation items
succeeds and then it all gets committed successfully? i.e., do you have
a reproducer for this case?

3. If this is a realistic scenario, I'm worried about the squotas case,
which doesn't have any rescan capability to fallback on. However, I
don't see why my (1) above doesn't just save us anyway. If we commit the
relation item, then we also commit the status/info items. And the txn
commit run_qgroups is not allowed to fail.

4. Let's say that 1. is not strictly essential, and the txn commit is
going to fix us. In that case, is it really accurate to say we are
inconsistent any more than just during a transaction before we run
qgroups? I suppose compared to the case where this succeeded, we are. I
just have a weird feeling we are stretching the meaning of inconsistent.
Though not in an egregious way or anything, as we are reporting a
non-fatal error?

Sorry for the slightly rambling review..

Thanks,
Boris

> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ioctl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 31c4aea8b93a..f893a6b711c6 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3877,8 +3877,9 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>  	err = btrfs_run_qgroups(trans);
>  	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>  	if (err < 0)
> -		btrfs_handle_fs_error(fs_info, err,
> -				      "failed to update qgroup status and info");
> +		btrfs_warn(fs_info,
> +			   "qgroup status update failed after %s relation, marked as inconsistent",
> +			   sa->assign ? "adding" : "deleting");
>  	err = btrfs_end_transaction(trans);
>  	if (err && !ret)
>  		ret = err;
> -- 
> 2.45.0
> 

Re: [PATCH v2] btrfs: qgroup: preallocate memory before adding a relation

[Qu Wenruo]

在 2024/6/21 03:16, David Sterba 写道:
> There's a transaction joined in the qgroup relation add/remove ioctl and
> any error will lead to abort/error. We could lift the allocation from
> btrfs_add_qgroup_relation() and move it outside of the transaction
> context. The relation deletion does not need that.
>
> The ownership of the structure is moved to the add relation handler.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>
> v2:
> - preallocate only when adding the relation
>
>   fs/btrfs/ioctl.c  | 17 ++++++++++++++++-
>   fs/btrfs/qgroup.c | 25 ++++++++-----------------
>   fs/btrfs/qgroup.h | 11 ++++++++++-
>   3 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index d28ebabe3720..dc7300f2815f 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3829,6 +3829,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
>   	struct btrfs_root *root = BTRFS_I(inode)->root;
>   	struct btrfs_ioctl_qgroup_assign_args *sa;
> +	struct btrfs_qgroup_list *prealloc = NULL;
>   	struct btrfs_trans_handle *trans;
>   	int ret;
>   	int err;
> @@ -3849,14 +3850,27 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   		goto drop_write;
>   	}
>
> +	if (sa->assign) {
> +		prealloc = kzalloc(sizeof(*prealloc), GFP_KERNEL);
> +		if (!prealloc) {
> +			ret = -ENOMEM;
> +			goto drop_write;
> +		}
> +	}
> +
>   	trans = btrfs_join_transaction(root);
>   	if (IS_ERR(trans)) {
>   		ret = PTR_ERR(trans);
>   		goto out;
>   	}
>
> +	/*
> +	 * Prealloc ownership is moved to the relation handler, there it's used
> +	 * or freed on error.
> +	 */
>   	if (sa->assign) {
> -		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst);
> +		ret = btrfs_add_qgroup_relation(trans, sa->src, sa->dst, prealloc);
> +		prealloc = NULL;
>   	} else {
>   		ret = btrfs_del_qgroup_relation(trans, sa->src, sa->dst);
>   	}
> @@ -3873,6 +3887,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   		ret = err;
>
>   out:
> +	kfree(prealloc);
>   	kfree(sa);
>   drop_write:
>   	mnt_drop_write_file(file);
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 3edbe5bb19c6..4ae01c87e418 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -155,16 +155,6 @@ static inline u64 btrfs_qgroup_get_new_refcnt(const struct btrfs_qgroup *qg, u64
>   	return qg->new_refcnt - seq;
>   }
>
> -/*
> - * glue structure to represent the relations between qgroups.
> - */
> -struct btrfs_qgroup_list {
> -	struct list_head next_group;
> -	struct list_head next_member;
> -	struct btrfs_qgroup *group;
> -	struct btrfs_qgroup *member;
> -};
> -
>   static int
>   qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>   		   int init_flags);
> @@ -1568,15 +1558,21 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>   	return ret;
>   }
>
> -int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst)
> +/*
> + * Add relation between @src and @dst qgroup. The @prealloc is allocated by the
> + * callers and transferred here (either used or freed on error).
> + */
> +int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
> +			      struct btrfs_qgroup_list *prealloc)
>   {
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
>   	struct btrfs_qgroup *parent;
>   	struct btrfs_qgroup *member;
>   	struct btrfs_qgroup_list *list;
> -	struct btrfs_qgroup_list *prealloc = NULL;
>   	int ret = 0;
>
> +	ASSERT(prealloc);
> +
>   	/* Check the level of src and dst first */
>   	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst))
>   		return -EINVAL;
> @@ -1601,11 +1597,6 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
>   		}
>   	}
>
> -	prealloc = kzalloc(sizeof(*list), GFP_NOFS);
> -	if (!prealloc) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
>   	ret = add_qgroup_relation_item(trans, src, dst);
>   	if (ret)
>   		goto out;
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index 95881dcab684..deb479d176a9 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -278,6 +278,14 @@ struct btrfs_qgroup {
>   	struct kobject kobj;
>   };
>
> +/* Glue structure to represent the relations between qgroups. */
> +struct btrfs_qgroup_list {
> +	struct list_head next_group;
> +	struct list_head next_member;
> +	struct btrfs_qgroup *group;
> +	struct btrfs_qgroup *member;
> +};
> +
>   struct btrfs_squota_delta {
>   	/* The fstree root this delta counts against. */
>   	u64 root;
> @@ -321,7 +329,8 @@ int btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info);
>   void btrfs_qgroup_rescan_resume(struct btrfs_fs_info *fs_info);
>   int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,
>   				     bool interruptible);
> -int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst);
> +int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
> +			      struct btrfs_qgroup_list *prealloc);
>   int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
>   			      u64 dst);
>   int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid);

Re: [PATCH 5/5] btrfs: qgroup: warn about inconsistent qgroups when relation update fails

[Qu Wenruo]

在 2024/6/21 06:58, Boris Burkov 写道:
> On Wed, Jun 19, 2024 at 07:09:20PM +0200, David Sterba wrote:
>> Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to
>> update the qgroup status is probably not necessary, this would turn the
>> filesystem to read-only. For the same reason aborting the transaction is
>> also not a good option.
>>
>> The state is left inconsistent and can be fixed by rescan, printing a
>> warning should be sufficient. Return code reflects the status of
>> adding/deleting the relation and if the transaction was ended properly.
>
> A few thoughts/questions about this:
>
> 1. Why do we even need to btrfs_run_qgroups() here? Won't we btrfs_run_qgroups
> in the transaction that actually commits the qgroup relation items? I'm
> guessing some qgroup lookup sees the not-committed items in a way users
> care about? Are we expecting such high-scale `qgroup assign` that we
> can't just commit this txn and make it simpler?

I agree with this.

I'm originally thinking some weird situation that one have called
btrfs_run_qgroups(), then we do relation update without
btrfs_run_qgroups(), in this case it may cause problems.

But since btrfs_run_qgroups() is really only called in committing
transaction and this is the only other location, I do not think it's
going to cause much problem.

So overall we should just remove the btrfs_run_qgroups() call and no
need to handle the error (even no need to commit tranasaction).

Thanks,
Qu

>
> 2. Is this really failing in cases where adding the relation items
> succeeds and then it all gets committed successfully? i.e., do you have
> a reproducer for this case?
>
> 3. If this is a realistic scenario, I'm worried about the squotas case,
> which doesn't have any rescan capability to fallback on. However, I
> don't see why my (1) above doesn't just save us anyway. If we commit the
> relation item, then we also commit the status/info items. And the txn
> commit run_qgroups is not allowed to fail.
>
> 4. Let's say that 1. is not strictly essential, and the txn commit is
> going to fix us. In that case, is it really accurate to say we are
> inconsistent any more than just during a transaction before we run
> qgroups? I suppose compared to the case where this succeeded, we are. I
> just have a weird feeling we are stretching the meaning of inconsistent.
> Though not in an egregious way or anything, as we are reporting a
> non-fatal error?
>
> Sorry for the slightly rambling review..
>
> Thanks,
> Boris
>
>>
>> Signed-off-by: David Sterba <dsterba@suse.com>
>> ---
>>   fs/btrfs/ioctl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 31c4aea8b93a..f893a6b711c6 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3877,8 +3877,9 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>>   	err = btrfs_run_qgroups(trans);
>>   	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>>   	if (err < 0)
>> -		btrfs_handle_fs_error(fs_info, err,
>> -				      "failed to update qgroup status and info");
>> +		btrfs_warn(fs_info,
>> +			   "qgroup status update failed after %s relation, marked as inconsistent",
>> +			   sa->assign ? "adding" : "deleting");
>>   	err = btrfs_end_transaction(trans);
>>   	if (err && !ret)
>>   		ret = err;
>> --
>> 2.45.0
>>
>