[RFC, PATCH] xfs: make superblock version checks reflect reality

[RFC, PATCH] xfs: make superblock version checks reflect reality

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

We only support filesystems that have v2 directory support, and than
means all the checking and handling of superblock versions prior to
this support being added is completely unnecessary overhead.

Strip out all the version 1-3 support, sanitise the good version
checking to reflect the supported versions, update all the feature
supported functions and clean up all the support bit definitions to
reflect the fact that we no longer care about Irix bootloader flag
regions for v4 feature bits.

Because the feature bit checking is all inline code, this relatively
small cleanup has a noticable impact on code size:

   text    data     bss     dec     hex filename
 802836   99339     632  902807   dc697 fs/xfs/xfs.o.orig
 801620   99339     632  901591   dc1d7 fs/xfs/xfs.o.patched

i.e. it reduces it by more than 1200 bytes.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_sb.h | 175 ++++++++++++++++++++++----------------------------------
 1 file changed, 67 insertions(+), 108 deletions(-)

diff --git a/fs/xfs/xfs_sb.h b/fs/xfs/xfs_sb.h
index f7b2fe7..1fe9818 100644
--- a/fs/xfs/xfs_sb.h
+++ b/fs/xfs/xfs_sb.h
@@ -36,8 +36,6 @@ struct xfs_trans;
 #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
-#define	XFS_SB_VERSION_REALFBITS	0x0ff0
 #define	XFS_SB_VERSION_ATTRBIT		0x0010
 #define	XFS_SB_VERSION_NLINKBIT		0x0020
 #define	XFS_SB_VERSION_QUOTABIT		0x0040
@@ -50,24 +48,26 @@ struct xfs_trans;
 #define	XFS_SB_VERSION_DIRV2BIT		0x2000
 #define	XFS_SB_VERSION_BORGBIT		0x4000	/* ASCII only case-insens. */
 #define	XFS_SB_VERSION_MOREBITSBIT	0x8000
