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

[Brian Foster <bfoster@redhat.com>]

On Wed, Mar 25, 2020 at 12:12:25AM -0700, Christoph Hellwig wrote:
> On Tue, Mar 24, 2020 at 12:57:00PM -0400, Brian Foster wrote:
> > 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.
> 
> Weird..
> 
> >  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);
> 
> Can you just return an error from xlog_map_iclog_data and shut down
> in the caller?  Besides keeping the abstraction levels similar I had
> also hoped to lift xlog_map_iclog_data into the block layer eventually.
> 

Sure. That's probably more appropriate now that I look again because it
looks like we still submit the current bio with this patch. Something
like the following..?

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2a90a483c2d6..92a58a6bc32b 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1703,7 +1703,7 @@ xlog_bio_end_io(
 		   &iclog->ic_end_io_work);
 }
 
-static void
+static int
 xlog_map_iclog_data(
 	struct bio		*bio,
 	void			*data,
@@ -1714,11 +1714,14 @@ xlog_map_iclog_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)
+			break;
 
 		data += len;
 		count -= len;
 	} while (count);
+
+	return count;
 }
 
 STATIC void
@@ -1762,7 +1765,10 @@ xlog_write_iclog(
 	if (need_flush)
 		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
 
-	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count);
+	if (xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count)) {
+		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
+		return;
+	}
 	if (is_vmalloc_addr(iclog->ic_data))
 		flush_kernel_vmap_range(iclog->ic_data, count);