On Wed, Jun 8, 2016 at 5:36 AM, Jeff Mahoney <jeffm@xxxxxxxx> wrote:
> The test for !trans->blocks_used in btrfs_abort_transaction is
> insufficient to determine whether it's safe to drop the transaction
> handle on the floor. btrfs_cow_block, informed by should_cow_block,
> can return blocks that have already been CoW'd in the current
> transaction. trans->blocks_used is only incremented for new block
> allocations. If an operation overlaps the blocks in the current
> transaction entirely and must abort the transaction, we'll happily
> let it clean up the trans handle even though it may have modified
> the blocks and will commit an incomplete operation.
IMHO it wouldn't hurt to be more explicit here and mention that if the
transaction handle ends up not COWing any nodes/leafs, calling abort
against the handle won't abort the transaction.
>
> In the long-term, I'd like to do closer tracking of when the fs
> is actually modified so we can still recover as gracefully as possible,
> but that approach will need some discussion. In the short term,
> since this is the only code using trans->blocks_used, let's just
> switch it to a bool indicating whether any blocks were used and set
> it when should_cow_block returns false.
>
> Cc: stable@xxxxxxxxxxxxxxx # 3.4+
> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
But anyway,
Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
> ---
> fs/btrfs/ctree.c | 5 ++++-
> fs/btrfs/extent-tree.c | 2 +-
> fs/btrfs/super.c | 2 +-
> fs/btrfs/transaction.h | 2 +-
> 4 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 427c36b..135af4e 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1552,6 +1552,7 @@ noinline int btrfs_cow_block(struct btrfs_trans_handle *trans,
> trans->transid, root->fs_info->generation);
>
> if (!should_cow_block(trans, root, buf)) {
> + trans->dirty = true;
> *cow_ret = buf;
> return 0;
> }
> @@ -2773,8 +2774,10 @@ again:
> * then we don't want to set the path blocking,
> * so we test it here
> */
> - if (!should_cow_block(trans, root, b))
> + if (!should_cow_block(trans, root, b)) {
> + trans->dirty = true;
> 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 689d25a..1ed31eb 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -8044,7 +8044,7 @@ 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->blocks_used++;
> + 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 4e59a91..bd07e01 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -235,7 +235,7 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
> trans->aborted = errno;
> /* Nothing used. The other threads that have joined this
> * transaction may be able to continue. */
> - if (!trans->blocks_used && list_empty(&trans->new_bgs)) {
> + if (!trans->dirty && list_empty(&trans->new_bgs)) {
> const char *errstr;
>
> errstr = btrfs_decode_error(errno);
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index 9fe0ec2..c5abee4 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -110,7 +110,6 @@ struct btrfs_trans_handle {
> u64 chunk_bytes_reserved;
> unsigned long use_count;
> unsigned long blocks_reserved;
> - unsigned long blocks_used;
> unsigned long delayed_ref_updates;
> struct btrfs_transaction *transaction;
> struct btrfs_block_rsv *block_rsv;
> @@ -121,6 +120,7 @@ struct btrfs_trans_handle {
> bool can_flush_pending_bgs;
> bool reloc_reserved;
> bool sync;
> + bool dirty;
> unsigned int type;
> /*
> * this root is only needed to validate that the root passed to
> --
> 2.7.1
>
>
> --
> Jeff Mahoney
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
