Re: [PATCH] xfs: up(ic_sema) if flushing data device fails

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

On Mon, Oct 23, 2023 at 11:14:10AM -0700, Leah Rumancik wrote:
> We flush the data device cache before we issue external log IO. Since
> 7d839e325af2, we check the return value of the flush, and if the flush
> failed, we shut down the log immediately and return. However, the
> iclog->ic_sema is left in a decremented state so let's add an up().
> Prior to this patch, xfs/438 would fail consistently when running with
> an external log device:
> 
> sync
>   -> xfs_log_force
>   -> xlog_write_iclog
>       -> down(&iclog->ic_sema)
>       -> blkdev_issue_flush (fail causes us to intiate shutdown)
>           -> xlog_force_shutdown
>           -> return
> 
> unmount
>   -> xfs_log_umount
>       -> xlog_wait_iclog_completion
>           -> down(&iclog->ic_sema) --------> HANG
> 
> There is a second early return / shutdown. Add an up() there as well.

Ow.  Yes, I think it's correct that both of those error returns need to
drop ic_sema since we don't submit_bio, so there is no xlog_ioend_work
to do it for us.

> Fixes: 7d839e325af2 ("xfs: check return codes when flushing block devices")

Hmm.  This bug was introduced in b5d721eaae47e ("xfs: external logs need
to flush data device"), not 7d839.  That said, this patch only applies
cleanly to 7d839e325af2.

b5d721 was introduced in 5.14 and 7d839 came in via 6.0, so ... this is
where I would have spat out:

Fixes: 7d839e325af2 ("xfs: check return codes when flushing block devices")
Actually-Fixes: b5d721eaae47e ("xfs: external logs need to flush data device")

> Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>
> ---
> 
> Notes:
>     Tested auto group for xfs/4k and xfs/logdev configs with no regressions
>     seen.
> 
>  fs/xfs/xfs_log.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 51c100c86177..b4a8105299c2 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1926,6 +1926,7 @@ xlog_write_iclog(
>  		 */
>  		if (log->l_targ != log->l_mp->m_ddev_targp &&
>  		    blkdev_issue_flush(log->l_mp->m_ddev_targp->bt_bdev)) {
> +			up(&iclog->ic_sema);
>  			xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
>  			return;
>  		}
> @@ -1936,6 +1937,7 @@ xlog_write_iclog(
>  	iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);
>  
>  	if (xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count)) {
> +		up(&iclog->ic_sema);
>  		xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);

I wonder if these two should both become a cleanup clause at the end?

		if (log->l_targ != log->l_mp->m_ddev_targp &&
		    blkdev_issue_flush(log->l_mp->m_ddev_targp->bt_bdev))
			goto shutdown;

...

	if (xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count))
		goto shutdown;

...

	submit_bio(&iclog->ic_bio);
	return;

shutdown:
	up(&iclog->ic_sema);
	xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
}

Seeing as we've screwed this up twice now and not a whole lot of people
actually use external logs, and somehow I've never seen this on my test
fleet.

Anyway the code change looks correct so modulo the stylistic thing,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  		return;
>  	}
> -- 
> 2.42.0.758.gaed0368e0e-goog
>