[PATCH] Btrfs: convert to add transaction protection for btrfs send

[PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>

I sent a patch to kick off transaction from btrfs send, however it gets
a regression that btrfs send try to search extent commit root without
transaction protection.

To fix this regression, we have two ideas:

 1. don't use extent commit root for sending.

 2. add transaction protection to use extent commit root safely.

Both approaches need transaction actually, however, the first approach
will add extent tree lock contention, so we'd better adopt the second
approach.

Luckily, now we only need transaction protection when iterating
extent root, the protection's *range* is smaller than before.

Cc: Josef Bacik <jbacik@fb.com>
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 fs/btrfs/send.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 04c07ed..8d5e151 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4511,6 +4511,7 @@ static int process_extent(struct send_ctx *sctx,
 			  struct btrfs_key *key)
 {
 	struct clone_root *found_clone = NULL;
+	struct btrfs_trans_handle *trans;
 	int ret = 0;
 
 	if (S_ISLNK(sctx->cur_inode_mode))
@@ -4551,10 +4552,24 @@ static int process_extent(struct send_ctx *sctx,
 			}
 		}
 	}
+	/*
+	 * We need to make sure the transaction does not get committed
+	 * while we are walking backrefs on extent commit root.
+	 */
+	trans = btrfs_join_transaction(sctx->send_root);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out;
+	}
 
 	ret = find_extent_clone(sctx, path, key->objectid, key->offset,
 			sctx->cur_inode_size, &found_clone);
-	if (ret != -ENOENT && ret < 0)
+	if (ret != -ENOENT && ret < 0) {
+		btrfs_end_transaction(trans, sctx->send_root);
+		goto out;
+	}
+	ret = btrfs_end_transaction(trans, sctx->send_root);
+	if (ret)
 		goto out;
 
 	ret = send_write_or_clone(sctx, path, key, found_clone);
-- 
1.8.4

[PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>

I sent a patch to kick off transaction from btrfs send, however it gets
a regression that btrfs send try to search extent commit root without
transaction protection.

To fix this regression, we have two ideas:

 1. don't use extent commit root for sending.

 2. add transaction protection to use extent commit root safely.

Both approaches need transaction actually, however, the first approach
will add extent tree lock contention, so we'd better adopt the second
approach.

Luckily, now we only need transaction protection when iterating
extent root, the protection's *range* is smaller than before.

Cc: Josef Bacik <jbacik@fb.com>
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 fs/btrfs/send.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 04c07ed..8d5e151 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4511,6 +4511,7 @@ static int process_extent(struct send_ctx *sctx,
 			  struct btrfs_key *key)
 {
 	struct clone_root *found_clone = NULL;
+	struct btrfs_trans_handle *trans;
 	int ret = 0;
 
 	if (S_ISLNK(sctx->cur_inode_mode))
@@ -4551,10 +4552,24 @@ static int process_extent(struct send_ctx *sctx,
 			}
 		}
 	}
+	/*
+	 * We need to make sure the transaction does not get committed
+	 * while we are walking backrefs on extent commit root.
+	 */
+	trans = btrfs_join_transaction(sctx->send_root);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out;
+	}
 
 	ret = find_extent_clone(sctx, path, key->objectid, key->offset,
 			sctx->cur_inode_size, &found_clone);
-	if (ret != -ENOENT && ret < 0)
+	if (ret != -ENOENT && ret < 0) {
+		btrfs_end_transaction(trans, sctx->send_root);
+		goto out;
+	}
+	ret = btrfs_end_transaction(trans, sctx->send_root);
+	if (ret)
 		goto out;
 
 	ret = send_write_or_clone(sctx, path, key, found_clone);
-- 
1.8.4

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Josef Bacik]

On 01/29/2014 10:32 AM, Wang Shilong wrote:
> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>
> I sent a patch to kick off transaction from btrfs send, however it gets
> a regression that btrfs send try to search extent commit root without
> transaction protection.
>
> To fix this regression, we have two ideas:
>
>   1. don't use extent commit root for sending.
>
>   2. add transaction protection to use extent commit root safely.
>
> Both approaches need transaction actually, however, the first approach
> will add extent tree lock contention, so we'd better adopt the second
> approach.
>
> Luckily, now we only need transaction protection when iterating
> extent root, the protection's *range* is smaller than before.
So what is the problem exactly?  How does it show up and what are you 
doing to make it happen?  I'd really like to kill the transaction taking 
completely in the send path so I'd like to know what is going wrong so 
we can either take the extent commit semaphore and be satisfied that is 
ok or come up with a different solution.  Thanks,

Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