-#define	XFS_SB_VERSION_OKSASHFBITS	\
-	(XFS_SB_VERSION_EXTFLGBIT | \
-	 XFS_SB_VERSION_DIRV2BIT | \
-	 XFS_SB_VERSION_BORGBIT)
-#define	XFS_SB_VERSION_OKREALFBITS	\
-	(XFS_SB_VERSION_ATTRBIT | \
-	 XFS_SB_VERSION_NLINKBIT | \
-	 XFS_SB_VERSION_QUOTABIT | \
-	 XFS_SB_VERSION_ALIGNBIT | \
-	 XFS_SB_VERSION_DALIGNBIT | \
-	 XFS_SB_VERSION_SHAREDBIT | \
-	 XFS_SB_VERSION_LOGV2BIT | \
-	 XFS_SB_VERSION_SECTORBIT | \
+
+/*
+ * We only support superblocks that have at least V2 Dir capability. Any feature
+ * bit added after v2 dir capability is also indicates a supported superblock
+ * format.
+ */
+#define XFS_SB_NEEDED_FEATURES		\
+	(XFS_SB_VERSION_DIRV2BIT	| \
+	 XFS_SB_VERSION_LOGV2BIT	| \
+	 XFS_SB_VERSION_SECTORBIT	| \
+	 XFS_SB_VERSION_BORGBIT		| \
 	 XFS_SB_VERSION_MOREBITSBIT)
-#define	XFS_SB_VERSION_OKREALBITS	\
-	(XFS_SB_VERSION_NUMBITS | \
-	 XFS_SB_VERSION_OKREALFBITS | \
-	 XFS_SB_VERSION_OKSASHFBITS)
+
+/*
+ * Supported feature bit list is just all bits in the versionnum field because
+ * we've used them all up and understand them all.
+ */
+#define	XFS_SB_VERSION_OKBITS		\
+	(XFS_SB_VERSION_NUMBITS		| \
+	 XFS_SB_VERSION_ALLFBITS)
 
 /*
  * There are two words to hold XFS "feature" bits: the original
@@ -76,7 +76,6 @@ struct xfs_trans;
  *
  * These defines represent bits in sb_features2.
  */
-#define XFS_SB_VERSION2_REALFBITS	0x00ffffff	/* Mask: features */
 #define XFS_SB_VERSION2_RESERVED1BIT	0x00000001
 #define XFS_SB_VERSION2_LAZYSBCOUNTBIT	0x00000002	/* Superblk counters */
 #define XFS_SB_VERSION2_RESERVED4BIT	0x00000004
@@ -86,16 +85,11 @@ struct xfs_trans;
 #define XFS_SB_VERSION2_CRCBIT		0x00000100	/* metadata CRCs */
 #define XFS_SB_VERSION2_FTYPE		0x00000200	/* inode type in dir */
 
-#define	XFS_SB_VERSION2_OKREALFBITS	\
+#define	XFS_SB_VERSION2_OKBITS		\
 	(XFS_SB_VERSION2_LAZYSBCOUNTBIT	| \
 	 XFS_SB_VERSION2_ATTR2BIT	| \
 	 XFS_SB_VERSION2_PROJID32BIT	| \
 	 XFS_SB_VERSION2_FTYPE)
-#define	XFS_SB_VERSION2_OKSASHFBITS	\
-	(0)
-#define XFS_SB_VERSION2_OKREALBITS	\
-	(XFS_SB_VERSION2_OKREALFBITS |	\
-	 XFS_SB_VERSION2_OKSASHFBITS )
 
 /*
  * Superblock - in core version.  Must match the ondisk version below.
@@ -345,28 +339,40 @@ typedef enum {
 
 #define	XFS_SB_VERSION_NUM(sbp)	((sbp)->sb_versionnum & XFS_SB_VERSION_NUMBITS)
 
+/*
+ * The first XFS version we support is filesytsems with V2 directories.
+ */
 static inline int xfs_sb_good_version(xfs_sb_t *sbp)
 {
-	/* We always support version 1-3 */
-	if (sbp->sb_versionnum >= XFS_SB_VERSION_1 &&
-	    sbp->sb_versionnum <= XFS_SB_VERSION_3)
+	/* We only support v4 and v5 */
+	if (XFS_SB_VERSION_NUM(sbp) < XFS_SB_VERSION_4 ||
+	    XFS_SB_VERSION_NUM(sbp) > XFS_SB_VERSION_5)
+		return 0;
+
+	/*
+	 * Version 5 feature checks are done separately.
+	 */
+	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
 		return 1;
 
-	/* We support version 4 if all feature bits are supported */
-	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4) {
-		if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKREALBITS) ||
-		    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
-		     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKREALBITS)))
-			return 0;
+	/*
+	 * We only support version 4 for if at least one of the needed feature
+	 * bits are set and all the feature bits are supported.
+	 */
+	ASSERT(XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4);
+
+	if (!(sbp->sb_versionnum & XFS_SB_NEEDED_FEATURES))
+		return 0;
 
-		if (sbp->sb_shared_vn > XFS_SB_MAX_SHARED_VN)
+	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
+	    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
+	     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
 			return 0;
-		return 1;
-	}
-	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
-		return 1;
 
