xfs
[Top] [All Lists]

Re: [PATCH 3/3] xfs: prevent needless mount warning causing test failure

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 3/3] xfs: prevent needless mount warning causing test failures
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 9 May 2012 08:42:24 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120508162942.GK16881@xxxxxxx>
References: <1335519922-14371-1-git-send-email-david@xxxxxxxxxxxxx> <1335519922-14371-4-git-send-email-david@xxxxxxxxxxxxx> <20120508162942.GK16881@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, May 08, 2012 at 11:29:42AM -0500, Ben Myers wrote:
> On Fri, Apr 27, 2012 at 07:45:22PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Often mounting small filesystem with small logs will emit a warning
> > such as:
> > 
> > XFS (vdb): Invalid block length (0x2000) for buffer
> > 
> > during log recovery. This causes tests to randomly fail because this
> > output causes the clean filesystem checks on test completion to
> > think the filesystem is inconsistent.
> > 
> > The cause of the error is simply that log recovery is asking for a
> > buffer size that is larger than the log when zeroing the tail. This
> > is because the buffer size is rounded up, and if the right head and
> > tail conditions exist then the buffer size can be larger than the log.
> > Limit the variable size xlog_get_bp() callers to requesting buffers
> > smaller than the log.
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> >  fs/xfs/xfs_log_recover.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index d7abe5f..ca38690 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -441,6 +441,8 @@ xlog_find_verify_cycle(
> >      * a log sector, or we're out of luck.
> >      */
> >     bufblks = 1 << ffs(nbblks);
> > +   while (bufblks > log->l_logBBsize)
> > +           bufblks >>= 1;
> 
> AFAICS you don't need a loop here.  The following would be sufficient to make
> xlog_buf_bbcount_valid return 0. 
> 
> if (bufblks > log->l_logBBsize)
>       bufblks = log->l_logBBsize;

Yes, I could do that, but then there is a different set of boundary
conditions to test. I know that the >>=1 logic works, but I have no
idea what new corner cases occur when bufblks == log->l_logBBsize.

> It is a bit more obviously correct.

It may be to read, but it's certainly more different from a
verification point of view. Given how long and arduous the process
was to find the source of the problem, I am very wary of changing
logic to run in ways that are different and very difficult to
actually test....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>