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

[PATCH v2 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/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 v1->v2:
	on the right way to fix the problem.(thanks to David)
---
 fs/btrfs/send.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index bff0b1a..5b69785 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,19 @@ 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);
+
+		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

[PATCH v2 2/4] Btrfs: fix protection between send and root deletion

[Wang Shilong]

We should gurantee that parent and clone roots can not be destroyed
during send, for this we have two ideas.

1.by holding @subvol_sem, this might be a nightmare, because it will
block all subvolumes deletion for a long time.

2.Miao pointed out we can reuse @send_in_progress, that mean we will
skip snapshot deletion if root sending is in progress.

Here we adopt the second approach since it won't block other subvolumes
deletion for a long time.

Besides in btrfs_clean_one_deleted_snapshot(), we only check first root
, if this root is involved in send, we return directly rather than
continue to check.There are several reasons about it:

1.this case happen seldomly.
2.after sending,cleaner thread can continue to drop that root.
3.make code simple

Cc: David Sterba <dsterba@suse.cz>
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
Changelog v1->v2:
	use right root to check(pointed by David)
---
 fs/btrfs/send.c        | 16 ++++++++++++++++
 fs/btrfs/transaction.c | 13 +++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 5b69785..4e2461b 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4753,6 +4753,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 	u64 *clone_sources_tmp = NULL;
 	int clone_sources_to_rollback = 0;
 	int sort_clone_roots = 0;
+	int index;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -4893,8 +4894,12 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 			key.objectid = clone_sources_tmp[i];
 			key.type = BTRFS_ROOT_ITEM_KEY;
 			key.offset = (u64)-1;
+
+			index = srcu_read_lock(&fs_info->subvol_srcu);
+
 			clone_root = btrfs_read_fs_root_no_name(fs_info, &key);
 			if (IS_ERR(clone_root)) {
+				srcu_read_unlock(&fs_info->subvol_srcu, index);
 				ret = PTR_ERR(clone_root);
 				goto out;
 			}
@@ -4903,10 +4908,13 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 			clone_root->send_in_progress++;
 			if (!btrfs_root_readonly(clone_root)) {
 				spin_unlock(&clone_root->root_item_lock);
+				srcu_read_unlock(&fs_info->subvol_srcu, index);
 				ret = -EPERM;
 				goto out;
 			}
 			spin_unlock(&clone_root->root_item_lock);
+			srcu_read_unlock(&fs_info->subvol_srcu, index);
+
 			sctx->clone_roots[i].root = clone_root;
 		}
 		vfree(clone_sources_tmp);
@@ -4917,19 +4925,27 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
 		key.objectid = arg->parent_root;
 		key.type = BTRFS_ROOT_ITEM_KEY;
 		key.offset = (u64)-1;
+
+		index = srcu_read_lock(&fs_info->subvol_srcu);
+
 		sctx->parent_root = btrfs_read_fs_root_no_name(fs_info, &key);
 		if (IS_ERR(sctx->parent_root)) {
+			srcu_read_unlock(&fs_info->subvol_srcu, index);
 			ret = PTR_ERR(sctx->parent_root);
 			goto out;
 		}
+
 		spin_lock(&sctx->parent_root->root_item_lock);
 		sctx->parent_root->send_in_progress++;
 		if (!btrfs_root_readonly(sctx->parent_root)) {
 			spin_unlock(&sctx->parent_root->root_item_lock);
+			srcu_read_unlock(&fs_info->subvol_srcu, index);
 			ret = -EPERM;
 			goto out;
 		}
 		spin_unlock(&sctx->parent_root->root_item_lock);
