| Message ID | 1020602e269415d91c713afdfb9a11921a3ceda6.1619087848.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: do not consider send context as valid when trying to flush qgroups | expand |
On 2021/4/22 下午7:09, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At qgroup.c:try_flush_qgroup() we are asserting that current->journal_info > is either NULL or has the value BTRFS_SEND_TRANS_STUB. > > However allowing for BTRFS_SEND_TRANS_STUB makes no sense because: > > 1) It is misleading, because send operations are read-only and do not > ever need to reserve qgroup space; > > 2) We already assert that current->journal_info != BTRFS_SEND_TRANS_STUB > at transaction.c:start_transaction(); > > 3) On a kernel without CONFIG_BTRFS_ASSERT=y set, it would result in > a crash if try_flush_qgroup() is ever called in a send context, because > at transaction.c:start_transaction we cast current->journal_info into > a struct btrfs_trans_handle pointer and then dereference it. > > So just do allow a send context at try_flush_qgroup() and update the > comment about it. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/qgroup.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 366a6a289796..3ded812f522c 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -3545,11 +3545,15 @@ static int try_flush_qgroup(struct btrfs_root *root) > struct btrfs_trans_handle *trans; > int ret; > > - /* Can't hold an open transaction or we run the risk of deadlocking */ > - ASSERT(current->journal_info == NULL || > - current->journal_info == BTRFS_SEND_TRANS_STUB); > - if (WARN_ON(current->journal_info && > - current->journal_info != BTRFS_SEND_TRANS_STUB)) > + /* > + * Can't hold an open transaction or we run the risk of deadlocking, > + * and can't either be under the context of a send operation (where > + * current->journal_info is set to BTRFS_SEND_TRANS_STUB), as that > + * would result in a crash when starting a transaction and does not > + * make sense either (send is a read-only operation). > + */ > + ASSERT(current->journal_info == NULL); > + if (WARN_ON(current->journal_info)) > return 0; > > /* >
On Thu, Apr 22, 2021 at 12:09:21PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At qgroup.c:try_flush_qgroup() we are asserting that current->journal_info > is either NULL or has the value BTRFS_SEND_TRANS_STUB. > > However allowing for BTRFS_SEND_TRANS_STUB makes no sense because: > > 1) It is misleading, because send operations are read-only and do not > ever need to reserve qgroup space; > > 2) We already assert that current->journal_info != BTRFS_SEND_TRANS_STUB > at transaction.c:start_transaction(); > > 3) On a kernel without CONFIG_BTRFS_ASSERT=y set, it would result in > a crash if try_flush_qgroup() is ever called in a send context, because > at transaction.c:start_transaction we cast current->journal_info into > a struct btrfs_trans_handle pointer and then dereference it. > > So just do allow a send context at try_flush_qgroup() and update the > comment about it. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 366a6a289796..3ded812f522c 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -3545,11 +3545,15 @@ static int try_flush_qgroup(struct btrfs_root *root) struct btrfs_trans_handle *trans; int ret; - /* Can't hold an open transaction or we run the risk of deadlocking */ - ASSERT(current->journal_info == NULL || - current->journal_info == BTRFS_SEND_TRANS_STUB); - if (WARN_ON(current->journal_info && - current->journal_info != BTRFS_SEND_TRANS_STUB)) + /* + * Can't hold an open transaction or we run the risk of deadlocking, + * and can't either be under the context of a send operation (where + * current->journal_info is set to BTRFS_SEND_TRANS_STUB), as that + * would result in a crash when starting a transaction and does not + * make sense either (send is a read-only operation). + */ + ASSERT(current->journal_info == NULL); + if (WARN_ON(current->journal_info)) return 0; /*