Hi Josef,

> 
> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> 
>> I sent a patch to kick off transaction from btrfs send, however it gets
>> a regression that btrfs send try to search extent commit root without
>> transaction protection.
>> 
>> To fix this regression, we have two ideas:
>> 
>>  1. don't use extent commit root for sending.
>> 
>>  2. add transaction protection to use extent commit root safely.
>> 
>> Both approaches need transaction actually, however, the first approach
>> will add extent tree lock contention, so we'd better adopt the second
>> approach.
>> 
>> Luckily, now we only need transaction protection when iterating
>> extent root, the protection's *range* is smaller than before.
> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,

See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
in the initial send.

It is easy to trigger problems like the following steps:

# mkfs.btrfs -f /dev/sda8
# mount /dev/sda8 /mnt
# dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
# btrfs sub snapshot -r /mnt /mnt/snap
# btrfs send /mnt/snap -f /mnt/send_file &
# btrfs sub snapshot /mnt/snap /mnt/snap_1

Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).

Thanks,
Wang
> ,
> Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Josef Bacik]

On 01/30/2014 04:42 AM, Wang Shilong wrote:
> Hi Josef,
>
>> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>
>>> I sent a patch to kick off transaction from btrfs send, however it gets
>>> a regression that btrfs send try to search extent commit root without
>>> transaction protection.
>>>
>>> To fix this regression, we have two ideas:
>>>
>>>   1. don't use extent commit root for sending.
>>>
>>>   2. add transaction protection to use extent commit root safely.
>>>
>>> Both approaches need transaction actually, however, the first approach
>>> will add extent tree lock contention, so we'd better adopt the second
>>> approach.
>>>
>>> Luckily, now we only need transaction protection when iterating
>>> extent root, the protection's *range* is smaller than before.
>> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,
> See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
> i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
> in the initial send.
>
> It is easy to trigger problems like the following steps:
>
> # mkfs.btrfs -f /dev/sda8
> # mount /dev/sda8 /mnt
> # dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
> # btrfs sub snapshot -r /mnt /mnt/snap
> # btrfs send /mnt/snap -f /mnt/send_file &
> # btrfs sub snapshot /mnt/snap /mnt/snap_1
>
> Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).
>
Ok so this is a lot of broken things, but it's not really the extent 
root, cause like I said before nothings going to change that matters for 
the snapshots bytes.

What _does_ matter is the actual commit root for the actual fs root, and 
that requires quite a bit of manoeuvring to get right.  So I'll send a 
patch in a few minutes when I'm happy with what I have to fix this.  In 
the meantime would you rig this example up into an xfstest so we can 
make sure we don't have this problem in the future? Thanks,

Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

Hello Josef,

> 
> On 01/30/2014 04:42 AM, Wang Shilong wrote:
>> Hi Josef,
>> 
>>> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>> 
>>>> I sent a patch to kick off transaction from btrfs send, however it gets
>>>> a regression that btrfs send try to search extent commit root without
>>>> transaction protection.
>>>> 
>>>> To fix this regression, we have two ideas:
>>>> 
>>>>  1. don't use extent commit root for sending.
>>>> 
>>>>  2. add transaction protection to use extent commit root safely.
>>>> 
>>>> Both approaches need transaction actually, however, the first approach
>>>> will add extent tree lock contention, so we'd better adopt the second
>>>> approach.
>>>> 
>>>> Luckily, now we only need transaction protection when iterating
>>>> extent root, the protection's *range* is smaller than before.
>>> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,
>> See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
>> i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
>> in the initial send.
>> 
>> It is easy to trigger problems like the following steps:
>> 
>> # mkfs.btrfs -f /dev/sda8
>> # mount /dev/sda8 /mnt
>> # dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
>> # btrfs sub snapshot -r /mnt /mnt/snap
>> # btrfs send /mnt/snap -f /mnt/send_file &
>> # btrfs sub snapshot /mnt/snap /mnt/snap_1
>> 
>> Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).
>> 
> Ok so this is a lot of broken things, but it's not really the extent root, cause like I said before nothings going to change that matters for the snapshots bytes.
> 
> What _does_ matter is the actual commit root for the actual fs root, and that requires quite a bit of manoeuvring to get right.  So I'll send a patch in a few minutes when I'm happy with what I have to fix this.  In the meantime would you rig this example up into an xfstest so we can make sure we don't have this problem in the future? Thanks,

I am a little confused that we don't need protect extent commit root anyway, it is really safe to search extent commit  root without any transaction protection^_^….
And i am ok to send a xfstest case for this..

Thanks,
Wang
> 
> Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Josef Bacik]

On 01/30/2014 11:20 AM, Wang Shilong wrote:
> Hello Josef,
>
>> On 01/30/2014 04:42 AM, Wang Shilong wrote:
>>> Hi Josef,
>>>
>>>> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>>>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>>>
>>>>> I sent a patch to kick off transaction from btrfs send, however it gets
>>>>> a regression that btrfs send try to search extent commit root without
>>>>> transaction protection.
>>>>>
>>>>> To fix this regression, we have two ideas:
>>>>>
>>>>>   1. don't use extent commit root for sending.
>>>>>
>>>>>   2. add transaction protection to use extent commit root safely.
>>>>>
>>>>> Both approaches need transaction actually, however, the first approach
>>>>> will add extent tree lock contention, so we'd better adopt the second
>>>>> approach.
>>>>>
>>>>> Luckily, now we only need transaction protection when iterating
>>>>> extent root, the protection's *range* is smaller than before.
>>>> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,
>>> See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
>>> i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
>>> in the initial send.
>>>
>>> It is easy to trigger problems like the following steps:
>>>
>>> # mkfs.btrfs -f /dev/sda8
>>> # mount /dev/sda8 /mnt
>>> # dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
>>> # btrfs sub snapshot -r /mnt /mnt/snap
>>> # btrfs send /mnt/snap -f /mnt/send_file &
>>> # btrfs sub snapshot /mnt/snap /mnt/snap_1
>>>
>>> Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).
>>>
>> Ok so this is a lot of broken things, but it's not really the extent root, cause like I said before nothings going to change that matters for the snapshots bytes.
>>
>> What _does_ matter is the actual commit root for the actual fs root, and that requires quite a bit of manoeuvring to get right.  So I'll send a patch in a few minutes when I'm happy with what I have to fix this.  In the meantime would you rig this example up into an xfstest so we can make sure we don't have this problem in the future? Thanks,
> I am a little confused that we don't need protect extent commit root anyway, it is really safe to search extent commit  root without any transaction protection^_^….
> And i am ok to send a xfstest case for this..
>
Sorry I didn't say that quite right.  We definitely need to protect the 
commit root for the extent root because we could easily swap it out and 
then write over blocks as we search down it, which would break things.  
But that's not what was screwing up here, we are cow'ing the root for 
/mnt/snap and swapping out the commit root out from under us which is 
screwing us up because we end up with a different root level than what 
we are expecting.

So we need to use extent_commit_sem anywhere we search the commit root 
for the extent tree, but we also need to do the same for searching the 
fs roots.  Thanks,

Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

在 2014-1-31，上午12:23，Josef Bacik <jbacik@fb.com> 写道：

> 
> On 01/30/2014 11:20 AM, Wang Shilong wrote:
>> Hello Josef,
>> 
>>> On 01/30/2014 04:42 AM, Wang Shilong wrote:
>>>> Hi Josef,
>>>> 
>>>>> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>>>>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>>>> 
>>>>>> I sent a patch to kick off transaction from btrfs send, however it gets
>>>>>> a regression that btrfs send try to search extent commit root without
>>>>>> transaction protection.
>>>>>> 
>>>>>> To fix this regression, we have two ideas:
>>>>>> 
>>>>>>  1. don't use extent commit root for sending.
>>>>>> 
>>>>>>  2. add transaction protection to use extent commit root safely.
>>>>>> 
>>>>>> Both approaches need transaction actually, however, the first approach
>>>>>> will add extent tree lock contention, so we'd better adopt the second
>>>>>> approach.
>>>>>> 
>>>>>> Luckily, now we only need transaction protection when iterating
>>>>>> extent root, the protection's *range* is smaller than before.
>>>>> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,
>>>> See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
>>>> i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
>>>> in the initial send.
>>>> 
>>>> It is easy to trigger problems like the following steps:
>>>> 
>>>> # mkfs.btrfs -f /dev/sda8
>>>> # mount /dev/sda8 /mnt
>>>> # dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
>>>> # btrfs sub snapshot -r /mnt /mnt/snap
>>>> # btrfs send /mnt/snap -f /mnt/send_file &
>>>> # btrfs sub snapshot /mnt/snap /mnt/snap_1
>>>> 
>>>> Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).
>>>> 
>>> Ok so this is a lot of broken things, but it's not really the extent root, cause like I said before nothings going to change that matters for the snapshots bytes.
>>> 
>>> What _does_ matter is the actual commit root for the actual fs root, and that requires quite a bit of manoeuvring to get right.  So I'll send a patch in a few minutes when I'm happy with what I have to fix this.  In the meantime would you rig this example up into an xfstest so we can make sure we don't have this problem in the future? Thanks,
>> I am a little confused that we don't need protect extent commit root anyway, it is really safe to search extent commit  root without any transaction protection^_^….
>> And i am ok to send a xfstest case for this..
>> 
> Sorry I didn't say that quite right.  We definitely need to protect the commit root for the extent root because we could easily swap it out and then write over blocks as we search down it, which would break things.  But that's not what was screwing up here, we are cow'ing the root for /mnt/snap and swapping out the commit root out from under us which is screwing us up because we end up with a different root level than what we are expecting.
> 
> So we need to use extent_commit_sem anywhere we search the commit root for the extent tree, but we also need to do the same for searching the fs roots.  Thanks,

Looking into codes, we are always searching commit fs root for sending, snapshot will change src root's level(it will cow everything in tree root).
Since we only support readonly root to send, we only need to protect extent root, and the search from tree root.

So here i think by holding @extent_commit_sem outside find_extent_clone() is ok, right?

Thank,
Wang

> 
> Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

Hello Josef,

> 
> 在 2014-1-31，上午12:23，Josef Bacik <jbacik@fb.com> 写道：
> 
>> 
>> On 01/30/2014 11:20 AM, Wang Shilong wrote:
>>> Hello Josef,
>>> 
>>>> On 01/30/2014 04:42 AM, Wang Shilong wrote:
>>>>> Hi Josef,
>>>>> 
>>>>>> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>>>>>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>>>>> 
>>>>>>> I sent a patch to kick off transaction from btrfs send, however it gets
>>>>>>> a regression that btrfs send try to search extent commit root without
>>>>>>> transaction protection.
>>>>>>> 
>>>>>>> To fix this regression, we have two ideas:
>>>>>>> 
>>>>>>> 1. don't use extent commit root for sending.
>>>>>>> 
>>>>>>> 2. add transaction protection to use extent commit root safely.
>>>>>>> 
>>>>>>> Both approaches need transaction actually, however, the first approach
>>>>>>> will add extent tree lock contention, so we'd better adopt the second
>>>>>>> approach.
>>>>>>> 
>>>>>>> Luckily, now we only need transaction protection when iterating
>>>>>>> extent root, the protection's *range* is smaller than before.
>>>>>> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,
>>>>> See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
>>>>> i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
>>>>> in the initial send.
>>>>> 
>>>>> It is easy to trigger problems like the following steps:
>>>>> 
>>>>> # mkfs.btrfs -f /dev/sda8
>>>>> # mount /dev/sda8 /mnt
>>>>> # dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
>>>>> # btrfs sub snapshot -r /mnt /mnt/snap
>>>>> # btrfs send /mnt/snap -f /mnt/send_file &
>>>>> # btrfs sub snapshot /mnt/snap /mnt/snap_1
>>>>> 
>>>>> Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).
>>>>> 
>>>> Ok so this is a lot of broken things, but it's not really the extent root, cause like I said before nothings going to change that matters for the snapshots bytes.
>>>> 
>>>> What _does_ matter is the actual commit root for the actual fs root, and that requires quite a bit of manoeuvring to get right.  So I'll send a patch in a few minutes when I'm happy with what I have to fix this.  In the meantime would you rig this example up into an xfstest so we can make sure we don't have this problem in the future? Thanks,
>>> I am a little confused that we don't need protect extent commit root anyway, it is really safe to search extent commit  root without any transaction protection^_^….
>>> And i am ok to send a xfstest case for this..
>>> 
>> Sorry I didn't say that quite right.  We definitely need to protect the commit root for the extent root because we could easily swap it out and then write over blocks as we search down it, which would break things.  But that's not what was screwing up here, we are cow'ing the root for /mnt/snap and swapping out the commit root out from under us which is screwing us up because we end up with a different root level than what we are expecting.
>> 
>> So we need to use extent_commit_sem anywhere we search the commit root for the extent tree, but we also need to do the same for searching the fs roots.  Thanks,

By some debugging, i found snapshots  will cow src root(this is a little strange...), we need do the same thing
for searching fs roots. Really thanks for looking into issue, and correct me,  waiting for your fix.^_^ ^_^

Thanks,
Wang
> 
> Looking into codes, we are always searching commit fs root for sending, snapshot will change src root's level(it will cow everything in tree root).
> Since we only support readonly root to send, we only need to protect extent root, and the search from tree root.
> 
> So here i think by holding @extent_commit_sem outside find_extent_clone() is ok, right?
> 
> Thank,
> Wang
> 
>> 
>> Josef
> 

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Josef Bacik]

On 01/31/2014 11:37 AM, Wang Shilong wrote:
> Hello Josef,
>
>> 在 2014-1-31，上午12:23，Josef Bacik <jbacik@fb.com> 写道：
>>
>>> On 01/30/2014 11:20 AM, Wang Shilong wrote:
>>>> Hello Josef,
>>>>
>>>>> On 01/30/2014 04:42 AM, Wang Shilong wrote:
>>>>>> Hi Josef,
>>>>>>
>>>>>>> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>>>>>>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>>>>>>
>>>>>>>> I sent a patch to kick off transaction from btrfs send, however it gets
>>>>>>>> a regression that btrfs send try to search extent commit root without
>>>>>>>> transaction protection.
>>>>>>>>
>>>>>>>> To fix this regression, we have two ideas:
>>>>>>>>
>>>>>>>> 1. don't use extent commit root for sending.
>>>>>>>>
>>>>>>>> 2. add transaction protection to use extent commit root safely.
>>>>>>>>
>>>>>>>> Both approaches need transaction actually, however, the first approach
>>>>>>>> will add extent tree lock contention, so we'd better adopt the second
>>>>>>>> approach.
>>>>>>>>
>>>>>>>> Luckily, now we only need transaction protection when iterating
>>>>>>>> extent root, the protection's *range* is smaller than before.
>>>>>>> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,
>>>>>> See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
>>>>>> i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
>>>>>> in the initial send.
>>>>>>
>>>>>> It is easy to trigger problems like the following steps:
>>>>>>
>>>>>> # mkfs.btrfs -f /dev/sda8
>>>>>> # mount /dev/sda8 /mnt
>>>>>> # dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
>>>>>> # btrfs sub snapshot -r /mnt /mnt/snap
>>>>>> # btrfs send /mnt/snap -f /mnt/send_file &
>>>>>> # btrfs sub snapshot /mnt/snap /mnt/snap_1
>>>>>>
>>>>>> Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).
>>>>>>
>>>>> Ok so this is a lot of broken things, but it's not really the extent root, cause like I said before nothings going to change that matters for the snapshots bytes.
>>>>>
>>>>> What _does_ matter is the actual commit root for the actual fs root, and that requires quite a bit of manoeuvring to get right.  So I'll send a patch in a few minutes when I'm happy with what I have to fix this.  In the meantime would you rig this example up into an xfstest so we can make sure we don't have this problem in the future? Thanks,
>>>> I am a little confused that we don't need protect extent commit root anyway, it is really safe to search extent commit  root without any transaction protection^_^….
>>>> And i am ok to send a xfstest case for this..
>>>>
>>> Sorry I didn't say that quite right.  We definitely need to protect the commit root for the extent root because we could easily swap it out and then write over blocks as we search down it, which would break things.  But that's not what was screwing up here, we are cow'ing the root for /mnt/snap and swapping out the commit root out from under us which is screwing us up because we end up with a different root level than what we are expecting.
>>>
>>> So we need to use extent_commit_sem anywhere we search the commit root for the extent tree, but we also need to do the same for searching the fs roots.  Thanks,
> By some debugging, i found snapshots  will cow src root(this is a little strange...), we need do the same thing
> for searching fs roots. Really thanks for looking into issue, and correct me,  waiting for your fix.^_^ ^_^
>

Yeah I've spent all day on this and protecting the commit roots doesn't
fix it completely, just makes it harder to hit. I'm still trying to
figure out what is going on, hopefully I'll have a patch early next
week. Thanks,

Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Josef Bacik]

On 01/31/2014 11:37 AM, Wang Shilong wrote:
> Hello Josef,
>
>> 在 2014-1-31，上午12:23，Josef Bacik <jbacik@fb.com> 写道：
>>
>>> On 01/30/2014 11:20 AM, Wang Shilong wrote:
>>>> Hello Josef,
>>>>
>>>>> On 01/30/2014 04:42 AM, Wang Shilong wrote:
>>>>>> Hi Josef,
>>>>>>
>>>>>>> On 01/29/2014 10:32 AM, Wang Shilong wrote:
>>>>>>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>>>>>>
>>>>>>>> I sent a patch to kick off transaction from btrfs send, however it gets
>>>>>>>> a regression that btrfs send try to search extent commit root without
>>>>>>>> transaction protection.
>>>>>>>>
>>>>>>>> To fix this regression, we have two ideas:
>>>>>>>>
>>>>>>>> 1. don't use extent commit root for sending.
>>>>>>>>
>>>>>>>> 2. add transaction protection to use extent commit root safely.
>>>>>>>>
>>>>>>>> Both approaches need transaction actually, however, the first approach
>>>>>>>> will add extent tree lock contention, so we'd better adopt the second
>>>>>>>> approach.
>>>>>>>>
>>>>>>>> Luckily, now we only need transaction protection when iterating
>>>>>>>> extent root, the protection's *range* is smaller than before.
>>>>>>> So what is the problem exactly?  How does it show up and what are you doing to make it happen?  I'd really like to kill the transaction taking completely in the send path so I'd like to know what is going wrong so we can either take the extent commit semaphore and be satisfied that is ok or come up with a different solution.  Thanks,
>>>>>> See in find_extent_clone(), we have to walk backrefs  while we have to search extent tree!
>>>>>> i was thinking to kick off transaction for initial  full send, however, we need to consider ref links even
>>>>>> in the initial send.
>>>>>>
>>>>>> It is easy to trigger problems like the following steps:
>>>>>>
>>>>>> # mkfs.btrfs -f /dev/sda8
>>>>>> # mount /dev/sda8 /mnt
>>>>>> # dd if=/dev/zero of=/mnt/data bs=4k count=102400 oflag=direct
>>>>>> # btrfs sub snapshot -r /mnt /mnt/snap
>>>>>> # btrfs send /mnt/snap -f /mnt/send_file &
>>>>>> # btrfs sub snapshot /mnt/snap /mnt/snap_1
>>>>>>
>>>>>> Feel free to correct me if i miss something here^_^(As i sometimes made some mistakes).
>>>>>>
>>>>> Ok so this is a lot of broken things, but it's not really the extent root, cause like I said before nothings going to change that matters for the snapshots bytes.
>>>>>
>>>>> What _does_ matter is the actual commit root for the actual fs root, and that requires quite a bit of manoeuvring to get right.  So I'll send a patch in a few minutes when I'm happy with what I have to fix this.  In the meantime would you rig this example up into an xfstest so we can make sure we don't have this problem in the future? Thanks,
>>>> I am a little confused that we don't need protect extent commit root anyway, it is really safe to search extent commit  root without any transaction protection^_^….
>>>> And i am ok to send a xfstest case for this..
>>>>
>>> Sorry I didn't say that quite right.  We definitely need to protect the commit root for the extent root because we could easily swap it out and then write over blocks as we search down it, which would break things.  But that's not what was screwing up here, we are cow'ing the root for /mnt/snap and swapping out the commit root out from under us which is screwing us up because we end up with a different root level than what we are expecting.
>>>
>>> So we need to use extent_commit_sem anywhere we search the commit root for the extent tree, but we also need to do the same for searching the fs roots.  Thanks,
> By some debugging, i found snapshots  will cow src root(this is a little strange...), we need do the same thing
> for searching fs roots. Really thanks for looking into issue, and correct me,  waiting for your fix.^_^ ^_^
>
So I've figured it out. We definitely need to protect the commit roots,
but that's not what is screwing us. Say we have commit root for snap at
block 1 and we search down the extent tree and see that it is at 1. Then
we go to do the search down to level on the root for that block, but in
the meantime we've snapshotted and switched the commit root for that
fs_tree to block 2. We go to search down and don't find our bytenr we
were looking for and we exit out without finding our original subvolume.

So there are a few things we can do here

1) Only switch the commit roots for the fs_root _after_ we switch the
extent root commit root. This works out well because we'd need to hold
the extent_commit_sem for the entirety of this operation so we'd end up
with a consistent view of everything. The drawback of this is that we
have to process the fs_roots twice, once to update the root items and
then again to swap the commit roots.

2) Remove the per-root rwsem for the commit root and just make one big
rwsem that covers all commit root switching. This way everybody who
wants to search with the commit root can just use this semaphore and all
be safe. It will mean that the inode cache stuff may block longer than
normal but I don't think that's too big of a deal.

3) Go back to using btrfs_join_transaction(). This is probably the least
likely to bite somebody in the ass, but it's taking a transaction in the
place where we really just want read-only protection without some
asshole taking a snapshot and screwing us.

I'm inclined to go with (and am coding up) #2. Yell if you have any
better suggestions or hate my suggestion. Thanks,

Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

Hi Josef,

[..SNIP..]
> 
> On 01/31/2014 11:37 AM, Wang Shilong wrote:
>> Hello Josef,
>> 
>>>  
> 
> 2) Remove the per-root rwsem for the commit root and just make one big
> rwsem that covers all commit root switching. This way everybody who
> wants to search with the commit root can just use this semaphore and all
> be safe. It will mean that the inode cache stuff may block longer than
> normal but I don't think that's too big of a deal.
> 

I am ok with this fix,  I wanted to talk something about protecting searching commit file root, this is really a
problem especially for full send.

I have some ideas about this issue:

#1.don't use commit file root to search.
This will become a nightmare when we are doing full send which will iterate the whole file tree,
at the same time, we snapshot send root, snapshots will be blocked until send finished.

#2. don't allow snapshot if we are sending root.
This may be a little confusing, snapshots are readonly, but users can not snapshot it.

#3. after one iteration, we do check send_root's generation, and make sure it doesn't
change, if it changed, then we restart send again.

I don't know which approach is better,and also snapshot-aware defragment will change
read-only snapshot?

Did you have any better ideas about this issue? Share it with me here.^_^

Thanks,
Wang
> 
> Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Josef Bacik]

On 02/05/2014 03:59 AM, Wang Shilong wrote:
> Hi Josef,
>
> [..SNIP..]
>> On 01/31/2014 11:37 AM, Wang Shilong wrote:
>>> Hello Josef,
>>>
>>>>  
>> 2) Remove the per-root rwsem for the commit root and just make one big
>> rwsem that covers all commit root switching. This way everybody who
>> wants to search with the commit root can just use this semaphore and all
>> be safe. It will mean that the inode cache stuff may block longer than
>> normal but I don't think that's too big of a deal.
>>
> I am ok with this fix,  I wanted to talk something about protecting searching commit file root, this is really a
> problem especially for full send.
>
> I have some ideas about this issue:
>
> #1.don't use commit file root to search.
> This will become a nightmare when we are doing full send which will iterate the whole file tree,
> at the same time, we snapshot send root, snapshots will be blocked until send finished.
>
> #2. don't allow snapshot if we are sending root.
> This may be a little confusing, snapshots are readonly, but users can not snapshot it.
I think this is the best bet. The fact is we don't want to hold this
commit_root_sem for the entire duration of the send, it would block
people trying to commit the transaction. We could check for contention
and drop the sem and re-search down to where we were but I think that
would be prone to errors. If we just check to see if the snapshot is
being sent and just return -EBUSY when we try to create a snapshot I
think that's perfectly reasonable.
> #3. after one iteration, we do check send_root's generation, and make sure it doesn't
> change, if it changed, then we restart send again.
>
> I don't know which approach is better,and also snapshot-aware defragment will change
> read-only snapshot?
>
> Did you have any better ideas about this issue? Share it with me here.^_^
>
Snapshot-aware defrag will definitely screw us here. I think we need to
do the same thing above as we do here, which is to simply skip the
snapshot aware defrag if we are currently using that root for send. This
sound reasonable to you? Thanks,

Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

Hi Josef,

> 
> On 02/05/2014 03:59 AM, Wang Shilong wrote:
>> Hi Josef,
>> 
>> [..SNIP..]
>>> On 01/31/2014 11:37 AM, Wang Shilong wrote:
>>>> Hello Josef,
>>>> 
>>>>> 
>>> 2) Remove the per-root rwsem for the commit root and just make one big
>>> rwsem that covers all commit root switching. This way everybody who
>>> wants to search with the commit root can just use this semaphore and all
>>> be safe. It will mean that the inode cache stuff may block longer than
>>> normal but I don't think that's too big of a deal.
>>> 
>> I am ok with this fix,  I wanted to talk something about protecting searching commit file root, this is really a
>> problem especially for full send.
>> 
>> I have some ideas about this issue:
>> 
>> #1.don't use commit file root to search.
>> This will become a nightmare when we are doing full send which will iterate the whole file tree,
>> at the same time, we snapshot send root, snapshots will be blocked until send finished.
>> 
>> #2. don't allow snapshot if we are sending root.
>> This may be a little confusing, snapshots are readonly, but users can not snapshot it.
> I think this is the best bet. The fact is we don't want to hold this
> commit_root_sem for the entire duration of the send, it would block
> people trying to commit the transaction. We could check for contention
> and drop the sem and re-search down to where we were but I think that
> would be prone to errors. If we just check to see if the snapshot is
> being sent and just return -EBUSY when we try to create a snapshot I
> think that's perfectly reasonable.
>> #3. after one iteration, we do check send_root's generation, and make sure it doesn't
>> change, if it changed, then we restart send again.
>> 
>> I don't know which approach is better,and also snapshot-aware defragment will change
>> read-only snapshot?
>> 
>> Did you have any better ideas about this issue? Share it with me here.^_^
>> 
> Snapshot-aware defrag will definitely screw us here. I think we need to
> do the same thing above as we do here, which is to simply skip the
> snapshot aware defrag if we are currently using that root for send. This
> sound reasonable to you? Thanks,

