Re: [PATCH] xfs: use log_incompat feature instead of speculate matching

[Gao Xiang <hsiangkao@redhat.com>]

Hi Darrick,

On Tue, Aug 25, 2020 at 07:54:58AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 25, 2020 at 06:06:01PM +0800, Gao Xiang wrote:
> > Add a log_incompat (v5) or sb_features2 (v4) feature
> > of a single long iunlinked list just to be safe. Hence,
> > older kernels will refuse to replay log for v5 images
> > or mount entirely for v4 images.
> > 
> > If the current mount is in RO state, it will defer
> > to the next RW (re)mount to add such flag instead.
> 
> This commit log needs to state /why/ we need a new feature flag in
> addition to summarizing what is being added here.  For example,
> 
> "Introduce a new feature flag to collapse the unlinked hash to a single
> bucket.  Doing so removes the need to lock the AGI in addition to the
> previous and next items in the unlinked list.  Older kernels will think
> that inodes are in the wrong unlinked hash bucket and declare the fs
> corrupt, so the new feature is needed to prevent them from touching the
> filesystem."
> 
> (or whatever the real reason is, I'm attending DebConf and LPC and
> wasn't following 100%...)
> 
> Note that the above was a guess, because I actually can't tell if this
> feature is needed to prevent old kernels from tripping over our new
> strategy, or to prevent new kernels from running off the road if an old
> kernel wrote all the hash buckets.  I would've thought both cases would
> be fine...?

To prevent old kernels from tripping over our new strategy.

Images generated by old kernels would be fine.

> 

...

> >  #define	XFS_SB_VERSION2_OKBITS		\
> >  	(XFS_SB_VERSION2_LAZYSBCOUNTBIT	| \
> >  	 XFS_SB_VERSION2_ATTR2BIT	| \
> >  	 XFS_SB_VERSION2_PROJID32BIT	| \
> > -	 XFS_SB_VERSION2_FTYPE)
> > +	 XFS_SB_VERSION2_FTYPE		| \
> > +	 XFS_SB_VERSION2_NEW_IUNLINK)
> 
> NAK on this part; as I said earlier, don't add things to V4 filesystems.
> 
> If the rest of you have compelling reasons to want V4 support, now is
> the time to speak up.

The simple reason is that the current xfs_iunlink() code only generates
unlinked list in the new way but no multiple buckets. So, we must have
a choice for V4 since it's still supported by the current kernel:

 1) add some feature to entirely refuse new v4 images on older kernels;
 2) allow speculate matching so older kernel would bail out as fs corruption
    (but I have no idea if it has any harm);  

> 
> >  /* Maximum size of the xfs filesystem label, no terminating NULL */
> >  #define XFSLABEL_MAX			12
> > @@ -479,7 +481,9 @@ xfs_sb_has_incompat_feature(
> >  	return (sbp->sb_features_incompat & feature) != 0;
> >  }
> >  
> > -#define XFS_SB_FEAT_INCOMPAT_LOG_ALL 0
> > +#define XFS_SB_FEAT_INCOMPAT_LOG_NEW_IUNLINK	(1 << 0)
> > +#define XFS_SB_FEAT_INCOMPAT_LOG_ALL	\
> > +		(XFS_SB_FEAT_INCOMPAT_LOG_NEW_IUNLINK)
> 
> There's a trick here: Define the feature flag at the very start of your
> patchset, then make the last patch in the set add it to the _ALL macro
> so that people bisecting their way through the git tree (with this
> feature turned on) won't unwittingly build a kernel with the feature
> half built and blow their filesystem to pieces.

hmmm... not quite get the point though.
For this specific patch, I think it'll be folded into some patch or
rearranged.

It should not be a followed-up patch (we must do some decision in advance
 -- whether or not add this feature).

> 
> >  #define XFS_SB_FEAT_INCOMPAT_LOG_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_LOG_ALL
> >  static inline bool
> >  xfs_sb_has_incompat_log_feature(
> > @@ -563,6 +567,27 @@ static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
> >  		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
> >  }
> >  
> > +static inline bool xfs_sb_has_new_iunlink(struct xfs_sb *sbp)
> > +{
> > +	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
> > +		return sbp->sb_features_log_incompat &
> > +			XFS_SB_FEAT_INCOMPAT_LOG_NEW_IUNLINK;
> > +
> > +	return xfs_sb_version_hasmorebits(sbp) &&
> > +		(sbp->sb_features2 & XFS_SB_VERSION2_NEW_IUNLINK);
> > +}
> > +
> > +static inline void xfs_sb_add_new_iunlink(struct xfs_sb *sbp)
> > +{
> > +	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
> > +		sbp->sb_features_log_incompat |=
> > +			XFS_SB_FEAT_INCOMPAT_LOG_NEW_IUNLINK;
> > +		return;
> > +	}
> > +	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
> > +	sbp->sb_features2 |= XFS_SB_VERSION2_NEW_IUNLINK;
> 
> All metadata updates need to be logged.  Dave just spent a bunch of time
> heckling me for that in the y2038 patchset. ;)

hmmm... xfs_sync_sb in xfs_mountfs() will generate a sb transaction,
right? I don't get the risk here.

> 
> Also, I don't think it's a good idea to enable new incompat features
> automatically, since this makes the fs unmountable on old kernels.

As I said above, new xfs_iunlink() doesn't support multiple buckets
anymore (just support it for log recovery). So this feature would be
needed.

If supporting old multiple buckets xfs_iunlink() is needed, that's
a quite large modification of this entire patchset.

Thanks,
Gao Xiang