From: Chandra Seetharaman <sekharan@us.ibm.com>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 21/21] xfs: add CRC checks to the superblock
Date: Tue, 26 Mar 2013 15:58:47 -0500 [thread overview]
Message-ID: <1364331527.32345.135.camel@chandra-dt.ibm.com> (raw)
In-Reply-To: <1363091454-8852-22-git-send-email-david@fromorbit.com>
On Tue, 2013-03-12 at 23:30 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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@redhat.com>
> ---
> 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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next prev parent reply other threads:[~2013-03-26 20:59 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-12 12:30 [PATCH 00/21] xfs: metadata CRCs, third version Dave Chinner
2013-03-12 12:30 ` [PATCH 01/21] xfs: ensure we capture IO errors correctly Dave Chinner
2013-03-12 12:30 ` [PATCH 02/21] xfs: increase hexdump output in xfs_corruption_error Dave Chinner
2013-03-14 21:18 ` Ben Myers
2013-03-15 1:13 ` Dave Chinner
2013-03-12 12:30 ` [PATCH 03/21] xfs: take inode version into account in XFS_LITINO Dave Chinner
2013-03-12 12:30 ` [PATCH 04/21] xfs: add support for large btree blocks Dave Chinner
2013-03-12 12:30 ` [PATCH 05/21] xfs: add CRC checks to the AGF Dave Chinner
2013-03-12 12:30 ` [PATCH 06/21] xfs: add CRC checks to the AGFL Dave Chinner
2013-03-12 12:30 ` [PATCH 07/21] xfs: add CRC checks to the AGI Dave Chinner
2013-03-12 12:30 ` [PATCH 08/21] xfs: add CRC checks for quota blocks Dave Chinner
2013-03-12 12:30 ` [PATCH 09/21] xfs: add version 3 inode format with CRCs Dave Chinner
2013-03-14 16:03 ` Ben Myers
2013-03-14 19:01 ` Ben Myers
2013-03-15 1:11 ` Dave Chinner
2013-03-26 22:56 ` Dave Chinner
2013-03-27 0:53 ` Ben Myers
2013-03-27 1:48 ` Dave Chinner
2013-04-02 22:44 ` Ben Myers
2013-04-03 4:08 ` Dave Chinner
2013-04-02 22:49 ` Ben Myers
2013-03-12 12:30 ` [PATCH 10/21] xfs: add CRC checks to remote symlinks Dave Chinner
2013-03-20 21:14 ` Ben Myers
2013-03-21 1:22 ` Dave Chinner
2013-03-21 14:59 ` Ben Myers
2013-03-20 22:03 ` Ben Myers
2013-03-21 1:32 ` Dave Chinner
2013-03-12 12:30 ` [PATCH 11/21] xfs: add CRC checks to block format directory blocks Dave Chinner
2013-03-26 18:39 ` Ben Myers
2013-03-26 21:40 ` Dave Chinner
2013-03-12 12:30 ` [PATCH 12/21] xfs: add CRC checking to dir2 free blocks Dave Chinner
2013-03-28 23:40 ` Ben Myers
2013-03-29 3:13 ` Dave Chinner
2013-03-12 12:30 ` [PATCH 13/21] xfs: add CRC checking to dir2 data blocks Dave Chinner
2013-04-03 22:13 ` Ben Myers
2013-03-12 12:30 ` [PATCH 14/21] xfs: add CRC checking to dir2 leaf blocks Dave Chinner
2013-03-12 12:30 ` [PATCH 15/21] xfs: shortform directory offsets change for dir3 format Dave Chinner
2013-03-12 12:30 ` [PATCH 16/21] xfs: add CRCs to dir2/da node blocks Dave Chinner
2013-03-12 12:30 ` [PATCH 17/21] xfs: add CRCs to attr leaf blocks Dave Chinner
2013-03-12 12:30 ` [PATCH 18/21] xfs: split remote attribute code out Dave Chinner
2013-03-12 12:30 ` [PATCH 19/21] xfs: add CRC protection to remote attributes Dave Chinner
2013-03-12 12:30 ` [PATCH 20/21] xfs: add buffer types to directory and attribute buffers Dave Chinner
2013-03-12 12:30 ` [PATCH 21/21] xfs: add CRC checks to the superblock Dave Chinner
2013-03-26 20:58 ` Chandra Seetharaman [this message]
2013-03-27 1:06 ` Dave Chinner
2013-03-27 23:07 ` Chandra Seetharaman
2013-03-28 1:36 ` Dave Chinner
2013-03-12 12:43 ` [PATCH 22/21] xfs: Fix magic number assert in xfs_dir3_leaf_log_bests Dave Chinner
2013-03-13 0:29 ` [PATCH 23/21] xfs: fix endian issues reported by sparse Dave Chinner
2013-03-13 1:34 ` [PATCH 24/21] xfs: buffer type overruns blf_flags field Dave Chinner
2013-03-14 21:41 ` [PATCH 00/21] xfs: metadata CRCs, third version Ben Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1364331527.32345.135.camel@chandra-dt.ibm.com \
--to=sekharan@us.ibm.com \
--cc=david@fromorbit.com \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox