[PATCH 1/2] Btrfs: skip readonly root for snapshot-aware defragment

[PATCH 1/2] Btrfs: skip readonly root for snapshot-aware defragment

[Wang Shilong]

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

Btrfs send is assuming readonly root won't change, let's skip readonly root.

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 fs/btrfs/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1af34d0..e8dfd83 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2239,6 +2239,11 @@ static noinline int relink_extent_backref(struct btrfs_path *path,
 		return PTR_ERR(root);
 	}
 
+	if (btrfs_root_readonly(root)) {
+		srcu_read_unlock(&fs_info->subvol_srcu, index);
+		return 0;
+	}
+
 	/* step 2: get inode */
 	key.objectid = backref->inum;
 	key.type = BTRFS_INODE_ITEM_KEY;
-- 
1.8.4

[RFC PATCH 2/2] Revert "Btrfs: remove transaction from btrfs send"

[Wang Shilong]

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

This reverts commit 41ce9970a8a6a362ae8df145f7a03d789e9ef9d2.
Previously i was thinking we can use readonly root's commit root
safely while it is not true, readonly root may be cowed with the
following cases.

1.snapshot send root will cow source root.
2.balance,device operations will also cow readonly send root
to relocate.

So i have two ideas to make us safe to use commit root.

-->approach 1:
make it protected by transaction and end transaction properly and we research
next item from root node(see btrfs_search_slot_for_read()).

-->approach 2:
add another counter to local root structure to sync snapshot with send.
and add a global counter to sync send with exclusive device operations.

So with approach 2, send can use commit root safely, because we make sure
send root can not be cowed during send. Unfortunately, it make codes *ugly*
and more complex to maintain.

To make snapshot and send exclusively, device operations and send operation
exclusively with each other is a little confusing for common users.

So why not drop into previous way.

Cc: Josef Bacik <jbacik@fb.com>
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
Josef, if we reach agreement to adopt this approach, please revert
Filipe's patch(Btrfs: make some tree searches in send.c more efficient)
from btrfs-next.
---
 fs/btrfs/send.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 168b9ec..e9d1265 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -5098,6 +5098,7 @@ out:
 static int full_send_tree(struct send_ctx *sctx)
 {
 	int ret;
+	struct btrfs_trans_handle *trans = NULL;
 	struct btrfs_root *send_root = sctx->send_root;
 	struct btrfs_key key;
 	struct btrfs_key found_key;
@@ -5119,6 +5120,19 @@ static int full_send_tree(struct send_ctx *sctx)
 	key.type = BTRFS_INODE_ITEM_KEY;
 	key.offset = 0;
 
+join_trans:
+	/*
+	 * We need to make sure the transaction does not get committed
+	 * while we do anything on commit roots. Join a transaction to prevent
+	 * this.
+	 */
+	trans = btrfs_join_transaction(send_root);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		trans = NULL;
+		goto out;
+	}
+
 	/*
 	 * Make sure the tree has not changed after re-joining. We detect this
 	 * by comparing start_ctransid and ctransid. They should always match.
@@ -5142,6 +5156,19 @@ static int full_send_tree(struct send_ctx *sctx)
 		goto out_finish;
 
 	while (1) {
+		/*
+		 * When someone want to commit while we iterate, end the
+		 * joined transaction and rejoin.
+		 */
+		if (btrfs_should_end_transaction(trans, send_root)) {
+			ret = btrfs_end_transaction(trans, send_root);
+			trans = NULL;
+			if (ret < 0)
+				goto out;
+			btrfs_release_path(path);
+			goto join_trans;
+		}
+
 		eb = path->nodes[0];
 		slot = path->slots[0];
 		btrfs_item_key_to_cpu(eb, &found_key, slot);
@@ -5169,6 +5196,12 @@ out_finish:
 
 out:
 	btrfs_free_path(path);
+	if (trans) {
+		if (!ret)
+			ret = btrfs_end_transaction(trans, send_root);
+		else
+			btrfs_end_transaction(trans, send_root);
+	}
 	return ret;
 }
 
