On Tue, May 23, 2023 at 02:25:54PM +0800, Wu Guanghao wrote:
> After testing xfs_growfs + fsstress + fault injection, the following stack appeared
> when mounting the filesystem:
>
> [ 149.902032] XFS (loop0): xfs_buf_map_verify: daddr 0x200001 out of range, EOFS 0x200000
> [ 149.902072] WARNING: CPU: 12 PID: 3045 at fs/xfs/xfs_buf.c:535 xfs_buf_get_map+0x5ae/0x650 [xfs]
> ...
> [ 149.902473] xfs_buf_read_map+0x59/0x330 [xfs]
> [ 149.902621] ? xlog_recover_items_pass2+0x55/0xd0 [xfs]
> [ 149.902809] xlog_recover_buf_commit_pass2+0xff/0x640 [xfs]
> [ 149.902959] ? xlog_recover_items_pass2+0x55/0xd0 [xfs]
> [ 149.903104] xlog_recover_items_pass2+0x55/0xd0 [xfs]
> [ 149.903247] xlog_recover_commit_trans+0x2e0/0x330 [xfs]
> [ 149.903390] xlog_recovery_process_trans+0x8e/0xf0 [xfs]
> [ 149.903531] xlog_recover_process_data+0x9c/0x130 [xfs]
> [ 149.903687] xlog_do_recovery_pass+0x3cc/0x5d0 [xfs]
> [ 149.903843] xlog_do_log_recovery+0x5c/0x80 [xfs]
> [ 149.903984] xlog_do_recover+0x33/0x1c0 [xfs]
> [ 149.904125] xlog_recover+0xdd/0x190 [xfs]
> [ 149.904265] xfs_log_mount+0x125/0x2f0 [xfs]
> [ 149.904410] xfs_mountfs+0x41a/0x910 [xfs]
> [ 149.904558] ? __pfx_xfs_fstrm_free_func+0x10/0x10 [xfs]
> [ 149.904725] xfs_fs_fill_super+0x4b7/0x940 [xfs]
> [ 149.904873] ? __pfx_xfs_fs_fill_super+0x10/0x10 [xfs]
> [ 149.905016] get_tree_bdev+0x19a/0x280
> [ 149.905020] vfs_get_tree+0x29/0xd0
> [ 149.905023] path_mount+0x69e/0x9b0
> [ 149.905026] do_mount+0x7d/0xa0
> [ 149.905029] __x64_sys_mount+0xdc/0x100
> [ 149.905032] do_syscall_64+0x3e/0x90
> [ 149.905035] entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
> The trigger process is as follows:
>
> 1. Growfs size from 0x200000 to 0x300000
> 2. Using the space range of 0x200000~0x300000
> 3. The above operations have only been written to the log area on disk
> 4. Fault injection and shutdown filesystem
> 5. Mount the filesystem and replay the log about growfs, but only modify the
> superblock buffer without modifying the mp->m_sb structure in memory
> 6. Continuing the log replay, at this point we are replaying operation 2, then
> it was discovered that the blocks used more than mp->m_sb.sb_dblocks
>
> Therefore, during log replay, if there are any modifications made to the
> superblock, we should refresh the information recorded in the mp->m_sb.
Heh, clever. Thanks for supplying a patch. :)
> Signed-off-by: Wu Guanghao <wuguanghao3@xxxxxxxxxx>
> ---
> fs/xfs/xfs_buf_item_recover.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 43167f543afc..2ac3d2083188 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -22,6 +22,8 @@
> #include "xfs_inode.h"
> #include "xfs_dir2.h"
> #include "xfs_quota.h"
> +#include "xfs_sb.h"
> +#include "xfs_ag.h"
>
> /*
> * This is the number of entries in the l_buf_cancel_table used during
> @@ -969,6 +971,29 @@ xlog_recover_buf_commit_pass2(
> goto out_release;
> } else {
> xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn);
> + /*
> + * If the superblock buffer is modified, we also need to modify the
> + * content of the mp.
> + */
> + if (bp->b_maps[0].bm_bn == XFS_SB_DADDR && bp->b_ops) {
> + struct xfs_dsb *sb = bp->b_addr;
I think the body of this if statement ought to be a separate function,
e.g.
STATIC int
xlog_recover_sb_buffer(...)
{
struct xfs_dsb *sb = bp->b_addr;
bp->b_ops->verify_write(bp);
...
}
Also, I think the callsite is better placed at the end of
xlog_recover_do_reg_buffer.
> +
> + bp->b_ops->verify_write(bp);
I was about to ask why you ran the full write verifier here instead of
calling ->verify_struct (which skips the crc computation), but then I
realized:
const struct xfs_buf_ops xfs_sb_buf_ops = {
.name = "xfs_sb",
.magic = { cpu_to_be32(XFS_SB_MAGIC), cpu_to_be32(XFS_SB_MAGIC) },
.verify_read = xfs_sb_read_verify,
.verify_write = xfs_sb_write_verify,
};
So, heh. No structure verifier. I think for completeness
xfs_sb_buf_ops ought to have one, but I'm willing to live with this for
now.
> + error = bp->b_error;
> + if (error)
> + goto out_release;
> +
> + if (be32_to_cpu(sb->sb_agcount) > mp->m_sb.sb_agcount) {
> + error = xfs_initialize_perag(mp,
> + be32_to_cpu(sb->sb_agcount),
> + be64_to_cpu(sb->sb_dblocks),
> + &mp->m_maxagi);
> + if (error)
> + goto out_release;
Might want to log a message here that the perag init is what killed log
recovery.
Other than those reorganization suggestions, I think this looks correct.
Would you mind submitting your testcase to fstests?
--D
> + }
> +
> + xfs_sb_from_disk(&mp->m_sb, sb);
> + }
> }
>
> /*
> --
> 2.27.0