+
+		srcu_read_unlock(&fs_info->subvol_srcu, index);
 	}
 
 	/*
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 30d11bb..1ef8e28 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1971,6 +1971,19 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
 	}
 	root = list_first_entry(&fs_info->dead_roots,
 			struct btrfs_root, root_list);
+	/*
+	 * Make sure root is not involved in send,
+	 * if we fail with first root, we return
+	 * directly rather than continue.
+	 */
+	spin_lock(&root->root_item_lock);
+	if (root->send_in_progress) {
+		spin_unlock(&fs_info->trans_lock);
+		spin_unlock(&root->root_item_lock);
+		return 0;
+	}
+	spin_unlock(&root->root_item_lock);
+
 	list_del_init(&root->root_list);
 	spin_unlock(&fs_info->trans_lock);
 
-- 
1.8.3.1

Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion

[David Sterba]

On Tue, Jan 07, 2014 at 05:25:19PM +0800, Wang Shilong wrote:
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1971,6 +1971,19 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
>  	}
>  	root = list_first_entry(&fs_info->dead_roots,
>  			struct btrfs_root, root_list);

You're pulling the root from dead_roots here ...

> +	/*
> +	 * Make sure root is not involved in send,

... and there's no way to send such a thing.

> +	 * if we fail with first root, we return
> +	 * directly rather than continue.
> +	 */
> +	spin_lock(&root->root_item_lock);
> +	if (root->send_in_progress) {
> +		spin_unlock(&fs_info->trans_lock);
> +		spin_unlock(&root->root_item_lock);
> +		return 0;
> +	}
> +	spin_unlock(&root->root_item_lock);
> +
>  	list_del_init(&root->root_list);
>  	spin_unlock(&fs_info->trans_lock);

Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion

[Miao Xie]

On mon, 13 Jan 2014 19:27:45 +0100, David Sterba wrote:
> On Tue, Jan 07, 2014 at 05:25:19PM +0800, Wang Shilong wrote:
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1971,6 +1971,19 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
>>  	}
>>  	root = list_first_entry(&fs_info->dead_roots,
>>  			struct btrfs_root, root_list);
> 
> You're pulling the root from dead_roots here ...
> 
>> +	/*
>> +	 * Make sure root is not involved in send,
> 
> ... and there's no way to send such a thing.

The root what is about to be sent out may be deleted after we get it, if
there is no in-memory i-node in it, it will be inserted into the dead_roots list
immediately, then the cleaner thread will drop it. But the sender doesn't know,
the sender will access a broken tree.

Thanks
Miao

> 
>> +	 * if we fail with first root, we return
>> +	 * directly rather than continue.
>> +	 */
>> +	spin_lock(&root->root_item_lock);
>> +	if (root->send_in_progress) {
>> +		spin_unlock(&fs_info->trans_lock);
>> +		spin_unlock(&root->root_item_lock);
>> +		return 0;
>> +	}
>> +	spin_unlock(&root->root_item_lock);
>> +
>>  	list_del_init(&root->root_list);
>>  	spin_unlock(&fs_info->trans_lock);
> --
> 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 v2 2/4] Btrfs: fix protection between send and root deletion

[David Sterba]

On Tue, Jan 14, 2014 at 10:22:56AM +0800, Miao Xie wrote:
> On mon, 13 Jan 2014 19:27:45 +0100, David Sterba wrote:
> The root what is about to be sent out may be deleted after we get it, if
> there is no in-memory i-node in it, it will be inserted into the dead_roots list
> immediately, then the cleaner thread will drop it. But the sender doesn't know,
> the sender will access a broken tree.

Thanks, I see the race now, but I think that we should prevent the
subvolume deletion at a higher level, similar to the RO->RW protection.

Your fix makes sure that the deleted root will not get cleaned and stays
during the send. Only after it finishes it will be cleaned. Now, what if
send fails or is interrupted? There's no way to redo it. Yes the user
can be blamed for the mistake, or the tools will prevent him to do it.

I see the latter as more user-friendly. Doing a 'send and forget' where
I don't care if the data will be sent properly does not fit the primary
purpose of send/receive with backups.

