Btrfs devel - Dec 2011 - [PATCH] Btrfs: protect orphan block rsv with spin_lock

We''ve been seeing warnings coming out of the orphan commit stuff
forever from
ceph.  Turns out it''s because we''re racing with checking if
the orphan block
reserve is set, because we clear it outside of the spin_lock.  So leave the
normal fastpath checks where they are, but take the spin_lock and _recheck_ to
make sure we haven''t had an orphan block rsv added in the meantime. 
Then clear
the root''s orphan block rsv and release the lock.  With this patch a
user said
the warnings went away and they usually showed up pretty soon after he started
ceph.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/btrfs/inode.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9ae9c2e..9de15f1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1951,12 +1951,28 @@ enum btrfs_orphan_cleanup_state {
 void btrfs_orphan_commit_root(struct btrfs_trans_handle *trans,
 			      struct btrfs_root *root)
 {
+	struct btrfs_block_rsv *block_rsv;
 	int ret;
 
 	if (!list_empty(&root->orphan_list) ||
 	    root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE)
 		return;
 
+	spin_lock(&root->orphan_lock);
+	if (!list_empty(&root->orphan_list)) {
+		spin_unlock(&root->orphan_lock);
+		return;
+	}
+
+	if (root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE) {
+		spin_unlock(&root->orphan_lock);
+		return;
+	}
+
+	block_rsv = root->orphan_block_rsv;
+	root->orphan_block_rsv = NULL;
+	spin_unlock(&root->orphan_lock);
+
 	if (root->orphan_item_inserted &&
 	    btrfs_root_refs(&root->root_item) > 0) {
 		ret = btrfs_del_orphan_item(trans, root->fs_info->tree_root,
@@ -1965,10 +1981,9 @@ void btrfs_orphan_commit_root(struct btrfs_trans_handle
*trans,
 		root->orphan_item_inserted = 0;
 	}
 
-	if (root->orphan_block_rsv) {
-		WARN_ON(root->orphan_block_rsv->size > 0);
-		btrfs_free_block_rsv(root, root->orphan_block_rsv);
-		root->orphan_block_rsv = NULL;
+	if (block_rsv) {
+		WARN_ON(block_rsv->size > 0);
+		btrfs_free_block_rsv(root, block_rsv);
 	}
 }
 
-- 
1.7.5.2

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

Hello Josef,

I''ve tried this patch but I''m still seeing the orphan commit
warnings.
I''ve tried your patch on 3.2-rc4. After the warnings I restarted the 
OSDs and tried to do a mkcephfs (so I had a clean fs again), but it hung 
on umounting the current btrfs fs on the first OSD (the ceph_osd service 
was started at boot). Doing a dmesg on the first OSD I saw the kernel 
BUG at fs/btrfs/extent-tree.c:2287 again.

I''ve pasted the whole dmesg ( http://pastebin.com/g4twVNj4), I
couldn''t
get a trace. There are also some errors on mounting.

Hope this information will be of some use for you. If I get more 
information in another run I''ll send them.

Stefan

On 12/02/2011 09:44 PM, Josef Bacik wrote:
> We''ve been seeing warnings coming out of the orphan commit stuff
forever from
> ceph.  Turns out it''s because we''re racing with checking
if the orphan block
> reserve is set, because we clear it outside of the spin_lock.  So leave the
> normal fastpath checks where they are, but take the spin_lock and _recheck_
to
> make sure we haven''t had an orphan block rsv added in the
meantime.  Then clear
> the root''s orphan block rsv and release the lock.  With this patch
a user said
> the warnings went away and they usually showed up pretty soon after he
started
> ceph.  Thanks,
>
> Signed-off-by: Josef Bacik<josef@redhat.com>
> ---
>   fs/btrfs/inode.c |   23 +++++++++++++++++++----
>   1 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 9ae9c2e..9de15f1 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1951,12 +1951,28 @@ enum btrfs_orphan_cleanup_state {
>   void btrfs_orphan_commit_root(struct btrfs_trans_handle *trans,
>   			      struct btrfs_root *root)
>   {
> +	struct btrfs_block_rsv *block_rsv;
>   	int ret;
>
>   	if (!list_empty(&root->orphan_list) ||
>   	    root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE)
>   		return;
>
> +	spin_lock(&root->orphan_lock);
> +	if (!list_empty(&root->orphan_list)) {
> +		spin_unlock(&root->orphan_lock);
> +		return;
> +	}
> +
> +	if (root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE) {
> +		spin_unlock(&root->orphan_lock);
> +		return;
> +	}
> +
> +	block_rsv = root->orphan_block_rsv;
> +	root->orphan_block_rsv = NULL;
> +	spin_unlock(&root->orphan_lock);
> +
>   	if (root->orphan_item_inserted&&
>   	btrfs_root_refs(&root->root_item)>  0) {
>   		ret = btrfs_del_orphan_item(trans, root->fs_info->tree_root,
> @@ -1965,10 +1981,9 @@ void btrfs_orphan_commit_root(struct
btrfs_trans_handle *trans,
>   		root->orphan_item_inserted = 0;
>   	}
>
> -	if (root->orphan_block_rsv) {
> -		WARN_ON(root->orphan_block_rsv->size>  0);
> -		btrfs_free_block_rsv(root, root->orphan_block_rsv);
> -		root->orphan_block_rsv = NULL;
> +	if (block_rsv) {
> +		WARN_ON(block_rsv->size>  0);
> +		btrfs_free_block_rsv(root, block_rsv);
>   	}
>   }
>
--
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

2011/12/2 Josef Bacik <josef@redhat.com>:
> We''ve been seeing warnings coming out of the orphan commit stuff
forever from
> ceph.  Turns out it''s because we''re racing with checking
if the orphan block
> reserve is set, because we clear it outside of the spin_lock.  So leave the
> normal fastpath checks where they are, but take the spin_lock and _recheck_
to
> make sure we haven''t had an orphan block rsv added in the
meantime.  Then clear
> the root''s orphan block rsv and release the lock.  With this patch
a user said
> the warnings went away and they usually showed up pretty soon after he
started
> ceph.  Thanks,
*sigh* - As soon as I turned my back to the serve console it also
happened again on one of our nodes. That was 25 hours after I started
the system. Usually I see these warnings a few minutes after the
start, but the have been cases in the past where it took longer. So
I''m not sure if the improvement is due to the patch.

Josef: I was still running the patch you sent me, but there was no
message from the printk''s you added.

Thanks,
Christian
--
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

On Mon, Dec 05, 2011 at 12:50:39PM +0100, Christian Brunner
wrote:
> 2011/12/2 Josef Bacik <josef@redhat.com>:
> > We''ve been seeing warnings coming out of the orphan commit
stuff forever from
> > ceph.  Turns out it''s because we''re racing with
checking if the orphan block
> > reserve is set, because we clear it outside of the spin_lock.  So
leave the
> > normal fastpath checks where they are, but take the spin_lock and
_recheck_ to
> > make sure we haven''t had an orphan block rsv added in the
meantime.  Then clear
> > the root''s orphan block rsv and release the lock.  With this
patch a user said
> > the warnings went away and they usually showed up pretty soon after he
started
> > ceph.  Thanks,
> 
> *sigh* - As soon as I turned my back to the serve console it also
> happened again on one of our nodes. That was 25 hours after I started
> the system. Usually I see these warnings a few minutes after the
> start, but the have been cases in the past where it took longer. So
> I''m not sure if the improvement is due to the patch.
> 
> Josef: I was still running the patch you sent me, but there was no
> message from the printk''s you added.
> 
:( ok I''ll try again.  Thanks,

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