On Thu, Feb 28, 2013 at 10:20:45AM +1100, Dave Chinner wrote:
> On Wed, Feb 27, 2013 at 04:37:50PM -0600, Ben Myers wrote:
> > Hi Dave,
> >
> > On Tue, Jan 22, 2013 at 12:25:55AM +1100, Dave Chinner wrote:
> > > From: Christoph Hellwig <hch@xxxxxx>
> > >
> > > Add CRC checks, location information and a magic number to the AGFL.
> > > Previously the AGFL was just a block containing nothing but the
> > > free block pointers. The new AGFL has a real header with the usual
> > > boilerplate instead, so that we can verify it's not corrupted and
> > > written into the right place.
> > >
> > > [dchinner@xxxxxxxxxx] Added LSN field, reworked significantly to fit
> > > into new verifier structure and growfs structure, enabled full
> > > verifier functionality now there is a header to verify and we can
> > > guarantee an initialised AGFL.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > I have a couple comments below.
> .....
> > > for (i = 0; i < XFS_AGFL_SIZE(mp); i++) {
> > > - if (be32_to_cpu(agfl->agfl_bno[i]) == NULLAGBLOCK ||
> > > + if (be32_to_cpu(agfl->agfl_bno[i]) != NULLAGBLOCK &&
> > > be32_to_cpu(agfl->agfl_bno[i]) >= mp->m_sb.sb_agblocks)
> > <
>
> No, we are checking for the agbno being out of range here, not in
> range.
Ah. Good.
> The previous code (which was ifdef'd out) reflected the fact that
> NULLAGBLOCK could not appear in a AGFL (initialised to zero, not
> NULLAGBLOCK), For CRC enabled filesystems - where this check is run,
> we guarantee that unused entries are initialised to NULLAGBLOCK by
> mkfs and growfs, and this change reflects that.
>
> > Any non NULLAGBLOCK should be less than m_sb.sb_agblocks, correct?
>
> xfs_agblock_t is an unsigned value, therefore it has a value of
> 0xffffffff. be32-to_cpu() also returns an unsigned value.
> So, no, is it never less than mp->m_sb.sb_agblocks.
>
> But we don't want to rely on an implicit comparison against
> mp->m_sb.sb_agblocks to detect this, and hence we *always* check
> explicitly for it being a NULLAGBLOCK.
>
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index 65c35d5..81d3cc5a 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -1961,6 +1961,13 @@ xlog_recover_do_reg_buffer(
> > > }
> > > bp->b_ops = &xfs_agf_buf_ops;
> > > break;
> > > + case XFS_BLF_AGFL_BUF:
> > > + if (*(__be32 *)bp->b_addr != cpu_to_be32(XFS_AGFL_MAGIC)) {
> > > + xfs_warn(mp, "Bad AGFL block magic!");
> > > + ASSERT(0);
> > > + }
> > > + bp->b_ops = &xfs_agfl_buf_ops;
> > > + break;
> >
> > Your changes for v2 in this section look good.
>
> Actually, the above hunk is broken. The magic number should only be
> checked for CRC enabled filesystems. My current code has this check,
> though I thought I fixed that long before I reposted this series...
I was pointing out that the changes included in v2 of this patch in this
section of code look good.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