My idea to fix that:
- add an internal root_item flag to denote a dead root
- set this flag in btrfs_add_dead_root()
- check the flag in send similar to the btrfs_root_readonly checks, for
  all involved roots
- in 'destroy subvolume, check if the send_in_progress is set and refuse
  to delete

The root lookup in send via btrfs_read_fs_root_no_name will check if the
root is dead or not. If it is, ENOENT, aborted send. If it's alive, it's
protected by send_in_progress, send can continue.

This has become more complex than the original patch, I'll try to think
about it again later if it still makes sense. Comments welcome of course.


david

Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion

[Miao Xie]

On wed, 15 Jan 2014 17:40:38 +0100, David Sterba wrote:
> On Tue, Jan 14, 2014 at 10:22:56AM +0800, Miao Xie wrote:
>> On mon, 13 Jan 2014 19:27:45 +0100, David Sterba wrote:
>> The root what is about to be sent out may be deleted after we get it, if
>> there is no in-memory i-node in it, it will be inserted into the dead_roots list
>> immediately, then the cleaner thread will drop it. But the sender doesn't know,
>> the sender will access a broken tree.
> 
> Thanks, I see the race now, but I think that we should prevent the
> subvolume deletion at a higher level, similar to the RO->RW protection.
> 
> Your fix makes sure that the deleted root will not get cleaned and stays
> during the send. Only after it finishes it will be cleaned. Now, what if
> send fails or is interrupted? There's no way to redo it. Yes the user
> can be blamed for the mistake, or the tools will prevent him to do it.

I don't think so. The users should be responsible for their behavior if they
destroy the subvolume.

> 
> I see the latter as more user-friendly. Doing a 'send and forget' where
> I don't care if the data will be sent properly does not fit the primary
> purpose of send/receive with backups.
> 
> My idea to fix that:
> - add an internal root_item flag to denote a dead root
> - set this flag in btrfs_add_dead_root()
> - check the flag in send similar to the btrfs_root_readonly checks, for
>   all involved roots
> - in 'destroy subvolume, check if the send_in_progress is set and refuse
>   to delete

It is similar to our approach. But I think our idea is better because
- we needn't add a new flag
- The subvolumes are special directory, the most operations of them should
  be similar to the common directory. Since we can remove a directory while
  someone is accessing it, it is better that we can destroy a subvolume
  while we are using it as a send parent.

Thanks
Miao

> 
> The root lookup in send via btrfs_read_fs_root_no_name will check if the
> root is dead or not. If it is, ENOENT, aborted send. If it's alive, it's
> protected by send_in_progress, send can continue.
> 
> This has become more complex than the original patch, I'll try to think
> about it again later if it still makes sense. Comments welcome of course.
> 
> 
> 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 v2 2/4] Btrfs: fix protection between send and root deletion

[David Sterba]

On Thu, Jan 16, 2014 at 10:32:38AM +0800, Miao Xie wrote:
> > Your fix makes sure that the deleted root will not get cleaned and stays
> > during the send. Only after it finishes it will be cleaned. Now, what if
> > send fails or is interrupted? There's no way to redo it. Yes the user
> > can be blamed for the mistake, or the tools will prevent him to do it.
> 
> I don't think so. The users should be responsible for their behavior if they
> destroy the subvolume.

Right now it's not possible to determine if a subvolume is involved in a
send (other than the user knows by himself that he started send). Send
or subvolume cleaning can be performed on the background. Although the
user is responsible for his actions, the consequence here is not
obvious, silent and irreversible.

> > I see the latter as more user-friendly. Doing a 'send and forget' where
> > I don't care if the data will be sent properly does not fit the primary
> > purpose of send/receive with backups.
> > 
> > My idea to fix that:
> > - add an internal root_item flag to denote a dead root
> > - set this flag in btrfs_add_dead_root()
> > - check the flag in send similar to the btrfs_root_readonly checks, for
> >   all involved roots
> > - in 'destroy subvolume, check if the send_in_progress is set and refuse
> >   to delete
> 
> It is similar to our approach. But I think our idea is better because
> - we needn't add a new flag

