Re: [PATCH 3/9] xfs: add CRC checks to the AGF

Ben Myers
2013-02-21T22:53:06Z

Dave,

Just some minor nits.

On Tue, Jan 22, 2013 at 12:25:54AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> THe AGF already has some self identifying fields (e.g. the sequence
The

> number) so we only need to add the uuid to it to identify the
> filesystem it belongs to. The location is fixed based on the
> sequence number, so there's no need to add a block number, either.
>
> Hence the only additional fields are the CRC and LSN fields. These
> are unlogged, so place some space between the end of the logged
> fields and them so that future expansion of the AGF for legged
logged

> fields can be placed adjacent to the existing logged fields and
> hence not complicate the field-derived range based logging we
> currently have.
>
> Based originally on a patch from myself, modified further by
> Christoph Hellwig and then modified again to fit into the
> verifier structure with additional fields by myself. The multiple
> signed-off-by tags indicate the age and history of this patch.
>
> Signed-off-by: Dave Chinner <dgc@sgi.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_ag.h | 21 +++++++++++-
> fs/xfs/xfs_alloc.c | 80 ++++++++++++++++++++++++++++++++--------------
> fs/xfs/xfs_buf_item.h | 4 ++-
> fs/xfs/xfs_fsops.c | 3 ++
> fs/xfs/xfs_log_recover.c | 7 ++++
> 5 files changed, 89 insertions(+), 26 deletions(-)
>
> diff --git a/fs/xfs/xfs_ag.h b/fs/xfs/xfs_ag.h
> index f2aeedb..b382703 100644
> --- a/fs/xfs/xfs_ag.h
> +++ b/fs/xfs/xfs_ag.h
> @@ -63,12 +63,29 @@ typedef struct xfs_agf {
> __be32 agf_spare0; /* spare field */
> __be32 agf_levels[XFS_BTNUM_AGF]; /* btree levels */
> __be32 agf_spare1; /* spare field */
> +
> __be32 agf_flfirst; /* first freelist block's index */
> __be32 agf_fllast; /* last freelist block's index */
> __be32 agf_flcount; /* count of blocks in freelist */
> __be32 agf_freeblks; /* total free blocks */
> +
> __be32 agf_longest; /* longest free space */
> __be32 agf_btreeblks; /* # of blocks held in AGF btrees */
> + uuid_t agf_uuid; /* uuid of filesystem */
> +
> + /*
> + * reserve some contiguous space for future logged fields before we add
> + * the unlogged fields. This makes the range logging via flags and
> + * structure offsets much simpler.
> + */
> + __be64 agf_spare64[16];
> +
> + /* unlogged fields, written during buffer writeback. */
> + __be64 agf_lsn; /* last write sequence */
> + __be32 agf_crc; /* crc of agf sector */
> + __be32 agf_spare2;
> +
> + /* structure must be padded to 64 bit alignment */
> } xfs_agf_t;
>
> #define XFS_AGF_MAGICNUM 0x00000001
> @@ -83,6 +100,7 @@ typedef struct xfs_agf {
> #define XFS_AGF_FREEBLKS 0x00000200
> #define XFS_AGF_LONGEST 0x00000400
> #define XFS_AGF_BTREEBLKS 0x00000800
> +#define XFS_AGF_UUID 0x00001000
> #define XFS_AGF_NUM_BITS 12
13 ?
You added a 13th bit.

How do you envision having the uuid in these structures will be helpful?

Regards,
Ben