| Message ID | 20221205095122.17011-2-robbieko@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix unexpected -ENOMEM with percpu_counter_init when create snapshot | expand |
On Mon, Dec 5, 2022 at 10:11 AM robbieko <robbieko@synology.com> wrote: > > From: Robbie Ko <robbieko@synology.com> > > Create a new structure called btrfs_new_fs_root_args that > refactor the type of anon_dev. > With the new structure, btrfs_get_new_fs_root can input > btrfs_new_fs_root_args into btrfs_init_fs_root by order. > > Signed-off-by: Robbie Ko <robbieko@synology.com> > --- > fs/btrfs/disk-io.c | 63 ++++++++++++++++++++++++++++++------------ > fs/btrfs/disk-io.h | 10 ++++++- > fs/btrfs/ioctl.c | 34 +++++++++++------------ > fs/btrfs/transaction.c | 2 +- > fs/btrfs/transaction.h | 5 ++-- > 5 files changed, 74 insertions(+), 40 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index d99bf7c64611..afe16e1f0b18 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -1422,12 +1422,44 @@ struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root, > return root; > } > > +struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask) Can we remove the argument? Even after the whole patchset, it will always be GFP_KERNEL. I know patch 3/3 adds an assertion to verify we get GFP_KERNEL, but lockdep is smart enough to detect an issue and complain. Also, can you name the function something like btrfs_alloc_new_fs_roots_arg() ? The naming pattern we use for allocator functions is to use "alloc" in the name, for e.g. btrfs_alloc_path(), alloc_extent_map(), etc. So having an allocator named *_prepare() breaks the pattern and it's also confusing (as it allocates the structure). > +{ > + int err; > + struct btrfs_new_fs_root_args *args; > + > + args = kzalloc(sizeof(*args), gfp_mask); > + if (!args) { > + err = -ENOMEM; > + goto error; > + } if (!args) return ERR_PTR(-ENOMEM); That's simpler. > + > + err = get_anon_bdev(&args->anon_dev); > + if (err) > + goto error; > + > + return args; > + > +error: > + btrfs_new_fs_root_args_destroy(args); > + return ERR_PTR(err); > +} > + > +void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args) Same thing about the naming consistency here. Something like: btrfs_free_new_fs_roots_arg() We have btrfs_free_path(), free_extent_map(), etc. Thanks. > +{ > + if (!args) > + return; > + if (args->anon_dev) > + free_anon_bdev(args->anon_dev); > + kfree(args); > +} > + > /* > * Initialize subvolume root in-memory structure > * > * @anon_dev: anonymous device to attach to the root, if zero, allocate new > */ > -static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) > +static int btrfs_init_fs_root(struct btrfs_root *root, > + struct btrfs_new_fs_root_args *new_fs_root_args) > { > int ret; > unsigned int nofs_flag; > @@ -1454,12 +1486,13 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) > */ > if (is_fstree(root->root_key.objectid) && > btrfs_root_refs(&root->root_item) > 0) { > - if (!anon_dev) { > + if (!new_fs_root_args || !new_fs_root_args->anon_dev) { > ret = get_anon_bdev(&root->anon_dev); > if (ret) > goto fail; > } else { > - root->anon_dev = anon_dev; > + root->anon_dev = new_fs_root_args->anon_dev; > + new_fs_root_args->anon_dev = 0; > } > } > > @@ -1633,7 +1666,8 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info) > * for orphan roots > */ > static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, > - u64 objectid, dev_t anon_dev, > + u64 objectid, > + struct btrfs_new_fs_root_args *new_fs_root_args, > bool check_ref) > { > struct btrfs_root *root; > @@ -1647,8 +1681,8 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, > again: > root = btrfs_lookup_fs_root(fs_info, objectid); > if (root) { > - /* Shouldn't get preallocated anon_dev for cached roots */ > - ASSERT(!anon_dev); > + /* Shouldn't get new_fs_root_args for cached roots */ > + ASSERT(!new_fs_root_args); > if (check_ref && btrfs_root_refs(&root->root_item) == 0) { > btrfs_put_root(root); > return ERR_PTR(-ENOENT); > @@ -1668,7 +1702,7 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, > goto fail; > } > > - ret = btrfs_init_fs_root(root, anon_dev); > + ret = btrfs_init_fs_root(root, new_fs_root_args); > if (ret) > goto fail; > > @@ -1698,14 +1732,6 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, > } > return root; > fail: > - /* > - * If our caller provided us an anonymous device, then it's his > - * responsibility to free it in case we fail. So we have to set our > - * root's anon_dev to 0 to avoid a double free, once by btrfs_put_root() > - * and once again by our caller. > - */ > - if (anon_dev) > - root->anon_dev = 0; > btrfs_put_root(root); > return ERR_PTR(ret); > } > @@ -1720,7 +1746,7 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, > struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, > u64 objectid, bool check_ref) > { > - return btrfs_get_root_ref(fs_info, objectid, 0, check_ref); > + return btrfs_get_root_ref(fs_info, objectid, NULL, check_ref); > } > > /* > @@ -1732,9 +1758,10 @@ struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, > * parameter value > */ > struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info, > - u64 objectid, dev_t anon_dev) > + u64 objectid, > + struct btrfs_new_fs_root_args *new_fs_root_args) > { > - return btrfs_get_root_ref(fs_info, objectid, anon_dev, true); > + return btrfs_get_root_ref(fs_info, objectid, new_fs_root_args, true); > } > > /* > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index 9fa923e005a3..a7c91bfb0c16 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -25,6 +25,11 @@ static inline u64 btrfs_sb_offset(int mirror) > return BTRFS_SUPER_INFO_OFFSET; > } > > +struct btrfs_new_fs_root_args { > + /* Preallocated anonymous block device number */ > + dev_t anon_dev; > +}; > + > struct btrfs_device; > struct btrfs_fs_devices; > > @@ -58,6 +63,8 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, > int btrfs_commit_super(struct btrfs_fs_info *fs_info); > struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root, > struct btrfs_key *key); > +struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask); > +void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args); > int btrfs_insert_fs_root(struct btrfs_fs_info *fs_info, > struct btrfs_root *root); > void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info); > @@ -65,7 +72,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info); > struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, > u64 objectid, bool check_ref); > struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info, > - u64 objectid, dev_t anon_dev); > + u64 objectid, > + struct btrfs_new_fs_root_args *new_fs_root_args); > struct btrfs_root *btrfs_get_fs_root_commit_root(struct btrfs_fs_info *fs_info, > struct btrfs_path *path, > u64 objectid); > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 5ba2e810dc6e..5785336ab7cf 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -588,8 +588,8 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, > }; > unsigned int trans_num_items; > int ret; > - dev_t anon_dev; > u64 objectid; > + struct btrfs_new_fs_root_args *new_fs_root_args = NULL; > > root_item = kzalloc(sizeof(*root_item), GFP_KERNEL); > if (!root_item) > @@ -608,14 +608,17 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, > goto out_root_item; > } > > - ret = get_anon_bdev(&anon_dev); > - if (ret < 0) > + new_fs_root_args = btrfs_new_fs_root_args_prepare(GFP_KERNEL); > + if (IS_ERR(new_fs_root_args)) { > + ret = PTR_ERR(new_fs_root_args); > + new_fs_root_args = NULL; > goto out_root_item; > + } > > new_inode_args.inode = btrfs_new_subvol_inode(mnt_userns, dir); > if (!new_inode_args.inode) { > ret = -ENOMEM; > - goto out_anon_dev; > + goto out_root_item; > } > ret = btrfs_new_inode_prepare(&new_inode_args, &trans_num_items); > if (ret) > @@ -706,14 +709,12 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, > free_extent_buffer(leaf); > leaf = NULL; > > - new_root = btrfs_get_new_fs_root(fs_info, objectid, anon_dev); > + new_root = btrfs_get_new_fs_root(fs_info, objectid, new_fs_root_args); > if (IS_ERR(new_root)) { > ret = PTR_ERR(new_root); > btrfs_abort_transaction(trans, ret); > goto out; > } > - /* anon_dev is owned by new_root now. */ > - anon_dev = 0; > BTRFS_I(new_inode_args.inode)->root = new_root; > /* ... and new_root is owned by new_inode_args.inode now. */ > > @@ -752,10 +753,8 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, > btrfs_new_inode_args_destroy(&new_inode_args); > out_inode: > iput(new_inode_args.inode); > -out_anon_dev: > - if (anon_dev) > - free_anon_bdev(anon_dev); > out_root_item: > + btrfs_new_fs_root_args_destroy(new_fs_root_args); > kfree(root_item); > return ret; > } > @@ -791,9 +790,13 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, > if (!pending_snapshot) > return -ENOMEM; > > - ret = get_anon_bdev(&pending_snapshot->anon_dev); > - if (ret < 0) > + pending_snapshot->new_fs_root_args = btrfs_new_fs_root_args_prepare(GFP_KERNEL); > + if (IS_ERR(pending_snapshot->new_fs_root_args)) { > + ret = PTR_ERR(pending_snapshot->new_fs_root_args); > + pending_snapshot->new_fs_root_args = NULL; > goto free_pending; > + } > + > pending_snapshot->root_item = kzalloc(sizeof(struct btrfs_root_item), > GFP_KERNEL); > pending_snapshot->path = btrfs_alloc_path(); > @@ -850,16 +853,11 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, > > d_instantiate(dentry, inode); > ret = 0; > - pending_snapshot->anon_dev = 0; > fail: > - /* Prevent double freeing of anon_dev */ > - if (ret && pending_snapshot->snap) > - pending_snapshot->snap->anon_dev = 0; > btrfs_put_root(pending_snapshot->snap); > btrfs_subvolume_release_metadata(root, &pending_snapshot->block_rsv); > free_pending: > - if (pending_snapshot->anon_dev) > - free_anon_bdev(pending_snapshot->anon_dev); > + btrfs_new_fs_root_args_destroy(pending_snapshot->new_fs_root_args); > kfree(pending_snapshot->root_item); > btrfs_free_path(pending_snapshot->path); > kfree(pending_snapshot); > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index d1f1da6820fb..614eb84422e1 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1777,7 +1777,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > } > > key.offset = (u64)-1; > - pending->snap = btrfs_get_new_fs_root(fs_info, objectid, pending->anon_dev); > + pending->snap = btrfs_get_new_fs_root(fs_info, objectid, pending->new_fs_root_args); > if (IS_ERR(pending->snap)) { > ret = PTR_ERR(pending->snap); > pending->snap = NULL; > diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h > index 970ff316069d..34648a617a2c 100644 > --- a/fs/btrfs/transaction.h > +++ b/fs/btrfs/transaction.h > @@ -10,6 +10,7 @@ > #include "btrfs_inode.h" > #include "delayed-ref.h" > #include "ctree.h" > +#include "disk-io.h" > > enum btrfs_trans_state { > TRANS_STATE_RUNNING, > @@ -161,8 +162,8 @@ struct btrfs_pending_snapshot { > struct btrfs_block_rsv block_rsv; > /* extra metadata reservation for relocation */ > int error; > - /* Preallocated anonymous block device number */ > - dev_t anon_dev; > + /* Preallocated new_fs_root_args */ > + struct btrfs_new_fs_root_args *new_fs_root_args; > bool readonly; > struct list_head list; > }; > -- > 2.17.1 >
Filipe Manana 於 2022/12/13 上午1:04 寫道: > On Mon, Dec 5, 2022 at 10:11 AM robbieko <robbieko@synology.com> wrote: >> From: Robbie Ko <robbieko@synology.com> >> >> Create a new structure called btrfs_new_fs_root_args that >> refactor the type of anon_dev. >> With the new structure, btrfs_get_new_fs_root can input >> btrfs_new_fs_root_args into btrfs_init_fs_root by order. >> >> Signed-off-by: Robbie Ko <robbieko@synology.com> >> --- >> fs/btrfs/disk-io.c | 63 ++++++++++++++++++++++++++++++------------ >> fs/btrfs/disk-io.h | 10 ++++++- >> fs/btrfs/ioctl.c | 34 +++++++++++------------ >> fs/btrfs/transaction.c | 2 +- >> fs/btrfs/transaction.h | 5 ++-- >> 5 files changed, 74 insertions(+), 40 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index d99bf7c64611..afe16e1f0b18 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -1422,12 +1422,44 @@ struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root, >> return root; >> } >> >> +struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask) > Can we remove the argument? > Even after the whole patchset, it will always be GFP_KERNEL. > > I know patch 3/3 adds an assertion to verify we get GFP_KERNEL, but > lockdep is smart enough to detect an issue and complain. Okay, I will remove the argument. Thanks. > > Also, can you name the function something like btrfs_alloc_new_fs_roots_arg() ? > The naming pattern we use for allocator functions is to use "alloc" in > the name, for e.g. btrfs_alloc_path(), alloc_extent_map(), etc. > So having an allocator named *_prepare() breaks the pattern and it's > also confusing (as it allocates the structure). Okay, I will modify the function name to comply with the coding style. Thanks. >> +{ >> + int err; >> + struct btrfs_new_fs_root_args *args; >> + >> + args = kzalloc(sizeof(*args), gfp_mask); >> + if (!args) { >> + err = -ENOMEM; >> + goto error; >> + } > if (!args) > return ERR_PTR(-ENOMEM); > > That's simpler. We hope to satisfy the single-exit rule as much as possible to increase code readability and uniform resource release. Thanks. >> + >> + err = get_anon_bdev(&args->anon_dev); >> + if (err) >> + goto error; >> + >> + return args; >> + >> +error: >> + btrfs_new_fs_root_args_destroy(args); >> + return ERR_PTR(err); >> +} >> + >> +void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args) > Same thing about the naming consistency here. Something like: > btrfs_free_new_fs_roots_arg() > We have btrfs_free_path(), free_extent_map(), etc. Okay, I will modify it. Thanks. > Thanks. >> +{ >> + if (!args) >> + return; >> + if (args->anon_dev) >> + free_anon_bdev(args->anon_dev); >> + kfree(args); >> +} >> + >> /* >> * Initialize subvolume root in-memory structure >> * >> * @anon_dev: anonymous device to attach to the root, if zero, allocate new >> */ >> -static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) >> +static int btrfs_init_fs_root(struct btrfs_root *root, >> + struct btrfs_new_fs_root_args *new_fs_root_args) >> { >> int ret; >> unsigned int nofs_flag; >> @@ -1454,12 +1486,13 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) >> */ >> if (is_fstree(root->root_key.objectid) && >> btrfs_root_refs(&root->root_item) > 0) { >> - if (!anon_dev) { >> + if (!new_fs_root_args || !new_fs_root_args->anon_dev) { >> ret = get_anon_bdev(&root->anon_dev); >> if (ret) >> goto fail; >> } else { >> - root->anon_dev = anon_dev; >> + root->anon_dev = new_fs_root_args->anon_dev; >> + new_fs_root_args->anon_dev = 0; >> } >> } >> >> @@ -1633,7 +1666,8 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info) >> * for orphan roots >> */ >> static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, >> - u64 objectid, dev_t anon_dev, >> + u64 objectid, >> + struct btrfs_new_fs_root_args *new_fs_root_args, >> bool check_ref) >> { >> struct btrfs_root *root; >> @@ -1647,8 +1681,8 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, >> again: >> root = btrfs_lookup_fs_root(fs_info, objectid); >> if (root) { >> - /* Shouldn't get preallocated anon_dev for cached roots */ >> - ASSERT(!anon_dev); >> + /* Shouldn't get new_fs_root_args for cached roots */ >> + ASSERT(!new_fs_root_args); >> if (check_ref && btrfs_root_refs(&root->root_item) == 0) { >> btrfs_put_root(root); >> return ERR_PTR(-ENOENT); >> @@ -1668,7 +1702,7 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, >> goto fail; >> } >> >> - ret = btrfs_init_fs_root(root, anon_dev); >> + ret = btrfs_init_fs_root(root, new_fs_root_args); >> if (ret) >> goto fail; >> >> @@ -1698,14 +1732,6 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, >> } >> return root; >> fail: >> - /* >> - * If our caller provided us an anonymous device, then it's his >> - * responsibility to free it in case we fail. So we have to set our >> - * root's anon_dev to 0 to avoid a double free, once by btrfs_put_root() >> - * and once again by our caller. >> - */ >> - if (anon_dev) >> - root->anon_dev = 0; >> btrfs_put_root(root); >> return ERR_PTR(ret); >> } >> @@ -1720,7 +1746,7 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, >> struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, >> u64 objectid, bool check_ref) >> { >> - return btrfs_get_root_ref(fs_info, objectid, 0, check_ref); >> + return btrfs_get_root_ref(fs_info, objectid, NULL, check_ref); >> } >> >> /* >> @@ -1732,9 +1758,10 @@ struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, >> * parameter value >> */ >> struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info, >> - u64 objectid, dev_t anon_dev) >> + u64 objectid, >> + struct btrfs_new_fs_root_args *new_fs_root_args) >> { >> - return btrfs_get_root_ref(fs_info, objectid, anon_dev, true); >> + return btrfs_get_root_ref(fs_info, objectid, new_fs_root_args, true); >> } >> >> /* >> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h >> index 9fa923e005a3..a7c91bfb0c16 100644 >> --- a/fs/btrfs/disk-io.h >> +++ b/fs/btrfs/disk-io.h >> @@ -25,6 +25,11 @@ static inline u64 btrfs_sb_offset(int mirror) >> return BTRFS_SUPER_INFO_OFFSET; >> } >> >> +struct btrfs_new_fs_root_args { >> + /* Preallocated anonymous block device number */ >> + dev_t anon_dev; >> +}; >> + >> struct btrfs_device; >> struct btrfs_fs_devices; >> >> @@ -58,6 +63,8 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, >> int btrfs_commit_super(struct btrfs_fs_info *fs_info); >> struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root, >> struct btrfs_key *key); >> +struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask); >> +void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args); >> int btrfs_insert_fs_root(struct btrfs_fs_info *fs_info, >> struct btrfs_root *root); >> void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info); >> @@ -65,7 +72,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info); >> struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, >> u64 objectid, bool check_ref); >> struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info, >> - u64 objectid, dev_t anon_dev); >> + u64 objectid, >> + struct btrfs_new_fs_root_args *new_fs_root_args); >> struct btrfs_root *btrfs_get_fs_root_commit_root(struct btrfs_fs_info *fs_info, >> struct btrfs_path *path, >> u64 objectid); >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 5ba2e810dc6e..5785336ab7cf 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -588,8 +588,8 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, >> }; >> unsigned int trans_num_items; >> int ret; >> - dev_t anon_dev; >> u64 objectid; >> + struct btrfs_new_fs_root_args *new_fs_root_args = NULL; >> >> root_item = kzalloc(sizeof(*root_item), GFP_KERNEL); >> if (!root_item) >> @@ -608,14 +608,17 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, >> goto out_root_item; >> } >> >> - ret = get_anon_bdev(&anon_dev); >> - if (ret < 0) >> + new_fs_root_args = btrfs_new_fs_root_args_prepare(GFP_KERNEL); >> + if (IS_ERR(new_fs_root_args)) { >> + ret = PTR_ERR(new_fs_root_args); >> + new_fs_root_args = NULL; >> goto out_root_item; >> + } >> >> new_inode_args.inode = btrfs_new_subvol_inode(mnt_userns, dir); >> if (!new_inode_args.inode) { >> ret = -ENOMEM; >> - goto out_anon_dev; >> + goto out_root_item; >> } >> ret = btrfs_new_inode_prepare(&new_inode_args, &trans_num_items); >> if (ret) >> @@ -706,14 +709,12 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, >> free_extent_buffer(leaf); >> leaf = NULL; >> >> - new_root = btrfs_get_new_fs_root(fs_info, objectid, anon_dev); >> + new_root = btrfs_get_new_fs_root(fs_info, objectid, new_fs_root_args); >> if (IS_ERR(new_root)) { >> ret = PTR_ERR(new_root); >> btrfs_abort_transaction(trans, ret); >> goto out; >> } >> - /* anon_dev is owned by new_root now. */ >> - anon_dev = 0; >> BTRFS_I(new_inode_args.inode)->root = new_root; >> /* ... and new_root is owned by new_inode_args.inode now. */ >> >> @@ -752,10 +753,8 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, >> btrfs_new_inode_args_destroy(&new_inode_args); >> out_inode: >> iput(new_inode_args.inode); >> -out_anon_dev: >> - if (anon_dev) >> - free_anon_bdev(anon_dev); >> out_root_item: >> + btrfs_new_fs_root_args_destroy(new_fs_root_args); >> kfree(root_item); >> return ret; >> } >> @@ -791,9 +790,13 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, >> if (!pending_snapshot) >> return -ENOMEM; >> >> - ret = get_anon_bdev(&pending_snapshot->anon_dev); >> - if (ret < 0) >> + pending_snapshot->new_fs_root_args = btrfs_new_fs_root_args_prepare(GFP_KERNEL); >> + if (IS_ERR(pending_snapshot->new_fs_root_args)) { >> + ret = PTR_ERR(pending_snapshot->new_fs_root_args); >> + pending_snapshot->new_fs_root_args = NULL; >> goto free_pending; >> + } >> + >> pending_snapshot->root_item = kzalloc(sizeof(struct btrfs_root_item), >> GFP_KERNEL); >> pending_snapshot->path = btrfs_alloc_path(); >> @@ -850,16 +853,11 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, >> >> d_instantiate(dentry, inode); >> ret = 0; >> - pending_snapshot->anon_dev = 0; >> fail: >> - /* Prevent double freeing of anon_dev */ >> - if (ret && pending_snapshot->snap) >> - pending_snapshot->snap->anon_dev = 0; >> btrfs_put_root(pending_snapshot->snap); >> btrfs_subvolume_release_metadata(root, &pending_snapshot->block_rsv); >> free_pending: >> - if (pending_snapshot->anon_dev) >> - free_anon_bdev(pending_snapshot->anon_dev); >> + btrfs_new_fs_root_args_destroy(pending_snapshot->new_fs_root_args); >> kfree(pending_snapshot->root_item); >> btrfs_free_path(pending_snapshot->path); >> kfree(pending_snapshot); >> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >> index d1f1da6820fb..614eb84422e1 100644 >> --- a/fs/btrfs/transaction.c >> +++ b/fs/btrfs/transaction.c >> @@ -1777,7 +1777,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, >> } >> >> key.offset = (u64)-1; >> - pending->snap = btrfs_get_new_fs_root(fs_info, objectid, pending->anon_dev); >> + pending->snap = btrfs_get_new_fs_root(fs_info, objectid, pending->new_fs_root_args); >> if (IS_ERR(pending->snap)) { >> ret = PTR_ERR(pending->snap); >> pending->snap = NULL; >> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h >> index 970ff316069d..34648a617a2c 100644 >> --- a/fs/btrfs/transaction.h >> +++ b/fs/btrfs/transaction.h >> @@ -10,6 +10,7 @@ >> #include "btrfs_inode.h" >> #include "delayed-ref.h" >> #include "ctree.h" >> +#include "disk-io.h" >> >> enum btrfs_trans_state { >> TRANS_STATE_RUNNING, >> @@ -161,8 +162,8 @@ struct btrfs_pending_snapshot { >> struct btrfs_block_rsv block_rsv; >> /* extra metadata reservation for relocation */ >> int error; >> - /* Preallocated anonymous block device number */ >> - dev_t anon_dev; >> + /* Preallocated new_fs_root_args */ >> + struct btrfs_new_fs_root_args *new_fs_root_args; >> bool readonly; >> struct list_head list; >> }; >> -- >> 2.17.1 >>
On Tue, Dec 13, 2022 at 11:29:09AM +0800, robbieko wrote: > Filipe Manana 於 2022/12/13 上午1:04 寫道: > >> + int err; > >> + struct btrfs_new_fs_root_args *args; > >> + > >> + args = kzalloc(sizeof(*args), gfp_mask); > >> + if (!args) { > >> + err = -ENOMEM; > >> + goto error; > >> + } > > if (!args) > > return ERR_PTR(-ENOMEM); > > > > That's simpler. > > We hope to satisfy the single-exit rule as much as possible to increase > code readability and uniform resource release. If there's no cleanup needed the first few checks can do direct return, and then the part with single exit and errors handled as goto.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index d99bf7c64611..afe16e1f0b18 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1422,12 +1422,44 @@ struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root, return root; } +struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask) +{ + int err; + struct btrfs_new_fs_root_args *args; + + args = kzalloc(sizeof(*args), gfp_mask); + if (!args) { + err = -ENOMEM; + goto error; + } + + err = get_anon_bdev(&args->anon_dev); + if (err) + goto error; + + return args; + +error: + btrfs_new_fs_root_args_destroy(args); + return ERR_PTR(err); +} + +void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args) +{ + if (!args) + return; + if (args->anon_dev) + free_anon_bdev(args->anon_dev); + kfree(args); +} + /* * Initialize subvolume root in-memory structure * * @anon_dev: anonymous device to attach to the root, if zero, allocate new */ -static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) +static int btrfs_init_fs_root(struct btrfs_root *root, + struct btrfs_new_fs_root_args *new_fs_root_args) { int ret; unsigned int nofs_flag; @@ -1454,12 +1486,13 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) */ if (is_fstree(root->root_key.objectid) && btrfs_root_refs(&root->root_item) > 0) { - if (!anon_dev) { + if (!new_fs_root_args || !new_fs_root_args->anon_dev) { ret = get_anon_bdev(&root->anon_dev); if (ret) goto fail; } else { - root->anon_dev = anon_dev; + root->anon_dev = new_fs_root_args->anon_dev; + new_fs_root_args->anon_dev = 0; } } @@ -1633,7 +1666,8 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info) * for orphan roots */ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, - u64 objectid, dev_t anon_dev, + u64 objectid, + struct btrfs_new_fs_root_args *new_fs_root_args, bool check_ref) { struct btrfs_root *root; @@ -1647,8 +1681,8 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, again: root = btrfs_lookup_fs_root(fs_info, objectid); if (root) { - /* Shouldn't get preallocated anon_dev for cached roots */ - ASSERT(!anon_dev); + /* Shouldn't get new_fs_root_args for cached roots */ + ASSERT(!new_fs_root_args); if (check_ref && btrfs_root_refs(&root->root_item) == 0) { btrfs_put_root(root); return ERR_PTR(-ENOENT); @@ -1668,7 +1702,7 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, goto fail; } - ret = btrfs_init_fs_root(root, anon_dev); + ret = btrfs_init_fs_root(root, new_fs_root_args); if (ret) goto fail; @@ -1698,14 +1732,6 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, } return root; fail: - /* - * If our caller provided us an anonymous device, then it's his - * responsibility to free it in case we fail. So we have to set our - * root's anon_dev to 0 to avoid a double free, once by btrfs_put_root() - * and once again by our caller. - */ - if (anon_dev) - root->anon_dev = 0; btrfs_put_root(root); return ERR_PTR(ret); } @@ -1720,7 +1746,7 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info, struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, u64 objectid, bool check_ref) { - return btrfs_get_root_ref(fs_info, objectid, 0, check_ref); + return btrfs_get_root_ref(fs_info, objectid, NULL, check_ref); } /* @@ -1732,9 +1758,10 @@ struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, * parameter value */ struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info, - u64 objectid, dev_t anon_dev) + u64 objectid, + struct btrfs_new_fs_root_args *new_fs_root_args) { - return btrfs_get_root_ref(fs_info, objectid, anon_dev, true); + return btrfs_get_root_ref(fs_info, objectid, new_fs_root_args, true); } /* diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index 9fa923e005a3..a7c91bfb0c16 100644 --- a/fs/btrfs/disk-io.h +++ b/fs/btrfs/disk-io.h @@ -25,6 +25,11 @@ static inline u64 btrfs_sb_offset(int mirror) return BTRFS_SUPER_INFO_OFFSET; } +struct btrfs_new_fs_root_args { + /* Preallocated anonymous block device number */ + dev_t anon_dev; +}; + struct btrfs_device; struct btrfs_fs_devices; @@ -58,6 +63,8 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, int btrfs_commit_super(struct btrfs_fs_info *fs_info); struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root, struct btrfs_key *key); +struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask); +void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args); int btrfs_insert_fs_root(struct btrfs_fs_info *fs_info, struct btrfs_root *root); void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info); @@ -65,7 +72,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info); struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info, u64 objectid, bool check_ref); struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info, - u64 objectid, dev_t anon_dev); + u64 objectid, + struct btrfs_new_fs_root_args *new_fs_root_args); struct btrfs_root *btrfs_get_fs_root_commit_root(struct btrfs_fs_info *fs_info, struct btrfs_path *path, u64 objectid); diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 5ba2e810dc6e..5785336ab7cf 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -588,8 +588,8 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, }; unsigned int trans_num_items; int ret; - dev_t anon_dev; u64 objectid; + struct btrfs_new_fs_root_args *new_fs_root_args = NULL; root_item = kzalloc(sizeof(*root_item), GFP_KERNEL); if (!root_item) @@ -608,14 +608,17 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, goto out_root_item; } - ret = get_anon_bdev(&anon_dev); - if (ret < 0) + new_fs_root_args = btrfs_new_fs_root_args_prepare(GFP_KERNEL); + if (IS_ERR(new_fs_root_args)) { + ret = PTR_ERR(new_fs_root_args); + new_fs_root_args = NULL; goto out_root_item; + } new_inode_args.inode = btrfs_new_subvol_inode(mnt_userns, dir); if (!new_inode_args.inode) { ret = -ENOMEM; - goto out_anon_dev; + goto out_root_item; } ret = btrfs_new_inode_prepare(&new_inode_args, &trans_num_items); if (ret) @@ -706,14 +709,12 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, free_extent_buffer(leaf); leaf = NULL; - new_root = btrfs_get_new_fs_root(fs_info, objectid, anon_dev); + new_root = btrfs_get_new_fs_root(fs_info, objectid, new_fs_root_args); if (IS_ERR(new_root)) { ret = PTR_ERR(new_root); btrfs_abort_transaction(trans, ret); goto out; } - /* anon_dev is owned by new_root now. */ - anon_dev = 0; BTRFS_I(new_inode_args.inode)->root = new_root; /* ... and new_root is owned by new_inode_args.inode now. */ @@ -752,10 +753,8 @@ static noinline int create_subvol(struct user_namespace *mnt_userns, btrfs_new_inode_args_destroy(&new_inode_args); out_inode: iput(new_inode_args.inode); -out_anon_dev: - if (anon_dev) - free_anon_bdev(anon_dev); out_root_item: + btrfs_new_fs_root_args_destroy(new_fs_root_args); kfree(root_item); return ret; } @@ -791,9 +790,13 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, if (!pending_snapshot) return -ENOMEM; - ret = get_anon_bdev(&pending_snapshot->anon_dev); - if (ret < 0) + pending_snapshot->new_fs_root_args = btrfs_new_fs_root_args_prepare(GFP_KERNEL); + if (IS_ERR(pending_snapshot->new_fs_root_args)) { + ret = PTR_ERR(pending_snapshot->new_fs_root_args); + pending_snapshot->new_fs_root_args = NULL; goto free_pending; + } + pending_snapshot->root_item = kzalloc(sizeof(struct btrfs_root_item), GFP_KERNEL); pending_snapshot->path = btrfs_alloc_path(); @@ -850,16 +853,11 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, d_instantiate(dentry, inode); ret = 0; - pending_snapshot->anon_dev = 0; fail: - /* Prevent double freeing of anon_dev */ - if (ret && pending_snapshot->snap) - pending_snapshot->snap->anon_dev = 0; btrfs_put_root(pending_snapshot->snap); btrfs_subvolume_release_metadata(root, &pending_snapshot->block_rsv); free_pending: - if (pending_snapshot->anon_dev) - free_anon_bdev(pending_snapshot->anon_dev); + btrfs_new_fs_root_args_destroy(pending_snapshot->new_fs_root_args); kfree(pending_snapshot->root_item); btrfs_free_path(pending_snapshot->path); kfree(pending_snapshot); diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index d1f1da6820fb..614eb84422e1 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1777,7 +1777,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, } key.offset = (u64)-1; - pending->snap = btrfs_get_new_fs_root(fs_info, objectid, pending->anon_dev); + pending->snap = btrfs_get_new_fs_root(fs_info, objectid, pending->new_fs_root_args); if (IS_ERR(pending->snap)) { ret = PTR_ERR(pending->snap); pending->snap = NULL; diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h index 970ff316069d..34648a617a2c 100644 --- a/fs/btrfs/transaction.h +++ b/fs/btrfs/transaction.h @@ -10,6 +10,7 @@ #include "btrfs_inode.h" #include "delayed-ref.h" #include "ctree.h" +#include "disk-io.h" enum btrfs_trans_state { TRANS_STATE_RUNNING, @@ -161,8 +162,8 @@ struct btrfs_pending_snapshot { struct btrfs_block_rsv block_rsv; /* extra metadata reservation for relocation */ int error; - /* Preallocated anonymous block device number */ - dev_t anon_dev; + /* Preallocated new_fs_root_args */ + struct btrfs_new_fs_root_args *new_fs_root_args; bool readonly; struct list_head list; };