Adding the flag is cheap.

> - The subvolumes are special directory, the most operations of them should
>   be similar to the common directory. Since we can remove a directory while
>   someone is accessing it, it is better that we can destroy a subvolume
>   while we are using it as a send parent.

Yes they're similar, but subvolumes have additional features that need
to be handled appropriately. One cannot send a directory.

So we disagree, I see a reason for the deletion protection and will do
the patch myself. Let's see if we can get more user feedback then.

I'm NAKing this patch in current state, if it helps anything.


david

Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion

[Wang Shilong]

Hi David,

On 01/22/2014 02:16 AM, David Sterba wrote:
> On Thu, Jan 16, 2014 at 10:32:38AM +0800, Miao Xie wrote:
>>> Your fix makes sure that the deleted root will not get cleaned and stays
>>> during the send. Only after it finishes it will be cleaned. Now, what if
>>> send fails or is interrupted? There's no way to redo it. Yes the user
>>> can be blamed for the mistake, or the tools will prevent him to do it.
>> I don't think so. The users should be responsible for their behavior if they
>> destroy the subvolume.
> Right now it's not possible to determine if a subvolume is involved in a
> send (other than the user knows by himself that he started send). Send
> or subvolume cleaning can be performed on the background. Although the
> user is responsible for his actions, the consequence here is not
> obvious, silent and irreversible.
>
>>> I see the latter as more user-friendly. Doing a 'send and forget' where
>>> I don't care if the data will be sent properly does not fit the primary
>>> purpose of send/receive with backups.
>>>
>>> My idea to fix that:
>>> - add an internal root_item flag to denote a dead root
>>> - set this flag in btrfs_add_dead_root()
>>> - check the flag in send similar to the btrfs_root_readonly checks, for
>>>    all involved roots
>>> - in 'destroy subvolume, check if the send_in_progress is set and refuse
>>>    to delete
>> It is similar to our approach. But I think our idea is better because
>> - we needn't add a new flag
> Adding the flag is cheap.
>
>> - The subvolumes are special directory, the most operations of them should
>>    be similar to the common directory. Since we can remove a directory while
>>    someone is accessing it, it is better that we can destroy a subvolume
>>    while we are using it as a send parent.
> Yes they're similar, but subvolumes have additional features that need
> to be handled appropriately. One cannot send a directory.
>
> So we disagree, I see a reason for the deletion protection and will do
> the patch myself. Let's see if we can get more user feedback then.
>
> I'm NAKing this patch in current state, if it helps anything.
Both ways are ok for me actually, don't be annoyed anyway,

You and Miao are really doing a good job to Btrfs, just go ahead, i
am ok with dropping this patch.^_^

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 v2 2/4] Btrfs: fix protection between send and root deletion

[David Sterba]

On Wed, Jan 22, 2014 at 04:44:14PM +0800, Wang Shilong wrote:
> >So we disagree, I see a reason for the deletion protection and will do
> >the patch myself. Let's see if we can get more user feedback then.
> >
> >I'm NAKing this patch in current state, if it helps anything.
> Both ways are ok for me actually, don't be annoyed anyway,

Nah, this is a message to maintainers that the patch discussion has
reached some conclusion and should help deciding if the patch should be
merged or not. We've seen in the past that after a moderate discussion
against patch inclusion, the patch ended up merged as if nothing
happend. _This_ can be annoying.

> You and Miao are really doing a good job to Btrfs, just go ahead, i
> am ok with dropping this patch.^_^

Ok, thanks.

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

[David Sterba]

On Tue, Jan 07, 2014 at 05:25:18PM +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.

Of course, the sort(). I think your fix adds some complexity that's not
necessary. Whether the clone_roots array is sorted is not important, we
just have to process each root once.