-	return 0;
+	if (sbp->sb_shared_vn > XFS_SB_MAX_SHARED_VN)
+		return 0;
+
+	return 1;
 }
 
 /*
@@ -378,88 +384,45 @@ static inline int xfs_sb_has_mismatched_features2(xfs_sb_t *sbp)
 	return (sbp->sb_bad_features2 != sbp->sb_features2);
 }
 
-static inline unsigned xfs_sb_version_tonew(unsigned v)
-{
-	if (v == XFS_SB_VERSION_1)
-		return XFS_SB_VERSION_4;
-
-	if (v == XFS_SB_VERSION_2)
-		return XFS_SB_VERSION_4 | XFS_SB_VERSION_ATTRBIT;
-
-	return XFS_SB_VERSION_4 | XFS_SB_VERSION_ATTRBIT |
-		XFS_SB_VERSION_NLINKBIT;
-}
-
-static inline unsigned xfs_sb_version_toold(unsigned v)
-{
-	if (v & (XFS_SB_VERSION_QUOTABIT | XFS_SB_VERSION_ALIGNBIT))
-		return 0;
-	if (v & XFS_SB_VERSION_NLINKBIT)
-		return XFS_SB_VERSION_3;
-	if (v & XFS_SB_VERSION_ATTRBIT)
-		return XFS_SB_VERSION_2;
-	return XFS_SB_VERSION_1;
-}
-
 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 &&
-		 (sbp->sb_versionnum & XFS_SB_VERSION_ATTRBIT));
+	return !!(sbp->sb_versionnum & XFS_SB_VERSION_ATTRBIT);
 }
 
 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)
-		sbp->sb_versionnum |= XFS_SB_VERSION_ATTRBIT;
-	else
-		sbp->sb_versionnum = XFS_SB_VERSION_4 | XFS_SB_VERSION_ATTRBIT;
+	sbp->sb_versionnum |= XFS_SB_VERSION_ATTRBIT;
 }
 
 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 &&
-		  (sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT));
+	return !!(sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT);
 }
 
 static inline void xfs_sb_version_addnlink(xfs_sb_t *sbp)
 {
-	if (sbp->sb_versionnum <= XFS_SB_VERSION_2)
-		sbp->sb_versionnum = XFS_SB_VERSION_3;
-	else
-		sbp->sb_versionnum |= XFS_SB_VERSION_NLINKBIT;
+	sbp->sb_versionnum |= XFS_SB_VERSION_NLINKBIT;
 }
 
 static inline int xfs_sb_version_hasquota(xfs_sb_t *sbp)
 {
-	return XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
-		(sbp->sb_versionnum & XFS_SB_VERSION_QUOTABIT);
+	return !!(sbp->sb_versionnum & XFS_SB_VERSION_QUOTABIT);
 }
 
 static inline void xfs_sb_version_addquota(xfs_sb_t *sbp)
 {
-	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) |
-					XFS_SB_VERSION_QUOTABIT;
+	sbp->sb_versionnum |= XFS_SB_VERSION_QUOTABIT;
 }
 
 static inline int xfs_sb_version_hasalign(xfs_sb_t *sbp)
 {
-	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
-	       (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
+	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
 		(sbp->sb_versionnum & XFS_SB_VERSION_ALIGNBIT));
 }
 
 static inline int xfs_sb_version_hasdalign(xfs_sb_t *sbp)
 {
-	return XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
-		(sbp->sb_versionnum & XFS_SB_VERSION_DALIGNBIT);
+	return !!(sbp->sb_versionnum & XFS_SB_VERSION_DALIGNBIT);
 }
 
 static inline int xfs_sb_version_hasshared(xfs_sb_t *sbp)
@@ -470,42 +433,36 @@ 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_5) ||
-	       (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 ||
+	       (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_5) ||
-	       (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 ||
+	       (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_5) ||
-	       (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 ||
+	       (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 &&
-		(sbp->sb_versionnum & XFS_SB_VERSION_SECTORBIT);
+	return !!(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 &&
-		(sbp->sb_versionnum & XFS_SB_VERSION_BORGBIT);
+	return !!(sbp->sb_versionnum & XFS_SB_VERSION_BORGBIT);
 }
 
 static inline int xfs_sb_version_hasmorebits(xfs_sb_t *sbp)
 {
-	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));
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
+	       (sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT);
 }
 
 /*
@@ -536,11 +493,13 @@ static inline void xfs_sb_version_addattr2(xfs_sb_t *sbp)
 {
 	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
 	sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT;
+	sbp->sb_bad_features2 |= XFS_SB_VERSION2_ATTR2BIT;
 }
 
 static inline void xfs_sb_version_removeattr2(xfs_sb_t *sbp)
 {
 	sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
+	sbp->sb_bad_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
 	if (!sbp->sb_features2)
 		sbp->sb_versionnum &= ~XFS_SB_VERSION_MOREBITSBIT;
 }
-- 
1.9.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC, PATCH] xfs: make superblock version checks reflect reality

[Christoph Hellwig]

On Thu, Mar 06, 2014 at 05:54:50PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We only support filesystems that have v2 directory support, and than
> means all the checking and handling of superblock versions prior to
> this support being added is completely unnecessary overhead.
> 
> Strip out all the version 1-3 support, sanitise the good version
> checking to reflect the supported versions, update all the feature
> supported functions and clean up all the support bit definitions to
> reflect the fact that we no longer care about Irix bootloader flag
> regions for v4 feature bits.

Good idea in general, I like it.

> Because the feature bit checking is all inline code, this relatively
> small cleanup has a noticable impact on code size:

I initially though moving it out of line might not be a bad idea,
but it seems after your diet it's lean enough to not bother.

> +/*
> + * We only support superblocks that have at least V2 Dir capability. Any feature
> + * bit added after v2 dir capability is also indicates a supported superblock
> + * format.
> + */
> +#define XFS_SB_NEEDED_FEATURES		\
> +	(XFS_SB_VERSION_DIRV2BIT	| \
> +	 XFS_SB_VERSION_LOGV2BIT	| \
> +	 XFS_SB_VERSION_SECTORBIT	| \
> +	 XFS_SB_VERSION_BORGBIT		| \
>  	 XFS_SB_VERSION_MOREBITSBIT)

