| Message ID | 20240620174618.4704-1-dsterba@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] btrfs: qgroup: preallocate memory before adding a relation | expand |
在 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);
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);
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(-)