-- 
1.8.4

Re: [RFC PATCH 2/2] Revert "Btrfs: remove transaction from btrfs send"

[Shilong Wang]

2014-02-08 23:46 GMT+08:00 Wang Shilong <wangshilong1991@gmail.com>:
> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>
> This reverts commit 41ce9970a8a6a362ae8df145f7a03d789e9ef9d2.
> Previously i was thinking we can use readonly root's commit root
> safely while it is not true, readonly root may be cowed with the
> following cases.
>
> 1.snapshot send root will cow source root.
> 2.balance,device operations will also cow readonly send root
> to relocate.
>
> So i have two ideas to make us safe to use commit root.
>
> -->approach 1:
> make it protected by transaction and end transaction properly and we research
> next item from root node(see btrfs_search_slot_for_read()).
>
> -->approach 2:
> add another counter to local root structure to sync snapshot with send.
> and add a global counter to sync send with exclusive device operations.
>
> So with approach 2, send can use commit root safely, because we make sure
> send root can not be cowed during send. Unfortunately, it make codes *ugly*
> and more complex to maintain.
>
> To make snapshot and send exclusively, device operations and send operation
> exclusively with each other is a little confusing for common users.
>
> So why not drop into previous way.
>
> Cc: Josef Bacik <jbacik@fb.com>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
> Josef, if we reach agreement to adopt this approach, please revert
> Filipe's patch(Btrfs: make some tree searches in send.c more efficient)
> from btrfs-next.

Oops, this patch guarantee searching commit roots are all protected by
transaction, Filipe's
patch is ok, we need update Josef's previous patch.

Wang

Re: [RFC PATCH 2/2] Revert "Btrfs: remove transaction from btrfs send"

[Filipe David Manana]

On Sun, Feb 9, 2014 at 2:39 AM, Shilong Wang <wangshilong1991@gmail.com> wrote:
> 2014-02-08 23:46 GMT+08:00 Wang Shilong <wangshilong1991@gmail.com>:
>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>
>> This reverts commit 41ce9970a8a6a362ae8df145f7a03d789e9ef9d2.
>> Previously i was thinking we can use readonly root's commit root
>> safely while it is not true, readonly root may be cowed with the
>> following cases.
>>
>> 1.snapshot send root will cow source root.
>> 2.balance,device operations will also cow readonly send root
>> to relocate.
>>
>> So i have two ideas to make us safe to use commit root.
>>
>> -->approach 1:
>> make it protected by transaction and end transaction properly and we research
>> next item from root node(see btrfs_search_slot_for_read()).
>>
>> -->approach 2:
>> add another counter to local root structure to sync snapshot with send.
>> and add a global counter to sync send with exclusive device operations.
>>
>> So with approach 2, send can use commit root safely, because we make sure
>> send root can not be cowed during send. Unfortunately, it make codes *ugly*
>> and more complex to maintain.
>>
>> To make snapshot and send exclusively, device operations and send operation
>> exclusively with each other is a little confusing for common users.
>>
>> So why not drop into previous way.
>>
>> Cc: Josef Bacik <jbacik@fb.com>
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>> Josef, if we reach agreement to adopt this approach, please revert
>> Filipe's patch(Btrfs: make some tree searches in send.c more efficient)
>> from btrfs-next.
>
> Oops, this patch guarantee searching commit roots are all protected by
> transaction, Filipe's
> patch is ok, we need update Josef's previous patch.

Hi Shilong,

I am confused. Can you explain why that optimization patch is a
problem, either with or without your patch or any other patch
currently flying around?

Either before or after the optimization, we search through the commit
root and after a key search we process a key while holding the leaf's
extent buffer. Both approaches call btrfs_next_leaf too (either
directly or via btrfs_search_slot_for_read).

Thanks

>
> Wang
> --
> 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."

Re: [RFC PATCH 2/2] Revert "Btrfs: remove transaction from btrfs send"

[Shilong Wang]

