[PATCH 1/4] Btrfs: fix wrong send_in_progress accounting

[PATCH 1/4] Btrfs: fix wrong send_in_progress accounting

[Wang Shilong]

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/snap2 -p /mnt/snap1

As @send_root will also add into clone_sources, and we should
take care not to decrease its count twice.

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
Hi david, i am ok to fold this patch into your previous patch
or as a seperated patch.
---
 fs/btrfs/send.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index bff0b1a..076b066 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4908,6 +4908,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 			spin_unlock(&clone_root->root_item_lock);
 			sctx->clone_roots[i].root = clone_root;
 		}
+		clone_sources_to_rollback--;
 		vfree(clone_sources_tmp);
 		clone_sources_tmp = NULL;
 	}
-- 
1.8.3.1

Re: [PATCH 1/4] Btrfs: fix wrong send_in_progress accounting

[David Sterba]

On Mon, Jan 06, 2014 at 05:25:06PM +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/snap2 -p /mnt/snap1
> 
> As @send_root will also add into clone_sources, and we should
> take care not to decrease its count twice.

Yes, send_root is appended to the clone_roots but the
clone_sources_to_rollback does not track this, it's not updated after
it's added in

  sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;

so send->root->send_in_progress is not modified in the loop that
decrements all clone_root's counters again.

I don't see the warning that should catch negative values of the counter
in btrfs_root_dec_send_in_progress. Am I missing something?


david

Re: [PATCH 1/4] Btrfs: fix wrong send_in_progress accounting

[Wang Shilong]

On 01/07/2014 12:30 AM, David Sterba wrote:
> On Mon, Jan 06, 2014 at 05:25:06PM +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/snap2 -p /mnt/snap1
>>
>> As @send_root will also add into clone_sources, and we should
>> take care not to decrease its count twice.
> Yes, send_root is appended to the clone_roots but the
> clone_sources_to_rollback does not track this, it's not updated after
> it's added in
>
>    sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;
>
> so send->root->send_in_progress is not modified in the loop that
> decrements all clone_root's counters again.
>
> I don't see the warning that should catch negative values of the counter
> in btrfs_root_dec_send_in_progress. Am I missing something?
just run 'dmesg' after the above commands.

Thanks,
Wang
>
>
> david
> --
> 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
>

Re: [PATCH 1/4] Btrfs: fix wrong send_in_progress accounting

[Wang Shilong]

Hi David,

On 01/07/2014 12:30 AM, David Sterba wrote:
> On Mon, Jan 06, 2014 at 05:25:06PM +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/snap2 -p /mnt/snap1
>>
>> As @send_root will also add into clone_sources, and we should
>> take care not to decrease its count twice.
> Yes, send_root is appended to the clone_roots but the
> clone_sources_to_rollback does not track this, it's not updated after
> it's added in
>
>    sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;
>
> so send->root->send_in_progress is not modified in the loop that
> decrements all clone_root's counters again.
>
> I don't see the warning that should catch negative values of the counter
> in btrfs_root_dec_send_in_progress. Am I missing something?
You are right here, but problem still exist.

The problem is that we will sort clone sources before we decrease count,
When sorting clone counts, we include send_root, and it will push to the 
first
place.....and when we try to free send_root count, it will be decreased 
twice.

Hope i am not wrong this time!

Thanks Daivd to force me dig more!

>
>
> david
> --
> 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
>

Re: [PATCH 1/4] Btrfs: fix wrong send_in_progress accounting

[Wang Shilong]

On 01/07/2014 11:10 AM, Wang Shilong wrote:
> Hi David,
>
> On 01/07/2014 12:30 AM, David Sterba wrote:
>> On Mon, Jan 06, 2014 at 05:25:06PM +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/snap2 -p /mnt/snap1
>>>
>>> As @send_root will also add into clone_sources, and we should
>>> take care not to decrease its count twice.
>> Yes, send_root is appended to the clone_roots but the
>> clone_sources_to_rollback does not track this, it's not updated after
>> it's added in
>>
>>    sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;
>>
>> so send->root->send_in_progress is not modified in the loop that
>> decrements all clone_root's counters again.
>>
>> I don't see the warning that should catch negative values of the counter
>> in btrfs_root_dec_send_in_progress. Am I missing something?
> You are right here, but problem still exist.
>
> The problem is that we will sort clone sources before we decrease count,
> When sorting clone counts, we include send_root, and it will push to 
> the first
> place.....and when we try to free send_root count, it will be 
> decreased twice.
>
> Hope i am not wrong this time!
>
> Thanks Daivd to force me dig more!

My steps is a little problem, if you try this, you will find the warnig:

# btrfs sub snapshot . -r snap1
# btrfs sub snapshot . -r snap2
# btrfs send snap1 -p snap2 -f 1
# dmesg


>
>>
>>
>> david
>> -- 
>> 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
>