On 02/07/13 07:02, Miao Xie wrote:
> The argument "inherit" of btrfs_ioctl_snap_create_transid() was assigned
> to NULL during we created the snapshots, so we didn't free it though we
> called kfree() in the caller.
>
> But since we are sure the snapshot creation is done after the function -
> btrfs_ioctl_snap_create_transid() - completes, it is safe that we don't
> assign the pointer "inherit" to NULL, and just free it in the caller of
> btrfs_ioctl_snap_create_transid(). In this way, the code can become more
> readable.
NAK. The snapshot creation is triggered from btrfs_commit_transaction,
I don't want to implicitly rely on commit_transaction being called for
each snapshot created. I'm not even sure the async path really commits
the transaction.
The responsibility for the creation is passed to the pending_snapshot
data structure, and so should the responsibility for the inherit struct.
-Arne
>
> Reported-by: Alex Lyakas <alex.btrfs@xxxxxxxxxxxxxxxxx>
> Cc: Arne Jansen <sensille@xxxxxxx>
> Signed-off-by: Miao Xie <miaox@xxxxxxxxxxxxxx>
> ---
> fs/btrfs/ioctl.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 02d3035..40f2fbf 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -367,7 +367,7 @@ static noinline int create_subvol(struct btrfs_root *root,
> struct dentry *dentry,
> char *name, int namelen,
> u64 *async_transid,
> - struct btrfs_qgroup_inherit **inherit)
> + struct btrfs_qgroup_inherit *inherit)
> {
> struct btrfs_trans_handle *trans;
> struct btrfs_key key;
> @@ -401,8 +401,7 @@ static noinline int create_subvol(struct btrfs_root *root,
> if (IS_ERR(trans))
> return PTR_ERR(trans);
>
> - ret = btrfs_qgroup_inherit(trans, root->fs_info, 0, objectid,
> - inherit ? *inherit : NULL);
> + ret = btrfs_qgroup_inherit(trans, root->fs_info, 0, objectid, inherit);
> if (ret)
> goto fail;
>
> @@ -530,7 +529,7 @@ fail:
>
> static int create_snapshot(struct btrfs_root *root, struct dentry *dentry,
> char *name, int namelen, u64 *async_transid,
> - bool readonly, struct btrfs_qgroup_inherit **inherit)
> + bool readonly, struct btrfs_qgroup_inherit *inherit)
> {
> struct inode *inode;
> struct btrfs_pending_snapshot *pending_snapshot;
> @@ -549,10 +548,7 @@ static int create_snapshot(struct btrfs_root *root, struct dentry *dentry,
> pending_snapshot->dentry = dentry;
> pending_snapshot->root = root;
> pending_snapshot->readonly = readonly;
> - if (inherit) {
> - pending_snapshot->inherit = *inherit;
> - *inherit = NULL; /* take responsibility to free it */
> - }
> + pending_snapshot->inherit = inherit;
>
> trans = btrfs_start_transaction(root->fs_info->extent_root, 6);
> if (IS_ERR(trans)) {
> @@ -692,7 +688,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
> char *name, int namelen,
> struct btrfs_root *snap_src,
> u64 *async_transid, bool readonly,
> - struct btrfs_qgroup_inherit **inherit)
> + struct btrfs_qgroup_inherit *inherit)
> {
> struct inode *dir = parent->dentry->d_inode;
> struct dentry *dentry;
> @@ -1454,7 +1450,7 @@ out:
> static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
> char *name, unsigned long fd, int subvol,
> u64 *transid, bool readonly,
> - struct btrfs_qgroup_inherit **inherit)
> + struct btrfs_qgroup_inherit *inherit)
> {
> int namelen;
> int ret = 0;
> @@ -1563,7 +1559,7 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
>
> ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
> vol_args->fd, subvol, ptr,
> - readonly, &inherit);
> + readonly, inherit);
>
> if (ret == 0 && ptr &&
> copy_to_user(arg +
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
