Re: [PATCH 1/4] xfs: fix recovery failure when log record header wraps log end

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 30, 2017 at 09:38:03PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 27, 2017 at 10:40:33AM -0400, Brian Foster wrote:
> > The high-level log recovery algorithm consists of two loops that
> > walk the physical log and process log records from the tail to the
> > head. The first loop handles the case where the tail is beyond the
> > head and processes records up to the end of the physical log. The
> > subsequent loop processes records from the beginning of the physical
> > log to the head.
> > 
> > Because log records can wrap around the end of the physical log, the
> > first loop mentioned above must handle this case appropriately.
> > Records are processed from in-core buffers, which means that this
> > algorithm must split the reads of such records into two partial
> > I/Os: 1.) from the beginning of the record to the end of the log and
> > 2.) from the beginning of the log to the end of the record. This is
> > further complicated by the fact that the log record header and log
> > record data are read into independent buffers.
> > 
> > The current handling of each buffer correctly splits the reads when
> > either the header or data starts before the end of the log and wraps
> > around the end. The data read does not correctly handle the case
> > where the prior header read wrapped or ends on the physical log end
> > boundary. blk_no is incremented to or beyond the log end after the
> > header read to point to the record data, but the split data read
> > logic triggers, attempts to read from an invalid log block and
> > ultimately causes log recovery to fail. This can be reproduced
> > fairly reliably via xfstests tests generic/047 and generic/388 with
> > large iclog sizes (256k) and small (10M) logs.
> > 
> > If the record header read has pushed beyond the end of the physical
> > log, the subsequent data read is actually contiguous. Update the
> > data read logic to detect the case where blk_no has wrapped, mod it
> > against the log size to read from the correct address and issue one
> > contiguous read for the log data buffer. The log record is processed
> > as normal from the buffer(s), the loop exits after the current
> > iteration and the subsequent loop picks up with the first new record
> > after the start of the log.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_log_recover.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index b6a40bd..9efcd12 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -5371,10 +5371,20 @@ xlog_do_recovery_pass(
> >  			bblks = (int)BTOBB(be32_to_cpu(rhead->h_len));
> >  			blk_no += hblks;
> >  
> > -			/* Read in data for log record */
> > -			if (blk_no + bblks <= log->l_logBBsize) {
> > -				error = xlog_bread(log, blk_no, bblks, dbp,
> > -						   &offset);
> > +			/*
> > +			 * Read the log record data in multiple reads if it
> > +			 * wraps around the end of the log. Note that if the
> > +			 * header already wrapped, blk_no could point past the
> > +			 * end of the log. The record data is contiguous in
> > +			 * that case.
> > +			 */
> > +			if (blk_no + bblks <= log->l_logBBsize ||
> > +			    blk_no >= log->l_logBBsize) {
> > +				/* mod blk_no in case the header wrapped and
> > +				 * pushed it beyond the end of the log */
> > +				error = xlog_bread(log,
> > +						   blk_no % log->l_logBBsize,
> 
> I /think/ this is ok, though isn't this 64-bit division?  ^
> 

Yep, looks like it. Will fix.. thanks for catching that..

Brian

> --D
> 
> > +						   bblks, dbp, &offset);
> >  				if (error)
> >  					goto bread_err2;
> >  			} else {
> > -- 
> > 2.7.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html