This seems a bit odd.  Shouldn't we simply check for
XFS_SB_VERSION_DIRV2BIT as we actually rely on that?  What if SGI had
backported any of those other features to some IRIX branch?

I'd vote to kill XFS_SB_NEEDED_FEATURES and just check the dirv2 bit
explicitly.

> +/*
> + * Supported feature bit list is just all bits in the versionnum field because
> + * we've used them all up and understand them all.
> + */
> +#define	XFS_SB_VERSION_OKBITS		\
> +	(XFS_SB_VERSION_NUMBITS		| \
> +	 XFS_SB_VERSION_ALLFBITS)
>  

> +#define	XFS_SB_VERSION2_OKBITS		\
>  	(XFS_SB_VERSION2_LAZYSBCOUNTBIT	| \
>  	 XFS_SB_VERSION2_ATTR2BIT	| \
>  	 XFS_SB_VERSION2_PROJID32BIT	| \
>  	 XFS_SB_VERSION2_FTYPE)

> +/*
> + * The first XFS version we support is filesytsems with V2 directories.
> + */

is a v4 superblock with v2 directories?

Also filesystem is mis-spelled.

>  static inline int xfs_sb_good_version(xfs_sb_t *sbp)
>  {
> +	/* We only support v4 and v5 */
> +	if (XFS_SB_VERSION_NUM(sbp) < XFS_SB_VERSION_4 ||
> +	    XFS_SB_VERSION_NUM(sbp) > XFS_SB_VERSION_5)
> +		return 0;
> +
> +	/*
> +	 * Version 5 feature checks are done separately.
> +	 */
> +	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
>  		return 1;

How about doing this a little different?

static inline int xfs_sb_good_version(struct xfs_sb *sbp)
{
	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
		return 1;
	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4)
		return xfs_sb_good_v4_features(sbp);
	return 0;
}

then move the bulk of the function into xfs_sb_good_v4_features?


> +	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> +	    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
> +	     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
>  			return 0;
> +	if (sbp->sb_shared_vn > XFS_SB_MAX_SHARED_VN)
> +		return 0;

Given that XFS_SB_MAX_SHARED_VN we might as well make that and != 0
check and document that we don't support any shared superblocks.

>  static inline int xfs_sb_version_hasattr(xfs_sb_t *sbp)
>  {
> +	return !!(sbp->sb_versionnum & XFS_SB_VERSION_ATTRBIT);
>  }

Should this become a bool?

>  
>  static inline void xfs_sb_version_addattr(xfs_sb_t *sbp)
>  {
> +	sbp->sb_versionnum |= XFS_SB_VERSION_ATTRBIT;
>  }

I'd rather not keep the wrappers for adding these flags - the callers
already know sb internals, might as well not keep a false abstraction
here.

>  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 &&
> -		  (sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT));
> +	return !!(sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT);
>  }

As we reject filesystems without the nlink bit we should just be able
to kill all code protected by xfs_sb_version_hasnlink checks, shouldn't
we?

>  static inline void xfs_sb_version_addnlink(xfs_sb_t *sbp)
>  {
> +	sbp->sb_versionnum |= XFS_SB_VERSION_NLINKBIT;
>  }

Same for addnlink.

>  static inline int xfs_sb_version_hasdirv2(xfs_sb_t *sbp)
>  {
> -	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_DIRV2BIT));
> +	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
> +	       (sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT);
>  }

dirv2 is another candidate.

