thr3ads.net - Btrfs devel - [PATCH 1/4] Btrfs: wake up transaction waiters when aborting a transaction [May 2012]

If this information is useful, please help other people find it:
Share via:

Josef Bacik

2012-May-31 20:04 UTC

[PATCH 1/4] Btrfs: wake up transaction waiters when aborting a transaction

I was getting lots of hung tasks and a NULL pointer dereference because we
are not cleaning up the transaction properly when it aborts.  First we need
to reset the running_transaction to NULL so we don''t get a bad
dereference
for any start_transaction callers after this.  Also we cannot rely on
waitqueue_active() since it''s just a list_empty(), so just call
wake_up()
directly since that will do the barrier for us and such.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/btrfs/disk-io.c     |    9 +++------
 fs/btrfs/transaction.c |    4 ++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0224c25..050db9b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3584,16 +3584,13 @@ void btrfs_cleanup_one_transaction(struct
btrfs_transaction *cur_trans,
 	/* FIXME: cleanup wait for commit */
 	cur_trans->in_commit = 1;
 	cur_trans->blocked = 1;
-	if (waitqueue_active(&root->fs_info->transaction_blocked_wait))
-		wake_up(&root->fs_info->transaction_blocked_wait);
+	wake_up(&root->fs_info->transaction_blocked_wait);
 
 	cur_trans->blocked = 0;
-	if (waitqueue_active(&root->fs_info->transaction_wait))
-		wake_up(&root->fs_info->transaction_wait);
+	wake_up(&root->fs_info->transaction_wait);
 
 	cur_trans->commit_done = 1;
-	if (waitqueue_active(&cur_trans->commit_wait))
-		wake_up(&cur_trans->commit_wait);
+	wake_up(&cur_trans->commit_wait);
 
 	btrfs_destroy_pending_snapshots(cur_trans);
 
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 7aed0e8..4e6f63e 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1205,6 +1205,10 @@ static void cleanup_transaction(struct btrfs_trans_handle
*trans,
 
 	spin_lock(&root->fs_info->trans_lock);
 	list_del_init(&cur_trans->list);
+	if (cur_trans == root->fs_info->running_transaction) {
+		root->fs_info->running_transaction = NULL;
+		root->fs_info->trans_no_join = 0;
+	}
 	spin_unlock(&root->fs_info->trans_lock);
 
 	btrfs_cleanup_one_transaction(trans->transaction, root);
-- 
1.7.7.6

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

Josef Bacik

2012-May-31 20:04 UTC

head link

[PATCH 2/4] Btrfs: track transaction aborted across the fs_info

If we abort a transaction during the commit phase we can have people who
start new transactions because we nave no way of signaling to them that
something went wrong.  So add a trans_aborted flag to the fs_info so the
start transaction code can be notified that the last transaction was aborted
and it needs to return an error.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/btrfs/ctree.h       |    1 +
 fs/btrfs/super.c       |    1 +
 fs/btrfs/transaction.c |   14 ++++++++++++++
 3 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 5f2c6d1..34549ca 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1213,6 +1213,7 @@ struct btrfs_fs_info {
 	int log_root_recovering;
 	int enospc_unlink;
 	int trans_no_join;
+	int trans_aborted;
 
 	u64 total_pinned;
 
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 85cef50..e0abe7f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -226,6 +226,7 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle
*trans,
 		return;
 	}
 	trans->transaction->aborted = errno;
+	root->fs_info->trans_aborted = errno;
 	__btrfs_std_error(root->fs_info, function, line, errno, NULL);
 }
 /*
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 4e6f63e..e292b83 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -77,13 +77,21 @@ loop:
 		if (cur_trans->aborted) {
 			spin_unlock(&root->fs_info->trans_lock);
 			return cur_trans->aborted;
+		} else if (root->fs_info->trans_aborted) {
+			WARN_ON(1);
+			spin_unlock(&root->fs_info->trans_lock);
+			return root->fs_info->trans_aborted;
 		}
 		atomic_inc(&cur_trans->use_count);
 		atomic_inc(&cur_trans->num_writers);
 		cur_trans->num_joined++;
 		spin_unlock(&root->fs_info->trans_lock);
 		return 0;
+	} else if (root->fs_info->trans_aborted) {
+		spin_unlock(&root->fs_info->trans_lock);
+		return root->fs_info->trans_aborted;
 	}
+
 	spin_unlock(&root->fs_info->trans_lock);
 
 	cur_trans = kmem_cache_alloc(btrfs_transaction_cachep, GFP_NOFS);
@@ -99,6 +107,10 @@ loop:
 		kmem_cache_free(btrfs_transaction_cachep, cur_trans);
 		cur_trans = root->fs_info->running_transaction;
 		goto loop;
+	} else if (root->fs_info->trans_aborted) {
+		spin_unlock(&root->fs_info->trans_lock);
+		kmem_cache_free(btrfs_transaction_cachep, cur_trans);
+		return root->fs_info->trans_aborted;
 	}
 
 	atomic_set(&cur_trans->num_writers, 1);
@@ -1209,6 +1221,8 @@ static void cleanup_transaction(struct btrfs_trans_handle
*trans,
 		root->fs_info->running_transaction = NULL;
 		root->fs_info->trans_no_join = 0;
 	}
+	if (!root->fs_info->trans_aborted)
+		root->fs_info->trans_aborted = -EROFS;
 	spin_unlock(&root->fs_info->trans_lock);
 
 	btrfs_cleanup_one_transaction(trans->transaction, root);
-- 
1.7.7.6

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

Josef Bacik

2012-May-31 20:04 UTC

head link

[PATCH 3/4] Btrfs: do not reset the eb ref count to 1 when aborting the transaction

This screws lots of things up and is just plain unnecessary, everything will
get cleaned up properly on it''s own.  Most notably this was causing
root
eb''s to be evicted forcefully which meant that any readers still doing
anything would block trying to grab the root eb since it had a ref count of
0.  Thanks,

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 050db9b..b147a86 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3519,11 +3519,9 @@ static int btrfs_destroy_marked_extents(struct btrfs_root
*root,
 			     &(&BTRFS_I(page->mapping->host)->io_tree)->buffer,
 					       offset >> PAGE_CACHE_SHIFT);
 			spin_unlock(&dirty_pages->buffer_lock);
-			if (eb) {
+			if (eb)
 				ret = test_and_clear_bit(EXTENT_BUFFER_DIRTY,
 							 &eb->bflags);
-				atomic_set(&eb->refs, 1);
-			}
 			if (PageWriteback(page))
 				end_page_writeback(page);
 
-- 
1.7.7.6

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

Josef Bacik

2012-May-31 20:04 UTC

head link

[PATCH 4/4] Btrfs: unlock everything properly in the error case for nocow

I was getting hung on umount when a transaction was aborted because a range
of one of the free space inodes was still locked.  This is because the nocow
stuff doesn''t unlock anything on error.  This fixed the problem and I
verified that is what was happening.  Thanks,

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e91f985..96b841d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1136,8 +1136,18 @@ static noinline int run_delalloc_nocow(struct inode
*inode,
 	u64 ino = btrfs_ino(inode);
 
 	path = btrfs_alloc_path();
-	if (!path)
+	if (!path) {
+		extent_clear_unlock_delalloc(inode,
+			     &BTRFS_I(inode)->io_tree,
+			     start, end, locked_page,
+			     EXTENT_CLEAR_UNLOCK_PAGE |
+			     EXTENT_CLEAR_UNLOCK |
+			     EXTENT_CLEAR_DELALLOC |
+			     EXTENT_CLEAR_DIRTY |
+			     EXTENT_SET_WRITEBACK |
+			     EXTENT_END_WRITEBACK);
 		return -ENOMEM;
+	}
 
 	nolock = btrfs_is_free_space_inode(root, inode);
 
@@ -1147,6 +1157,15 @@ static noinline int run_delalloc_nocow(struct inode
*inode,
 		trans = btrfs_join_transaction(root);
 
 	if (IS_ERR(trans)) {
+		extent_clear_unlock_delalloc(inode,
+			     &BTRFS_I(inode)->io_tree,
+			     start, end, locked_page,
+			     EXTENT_CLEAR_UNLOCK_PAGE |
+			     EXTENT_CLEAR_UNLOCK |
+			     EXTENT_CLEAR_DELALLOC |
+			     EXTENT_CLEAR_DIRTY |
+			     EXTENT_SET_WRITEBACK |
+			     EXTENT_END_WRITEBACK);
 		btrfs_free_path(path);
 		return PTR_ERR(trans);
 	}
@@ -1327,8 +1346,11 @@ out_check:
 	}
 	btrfs_release_path(path);
 
-	if (cur_offset <= end && cow_start == (u64)-1)
+	if (cur_offset <= end && cow_start == (u64)-1) {
 		cow_start = cur_offset;
+		cur_offset = end;
+	}
+
 	if (cow_start != (u64)-1) {
 		ret = cow_file_range(inode, locked_page, cow_start, end,
 				     page_started, nr_written, 1);
@@ -1347,6 +1369,17 @@ error:
 	if (!ret)
 		ret = err;
 
+	if (ret && cur_offset < end)
+		extent_clear_unlock_delalloc(inode,
+			     &BTRFS_I(inode)->io_tree,
+			     cur_offset, end, locked_page,
+			     EXTENT_CLEAR_UNLOCK_PAGE |
+			     EXTENT_CLEAR_UNLOCK |
+			     EXTENT_CLEAR_DELALLOC |
+			     EXTENT_CLEAR_DIRTY |
+			     EXTENT_SET_WRITEBACK |
+			     EXTENT_END_WRITEBACK);
+
 	btrfs_free_path(path);
 	return ret;
 }
-- 
1.7.7.6

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

Liu Bo

2012-Jun-01 01:22 UTC

head link

Re: [PATCH 2/4] Btrfs: track transaction aborted across the fs_info

On 06/01/2012 04:04 AM, Josef Bacik wrote:
> If we abort a transaction during the commit phase we can have people who
> start new transactions because we nave no way of signaling to them that
> something went wrong.  So add a trans_aborted flag to the fs_info so the
> start transaction code can be notified that the last transaction was
aborted
> and it needs to return an error.  Thanks,
> 

We already have a (root->fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR),
why don''t just use that?

thanks,
liubo
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
>  fs/btrfs/ctree.h       |    1 +
>  fs/btrfs/super.c       |    1 +
>  fs/btrfs/transaction.c |   14 ++++++++++++++
>  3 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 5f2c6d1..34549ca 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1213,6 +1213,7 @@ struct btrfs_fs_info {
>  	int log_root_recovering;
>  	int enospc_unlink;
>  	int trans_no_join;
> +	int trans_aborted;
>  
>  	u64 total_pinned;
>  
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 85cef50..e0abe7f 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -226,6 +226,7 @@ void __btrfs_abort_transaction(struct
btrfs_trans_handle *trans,
>  		return;
>  	}
>  	trans->transaction->aborted = errno;
> +	root->fs_info->trans_aborted = errno;
>  	__btrfs_std_error(root->fs_info, function, line, errno, NULL);
>  }
>  /*
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 4e6f63e..e292b83 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -77,13 +77,21 @@ loop:
>  		if (cur_trans->aborted) {
>  			spin_unlock(&root->fs_info->trans_lock);
>  			return cur_trans->aborted;
> +		} else if (root->fs_info->trans_aborted) {
> +			WARN_ON(1);
> +			spin_unlock(&root->fs_info->trans_lock);
> +			return root->fs_info->trans_aborted;
>  		}
>  		atomic_inc(&cur_trans->use_count);
>  		atomic_inc(&cur_trans->num_writers);
>  		cur_trans->num_joined++;
>  		spin_unlock(&root->fs_info->trans_lock);
>  		return 0;
> +	} else if (root->fs_info->trans_aborted) {
> +		spin_unlock(&root->fs_info->trans_lock);
> +		return root->fs_info->trans_aborted;
>  	}
> +
>  	spin_unlock(&root->fs_info->trans_lock);
>  
>  	cur_trans = kmem_cache_alloc(btrfs_transaction_cachep, GFP_NOFS);
> @@ -99,6 +107,10 @@ loop:
>  		kmem_cache_free(btrfs_transaction_cachep, cur_trans);
>  		cur_trans = root->fs_info->running_transaction;
>  		goto loop;
> +	} else if (root->fs_info->trans_aborted) {
> +		spin_unlock(&root->fs_info->trans_lock);
> +		kmem_cache_free(btrfs_transaction_cachep, cur_trans);
> +		return root->fs_info->trans_aborted;
>  	}
>  
>  	atomic_set(&cur_trans->num_writers, 1);
> @@ -1209,6 +1221,8 @@ static void cleanup_transaction(struct
btrfs_trans_handle *trans,
>  		root->fs_info->running_transaction = NULL;
>  		root->fs_info->trans_no_join = 0;
>  	}
> +	if (!root->fs_info->trans_aborted)
> +		root->fs_info->trans_aborted = -EROFS;
>  	spin_unlock(&root->fs_info->trans_lock);
>  
>  	btrfs_cleanup_one_transaction(trans->transaction, root);

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

Josef Bacik

2012-Jun-01 12:46 UTC

head link

Re: [PATCH 2/4] Btrfs: track transaction aborted across the fs_info

On Fri, Jun 01, 2012 at 09:22:53AM +0800, Liu Bo wrote:> On 06/01/2012 04:04 AM, Josef Bacik wrote:
> 
> > If we abort a transaction during the commit phase we can have people
who
> > start new transactions because we nave no way of signaling to them
that
> > something went wrong.  So add a trans_aborted flag to the fs_info so
the
> > start transaction code can be notified that the last transaction was
aborted
> > and it needs to return an error.  Thanks,
> > 
> 
> 
> We already have a (root->fs_info->fs_state &
BTRFS_SUPER_FLAG_ERROR),
> why don''t just use that?
> 
Ah I missed all of that, will do, 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

Btrfs devel - May 2012 - [PATCH 1/4] Btrfs: wake up transaction waiters when aborting a transaction

[PATCH 1/4] Btrfs: wake up transaction waiters when aborting a transaction

[PATCH 2/4] Btrfs: track transaction aborted across the fs_info

[PATCH 3/4] Btrfs: do not reset the eb ref count to 1 when aborting the transaction

[PATCH 4/4] Btrfs: unlock everything properly in the error case for nocow

Re: [PATCH 2/4] Btrfs: track transaction aborted across the fs_info

Re: [PATCH 2/4] Btrfs: track transaction aborted across the fs_info