On Tue, Jan 21, 2014 at 4:45 PM, Wang Shilong <wangshilong1...@gmail.com> wrote:
>
> Hello Filipe,
>
>> On Sat, Jan 18, 2014 at 4:58 AM, Wang Shilong
>> <wangsl.f...@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 <dste...@suse.cz>
>>> Signed-off-by: Wang Shilong <wangsl.f...@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 majord...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