> @@ -536,11 +493,13 @@ static inline void xfs_sb_version_addattr2(xfs_sb_t *sbp)
>  {
>  	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
>  	sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT;
> +	sbp->sb_bad_features2 |= XFS_SB_VERSION2_ATTR2BIT;
>  }
>  
>  static inline void xfs_sb_version_removeattr2(xfs_sb_t *sbp)
>  {
>  	sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
> +	sbp->sb_bad_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;

Where is this coming from?  Seems unrelated to the other changes.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC, PATCH] xfs: make superblock version checks reflect reality

[Dave Chinner]

On Thu, Mar 06, 2014 at 10:05:34AM -0800, Christoph Hellwig wrote:
> On Thu, Mar 06, 2014 at 05:54:50PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We only support filesystems that have v2 directory support, and than
> > means all the checking and handling of superblock versions prior to
> > this support being added is completely unnecessary overhead.
> > 
> > Strip out all the version 1-3 support, sanitise the good version
> > checking to reflect the supported versions, update all the feature
> > supported functions and clean up all the support bit definitions to
> > reflect the fact that we no longer care about Irix bootloader flag
> > regions for v4 feature bits.
> 
> Good idea in general, I like it.
> 
> > Because the feature bit checking is all inline code, this relatively
> > small cleanup has a noticable impact on code size:
> 
> I initially though moving it out of line might not be a bad idea,
> but it seems after your diet it's lean enough to not bother.

Yeah, i think that chopping out all the branches from the code makes
it simple enough that it can remain inline.

> > +/*
> > + * We only support superblocks that have at least V2 Dir capability. Any feature
> > + * bit added after v2 dir capability is also indicates a supported superblock
> > + * format.
> > + */
> > +#define XFS_SB_NEEDED_FEATURES		\
> > +	(XFS_SB_VERSION_DIRV2BIT	| \
> > +	 XFS_SB_VERSION_LOGV2BIT	| \
> > +	 XFS_SB_VERSION_SECTORBIT	| \
> > +	 XFS_SB_VERSION_BORGBIT		| \
> >  	 XFS_SB_VERSION_MOREBITSBIT)
> 
> This seems a bit odd.  Shouldn't we simply check for
> XFS_SB_VERSION_DIRV2BIT as we actually rely on that?  What if SGI had
> backported any of those other features to some IRIX branch?

They did. logv2 came from irix, and the borgbit did too. Hence we
can be guaranteed that if any of these bits exist on irix, then
dirv2 fields are present in the superblock, too.

> I'd vote to kill XFS_SB_NEEDED_FEATURES and just check the dirv2 bit
> explicitly.

Ok. The only real reason I did this was in case there's a single bit
error that clears the dirv2 bit, but it still contains other bits
that indicate that the superblock is recent enough that we
understand it's contents and what should bein the fs.  e.g. for
db/repair purposes - if the dir2 bit is not set, but any of the
above bits are set and the m_dirblklog is and it is sane, we can
assume that we've lost the feature bit and repair it.

> > +/*
> > + * Supported feature bit list is just all bits in the versionnum field because
> > + * we've used them all up and understand them all.
> > + */
> > +#define	XFS_SB_VERSION_OKBITS		\
> > +	(XFS_SB_VERSION_NUMBITS		| \
> > +	 XFS_SB_VERSION_ALLFBITS)
> >  
> 
> > +#define	XFS_SB_VERSION2_OKBITS		\
> >  	(XFS_SB_VERSION2_LAZYSBCOUNTBIT	| \
> >  	 XFS_SB_VERSION2_ATTR2BIT	| \
> >  	 XFS_SB_VERSION2_PROJID32BIT	| \
> >  	 XFS_SB_VERSION2_FTYPE)
> 
> > +/*
> > + * The first XFS version we support is filesytsems with V2 directories.
> > + */
> 
> is a v4 superblock with v2 directories?

*nod*

> 
> Also filesystem is mis-spelled.
> 
> >  static inline int xfs_sb_good_version(xfs_sb_t *sbp)
> >  {
> > +	/* We only support v4 and v5 */
> > +	if (XFS_SB_VERSION_NUM(sbp) < XFS_SB_VERSION_4 ||
> > +	    XFS_SB_VERSION_NUM(sbp) > XFS_SB_VERSION_5)
> > +		return 0;
> > +
> > +	/*
> > +	 * Version 5 feature checks are done separately.
> > +	 */
> > +	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
> >  		return 1;
> 
> How about doing this a little different?
> 
> static inline int xfs_sb_good_version(struct xfs_sb *sbp)
> {
> 	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
> 		return 1;
> 	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4)
> 		return xfs_sb_good_v4_features(sbp);
> 	return 0;
> }
> 
> then move the bulk of the function into xfs_sb_good_v4_features?

