On 2022/1/4 02:52, David Sterba wrote:
On Thu, Dec 16, 2021 at 07:47:35PM +0800, Qu Wenruo wrote:
[BUG]
The following super simple script would crash btrfs at unmount time, if
CONFIG_BTRFS_ASSERT() is set.
mkfs.btrfs -f $dev
mount $dev $mnt
xfs_io -f -c "pwrite 0 4k" $mnt/file
umount $mnt
mount -r ro $dev $mnt
btrfs scrub start -Br $mnt
umount $mnt
This will trigger the following ASSERT() introduced by commit
0a31daa4b602 ("btrfs: add assertion for empty list of transactions at
late stage of umount").
That patch is deifnitely not the cause, it just makes enough noise for
us developer.
[CAUSE]
We will start transaction for the following call chain during scrub:
scrub_enumerate_chunks()
|- btrfs_inc_block_group_ro()
|- btrfs_join_transaction()
However for RO mount, there is no running transaction at all, thus
btrfs_join_transaction() will start a new transaction.
Furthermore, since it's read-only mount, btrfs_sync_fs() will not call
btrfs_commit_super() to commit the new but empty transaction.
And lead to the ASSERT() being triggered.
The bug should be there for a long time. Only the new ASSERT() makes it
noisy enough to be noticed.
[FIX]
For read-only scrub on read-only mount, there is no need to start a
transaction nor to allocate new chunks in btrfs_inc_block_group_ro().
Just do extra read-only mount check in btrfs_inc_block_group_ro(), and
if it's read-only, skip all chunk allocation and go inc_block_group_ro()
directly.
Since we're here, also add extra debug message at unmount for
btrfs_fs_info::trans_list.
Sometimes just knowing that there is no dirty metadata bytes for a
uncommitted transaction can tell us a lot of things.
Cc: stable@xxxxxxxxxxxxxxx # 5.4+
Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
---
fs/btrfs/block-group.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 1db24e6d6d90..702219361b12 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2544,6 +2544,19 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group *cache,
int ret;
bool dirty_bg_running;
+ /*
+ * This can only happen when we are doing read-only scrub on read-only
+ * mount.
+ * In that case we should not start a new transaction on read-only fs.
+ * Thus here we skip all chunk allocation.
+ */
+ if (sb_rdonly(fs_info->sb)) {
Should this also verify or at least assert that do_chunk_alloc is not
set? The scrub code is used for replace that can set the parameter to
true.
Replace start needs RW mount, thus we don't need to bother replace in
this case.
+ mutex_lock(&fs_info->ro_block_group_mutex);
+ ret = inc_block_group_ro(cache, 0);
+ mutex_unlock(&fs_info->ro_block_group_mutex);
+ return ret;
So this is taking a shortcut and skips a few things done in the function
that use the transaction. I'm not sure how safe this is, it depends on
the read-only status of superblock, that can chage any time, so what are
further calls to btrfs_inc_block_group_ro going to do regaring the
transaction?
By anytime you mean "remount". Thus if that's your concern, I can make
remount to stop read-only scrub, just to be extra safe.
Another thing is, only scrub and balance uses this function, for balance
it needs RW.
For scrub, if one scrub is already running, even it's RO and then the fs
mounted RW, then the next scrub run will return -EINPROGRESS or similar
error.
Thus I don't think we need to bother too much about this.
Thanks,
Qu
+ }
+
do {
trans = btrfs_join_transaction(root);
if (IS_ERR(trans))
--
2.34.1
