* [PATCH] Btrfs: check if previous transaction aborted to avoid fs corruption
@ 2015-08-12 14:11 fdmanana
2015-08-12 14:37 ` Josef Bacik
2015-08-12 14:44 ` [PATCH v2] " fdmanana
0 siblings, 2 replies; 4+ messages in thread
From: fdmanana @ 2015-08-12 14:11 UTC (permalink / raw)
To: linux-btrfs; +Cc: Filipe Manana
From: Filipe Manana <fdmanana@suse.com>
While we are committing a transaction, it's possible the previous one is
still finishing its commit and therefore we wait for it to finish first.
However we were not checking if that previous transaction ended up getting
aborted after we waited for it to commit, so we ended up committing the
current transaction which can lead to fs corruption because the new
superblock can point to trees that have had one or more nodes/leafs that
were never durably persisted.
The following sequence diagram exemplifies how this is possible:
CPU 0 CPU 1
transaction N starts
(...)
btrfs_commit_transaction(N)
cur_trans->state = TRANS_STATE_COMMIT_START;
(...)
cur_trans->state = TRANS_STATE_COMMIT_DOING;
(...)
cur_trans->state = TRANS_STATE_UNBLOCKED;
root->fs_info->running_transaction = NULL;
btrfs_start_transaction()
--> starts transaction N + 1
btrfs_write_and_wait_transaction(trans, root);
--> starts writing all new or COWed ebs created
at transaction N
creates some new ebs, COWs some
existing ebs but doesn't COW or
deletes eb X
btrfs_commit_transaction(N + 1)
(...)
cur_trans->state = TRANS_STATE_COMMIT_START;
(...)
wait_for_commit(root, prev_trans);
--> prev_trans == transaction N
btrfs_write_and_wait_transaction() continues
writing ebs
--> fails writing eb X, we abort transaction N
and set bit BTRFS_FS_STATE_ERROR on
fs_info->fs_state, so no new transactions
can start after setting that bit
cleanup_transaction()
btrfs_cleanup_one_transaction()
wakes up task at CPU 1
continues, doesn't abort because
cur_trans->aborted (transaction N + 1)
is zero, and no checks for bit
BTRFS_FS_STATE_ERROR in fs_info->fs_state
are made
btrfs_write_and_wait_transaction(trans, root);
--> succeeds, no errors during writeback
write_ctree_super(trans, root, 0);
--> succeeds
--> we have now a superblock that points us
to some root that uses eb X, which was
never written to disk
In this scenario future attempts to read eb X from disk results in an
error message like "parent transid verify failed on X wanted Y found Z".
So fix this by aborting the current transaction if after waiting for the
previous transaction we verify that it was aborted.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/transaction.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 51e0f0d..d15a43f 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1893,8 +1893,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
spin_unlock(&root->fs_info->trans_lock);
wait_for_commit(root, prev_trans);
+ ret = prev_trans->aborted;
btrfs_put_transaction(prev_trans);
+ if (ret)
+ goto cleanup_transaction;
} else {
spin_unlock(&root->fs_info->trans_lock);
}
--
2.1.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs: check if previous transaction aborted to avoid fs corruption
2015-08-12 14:11 [PATCH] Btrfs: check if previous transaction aborted to avoid fs corruption fdmanana
@ 2015-08-12 14:37 ` Josef Bacik
2015-08-12 14:44 ` [PATCH v2] " fdmanana
1 sibling, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2015-08-12 14:37 UTC (permalink / raw)
To: fdmanana, linux-btrfs; +Cc: Filipe Manana
On 08/12/2015 10:11 AM, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> While we are committing a transaction, it's possible the previous one is
> still finishing its commit and therefore we wait for it to finish first.
> However we were not checking if that previous transaction ended up getting
> aborted after we waited for it to commit, so we ended up committing the
> current transaction which can lead to fs corruption because the new
> superblock can point to trees that have had one or more nodes/leafs that
> were never durably persisted.
> The following sequence diagram exemplifies how this is possible:
>
> CPU 0 CPU 1
>
> transaction N starts
>
> (...)
>
> btrfs_commit_transaction(N)
>
> cur_trans->state = TRANS_STATE_COMMIT_START;
> (...)
> cur_trans->state = TRANS_STATE_COMMIT_DOING;
> (...)
>
> cur_trans->state = TRANS_STATE_UNBLOCKED;
> root->fs_info->running_transaction = NULL;
>
> btrfs_start_transaction()
> --> starts transaction N + 1
>
> btrfs_write_and_wait_transaction(trans, root);
> --> starts writing all new or COWed ebs created
> at transaction N
>
> creates some new ebs, COWs some
> existing ebs but doesn't COW or
> deletes eb X
>
> btrfs_commit_transaction(N + 1)
> (...)
> cur_trans->state = TRANS_STATE_COMMIT_START;
> (...)
> wait_for_commit(root, prev_trans);
> --> prev_trans == transaction N
>
> btrfs_write_and_wait_transaction() continues
> writing ebs
> --> fails writing eb X, we abort transaction N
> and set bit BTRFS_FS_STATE_ERROR on
> fs_info->fs_state, so no new transactions
> can start after setting that bit
>
> cleanup_transaction()
> btrfs_cleanup_one_transaction()
> wakes up task at CPU 1
>
> continues, doesn't abort because
> cur_trans->aborted (transaction N + 1)
> is zero, and no checks for bit
> BTRFS_FS_STATE_ERROR in fs_info->fs_state
> are made
>
> btrfs_write_and_wait_transaction(trans, root);
> --> succeeds, no errors during writeback
>
> write_ctree_super(trans, root, 0);
> --> succeeds
> --> we have now a superblock that points us
> to some root that uses eb X, which was
> never written to disk
>
> In this scenario future attempts to read eb X from disk results in an
> error message like "parent transid verify failed on X wanted Y found Z".
>
> So fix this by aborting the current transaction if after waiting for the
> previous transaction we verify that it was aborted.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
Eesh good catch Filpe,
Reviewed-by: Josef Bacik <jbacik@fb.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] Btrfs: check if previous transaction aborted to avoid fs corruption
2015-08-12 14:11 [PATCH] Btrfs: check if previous transaction aborted to avoid fs corruption fdmanana
2015-08-12 14:37 ` Josef Bacik
@ 2015-08-12 14:44 ` fdmanana
2015-08-12 15:19 ` Liu Bo
1 sibling, 1 reply; 4+ messages in thread
From: fdmanana @ 2015-08-12 14:44 UTC (permalink / raw)
To: linux-btrfs; +Cc: Filipe Manana, stable
From: Filipe Manana <fdmanana@suse.com>
While we are committing a transaction, it's possible the previous one is
still finishing its commit and therefore we wait for it to finish first.
However we were not checking if that previous transaction ended up getting
aborted after we waited for it to commit, so we ended up committing the
current transaction which can lead to fs corruption because the new
superblock can point to trees that have had one or more nodes/leafs that
were never durably persisted.
The following sequence diagram exemplifies how this is possible:
CPU 0 CPU 1
transaction N starts
(...)
btrfs_commit_transaction(N)
cur_trans->state = TRANS_STATE_COMMIT_START;
(...)
cur_trans->state = TRANS_STATE_COMMIT_DOING;
(...)
cur_trans->state = TRANS_STATE_UNBLOCKED;
root->fs_info->running_transaction = NULL;
btrfs_start_transaction()
--> starts transaction N + 1
btrfs_write_and_wait_transaction(trans, root);
--> starts writing all new or COWed ebs created
at transaction N
creates some new ebs, COWs some
existing ebs but doesn't COW or
deletes eb X
btrfs_commit_transaction(N + 1)
(...)
cur_trans->state = TRANS_STATE_COMMIT_START;
(...)
wait_for_commit(root, prev_trans);
--> prev_trans == transaction N
btrfs_write_and_wait_transaction() continues
writing ebs
--> fails writing eb X, we abort transaction N
and set bit BTRFS_FS_STATE_ERROR on
fs_info->fs_state, so no new transactions
can start after setting that bit
cleanup_transaction()
btrfs_cleanup_one_transaction()
wakes up task at CPU 1
continues, doesn't abort because
cur_trans->aborted (transaction N + 1)
is zero, and no checks for bit
BTRFS_FS_STATE_ERROR in fs_info->fs_state
are made
btrfs_write_and_wait_transaction(trans, root);
--> succeeds, no errors during writeback
write_ctree_super(trans, root, 0);
--> succeeds
--> we have now a superblock that points us
to some root that uses eb X, which was
never written to disk
In this scenario future attempts to read eb X from disk results in an
error message like "parent transid verify failed on X wanted Y found Z".
So fix this by aborting the current transaction if after waiting for the
previous transaction we verify that it was aborted.
Cc: stable@vger.kernel.org
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
V2: Added missing cc to stable and included Josef's reviewed-by tag.
No code changes.
fs/btrfs/transaction.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 51e0f0d..d15a43f 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1893,8 +1893,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
spin_unlock(&root->fs_info->trans_lock);
wait_for_commit(root, prev_trans);
+ ret = prev_trans->aborted;
btrfs_put_transaction(prev_trans);
+ if (ret)
+ goto cleanup_transaction;
} else {
spin_unlock(&root->fs_info->trans_lock);
}
--
2.1.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] Btrfs: check if previous transaction aborted to avoid fs corruption
2015-08-12 14:44 ` [PATCH v2] " fdmanana
@ 2015-08-12 15:19 ` Liu Bo
0 siblings, 0 replies; 4+ messages in thread
From: Liu Bo @ 2015-08-12 15:19 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs, Filipe Manana, stable
On Wed, Aug 12, 2015 at 03:44:51PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> While we are committing a transaction, it's possible the previous one is
> still finishing its commit and therefore we wait for it to finish first.
> However we were not checking if that previous transaction ended up getting
> aborted after we waited for it to commit, so we ended up committing the
> current transaction which can lead to fs corruption because the new
> superblock can point to trees that have had one or more nodes/leafs that
> were never durably persisted.
> The following sequence diagram exemplifies how this is possible:
>
> CPU 0 CPU 1
>
> transaction N starts
>
> (...)
>
> btrfs_commit_transaction(N)
>
> cur_trans->state = TRANS_STATE_COMMIT_START;
> (...)
> cur_trans->state = TRANS_STATE_COMMIT_DOING;
> (...)
>
> cur_trans->state = TRANS_STATE_UNBLOCKED;
> root->fs_info->running_transaction = NULL;
>
> btrfs_start_transaction()
> --> starts transaction N + 1
>
> btrfs_write_and_wait_transaction(trans, root);
> --> starts writing all new or COWed ebs created
> at transaction N
>
> creates some new ebs, COWs some
> existing ebs but doesn't COW or
> deletes eb X
>
> btrfs_commit_transaction(N + 1)
> (...)
> cur_trans->state = TRANS_STATE_COMMIT_START;
> (...)
> wait_for_commit(root, prev_trans);
> --> prev_trans == transaction N
>
> btrfs_write_and_wait_transaction() continues
> writing ebs
> --> fails writing eb X, we abort transaction N
> and set bit BTRFS_FS_STATE_ERROR on
> fs_info->fs_state, so no new transactions
> can start after setting that bit
>
> cleanup_transaction()
> btrfs_cleanup_one_transaction()
> wakes up task at CPU 1
>
> continues, doesn't abort because
> cur_trans->aborted (transaction N + 1)
> is zero, and no checks for bit
> BTRFS_FS_STATE_ERROR in fs_info->fs_state
> are made
>
> btrfs_write_and_wait_transaction(trans, root);
> --> succeeds, no errors during writeback
>
> write_ctree_super(trans, root, 0);
> --> succeeds
> --> we have now a superblock that points us
> to some root that uses eb X, which was
> never written to disk
>
> In this scenario future attempts to read eb X from disk results in an
> error message like "parent transid verify failed on X wanted Y found Z".
>
> So fix this by aborting the current transaction if after waiting for the
> previous transaction we verify that it was aborted.
Looks good to me.
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Thanks,
-liubo
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> Reviewed-by: Josef Bacik <jbacik@fb.com>
> ---
>
> V2: Added missing cc to stable and included Josef's reviewed-by tag.
> No code changes.
>
> fs/btrfs/transaction.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 51e0f0d..d15a43f 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1893,8 +1893,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
> spin_unlock(&root->fs_info->trans_lock);
>
> wait_for_commit(root, prev_trans);
> + ret = prev_trans->aborted;
>
> btrfs_put_transaction(prev_trans);
> + if (ret)
> + goto cleanup_transaction;
> } else {
> spin_unlock(&root->fs_info->trans_lock);
> }
> --
> 2.1.3
>
> --
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-08-12 15:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-12 14:11 [PATCH] Btrfs: check if previous transaction aborted to avoid fs corruption fdmanana
2015-08-12 14:37 ` Josef Bacik
2015-08-12 14:44 ` [PATCH v2] " fdmanana
2015-08-12 15:19 ` Liu Bo
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).