On 5/19/21 4:15 PM, David Sterba wrote:

On Wed, May 19, 2021 at 04:22:20PM +0000, Johannes Thumshirn wrote:

quoted

On 19/05/2021 17:29, Josef Bacik wrote:

quoted

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