Makes sense.

> 
> 
> > +	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> > +	    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
> > +	     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
> >  			return 0;
> > +	if (sbp->sb_shared_vn > XFS_SB_MAX_SHARED_VN)
> > +		return 0;
> 
> Given that XFS_SB_MAX_SHARED_VN we might as well make that and != 0
> check and document that we don't support any shared superblocks.

Should I just drop it out of the supported feature matrix and drop
all other checks on that field? That way we can then remove all the
the crap that tries to validate it from xfs_repair, too. I have no
idea what is actually valid for this field, so I think we should
simply drop support of it from everything.

> >  static inline int xfs_sb_version_hasattr(xfs_sb_t *sbp)
> >  {
> > +	return !!(sbp->sb_versionnum & XFS_SB_VERSION_ATTRBIT);
> >  }
> 
> Should this become a bool?

Yeah, I thought about that. Probably should, then the compiler will
add the cast rather than having to use !! to bring it back to a bool
value.

> 
> >  
> >  static inline void xfs_sb_version_addattr(xfs_sb_t *sbp)
> >  {
> > +	sbp->sb_versionnum |= XFS_SB_VERSION_ATTRBIT;
> >  }
> 
> I'd rather not keep the wrappers for adding these flags - the callers
> already know sb internals, might as well not keep a false abstraction
> here.

Ok, but I'd like to keep that to a separate cleanup....

> 
> >  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 &&
> > -		  (sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT));
> > +	return !!(sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT);
> >  }
> 
> As we reject filesystems without the nlink bit we should just be able
> to kill all code protected by xfs_sb_version_hasnlink checks, shouldn't
> we?
> Same for addnlink.
> dirv2 is another candidate.

Yes, that's true - but I think those are good candidates for
separate patches that follow up on this one.

> > @@ -536,11 +493,13 @@ static inline void xfs_sb_version_addattr2(xfs_sb_t *sbp)
> >  {
> >  	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
> >  	sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT;
> > +	sbp->sb_bad_features2 |= XFS_SB_VERSION2_ATTR2BIT;
> >  }
> >  
> >  static inline void xfs_sb_version_removeattr2(xfs_sb_t *sbp)
> >  {
> >  	sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
> > +	sbp->sb_bad_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
> 
> Where is this coming from?  Seems unrelated to the other changes.

I noticed that these feature bits weren't updating the bad_features2
field and keeping it consistent with features2, which they should.
I'll separate this out into another patch.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC, PATCH] xfs: make superblock version checks reflect reality

[Dave Chinner]

On Fri, Mar 07, 2014 at 09:55:41AM +1100, Dave Chinner wrote:
> On Thu, Mar 06, 2014 at 10:05:34AM -0800, Christoph Hellwig wrote:
> > On Thu, Mar 06, 2014 at 05:54:50PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > We only support filesystems that have v2 directory support, and than
> > > means all the checking and handling of superblock versions prior to
> > > this support being added is completely unnecessary overhead.
> > > 
> > > Strip out all the version 1-3 support, sanitise the good version
> > > checking to reflect the supported versions, update all the feature
> > > supported functions and clean up all the support bit definitions to
> > > reflect the fact that we no longer care about Irix bootloader flag
> > > regions for v4 feature bits.
> > 
> > Good idea in general, I like it.
.....
> > >  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 &&
> > > -		  (sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT));
> > > +	return !!(sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT);
> > >  }
> > 
> > As we reject filesystems without the nlink bit we should just be able
> > to kill all code protected by xfs_sb_version_hasnlink checks, shouldn't
> > we?
> > Same for addnlink.
> > dirv2 is another candidate.
> 
> Yes, that's true - but I think those are good candidates for
> separate patches that follow up on this one.

Ok, we don't reject filesystems that don't have the NLINK bit set.
Older filesystems that have only v1 inodes won't have that bit
set, and we didn't set NLINK by default in mkfs until late 2007.
Hence we need to keep some form of NLINK support around.

