* [PATCH] Btrfs: check to see if root_list is empty before adding it to dead roots
@ 2013-07-25 19:13 Josef Bacik
2013-07-25 21:31 ` Zach Brown
2013-07-26 9:38 ` Stefan Behrens
0 siblings, 2 replies; 4+ messages in thread
From: Josef Bacik @ 2013-07-25 19:13 UTC (permalink / raw)
To: linux-btrfs
A user reported a panic when running with autodefrag and deleting snapshots.
This is because we could end up trying to add the root to the dead roots list
twice. To fix this check to see if we are empty before adding ourselves to the
dead roots list. Thanks,
Signed-off-by: Josef Bacik <jbacik@fusionio.com>
---
fs/btrfs/transaction.c | 8 ++++----
fs/btrfs/transaction.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 61a5c2c..18f7e71 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -983,12 +983,12 @@ static noinline int commit_cowonly_roots(struct btrfs_trans_handle *trans,
* a dirty root struct and adds it into the list of dead roots that need to
* be deleted
*/
-int btrfs_add_dead_root(struct btrfs_root *root)
+void btrfs_add_dead_root(struct btrfs_root *root)
{
spin_lock(&root->fs_info->trans_lock);
- list_add_tail(&root->root_list, &root->fs_info->dead_roots);
+ if (list_empty(&root->root_list))
+ list_add_tail(&root->root_list, &root->fs_info->dead_roots);
spin_unlock(&root->fs_info->trans_lock);
- return 0;
}
/*
@@ -1925,7 +1925,7 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
}
root = list_first_entry(&fs_info->dead_roots,
struct btrfs_root, root_list);
- list_del(&root->root_list);
+ list_del_init(&root->root_list);
spin_unlock(&fs_info->trans_lock);
pr_debug("btrfs: cleaner removing %llu\n",
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 005b037..defbc42 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -143,7 +143,7 @@ int btrfs_wait_for_commit(struct btrfs_root *root, u64 transid);
int btrfs_write_and_wait_transaction(struct btrfs_trans_handle *trans,
struct btrfs_root *root);
-int btrfs_add_dead_root(struct btrfs_root *root);
+void btrfs_add_dead_root(struct btrfs_root *root);
int btrfs_defrag_root(struct btrfs_root *root);
int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root);
int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
--
1.7.7.6
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: check to see if root_list is empty before adding it to dead roots
2013-07-25 19:13 [PATCH] Btrfs: check to see if root_list is empty before adding it to dead roots Josef Bacik
@ 2013-07-25 21:31 ` Zach Brown
2013-07-26 9:38 ` Stefan Behrens
1 sibling, 0 replies; 4+ messages in thread
From: Zach Brown @ 2013-07-25 21:31 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs
> -int btrfs_add_dead_root(struct btrfs_root *root)
> +void btrfs_add_dead_root(struct btrfs_root *root)
> {
> spin_lock(&root->fs_info->trans_lock);
> - list_add_tail(&root->root_list, &root->fs_info->dead_roots);
> + if (list_empty(&root->root_list))
> + list_add_tail(&root->root_list, &root->fs_info->dead_roots);
> spin_unlock(&root->fs_info->trans_lock);
> - return 0;
> }
>
> /*
> @@ -1925,7 +1925,7 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
> }
> root = list_first_entry(&fs_info->dead_roots,
> struct btrfs_root, root_list);
> - list_del(&root->root_list);
> + list_del_init(&root->root_list);
Looking at other root_list deletions.. do we also need _del_init() in
btrfs_recover_relocation() or del_fs_roots()? (Maybe for consistency
and peace of mind, if nothing else?)
- z
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: check to see if root_list is empty before adding it to dead roots
2013-07-25 19:13 [PATCH] Btrfs: check to see if root_list is empty before adding it to dead roots Josef Bacik
2013-07-25 21:31 ` Zach Brown
@ 2013-07-26 9:38 ` Stefan Behrens
2013-07-26 13:09 ` Josef Bacik
1 sibling, 1 reply; 4+ messages in thread
From: Stefan Behrens @ 2013-07-26 9:38 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs
On Thu, 25 Jul 2013 15:13:40 -0400, Josef Bacik wrote:
> A user reported a panic when running with autodefrag and deleting snapshots.
> This is because we could end up trying to add the root to the dead roots list
> twice. To fix this check to see if we are empty before adding ourselves to the
> dead roots list. Thanks,
>
> Signed-off-by: Josef Bacik <jbacik@fusionio.com>
Tested-by: Stefan Behrens <sbehrens@giantdisaster.de>
The patch eliminates the crash. The question still is whether the double addition to the list is an indication for a problem and this patch just hides that there is a problem.
> ---
> fs/btrfs/transaction.c | 8 ++++----
> fs/btrfs/transaction.h | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 61a5c2c..18f7e71 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -983,12 +983,12 @@ static noinline int commit_cowonly_roots(struct btrfs_trans_handle *trans,
> * a dirty root struct and adds it into the list of dead roots that need to
> * be deleted
> */
> -int btrfs_add_dead_root(struct btrfs_root *root)
> +void btrfs_add_dead_root(struct btrfs_root *root)
> {
> spin_lock(&root->fs_info->trans_lock);
> - list_add_tail(&root->root_list, &root->fs_info->dead_roots);
> + if (list_empty(&root->root_list))
> + list_add_tail(&root->root_list, &root->fs_info->dead_roots);
> spin_unlock(&root->fs_info->trans_lock);
> - return 0;
> }
>
> /*
> @@ -1925,7 +1925,7 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
> }
> root = list_first_entry(&fs_info->dead_roots,
> struct btrfs_root, root_list);
> - list_del(&root->root_list);
> + list_del_init(&root->root_list);
> spin_unlock(&fs_info->trans_lock);
>
> pr_debug("btrfs: cleaner removing %llu\n",
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index 005b037..defbc42 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -143,7 +143,7 @@ int btrfs_wait_for_commit(struct btrfs_root *root, u64 transid);
> int btrfs_write_and_wait_transaction(struct btrfs_trans_handle *trans,
> struct btrfs_root *root);
>
> -int btrfs_add_dead_root(struct btrfs_root *root);
> +void btrfs_add_dead_root(struct btrfs_root *root);
> int btrfs_defrag_root(struct btrfs_root *root);
> int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root);
> int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: check to see if root_list is empty before adding it to dead roots
2013-07-26 9:38 ` Stefan Behrens
@ 2013-07-26 13:09 ` Josef Bacik
0 siblings, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2013-07-26 13:09 UTC (permalink / raw)
To: Stefan Behrens; +Cc: Josef Bacik, linux-btrfs
On Fri, Jul 26, 2013 at 11:38:47AM +0200, Stefan Behrens wrote:
> On Thu, 25 Jul 2013 15:13:40 -0400, Josef Bacik wrote:
> > A user reported a panic when running with autodefrag and deleting snapshots.
> > This is because we could end up trying to add the root to the dead roots list
> > twice. To fix this check to see if we are empty before adding ourselves to the
> > dead roots list. Thanks,
> >
> > Signed-off-by: Josef Bacik <jbacik@fusionio.com>
>
> Tested-by: Stefan Behrens <sbehrens@giantdisaster.de>
>
> The patch eliminates the crash. The question still is whether the double addition to the list is an indication for a problem and this patch just hides that there is a problem.
This is what is happening
start writing file
notice it was a random write, add it to the defrag list
Delete subvol
evict all dentry/icache
roots inode tree is empty, add to dead root
process defrag inodes
lookup inode based on its location and root
do our defrag
iput in finish_ordered_io, which removes us from the inode tree
notice the inode tree is empty, add root to dead root
EXPLOSION
So we could probably avoid this issue by checking the root's count before we go
to lookup the inode and just bailing if the used count is 0, but then if we race
between looking up the root and looking up the inode we could end up in the same
situation, because we'd have to check again after looking up the inode and iput
there if the count was 0 and we'd be in the same mess again, though with a much
smaller window. So this patch fixes the actual problem, it's not a paper over.
I will fix us to not do anything if the root has been deleted since theres no
sense in doing the extra work, but we'll still need this for the small window
between looking up the root and looking up the inode. Thanks,
Josef
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-07-26 13:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-25 19:13 [PATCH] Btrfs: check to see if root_list is empty before adding it to dead roots Josef Bacik
2013-07-25 21:31 ` Zach Brown
2013-07-26 9:38 ` Stefan Behrens
2013-07-26 13:09 ` Josef Bacik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).