Re: [PATCH 2/2] xfs: fix mount hang during primary superblock recovery failure

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jan 07, 2025 at 09:39:10PM +0800, Long Li wrote:
> On Mon, Jan 06, 2025 at 11:55:41AM -0800, Darrick J. Wong wrote:
> > On Tue, Dec 31, 2024 at 10:34:23AM +0800, Long Li wrote:
> > > When mounting an image containing a log with sb modifications that require
> > > log replay, the mount process hang all the time and stack as follows:
> > > 
> > >   [root@localhost ~]# cat /proc/557/stack
> > >   [<0>] xfs_buftarg_wait+0x31/0x70
> > >   [<0>] xfs_buftarg_drain+0x54/0x350
> > >   [<0>] xfs_mountfs+0x66e/0xe80
> > >   [<0>] xfs_fs_fill_super+0x7f1/0xec0
> > >   [<0>] get_tree_bdev_flags+0x186/0x280
> > >   [<0>] get_tree_bdev+0x18/0x30
> > >   [<0>] xfs_fs_get_tree+0x1d/0x30
> > >   [<0>] vfs_get_tree+0x2d/0x110
> > >   [<0>] path_mount+0xb59/0xfc0
> > >   [<0>] do_mount+0x92/0xc0
> > >   [<0>] __x64_sys_mount+0xc2/0x160
> > >   [<0>] x64_sys_call+0x2de4/0x45c0
> > >   [<0>] do_syscall_64+0xa7/0x240
> > >   [<0>] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > 
> > > During log recovery, while updating the in-memory superblock from the
> > > primary SB buffer, if an error is encountered, such as superblock
> > > corruption occurs or some other reasons, we will proceed to out_release
> > > and release the xfs_buf. However, this is insufficient because the
> > > xfs_buf's log item has already been initialized and the xfs_buf is held
> > > by the buffer log item as follows, the xfs_buf will not be released,
> > > causing the mount thread to hang.
> > > 
> > >   xlog_recover_do_primary_sb_buffer
> > >     xlog_recover_do_reg_buffer
> > >       xlog_recover_validate_buf_type
> > >         xfs_buf_item_init(bp, mp)
> > > 
> > > The solution is straightforward: we simply need to allow it to be
> > > handled by the normal buffer write process. The filesystem will be
> > > shutdown before the submission of buffer_list in xlog_do_recovery_pass(),
> > 
> > What shuts it down?  If xlog_recover_do_primary_sb_buffer trips over
> > something like "mp->m_sb.sb_rgcount < orig_rgcount" then we haven't shut
> > anything down yet.  Am I missing something? <confused>
> > 
> > --D
> > 
> 
> Hi Darrick,
> 
> Sorry for being unclear. I was referring to the shutdown in xlog_do_recovery_pass().
> Here's the specific flow after the fix: 
> 
>   xlog_do_recovery_pass
>    error = xlog_recover_process
>      xlog_recover_process_data
>        xlog_recover_process_ophdr
>          xlog_recovery_process_trans
>            ...
>              xlog_recover_buf_commit_pass2
>                error = xlog_recover_do_primary_sb_buffer
>                  //Encounter error and return
>                if (error)
>                  goto out_writebuf
>                ...
> out_writebuf:
>                xfs_buf_delwri_queue(bp, buffer_list)  //add bp to buffer_list
>                return  error
>            ...
>    if (!list_empty(&buffer_list))
>      if (error)
>        xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);  //log shutdown first
>      xfs_buf_delwri_submit(&buffer_list);
>        __xfs_buf_submit
>          if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
>            xfs_buf_ioend_fail(bp)  //release bp correctly
> 
> It might be clearer to put this process into a commit message.

Yes please, put that in a code comment.  That was too subtle for me to
figure out. :/

--D

> Thanks,
> Long Li
>