The alternative is to simply set the bit in the superblock if it is
not set, and then just assume everywhere that it is set and we are
using v2 inodes. That will get rid of the hasnlink/addnlink code
needed to modify the superblock when the link count goes above
MAX_NLINK_1, and will result in filesystems always converting v1
inodes to v2 inodes on writeback of dirty inodes. I don't see a
problem with taking this approach, bt maybe I'm missing something?

For the dirv2 function, it's only used for checking at mount time,
so if that is handled bu xfs_sb_good_version() we no longer need
it....

Your thoughts?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC, PATCH] xfs: make superblock version checks reflect reality

[Christoph Hellwig]

On Fri, Mar 07, 2014 at 09:55:41AM +1100, Dave Chinner wrote:
> > I'd vote to kill XFS_SB_NEEDED_FEATURES and just check the dirv2 bit
> > explicitly.
> 
> Ok. The only real reason I did this was in case there's a single bit
> error that clears the dirv2 bit, but it still contains other bits
> that indicate that the superblock is recent enough that we
> understand it's contents and what should bein the fs.  e.g. for
> db/repair purposes - if the dir2 bit is not set, but any of the
> above bits are set and the m_dirblklog is and it is sane, we can
> assume that we've lost the feature bit and repair it.

Seems like we should just special case that in repair instead of
allowing a filesystem to go through in the kernel that is guaranteed to
be corrupted.

> Should I just drop it out of the supported feature matrix and drop
> all other checks on that field? That way we can then remove all the
> the crap that tries to validate it from xfs_repair, too. I have no
> idea what is actually valid for this field, so I think we should
> simply drop support of it from everything.

I think we should pretending we know anything about the shared mount
support.  Everytime it came up I failed to find any hint on how it was
supposed to work.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC, PATCH] xfs: make superblock version checks reflect reality

[Christoph Hellwig]

On Fri, Mar 07, 2014 at 07:34:30PM +1100, Dave Chinner wrote:
> Ok, we don't reject filesystems that don't have the NLINK bit set.
> Older filesystems that have only v1 inodes won't have that bit
> set, and we didn't set NLINK by default in mkfs until late 2007.
> Hence we need to keep some form of NLINK support around.
> 
> The alternative is to simply set the bit in the superblock if it is
> not set, and then just assume everywhere that it is set and we are
> using v2 inodes. That will get rid of the hasnlink/addnlink code
> needed to modify the superblock when the link count goes above
> MAX_NLINK_1, and will result in filesystems always converting v1
> inodes to v2 inodes on writeback of dirty inodes. I don't see a
> problem with taking this approach, bt maybe I'm missing something?

I'd love to get rid of v1 support sprinkled all over.  Obviously this
should be a separate patch, but making both the code and the in-use
filesystems less diverse is a good idea.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC, PATCH] xfs: make superblock version checks reflect reality

[Dave Chinner]

On Fri, Mar 07, 2014 at 02:15:27AM -0800, Christoph Hellwig wrote:
> On Fri, Mar 07, 2014 at 09:55:41AM +1100, Dave Chinner wrote:
> > > I'd vote to kill XFS_SB_NEEDED_FEATURES and just check the dirv2 bit
> > > explicitly.
> > 
> > Ok. The only real reason I did this was in case there's a single bit
> > error that clears the dirv2 bit, but it still contains other bits
> > that indicate that the superblock is recent enough that we
> > understand it's contents and what should bein the fs.  e.g. for
> > db/repair purposes - if the dir2 bit is not set, but any of the
> > above bits are set and the m_dirblklog is and it is sane, we can
> > assume that we've lost the feature bit and repair it.
> 
> Seems like we should just special case that in repair instead of
> allowing a filesystem to go through in the kernel that is guaranteed to
> be corrupted.

Ok, that makes a lot of sense. I'll change it to do that.

> > Should I just drop it out of the supported feature matrix and drop
> > all other checks on that field? That way we can then remove all the
> > the crap that tries to validate it from xfs_repair, too. I have no
> > idea what is actually valid for this field, so I think we should
> > simply drop support of it from everything.
> 
> I think we should pretending we know anything about the shared mount
> support.  Everytime it came up I failed to find any hint on how it was
> supposed to work.

*nod*. I'll drop the shared bit from the supported matrix, and also
treat sb_shared_vn != 0 a corruption.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs