On Thu, 07 Feb 2013 09:43:47 +0100, Arne Jansen wrote:
> 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.
I don't agree with you.
We are sure the async path really commits the transaction because we pass 1
as the value of the third argument into btrfs_commit_transaction_async(). It
means we must wait for the completion of the current transaction. So Freeing
the inherit struct in the caller is safe.
Besides that, the pending_snapshot data structure is also allocated and freed
by the same function in fact, why not use this style for the inherit struct.
I think it is more readable. Assigning a pointer to be NULL and freeing it
in the caller is very strange for the people who reads the code. (It is also
the reason why I made the mistake at the beginning.)
So I think my patch is reasonable.
Thanks
Miao
> -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
