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
