* [PATCH] btrfs: abort the transaction if we fail to replay log trees
@ 2021-05-19 15:29 Josef Bacik
2021-05-19 16:22 ` Johannes Thumshirn
2021-05-20 1:08 ` Qu Wenruo
0 siblings, 2 replies; 6+ messages in thread
From: Josef Bacik @ 2021-05-19 15:29 UTC (permalink / raw)
To: linux-btrfs, kernel-team
During inspection of the return path for replay I noticed that we don't
actually abort the transaction if we get a failure during replay. This
isn't a problem necessarily, as we properly return the error and will
fail to mount. However we still leave this dangling transaction that
could conceivably be committed without thinking there was an error.
Handle this by making sure we abort the transaction on error to
safeguard us from any problems in the future.
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
fs/btrfs/tree-log.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 4dc74949040d..18009095908b 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -6352,8 +6352,10 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
return 0;
error:
- if (wc.trans)
+ if (wc.trans) {
+ btrfs_abort_transaction(wc.trans, ret);
btrfs_end_transaction(wc.trans);
+ }
btrfs_free_path(path);
return ret;
}
--
2.26.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: abort the transaction if we fail to replay log trees
2021-05-19 15:29 [PATCH] btrfs: abort the transaction if we fail to replay log trees Josef Bacik
@ 2021-05-19 16:22 ` Johannes Thumshirn
2021-05-19 20:15 ` David Sterba
2021-05-20 1:08 ` Qu Wenruo
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Thumshirn @ 2021-05-19 16:22 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, kernel-team
On 19/05/2021 17:29, Josef Bacik wrote:
> During inspection of the return path for replay I noticed that we don't
> actually abort the transaction if we get a failure during replay. This
> isn't a problem necessarily, as we properly return the error and will
> fail to mount. However we still leave this dangling transaction that
> could conceivably be committed without thinking there was an error.
> Handle this by making sure we abort the transaction on error to
> safeguard us from any problems in the future.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> fs/btrfs/tree-log.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 4dc74949040d..18009095908b 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6352,8 +6352,10 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>
> return 0;
> error:
> - if (wc.trans)
> + if (wc.trans) {
> + btrfs_abort_transaction(wc.trans, ret);
> btrfs_end_transaction(wc.trans);
> + }
> btrfs_free_path(path);
> return ret;
> }
>
Hmm our Wiki's Development notes page says:
Please keep all transaction abort exactly at the place where they happen and do not
merge them to one. This pattern should be used everwhere and is important when
debugging because we can pinpoint the line in the code from the syslog message and do
not have to guess which way it got to the merged call.
But there are 6 'goto error;' lines in btrfs_recover_log_trees() and changing all of
them might be a bit too much.
Anyways,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: abort the transaction if we fail to replay log trees
2021-05-19 16:22 ` Johannes Thumshirn
@ 2021-05-19 20:15 ` David Sterba
2021-05-19 20:22 ` Josef Bacik
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2021-05-19 20:15 UTC (permalink / raw)
To: Johannes Thumshirn; +Cc: Josef Bacik, linux-btrfs, kernel-team
On Wed, May 19, 2021 at 04:22:20PM +0000, Johannes Thumshirn wrote:
> On 19/05/2021 17:29, Josef Bacik wrote:
> > During inspection of the return path for replay I noticed that we don't
> > actually abort the transaction if we get a failure during replay. This
> > isn't a problem necessarily, as we properly return the error and will
> > fail to mount. However we still leave this dangling transaction that
> > could conceivably be committed without thinking there was an error.
> > Handle this by making sure we abort the transaction on error to
> > safeguard us from any problems in the future.
> >
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> > fs/btrfs/tree-log.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > index 4dc74949040d..18009095908b 100644
> > --- a/fs/btrfs/tree-log.c
> > +++ b/fs/btrfs/tree-log.c
> > @@ -6352,8 +6352,10 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
> >
> > return 0;
> > error:
> > - if (wc.trans)
> > + if (wc.trans) {
> > + btrfs_abort_transaction(wc.trans, ret);
> > btrfs_end_transaction(wc.trans);
> > + }
> > btrfs_free_path(path);
> > return ret;
> > }
> >
>
> Hmm our Wiki's Development notes page says:
>
> Please keep all transaction abort exactly at the place where they happen and do not
> merge them to one. This pattern should be used everwhere and is important when
> debugging because we can pinpoint the line in the code from the syslog message and do
> not have to guess which way it got to the merged call.
>
> But there are 6 'goto error;' lines in btrfs_recover_log_trees() and changing all of
> them might be a bit too much.
Good point and I want to keep the abort pattern consistent so it should
be called before the goto error's. Note that this function still uses
btrfs_handle_fs_error which predates the transaction abort framework and
should be replaced as needed.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: abort the transaction if we fail to replay log trees
2021-05-19 20:15 ` David Sterba
@ 2021-05-19 20:22 ` Josef Bacik
2021-05-20 13:16 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Josef Bacik @ 2021-05-19 20:22 UTC (permalink / raw)
To: dsterba, Johannes Thumshirn, linux-btrfs, kernel-team
On 5/19/21 4:15 PM, David Sterba wrote:
> On Wed, May 19, 2021 at 04:22:20PM +0000, Johannes Thumshirn wrote:
>> On 19/05/2021 17:29, Josef Bacik wrote:
>>> During inspection of the return path for replay I noticed that we don't
>>> actually abort the transaction if we get a failure during replay. This
>>> isn't a problem necessarily, as we properly return the error and will
>>> fail to mount. However we still leave this dangling transaction that
>>> could conceivably be committed without thinking there was an error.
>>> Handle this by making sure we abort the transaction on error to
>>> safeguard us from any problems in the future.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>> fs/btrfs/tree-log.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>>> index 4dc74949040d..18009095908b 100644
>>> --- a/fs/btrfs/tree-log.c
>>> +++ b/fs/btrfs/tree-log.c
>>> @@ -6352,8 +6352,10 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>>>
>>> return 0;
>>> error:
>>> - if (wc.trans)
>>> + if (wc.trans) {
>>> + btrfs_abort_transaction(wc.trans, ret);
>>> btrfs_end_transaction(wc.trans);
>>> + }
>>> btrfs_free_path(path);
>>> return ret;
>>> }
>>>
>>
>> Hmm our Wiki's Development notes page says:
>>
>> Please keep all transaction abort exactly at the place where they happen and do not
>> merge them to one. This pattern should be used everwhere and is important when
>> debugging because we can pinpoint the line in the code from the syslog message and do
>> not have to guess which way it got to the merged call.
>>
>> But there are 6 'goto error;' lines in btrfs_recover_log_trees() and changing all of
>> them might be a bit too much.
>
> Good point and I want to keep the abort pattern consistent so it should
> be called before the goto error's. Note that this function still uses
> btrfs_handle_fs_error which predates the transaction abort framework and
> should be replaced as needed.
>
Yeah this is a good point, I assume since we're now going to get the transaction
abort message for the spots I replace btrfs_handle_fs_error() we don't need to
replace the message? Thanks,
Josef
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: abort the transaction if we fail to replay log trees
2021-05-19 15:29 [PATCH] btrfs: abort the transaction if we fail to replay log trees Josef Bacik
2021-05-19 16:22 ` Johannes Thumshirn
@ 2021-05-20 1:08 ` Qu Wenruo
1 sibling, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2021-05-20 1:08 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, kernel-team
On 2021/5/19 下午11:29, Josef Bacik wrote:
> During inspection of the return path for replay I noticed that we don't
> actually abort the transaction if we get a failure during replay. This
> isn't a problem necessarily, as we properly return the error and will
> fail to mount. However we still leave this dangling transaction that
> could conceivably be committed without thinking there was an error.
> Handle this by making sure we abort the transaction on error to
> safeguard us from any problems in the future.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/tree-log.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 4dc74949040d..18009095908b 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6352,8 +6352,10 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>
> return 0;
> error:
> - if (wc.trans)
> + if (wc.trans) {
> + btrfs_abort_transaction(wc.trans, ret);
> btrfs_end_transaction(wc.trans);
> + }
> btrfs_free_path(path);
> return ret;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: abort the transaction if we fail to replay log trees
2021-05-19 20:22 ` Josef Bacik
@ 2021-05-20 13:16 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2021-05-20 13:16 UTC (permalink / raw)
To: Josef Bacik; +Cc: dsterba, Johannes Thumshirn, linux-btrfs, kernel-team
On Wed, May 19, 2021 at 04:22:33PM -0400, Josef Bacik wrote:
> On 5/19/21 4:15 PM, David Sterba wrote:
> > On Wed, May 19, 2021 at 04:22:20PM +0000, Johannes Thumshirn wrote:
> >> On 19/05/2021 17:29, Josef Bacik wrote:
> > Good point and I want to keep the abort pattern consistent so it should
> > be called before the goto error's. Note that this function still uses
> > btrfs_handle_fs_error which predates the transaction abort framework and
> > should be replaced as needed.
>
> Yeah this is a good point, I assume since we're now going to get the transaction
> abort message for the spots I replace btrfs_handle_fs_error() we don't need to
> replace the message? Thanks,
Yeah, plain btrfs_abort_transaction should be ok.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-20 13:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19 15:29 [PATCH] btrfs: abort the transaction if we fail to replay log trees Josef Bacik
2021-05-19 16:22 ` Johannes Thumshirn
2021-05-19 20:15 ` David Sterba
2021-05-19 20:22 ` Josef Bacik
2021-05-20 13:16 ` David Sterba
2021-05-20 1:08 ` Qu Wenruo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.