xfs
[Top] [All Lists]

Re: [PATCH 10/13] xfs: add CRC checks to the AGFL

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 10/13] xfs: add CRC checks to the AGFL
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sun, 15 Feb 2009 13:23:25 -0500
In-reply-to: <20090211075433.GQ8830@disturbed>
References: <20090210202241.546501000@xxxxxxxxxxxxxxxxxxxxxx> <20090210202941.150266000@xxxxxxxxxxxxxxxxxxxxxx> <20090211075433.GQ8830@disturbed>
User-agent: Mutt/1.5.18 (2008-05-17)
On Wed, Feb 11, 2009 at 06:54:33PM +1100, Dave Chinner wrote:
> I haven't had a chance to look over any of this series in detail - just
> a quick glance really - but this popped out as looking wrong:
> 
> > +/*
> > + * Size of the AGFL.  For CRC-enabled filesystes we steal the last two 
> > slots
> > + * for location information (agno) and the crc.
> > + */
> > +#define XFS_AGFL_SIZE(mp) \
> > +   ((mp)->m_sb.sb_sectsize / sizeof(xfs_agblock_t) - \
> > +    (xfs_sb_version_hascrc(&((mp)->m_sb)) ? sizeof(struct xfs_agfl) : 0))
> 
> sb_sectsize is in bytes, sizeof(xfs_agblock_t) is in bytes, which
> means that the numerator is a count of the number of agblocks that
> will fit in the AGFL. sizeof(struct xfs_agfl) is also in bytes,
> so your subtracting a number of bytes from a count of agblocks....
> 
> I think you mean:
> 
> #define XFS_AGFL_SIZE(mp) \
>       (((mp)->m_sb.sb_sectsize - (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
>                                               sizeof(struct xfs_agfl) : 0)) \
>         / sizeof(xfs_agblock_t))

Yeah.  Either that, or dividing the sizeof by four.

> >  typedef struct xfs_agfl {
> > -   __be32          agfl_bno[1];    /* actually XFS_AGFL_SIZE(mp) */
> > +   __be32          agfl_magicnum;
> > +   __be32          agfl_seqno;
> > +   uuid_t          agfl_uuid;
> > +   __be32          agfl_crc;
> > +   __be32          agfl_bno[];     /* actually XFS_AGFL_SIZE(mp) */
> >  } xfs_agfl_t;
> 
> And judging by that you are stealing more than 2 slots - more like
> 8 slots.

Yeah, two slots was the very first implementation, I'll fix up the
comment too.

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