[v3,1/4] Btrfs: fix wrong send_in_progress accounting
diff mbox

Message ID 1390021080-30788-1-git-send-email-wangsl.fnst@cn.fujitsu.com
State Accepted
Headers show

Commit Message

Wang Shilong Jan. 18, 2014, 4:58 a.m. UTC
Steps to reproduce:
 # mkfs.btrfs -f /dev/sda8
 # mount /dev/sda8 /mnt
 # btrfs sub snapshot -r /mnt /mnt/snap1
 # btrfs sub snapshot -r /mnt /mnt/snap2
 # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1
 # dmesg

The problem is that we will sort clone roots(include @send_root), it
might push @send_root before thus @send_root's @send_in_progress will
be decreased twice.

Cc: David Sterba <dsterba@suse.cz>
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
changelog
v2->v3:
	add comments as david addressed.
v1->v2:
	use right root to check
---
 fs/btrfs/send.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

David Sterba Jan. 21, 2014, 4:03 p.m. UTC | #1
On Sat, Jan 18, 2014 at 12:58:00PM +0800, Wang Shilong wrote:
> Steps to reproduce:
>  # mkfs.btrfs -f /dev/sda8
>  # mount /dev/sda8 /mnt
>  # btrfs sub snapshot -r /mnt /mnt/snap1
>  # btrfs sub snapshot -r /mnt /mnt/snap2
>  # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1
>  # dmesg
> 
> The problem is that we will sort clone roots(include @send_root), it
> might push @send_root before thus @send_root's @send_in_progress will
> be decreased twice.
> 
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
Reviewed-by: David Sterba <dsterba@suse.cz>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Filipe Manana Jan. 21, 2014, 4:25 p.m. UTC | #2
On Sat, Jan 18, 2014 at 4:58 AM, Wang Shilong
<wangsl.fnst@cn.fujitsu.com> wrote:
> Steps to reproduce:
>  # mkfs.btrfs -f /dev/sda8
>  # mount /dev/sda8 /mnt
>  # btrfs sub snapshot -r /mnt /mnt/snap1
>  # btrfs sub snapshot -r /mnt /mnt/snap2
>  # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1
>  # dmesg
>
> The problem is that we will sort clone roots(include @send_root), it
> might push @send_root before thus @send_root's @send_in_progress will
> be decreased twice.
>
> Cc: David Sterba <dsterba@suse.cz>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
> changelog
> v2->v3:
>         add comments as david addressed.
> v1->v2:
>         use right root to check
> ---
>  fs/btrfs/send.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index bff0b1a..83b6bdb 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -4752,6 +4752,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>         u32 i;
>         u64 *clone_sources_tmp = NULL;
>         int clone_sources_to_rollback = 0;
> +       int sort_clone_roots = 0;
>
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EPERM;
> @@ -4942,6 +4943,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>         sort(sctx->clone_roots, sctx->clone_roots_cnt,
>                         sizeof(*sctx->clone_roots), __clone_root_cmp_sort,
>                         NULL);
> +       sort_clone_roots = 1;
>
>         ret = send_subvol(sctx);
>         if (ret < 0)
> @@ -4957,11 +4959,22 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>         }
>
>  out:
> -       for (i = 0; sctx && i < clone_sources_to_rollback; i++)
> -               btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root);
> +       if (sort_clone_roots) {
> +               for (i = 0; i < sctx->clone_roots_cnt; i++)
> +                       btrfs_root_dec_send_in_progress(
> +                                       sctx->clone_roots[i].root);

sctx can be NULL here, anywhere after the 'out' label.

> +       } else {
> +               for (i = 0; sctx && i < clone_sources_to_rollback; i++)
> +                       btrfs_root_dec_send_in_progress(
> +                                       sctx->clone_roots[i].root);

Same here.

> +               /*
> +                * if we fail to add @send_root in clone roots, we still
> +                * need to decrease @send_in_progress count here.
> +                */
> +               btrfs_root_dec_send_in_progress(send_root);
> +       }
>         if (sctx && !IS_ERR_OR_NULL(sctx->parent_root))
>                 btrfs_root_dec_send_in_progress(sctx->parent_root);
> -       btrfs_root_dec_send_in_progress(send_root);
>
>         kfree(arg);
>         vfree(clone_sources_tmp);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wang Shilong Jan. 21, 2014, 4:45 p.m. UTC | #3
Hello Filipe, 

