| Message ID | 43cd8760-6275-cd2f-1d98-a62b5c85c5e5@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Jun 8, 2016 at 5:36 AM, Jeff Mahoney <jeffm@suse.com> 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@vger.kernel.org # 3.4+ > Signed-off-by: Jeff Mahoney <jeffm@suse.com> But anyway, Reviewed-by: Filipe Manana <fdmanana@suse.com> > --- > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. 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@vger.kernel.org # 3.4+ Signed-off-by: Jeff Mahoney <jeffm@suse.com> --- 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(-)