On Tue, 2013-03-12 at 23:30 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> With the addition of CRCs, there is such a wide and varied change to
> the on disk format that it makes sense to bump the superblock
> version number rather than try to use feature bits for all the new
> functionality.
>
> This commit introduces all the new superblock fields needed for all
> the new functionality: feature masks similar to ext4, separate
> project quota inodes, a LSN field for recovery and the CRC field.
>
> This commit does not bump the superblock version number, however.
> That will be done as a separate commit at the end of the series
> after all the new functionality is present so we switch it all on in
> one commit. This means that we can slowly introduce the changes
> without them being active and hence maintain bisectability of the
> tree.
>
> This patch is based on a patch originally written by myself back
> from SGI days, which was subsequently modified by Christoph Hellwig.
> There is relatively little of that patch remaining, but the history
> of the patch still should be acknowledged here.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_buf_item.h | 4 +-
> fs/xfs/xfs_log_recover.c | 8 ++++
> fs/xfs/xfs_mount.c | 97 ++++++++++++++++++++++++++++++++++++--------
> fs/xfs/xfs_mount.h | 1 +
> fs/xfs/xfs_sb.h | 100 ++++++++++++++++++++++++++++++++--------------
> 5 files changed, 164 insertions(+), 46 deletions(-)
<snip>
> diff --git a/fs/xfs/xfs_sb.h b/fs/xfs/xfs_sb.h
> index a05b451..457fefa 100644
> --- a/fs/xfs/xfs_sb.h
> +++ b/fs/xfs/xfs_sb.h
> @@ -32,6 +32,7 @@ struct xfs_mount;
> #define XFS_SB_VERSION_2 2 /* 6.2 - attributes */
> #define XFS_SB_VERSION_3 3 /* 6.2 - new inode version */
> #define XFS_SB_VERSION_4 4 /* 6.2+ - bitmask version */
> +#define XFS_SB_VERSION_5 5 /* CRC enabled filesystem */
> #define XFS_SB_VERSION_NUMBITS 0x000f
> #define XFS_SB_VERSION_ALLFBITS 0xfff0
> #define XFS_SB_VERSION_SASHFBITS 0xf000
> @@ -161,6 +162,18 @@ typedef struct xfs_sb {
> */
> __uint32_t sb_bad_features2;
>
> + /* version 5 superblock fields start here */
> +
> + /* feature masks */
> + __uint32_t sb_features_compat;
> + __uint32_t sb_features_ro_compat;
> + __uint32_t sb_features_incompat;
> +
> + __uint32_t sb_crc; /* superblock crc */
> +
> + xfs_ino_t sb_pquotino; /* project quota inode */
> + xfs_lsn_t sb_lsn; /* last write sequence */
> +
> /* must be padded to 64 bit alignment */
> } xfs_sb_t;
>
> @@ -229,7 +242,19 @@ typedef struct xfs_dsb {
> * for features2 bits. Easiest just to mark it bad and not use
> * it for anything else.
> */
> - __be32 sb_bad_features2;
> + __be32 sb_bad_features2;
> +
> + /* version 5 superblock fields start here */
> +
> + /* feature masks */
> + __be32 sb_features_compat;
> + __be32 sb_features_ro_compat;
> + __be32 sb_features_incompat;
> +
> + __le32 sb_crc; /* superblock crc */
> +
> + __be64 sb_pquotino; /* project quota inode */
> + __be64 sb_lsn; /* last write sequence */
>
> /* must be padded to 64 bit alignment */
> } xfs_dsb_t;
> @@ -250,7 +275,9 @@ typedef enum {
> XFS_SBS_GQUOTINO, XFS_SBS_QFLAGS, XFS_SBS_FLAGS, XFS_SBS_SHARED_VN,
> XFS_SBS_INOALIGNMT, XFS_SBS_UNIT, XFS_SBS_WIDTH, XFS_SBS_DIRBLKLOG,
> XFS_SBS_LOGSECTLOG, XFS_SBS_LOGSECTSIZE, XFS_SBS_LOGSUNIT,
> - XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2,
> + XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2, XFS_SBS_FEATURES_COMPAT,
> + XFS_SBS_FEATURES_RO_COMPAT, XFS_SBS_FEATURES_INCOMPAT, XFS_SBS_CRC,
> + XFS_SBS_PQUOTINO, XFS_SBS_LSN,
> XFS_SBS_FIELDCOUNT
> } xfs_sb_field_t;
>
> @@ -276,6 +303,11 @@ typedef enum {
> #define XFS_SB_FDBLOCKS XFS_SB_MVAL(FDBLOCKS)
> #define XFS_SB_FEATURES2 XFS_SB_MVAL(FEATURES2)
> #define XFS_SB_BAD_FEATURES2 XFS_SB_MVAL(BAD_FEATURES2)
> +#define XFS_SB_FEATURES_COMPAT XFS_SB_MVAL(FEATURES_COMPAT)
> +#define XFS_SB_FEATURES_RO_COMPAT XFS_SB_MVAL(FEATURES_RO_COMPAT)
> +#define XFS_SB_FEATURES_INCOMPAT XFS_SB_MVAL(FEATURES_INCOMPAT)
> +#define XFS_SB_CRC XFS_SB_MVAL(CRC)
> +#define XFS_SB_PQUOTINO XFS_SB_MVAL(PQUOTINO)
missing define for XFS_SB_LSN ?!
> #define XFS_SB_NUM_BITS ((int)XFS_SBS_FIELDCOUNT)
> #define XFS_SB_ALL_BITS ((1LL << XFS_SB_NUM_BITS) - 1)
> #define XFS_SB_MOD_BITS \
> @@ -283,7 +315,8 @@ typedef enum {
> XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO | XFS_SB_GQUOTINO | \
> XFS_SB_QFLAGS | XFS_SB_SHARED_VN | XFS_SB_UNIT | XFS_SB_WIDTH | \
> XFS_SB_ICOUNT | XFS_SB_IFREE | XFS_SB_FDBLOCKS | XFS_SB_FEATURES2 | \
> - XFS_SB_BAD_FEATURES2)
> + XFS_SB_BAD_FEATURES2 | XFS_SB_FEATURES_COMPAT | \
> + XFS_SB_FEATURES_RO_COMPAT | XFS_SB_FEATURES_INCOMPAT | XFS_SB_PQUOTINO)
missing XFS_SB_LSN ?!
>
>
> /*
> @@ -325,6 +358,8 @@ static inline int xfs_sb_good_version(xfs_sb_t *sbp)
>
> return 1;
> }
> + if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
> + return 1;
>
> return 0;
> }
> @@ -365,7 +400,7 @@ static inline int xfs_sb_version_hasattr(xfs_sb_t *sbp)
> {
> return sbp->sb_versionnum == XFS_SB_VERSION_2 ||
> sbp->sb_versionnum == XFS_SB_VERSION_3 ||
> - (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> (sbp->sb_versionnum & XFS_SB_VERSION_ATTRBIT));
> }
Do you expect version 5 and later have this bit to be exclusively set to
use attribues ? (i.e can't we just assume version 5 and later would have
attributes ?)
Since we are changing the version, can't we get rid of some on these
bits (i.e slowly deprecate them) ?
>
> @@ -373,7 +408,7 @@ static inline void xfs_sb_version_addattr(xfs_sb_t *sbp)
> {
> if (sbp->sb_versionnum == XFS_SB_VERSION_1)
> sbp->sb_versionnum = XFS_SB_VERSION_2;
> - else if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4)
> + else if (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4)
> sbp->sb_versionnum |= XFS_SB_VERSION_ATTRBIT;
> else
> sbp->sb_versionnum = XFS_SB_VERSION_4 | XFS_SB_VERSION_ATTRBIT;
> @@ -382,7 +417,7 @@ static inline void xfs_sb_version_addattr(xfs_sb_t *sbp)
> static inline int xfs_sb_version_hasnlink(xfs_sb_t *sbp)
> {
> return sbp->sb_versionnum == XFS_SB_VERSION_3 ||
> - (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> (sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT));
Same here
> }
>
> @@ -396,13 +431,13 @@ static inline void xfs_sb_version_addnlink(xfs_sb_t *sbp)
>
> static inline int xfs_sb_version_hasquota(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + return XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> (sbp->sb_versionnum & XFS_SB_VERSION_QUOTABIT);
same here
> }
>
> static inline void xfs_sb_version_addquota(xfs_sb_t *sbp)
> {
> - if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4)
> + if (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4)
> sbp->sb_versionnum |= XFS_SB_VERSION_QUOTABIT;
> else
> sbp->sb_versionnum = xfs_sb_version_tonew(sbp->sb_versionnum) |
> @@ -411,13 +446,14 @@ static inline void xfs_sb_version_addquota(xfs_sb_t *sbp)
>
> static inline int xfs_sb_version_hasalign(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> - (sbp->sb_versionnum & XFS_SB_VERSION_ALIGNBIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> + (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> + (sbp->sb_versionnum & XFS_SB_VERSION_ALIGNBIT));
> }
same here
>
> static inline int xfs_sb_version_hasdalign(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + return XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> (sbp->sb_versionnum & XFS_SB_VERSION_DALIGNBIT);
> }
same here
>
> @@ -429,38 +465,42 @@ static inline int xfs_sb_version_hasshared(xfs_sb_t *sbp)
>
> static inline int xfs_sb_version_hasdirv2(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> - (sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
Why some places we have version == 5 and others (version == 5 && (XXX))
> + (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + (sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT));
> }
>
> static inline int xfs_sb_version_haslogv2(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> - (sbp->sb_versionnum & XFS_SB_VERSION_LOGV2BIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> + (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> + (sbp->sb_versionnum & XFS_SB_VERSION_LOGV2BIT));
> }
>
> static inline int xfs_sb_version_hasextflgbit(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> - (sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> + (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + (sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT));
> }
>
> static inline int xfs_sb_version_hassector(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + return XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> (sbp->sb_versionnum & XFS_SB_VERSION_SECTORBIT);
> }
>
> static inline int xfs_sb_version_hasasciici(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + return XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> (sbp->sb_versionnum & XFS_SB_VERSION_BORGBIT);
> }
same here ?!
>
> static inline int xfs_sb_version_hasmorebits(xfs_sb_t *sbp)
> {
> - return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> - (sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> + (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> + (sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT));
Gets more confusing here. version 5 means it has more bits ? (even if
the MOREBITSBIT is not set ?
May be we shouldn't add version 5 check here ?!
> }
>
> /*
> @@ -475,14 +515,16 @@ static inline int xfs_sb_version_hasmorebits(xfs_sb_t *sbp)
>
> static inline int xfs_sb_version_haslazysbcount(xfs_sb_t *sbp)
> {
> - return xfs_sb_version_hasmorebits(sbp) &&
> - (sbp->sb_features2 & XFS_SB_VERSION2_LAZYSBCOUNTBIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> + (xfs_sb_version_hasmorebits(sbp) &&
> + (sbp->sb_features2 & XFS_SB_VERSION2_LAZYSBCOUNTBIT));
> }
If we remove the version 5 check in xfs_sb_hasmorebits(), then this
change is correct. Otherwise, this change is not needed.
>
> static inline int xfs_sb_version_hasattr2(xfs_sb_t *sbp)
> {
> - return xfs_sb_version_hasmorebits(sbp) &&
> - (sbp->sb_features2 & XFS_SB_VERSION2_ATTR2BIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> + (xfs_sb_version_hasmorebits(sbp) &&
> + (sbp->sb_features2 & XFS_SB_VERSION2_ATTR2BIT));
> }
If we remove the version 5 check in xfs_sb_hasmorebits(), then this
change is correct. Otherwise, this change is not needed.
>
> static inline void xfs_sb_version_addattr2(xfs_sb_t *sbp)
> @@ -500,14 +542,14 @@ static inline void xfs_sb_version_removeattr2(xfs_sb_t *sbp)
>
> static inline int xfs_sb_version_hasprojid32bit(xfs_sb_t *sbp)
> {
> - return xfs_sb_version_hasmorebits(sbp) &&
> - (sbp->sb_features2 & XFS_SB_VERSION2_PROJID32BIT);
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> + (xfs_sb_version_hasmorebits(sbp) &&
> + (sbp->sb_features2 & XFS_SB_VERSION2_PROJID32BIT));
> }
If we remove the version 5 check in xfs_sb_hasmorebits(), then this
change is correct. Otherwise, this change is not needed.
>
> static inline int xfs_sb_version_hascrc(xfs_sb_t *sbp)
> {
> - return (xfs_sb_version_hasmorebits(sbp) &&
> - (sbp->sb_features2 & XFS_SB_VERSION2_CRCBIT));
> + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
what happens to the superblocks that already have version 4 and this bit
set ?
Also don't xfs_sb_version_toold() and xfs_sb_version_tonew() need
changes ?
I see that xfs_sb_version_toold() is not used anywhere, can we remove
it ?
> }
>
> /*
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