> On Sat, Jan 18, 2014 at 4:58 AM, Wang Shilong
> <wangsl.fnst@cn.fujitsu.com> wrote:
>> Steps to reproduce:
>> # mkfs.btrfs -f /dev/sda8
>> # mount /dev/sda8 /mnt
>> # btrfs sub snapshot -r /mnt /mnt/snap1
>> # btrfs sub snapshot -r /mnt /mnt/snap2
>> # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1
>> # dmesg
>> 
>> The problem is that we will sort clone roots(include @send_root), it
>> might push @send_root before thus @send_root's @send_in_progress will
>> be decreased twice.
>> 
>> Cc: David Sterba <dsterba@suse.cz>
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>> changelog
>> v2->v3:
>>        add comments as david addressed.
>> v1->v2:
>>        use right root to check
>> ---
>> fs/btrfs/send.c | 19 ++++++++++++++++---
>> 1 file changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index bff0b1a..83b6bdb 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -4752,6 +4752,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>>        u32 i;
>>        u64 *clone_sources_tmp = NULL;
>>        int clone_sources_to_rollback = 0;
>> +       int sort_clone_roots = 0;
>> 
>>        if (!capable(CAP_SYS_ADMIN))
>>                return -EPERM;
>> @@ -4942,6 +4943,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>>        sort(sctx->clone_roots, sctx->clone_roots_cnt,
>>                        sizeof(*sctx->clone_roots), __clone_root_cmp_sort,
>>                        NULL);
>> +       sort_clone_roots = 1;
>> 
>>        ret = send_subvol(sctx);
>>        if (ret < 0)
>> @@ -4957,11 +4959,22 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>>        }
>> 
>> out:
>> -       for (i = 0; sctx && i < clone_sources_to_rollback; i++)
>> -               btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root);
>> +       if (sort_clone_roots) {
>> +               for (i = 0; i < sctx->clone_roots_cnt; i++)
>> +                       btrfs_root_dec_send_in_progress(
>> +                                       sctx->clone_roots[i].root);
> 
> sctx can be NULL here, anywhere after the 'out' label.

Not really, if @sort_clone_roots is set which means we must have allocated
sctx successfully.

> 
>> +       } else {
>> +               for (i = 0; sctx && i < clone_sources_to_rollback; i++)
>> +                       btrfs_root_dec_send_in_progress(
>> +                                       sctx->clone_roots[i].root);
> 
> Same here.

Notice there is a check above,

sctx && i < clone_sources_to_rollback

I really took care of that when i wrote the patch, please correct
me if i miss something here^_^.

Thanks,
Wang
> 
>> +               /*
>> +                * if we fail to add @send_root in clone roots, we still
>> +                * need to decrease @send_in_progress count here.
>> +                */
>> +               btrfs_root_dec_send_in_progress(send_root);
>> +       }
>>        if (sctx && !IS_ERR_OR_NULL(sctx->parent_root))
>>                btrfs_root_dec_send_in_progress(sctx->parent_root);
>> -       btrfs_root_dec_send_in_progress(send_root);
>> 
>>        kfree(arg);
>>        vfree(clone_sources_tmp);
>> --
>> 1.8.3.1
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> "Reasonable men adapt themselves to the world.
> Unreasonable men adapt the world to themselves.
> That's why all progress depends on unreasonable men."
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Filipe Manana Jan. 21, 2014, 4:52 p.m. UTC | #4
On Tue, Jan 21, 2014 at 4:45 PM, Wang Shilong <wangshilong1991@gmail.com> wrote:
>
> Hello Filipe,
>
>> On Sat, Jan 18, 2014 at 4:58 AM, Wang Shilong
>> <wangsl.fnst@cn.fujitsu.com> wrote:
>>> Steps to reproduce:
>>> # mkfs.btrfs -f /dev/sda8
>>> # mount /dev/sda8 /mnt
>>> # btrfs sub snapshot -r /mnt /mnt/snap1
>>> # btrfs sub snapshot -r /mnt /mnt/snap2
>>> # btrfs send /mnt/snap1 -p /mnt/snap2 -f /mnt/1
>>> # dmesg
>>>
>>> The problem is that we will sort clone roots(include @send_root), it
>>> might push @send_root before thus @send_root's @send_in_progress will
>>> be decreased twice.
>>>
>>> Cc: David Sterba <dsterba@suse.cz>
>>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>> ---
>>> changelog
>>> v2->v3:
>>>        add comments as david addressed.
>>> v1->v2:
>>>        use right root to check
>>> ---
>>> fs/btrfs/send.c | 19 ++++++++++++++++---
>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>>> index bff0b1a..83b6bdb 100644
>>> --- a/fs/btrfs/send.c
>>> +++ b/fs/btrfs/send.c
>>> @@ -4752,6 +4752,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>>>        u32 i;
>>>        u64 *clone_sources_tmp = NULL;
>>>        int clone_sources_to_rollback = 0;
>>> +       int sort_clone_roots = 0;
>>>
>>>        if (!capable(CAP_SYS_ADMIN))
>>>                return -EPERM;
>>> @@ -4942,6 +4943,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>>>        sort(sctx->clone_roots, sctx->clone_roots_cnt,
>>>                        sizeof(*sctx->clone_roots), __clone_root_cmp_sort,
>>>                        NULL);
>>> +       sort_clone_roots = 1;
>>>
>>>        ret = send_subvol(sctx);
>>>        if (ret < 0)
>>> @@ -4957,11 +4959,22 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>>>        }
>>>
>>> out:
>>> -       for (i = 0; sctx && i < clone_sources_to_rollback; i++)
>>> -               btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root);
>>> +       if (sort_clone_roots) {
>>> +               for (i = 0; i < sctx->clone_roots_cnt; i++)
>>> +                       btrfs_root_dec_send_in_progress(
>>> +                                       sctx->clone_roots[i].root);
>>
>> sctx can be NULL here, anywhere after the 'out' label.
>
> Not really, if @sort_clone_roots is set which means we must have allocated
> sctx successfully.

Right missed that, same below.
Static checker was complaining about this.

thanks

>
>>
>>> +       } else {
>>> +               for (i = 0; sctx && i < clone_sources_to_rollback; i++)
>>> +                       btrfs_root_dec_send_in_progress(
>>> +                                       sctx->clone_roots[i].root);
>>
>> Same here.
>
> Notice there is a check above,
>
> sctx && i < clone_sources_to_rollback
>
> I really took care of that when i wrote the patch, please correct
> me if i miss something here^_^.
>
> Thanks,
> Wang
>>
>>> +               /*
>>> +                * if we fail to add @send_root in clone roots, we still
>>> +                * need to decrease @send_in_progress count here.
>>> +                */
>>> +               btrfs_root_dec_send_in_progress(send_root);
>>> +       }
>>>        if (sctx && !IS_ERR_OR_NULL(sctx->parent_root))
>>>                btrfs_root_dec_send_in_progress(sctx->parent_root);
>>> -       btrfs_root_dec_send_in_progress(send_root);
>>>
>>>        kfree(arg);
>>>        vfree(clone_sources_tmp);
>>> --
>>> 1.8.3.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Filipe David Manana,
>>
>> "Reasonable men adapt themselves to the world.
>> Unreasonable men adapt the world to themselves.
>> That's why all progress depends on unreasonable men."
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch
diff mbox

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index bff0b1a..83b6bdb 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4752,6 +4752,7 @@  long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 	u32 i;
 	u64 *clone_sources_tmp = NULL;
 	int clone_sources_to_rollback = 0;
