btrfs: always abort the transaction if we abort a trans handle

While stress testing our error handling I noticed that sometimes we
would still commit the transaction even though we had aborted the
transaction.

Currently we track if a trans handle has dirtied any metadata, and if it
hasn't we mark the filesystem as having an error (so no new transactions
can be started), but we will allow the current transaction to complete
as we do not mark the transaction itself as having been aborted.

This sounds good in theory, but we were not properly tracking IO errors
in btrfs_finish_ordered_io, and thus committing the transaction with
bogus free space data.  This isn't necessarily a problem per-se with the
free space cache, as the other guards in place would have kept us from
accepting the free space cache as valid, but highlights a real world
case where we had a bug and could have corrupted the filesystem because
of it.

This "skip abort on empty trans handle" is nice in theory, but assumes
we have perfect error handling everywhere, which we clearly do not.
Also we do not allow further transactions to be started, so all this
does is save the last transaction that was happening, which doesn't
necessarily gain us anything other than the potential for real
corruption.

Remove this particular bit of code, if we decide we need to abort the
transaction then abort the current one and keep us from doing real harm
to the file system, regardless of whether this specific trans handle
dirtied anything or not.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a484fb72a01f0b620627331522e805a28b7101c0..4bc3ca2cbd7d41bed818f0d7831c9802c365e59f 100644 (file)
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -596,7 +596,6 @@ noinline int btrfs_cow_block(struct btrfs_trans_handle *trans,
                       trans->transid, fs_info->generation);
 
        if (!should_cow_block(trans, root, buf)) {
-               trans->dirty = true;
                *cow_ret = buf;
                return 0;
        }
@@ -1788,10 +1787,8 @@ again:
                         * then we don't want to set the path blocking,
                         * so we test it here
                         */
-                       if (!should_cow_block(trans, root, b)) {
-                               trans->dirty = true;
+                       if (!should_cow_block(trans, root, b))
                                goto cow_done;
-                       }
 
                        /*
                         * must have write locks on this node and the

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3d5c35e4cb76e095439904c37363025dbbe2d584..d2f39a122d89d097cc562d2dbb9a29c129f8bb29 100644 (file)
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4784,7 +4784,6 @@ btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
                set_extent_dirty(&trans->transaction->dirty_pages, buf->start,
                         buf->start + buf->len - 1, GFP_NOFS);
        }
-       trans->dirty = true;
        /* this returns a buffer locked for blocking */
        return buf;
 }

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 4a396c1147f177c159909390806a153f22fe9f55..bc613218c8c5bd23b62d96417c333f9cab479b51 100644 (file)
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -299,17 +299,6 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
        struct btrfs_fs_info *fs_info = trans->fs_info;
 
        WRITE_ONCE(trans->aborted, errno);
-       /* Nothing used. The other threads that have joined this
-        * transaction may be able to continue. */
-       if (!trans->dirty && list_empty(&trans->new_bgs)) {
-               const char *errstr;
-
-               errstr = btrfs_decode_error(errno);
-               btrfs_warn(fs_info,
-                          "%s:%d: Aborting unused transaction(%s).",
-                          function, line, errstr);
-               return;
-       }
        WRITE_ONCE(trans->transaction->aborted, errno);
        /* Wake up anybody who may be waiting on this transaction */
        wake_up(&fs_info->transaction_wait);

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index f75de9f6c0ada5a4600f93b55513f05c29567e47..e0a82aa7da897cc576b62270b10f3abd83be49b0 100644 (file)
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2074,14 +2074,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 
        ASSERT(refcount_read(&trans->use_count) == 1);
 
-       /*
-        * Some places just start a transaction to commit it.  We need to make
-        * sure that if this commit fails that the abort code actually marks the
-        * transaction as failed, so set trans->dirty to make the abort code do
-        * the right thing.
-        */
-       trans->dirty = true;
-
        /* Stop the commit early if ->aborted is set */
        if (TRANS_ABORTED(cur_trans)) {
                ret = cur_trans->aborted;

diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 364cfbb4c5c596c71dc2f02f838590a9edabcc44..c49e2266b28babe0c279fb91ca3706f26777ec46 100644 (file)
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -143,7 +143,6 @@ struct btrfs_trans_handle {
        bool allocating_chunk;
        bool can_flush_pending_bgs;
        bool reloc_reserved;
-       bool dirty;
        bool in_fsync;
        struct btrfs_root *root;
        struct btrfs_fs_info *fs_info;