xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 10/13] xfs: add CRC checks to the AGFL
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 11 Feb 2009 18:54:33 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20090210202941.150266000@xxxxxxxxxxxxxxxxxxxxxx>
Mail-followup-to: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
References: <20090210202241.546501000@xxxxxxxxxxxxxxxxxxxxxx> <20090210202941.150266000@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Tue, Feb 10, 2009 at 03:22:51PM -0500, Christoph Hellwig wrote:
> 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.

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))

>  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.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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