2014-02-09 21:52 GMT+08:00 Filipe David Manana <fdmanana@gmail.com>:
> On Sun, Feb 9, 2014 at 2:39 AM, Shilong Wang <wangshilong1991@gmail.com> wrote:
>> 2014-02-08 23:46 GMT+08:00 Wang Shilong <wangshilong1991@gmail.com>:
>>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>
>>> This reverts commit 41ce9970a8a6a362ae8df145f7a03d789e9ef9d2.
>>> Previously i was thinking we can use readonly root's commit root
>>> safely while it is not true, readonly root may be cowed with the
>>> following cases.
>>>
>>> 1.snapshot send root will cow source root.
>>> 2.balance,device operations will also cow readonly send root
>>> to relocate.
>>>
>>> So i have two ideas to make us safe to use commit root.
>>>
>>> -->approach 1:
>>> make it protected by transaction and end transaction properly and we research
>>> next item from root node(see btrfs_search_slot_for_read()).
>>>
>>> -->approach 2:
>>> add another counter to local root structure to sync snapshot with send.
>>> and add a global counter to sync send with exclusive device operations.
>>>
>>> So with approach 2, send can use commit root safely, because we make sure
>>> send root can not be cowed during send. Unfortunately, it make codes *ugly*
>>> and more complex to maintain.
>>>
>>> To make snapshot and send exclusively, device operations and send operation
>>> exclusively with each other is a little confusing for common users.
>>>
>>> So why not drop into previous way.
>>>
>>> Cc: Josef Bacik <jbacik@fb.com>
>>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>> ---
>>> Josef, if we reach agreement to adopt this approach, please revert
>>> Filipe's patch(Btrfs: make some tree searches in send.c more efficient)
>>> from btrfs-next.
>>
>> Oops, this patch guarantee searching commit roots are all protected by
>> transaction, Filipe's
>> patch is ok, we need update Josef's previous patch.
>
> Hi Shilong,
>
> I am confused. Can you explain why that optimization patch is a
> problem, either with or without your patch or any other patch
> currently flying around?
>
> Either before or after the optimization, we search through the commit
> root and after a key search we process a key while holding the leaf's
> extent buffer. Both approaches call btrfs_next_leaf too (either
> directly or via btrfs_search_slot_for_read).

Sorry my miss, your patch did not have problem, you did not notice
my following thread comments for this patch, we need update josef's previous
patch not yours. ^_^

Thanks,
Wang
>
> Thanks
>
>>
>> Wang
>> --
>> 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."

Re: [RFC PATCH 2/2] Revert "Btrfs: remove transaction from btrfs send"

[Josef Bacik]

On 02/08/2014 10:46 AM, Wang Shilong wrote:
> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>
> This reverts commit 41ce9970a8a6a362ae8df145f7a03d789e9ef9d2.
> Previously i was thinking we can use readonly root's commit root
> safely while it is not true, readonly root may be cowed with the
> following cases.
>
> 1.snapshot send root will cow source root.
> 2.balance,device operations will also cow readonly send root
> to relocate.
>
> So i have two ideas to make us safe to use commit root.
>
> -->approach 1:
> make it protected by transaction and end transaction properly and we research
> next item from root node(see btrfs_search_slot_for_read()).
>
> -->approach 2:
> add another counter to local root structure to sync snapshot with send.
> and add a global counter to sync send with exclusive device operations.
>
> So with approach 2, send can use commit root safely, because we make sure
> send root can not be cowed during send. Unfortunately, it make codes *ugly*
> and more complex to maintain.
>
> To make snapshot and send exclusively, device operations and send operation
> exclusively with each other is a little confusing for common users.
>
> So why not drop into previous way.
>
> Cc: Josef Bacik <jbacik@fb.com>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
> Josef, if we reach agreement to adopt this approach, please revert
> Filipe's patch(Btrfs: make some tree searches in send.c more efficient)
> from btrfs-next.

I agree, I'll leave Filipe's patch alone but I'll drop my search commit 
root patch since we don't need it any more.  Do you want me to take this 
or do you want to resubmit without the rfc?  Thanks,

Josef