+	int sort_clone_roots = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -4942,6 +4943,7 @@  long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 	sort(sctx->clone_roots, sctx->clone_roots_cnt,
 			sizeof(*sctx->clone_roots), __clone_root_cmp_sort,
 			NULL);
+	sort_clone_roots = 1;
 
 	ret = send_subvol(sctx);
 	if (ret < 0)
@@ -4957,11 +4959,22 @@  long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 	}
 
 out:
-	for (i = 0; sctx && i < clone_sources_to_rollback; i++)
-		btrfs_root_dec_send_in_progress(sctx->clone_roots[i].root);
+	if (sort_clone_roots) {
+		for (i = 0; i < sctx->clone_roots_cnt; i++)
+			btrfs_root_dec_send_in_progress(
+					sctx->clone_roots[i].root);
+	} else {
+		for (i = 0; sctx && i < clone_sources_to_rollback; i++)
+			btrfs_root_dec_send_in_progress(
+					sctx->clone_roots[i].root);
+		/*
+		 * if we fail to add @send_root in clone roots, we still
+		 * need to decrease @send_in_progress count here.
+		 */
+		btrfs_root_dec_send_in_progress(send_root);
+	}
 	if (sctx && !IS_ERR_OR_NULL(sctx->parent_root))
 		btrfs_root_dec_send_in_progress(sctx->parent_root);
-	btrfs_root_dec_send_in_progress(send_root);
 
 	kfree(arg);
 	vfree(clone_sources_tmp);