| Message ID | 20201124144413.3168075-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Annotate unlocked accesses to transaction state | expand |
On Tue, Nov 24, 2020 at 04:44:13PM +0200, Nikolay Borisov wrote: > btrfs_transaction::state is protected by btrfs_fs_info::trans_lock and > all accesses to this variable should be synchornized by it. However, > there are 2 exceptions, namely checking if currently running transaction > has entered its commit phase in start_transaction and in > btrfs_should_end_transaction. > > Annotate those 2, unlocked accesses with READ_ONCE respectively. Also > remove the full memory barrier in start_transaction as it provides no > ordering guarantees due to being unpaired. All other accsess to > transaction state happen under trans_lock being held. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/transaction.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index df5687a92798..e5a5c3604a9b 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -695,8 +695,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items, > h->can_flush_pending_bgs = true; > INIT_LIST_HEAD(&h->new_bgs); > > - smp_mb(); > - if (cur_trans->state >= TRANS_STATE_COMMIT_START && > + if (READ_ONCE(cur_trans->state) >= TRANS_STATE_COMMIT_START && > may_wait_transaction(fs_info, type)) { > current->journal_info = h; > btrfs_commit_transaction(h); > @@ -912,7 +911,7 @@ int btrfs_should_end_transaction(struct btrfs_trans_handle *trans) > { > struct btrfs_transaction *cur_trans = trans->transaction; > > - if (cur_trans->state >= TRANS_STATE_COMMIT_START || > + if (READ_ONCE(cur_trans->state) >= TRANS_STATE_COMMIT_START || > test_bit(BTRFS_DELAYED_REFS_FLUSHING, > &cur_trans->delayed_refs.flags)) > return 1; > -- > 2.25.1 > I think some parts of this have already gone in and the conditional has been lightly refactored, so this patch no longer applies to kdave/misc-next. Assuming this is a change for adherence to best practices rather than a fix for a specific issue (in which case it would be nice to have the issue described in the message), the patch looks good to me otherwise.
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index df5687a92798..e5a5c3604a9b 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -695,8 +695,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items, h->can_flush_pending_bgs = true; INIT_LIST_HEAD(&h->new_bgs); - smp_mb(); - if (cur_trans->state >= TRANS_STATE_COMMIT_START && + if (READ_ONCE(cur_trans->state) >= TRANS_STATE_COMMIT_START && may_wait_transaction(fs_info, type)) { current->journal_info = h; btrfs_commit_transaction(h); @@ -912,7 +911,7 @@ int btrfs_should_end_transaction(struct btrfs_trans_handle *trans) { struct btrfs_transaction *cur_trans = trans->transaction; - if (cur_trans->state >= TRANS_STATE_COMMIT_START || + if (READ_ONCE(cur_trans->state) >= TRANS_STATE_COMMIT_START || test_bit(BTRFS_DELAYED_REFS_FLUSHING, &cur_trans->delayed_refs.flags)) return 1;
btrfs_transaction::state is protected by btrfs_fs_info::trans_lock and all accesses to this variable should be synchornized by it. However, there are 2 exceptions, namely checking if currently running transaction has entered its commit phase in start_transaction and in btrfs_should_end_transaction. Annotate those 2, unlocked accesses with READ_ONCE respectively. Also remove the full memory barrier in start_transaction as it provides no ordering guarantees due to being unpaired. All other accsess to transaction state happen under trans_lock being held. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/transaction.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) -- 2.25.1