Yeah, very reasonable, if you don't mind, i would give a patch for this issue.

Thanks,
Wang
> 
> Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Josef Bacik]

On 02/05/2014 12:23 PM, Wang Shilong wrote:
> Hi Josef,
>
>> On 02/05/2014 03:59 AM, Wang Shilong wrote:
>>> Hi Josef,
>>>
>>> [..SNIP..]
>>>> On 01/31/2014 11:37 AM, Wang Shilong wrote:
>>>>> Hello Josef,
>>>>>
>>>> 2) Remove the per-root rwsem for the commit root and just make one big
>>>> rwsem that covers all commit root switching. This way everybody who
>>>> wants to search with the commit root can just use this semaphore and all
>>>> be safe. It will mean that the inode cache stuff may block longer than
>>>> normal but I don't think that's too big of a deal.
>>>>
>>> I am ok with this fix,  I wanted to talk something about protecting searching commit file root, this is really a
>>> problem especially for full send.
>>>
>>> I have some ideas about this issue:
>>>
>>> #1.don't use commit file root to search.
>>> This will become a nightmare when we are doing full send which will iterate the whole file tree,
>>> at the same time, we snapshot send root, snapshots will be blocked until send finished.
>>>
>>> #2. don't allow snapshot if we are sending root.
>>> This may be a little confusing, snapshots are readonly, but users can not snapshot it.
>> I think this is the best bet. The fact is we don't want to hold this
>> commit_root_sem for the entire duration of the send, it would block
>> people trying to commit the transaction. We could check for contention
>> and drop the sem and re-search down to where we were but I think that
>> would be prone to errors. If we just check to see if the snapshot is
>> being sent and just return -EBUSY when we try to create a snapshot I
>> think that's perfectly reasonable.
>>> #3. after one iteration, we do check send_root's generation, and make sure it doesn't
>>> change, if it changed, then we restart send again.
>>>
>>> I don't know which approach is better,and also snapshot-aware defragment will change
>>> read-only snapshot?
>>>
>>> Did you have any better ideas about this issue? Share it with me here.^_^
>>>
>> Snapshot-aware defrag will definitely screw us here. I think we need to
>> do the same thing above as we do here, which is to simply skip the
>> snapshot aware defrag if we are currently using that root for send. This
>> sound reasonable to you? Thanks,
> Yeah, very reasonable, if you don't mind, i would give a patch for this issue.
Go for it, you'll be faster than I will be, all I do is run xfstests and
try to reproduce things that will never reproduce for me.

Josef

Re: [PATCH] Btrfs: convert to add transaction protection for btrfs send

[Wang Shilong]

Hi Josef,

[..SNIP..]

>> Yeah, very reasonable, if you don't mind, i would give a patch for this issue.
> Go for it, you'll be faster than I will be, all I do is run xfstests and
> try to reproduce things that will never reproduce for me.

So Finally, i've figured everything out.

Previous send is almostly safe is because we deal every item by transaction protection, 
and we will search next item from root again next time (see btrfs_search_slot_for_read())
And we will check root's @ctransid to make sure root did not change during send. (For readonly root,
usually it should not except snapshot-aware defrag, ok we should fix it!!)

Since we kicked off transaction from send, life will become a little complex,we should take care of
everything can change readonly snapshot, from i can say now:

1. snapshot @send_root will cow everything
2. snapshot-aware defrag also work for readonly root.
3. balance, device resize, add,remove, scrub(repair mode)

So there are two ideas in my mind:

#Approach 1
convert to previous ways that means add transaction protection for send. and Filipe's optimizations
for send search should be reverted, this way is simple and make codes less complex, but we will involve
transaction protection.

#Approach 2.
Add a local flag to root structure to sync snapshot with send.
Add a global flag to sync balance's etc options with send
don't defray readonly snapshot for snapshot-aware defrag.

Approach 2 will make it safe to avoid transaction from send, but it add complexity to codes.Personally,
i do not like approach 2 very much because it make btrfs codes more *ugly*  and make it *harder* to maintain.

Anyway, I am ok to implement  both ways, Tell me if you have any better ideas or which approach is your
appreciated way to fix the issue^_^

Thanks,
Wang
> 
> Josef