On Wed, Jun 8, 2016 at 5:36 AM, Jeff Mahoney <[email protected]> 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: [email protected] # 3.4+ > Signed-off-by: Jeff Mahoney <[email protected]> But anyway, Reviewed-by: Filipe Manana <[email protected]> > --- > 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 [email protected] > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