send_root becomes a clone_root member, so the missing part is to account
in the rollback counter:

--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4937,6 +4937,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
         * for possible clone sources.
         */
        sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;
+       clone_sources_to_rollback++;

        /* We do a bsearch later */
        sort(sctx->clone_roots, sctx->clone_roots_cnt,
@@ -4961,7 +4962,6 @@ out:
                btrfs_root_dec_send_in_progress(sctx->clone_roots[i].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);
---

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

[Wang Shilong]

Hello David,

> On Tue, Jan 07, 2014 at 05:25:18PM +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.
> 
> Of course, the sort(). I think your fix adds some complexity that's not
> necessary. Whether the clone_roots array is sorted is not important, we
> just have to process each root once.
> 
> send_root becomes a clone_root member, so the missing part is to account
> in the rollback counter:
> 
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -4937,6 +4937,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>         * for possible clone sources.
>         */
>        sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;
> +       clone_sources_to_rollback++;

Not really, If we fail to come here, we still need decrease @send_root.
Please correct me if i miss something here.^_^

Thanks,
Wang
> 
>        /* We do a bsearch later */
>        sort(sctx->clone_roots, sctx->clone_roots_cnt,
> @@ -4961,7 +4962,6 @@ out:
>                btrfs_root_dec_send_in_progress(sctx->clone_roots[i].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);
> ---
> --
> 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 v2 1/4] Btrfs: fix wrong send_in_progress accounting

[David Sterba]

On Wed, Jan 08, 2014 at 11:09:02PM +0800, Wang Shilong wrote:
> 
> Hello David,
> 
> > On Tue, Jan 07, 2014 at 05:25:18PM +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.
> > 
> > Of course, the sort(). I think your fix adds some complexity that's not
> > necessary. Whether the clone_roots array is sorted is not important, we
> > just have to process each root once.
> > 
> > send_root becomes a clone_root member, so the missing part is to account
> > in the rollback counter:
> > 
> > --- a/fs/btrfs/send.c
> > +++ b/fs/btrfs/send.c
> > @@ -4937,6 +4937,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
> >         * for possible clone sources.
> >         */
> >        sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;
> > +       clone_sources_to_rollback++;
> 
> Not really, If we fail to come here, we still need decrease @send_root.

Right. I was thinking if the code can be simplified somehow, but don't
have anything vastly better. Can you please add a comment to the first
branch that send_root is processed in the loop and not missed? It looks
unabalanced when it's handled just a few lines below and not in the 1st
loop.

thanks,
david

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

[Wang Shilong]

On 01/14/2014 02:40 AM, David Sterba wrote:
> On Wed, Jan 08, 2014 at 11:09:02PM +0800, Wang Shilong wrote:
>> Hello David,
>>
>>> On Tue, Jan 07, 2014 at 05:25:18PM +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.
>>> Of course, the sort(). I think your fix adds some complexity that's not
>>> necessary. Whether the clone_roots array is sorted is not important, we
>>> just have to process each root once.
>>>
>>> send_root becomes a clone_root member, so the missing part is to account
>>> in the rollback counter:
>>>
>>> --- a/fs/btrfs/send.c
>>> +++ b/fs/btrfs/send.c
>>> @@ -4937,6 +4937,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
>>>          * for possible clone sources.
>>>          */
>>>         sctx->clone_roots[sctx->clone_roots_cnt++].root = sctx->send_root;
>>> +       clone_sources_to_rollback++;
>> Not really, If we fail to come here, we still need decrease @send_root.
> Right. I was thinking if the code can be simplified somehow, but don't
> have anything vastly better. Can you please add a comment to the first
> branch that send_root is processed in the loop and not missed? It looks
> unabalanced when it's handled just a few lines below and not in the 1st
> loop.
Reasonable, i will send v3 for this patch and add some comments.
David, really thanks for you costing time to review and correct me.^_^

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