Re: [PATCH] xfs: shutdown on failure to add page to log bio

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Mar 24, 2020 at 12:57:00PM -0400, Brian Foster wrote:
> If the bio_add_page() call fails, we proceed to write out a
> partially constructed log buffer. This corrupts the physical log
> such that log recovery is not possible. Worse, persistent
> occurrences of this error eventually lead to a BUG_ON() failure in
> bio_split() as iclogs wrap the end of the physical log, which
> triggers log recovery on subsequent mount.
> 
> Rather than warn about writing out a corrupted log buffer, shutdown
> the fs as is done for any log I/O related error. This preserves the
> consistency of the physical log such that log recovery succeeds on a
> subsequent mount. Note that this was observed on a 64k page debug
> kernel without upstream commit 59bb47985c1d ("mm, sl[aou]b:
> guarantee natural alignment for kmalloc(power-of-two)"), which
> demonstrated frequent iclog bio overflows due to unaligned (slab
> allocated) iclog data buffers.

Fixes: tag?

Otherwise, looks ok to me.

--D

> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2a90a483c2d6..ebb6a5c95332 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1705,16 +1705,22 @@ xlog_bio_end_io(
>  
>  static void
>  xlog_map_iclog_data(
> -	struct bio		*bio,
> -	void			*data,
> +	struct xlog_in_core	*iclog,
>  	size_t			count)
>  {
> +	struct xfs_mount	*mp = iclog->ic_log->l_mp;
> +	struct bio		*bio = &iclog->ic_bio;
> +	void			*data = iclog->ic_data;
> +
>  	do {
>  		struct page	*page = kmem_to_page(data);
>  		unsigned int	off = offset_in_page(data);
>  		size_t		len = min_t(size_t, count, PAGE_SIZE - off);
>  
> -		WARN_ON_ONCE(bio_add_page(bio, page, len, off) != len);
> +		if (bio_add_page(bio, page, len, off) != len) {
> +			xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
> +			break;
> +		}
>  
>  		data += len;
>  		count -= len;
> @@ -1762,7 +1768,7 @@ xlog_write_iclog(
>  	if (need_flush)
>  		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
>  
> -	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count);
> +	xlog_map_iclog_data(iclog, count);
>  	if (is_vmalloc_addr(iclog->ic_data))
>  		flush_kernel_vmap_range(iclog->ic_data, count);
>  
> -- 
> 2.21.1
>