Re: [PATCH 01/10] xfs: reflect sb features in xfs_mount

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 21, 2018 at 09:21:40AM -0400, Brian Foster wrote:
> On Mon, Aug 20, 2018 at 02:48:42PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Currently on-disk feature checks require decoding the superblock
> > fileds and so can be non-trivial. We have almost 400 hundred
> > individual feature checks in the XFS code, so this is a significant
> > amount of code. To reduce runtime check overhead, pre-process all
> > the version flags into a features field in the xfs_mount at mount
> > time so we can convert all the feature checks to a simple flag
> > check.
> > 
> > There is also a need to convert the dynamic feature flags to update
> > the m_features field. This is required for attr, attr2 and quota
> > features. New xfs_mount based wrappers are added for this.
> > 
> > Before:
> > 
> > $ size -t fs/xfs/built-in.a
> >    text	   data	    bss	    dec	    hex	filename
> > ....
> > 1294873	 182766	   1036	1478675	 169013	(TOTALS
> > 
> 
> Was some text truncated from the commit log description here? Did you
> mean to include the after size as well?

Yeah, I thought I did update that. Maybe forgot to refresh the
header once I did. The before and after are:

	text	   data	    bss	    dec	    hex	filename
before	1326155	 189006	   1036	1516197	 1722a5	(TOTALS)
after	1322929	 189006	   1036	1512971	 17160b	(TOTALS)

> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_format.h |  2 +-
> >  fs/xfs/libxfs/xfs_sb.c     | 61 +++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_sb.h     |  1 +
> >  fs/xfs/xfs_log_recover.c   |  1 +
> >  fs/xfs/xfs_mount.c         |  1 +
> >  fs/xfs/xfs_mount.h         | 79 ++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 144 insertions(+), 1 deletion(-)
> > 
> ...
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index a21dc61ec09e..5d0438ec07dd 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -5723,6 +5723,7 @@ xlog_do_recover(
> >  	xfs_buf_relse(bp);
> >  
> >  	/* re-initialise in-core superblock and geometry structures */
> > +	mp->m_features |= xfs_sb_version_to_features(sbp);
> 
> How is this a reinit if it ORs in fields?

The only feature bit that can be removed by log recovery is the
attr2 feature bit which means the last mount was mounted with
"noattr2". So apart from that feature bit, ORing in the new
superblock feature mask works just fine.

As it is, for v5 attr2 can't be turned off, so it's just fine for
v5 filesystems. For v4 the old code would set XFS_MOUNT_ATTR2 if the
sb feature bit was set /prior/ to log recovery being run. Hence if
log recovery removed the feature bit, the very next attr creation
will turn it straight back on.

The only way to not have attr2 enabled in this case is to use the
noattr2 mount option, and the new code has exactly the same
behaviour - the attr2 superblock bit and the feature flag will get
cleared after log recovery if the noattr2 mount option is set.

Also worth noting is that if there's a sb_bad_features2
interatction, it will re-add the feature bit because it just ORs the
two fields together.

IOWs, the behaviour of this patch is roughly the same as the
existing code when it comes to the attr2 flag being removed when the
superblock is recovered as well as when there is a features2
mismatch.

I think there's more work needed on the attr2 feature side of
things, as noted in the cover description. In addition to the
unpredictable behaviour via log recovery, I don't see a reason for
noattr2 existing these days. It doesn't get rid of existing attr2
format inodes on disk - it just stops new ones from being created.
We've used attr2 for more than 10 years now without it being an
issue, so there's no reason for needing to turn it off anymore. I
think we should deprecate the option and remove it.

> I guess I'm curious why we OR
> in fields in either case as opposed to using an assignment.

Because mount option features are already set in m_features, so we
can't just overwrite it with just the superblock features.

> 
> >  	xfs_reinit_percpu_counters(mp);
> >  	error = xfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
> >  	if (error) {
> ...
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 7964513c3128..92d947f17c69 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -127,6 +127,7 @@ typedef struct xfs_mount {
> >  	struct mutex		m_growlock;	/* growfs mutex */
> >  	int			m_fixedfsid[2];	/* unchanged for life of FS */
> >  	uint64_t		m_flags;	/* global mount flags */
> > +	uint64_t		m_features;	/* active filesystem features */
> >  	bool			m_inotbt_nores; /* no per-AG finobt resv. */
> >  	int			m_ialloc_inos;	/* inodes in inode allocation */
> >  	int			m_ialloc_blks;	/* blocks in inode allocation */
> > @@ -195,6 +196,84 @@ typedef struct xfs_mount {
> >  #endif
> >  } xfs_mount_t;
> >  
> > +/*
> > + * Flags for m_features.
> > + *
> > + * These are all the active features in the filesystem, regardless of how
> > + * they are configured.
> > + */
> > +#define	XFS_FEAT_ATTR		(1ULL << 0)	/* xattrs present in fs */
> > +#define	XFS_FEAT_NLINK		(1ULL << 1)	/* 32 bit link counts */
> > +#define	XFS_FEAT_QUOTA		(1ULL << 2)	/* quota active */
> > +#define	XFS_FEAT_ALIGN		(1ULL << 3)	/* inode alignment */
> > +#define	XFS_FEAT_DALIGN		(1ULL << 4)	/* data alignment */
> > +#define XFS_FEAT_LOGV2		(1ULL << 5)	/* version 2 logs */
> > +#define XFS_FEAT_SECTOR		(1ULL << 6)	/* sector size > 512 bytes */
> > +#define	XFS_FEAT_EXTFLG		(1ULL << 7)	/* unwritten extents */
> > +#define	XFS_FEAT_ASCIICI	(1ULL << 8)	/* ASCII only case-insens. */
> > +#define XFS_FEAT_LAZYSBCOUNT	(1ULL << 9)	/* Superblk counters */
> > +#define XFS_FEAT_ATTR2		(1ULL << 10)	/* dynamic attr fork */
> > +#define XFS_FEAT_PARENT		(1ULL << 11)	/* parent pointers */
> 
> Hmm, none of the parent pointer stuff has been merged yet, has it? If
> not, I'm sure it will be at some point, but I'd prefer not to include
> code for features that technically don't exist.

We already have the parent point feature bit declared and this is a
direct translation of the superblock feature bits. If it's unused,
then the compiler elides the xfs_has_parent() check function,
otherwise it's just a flag...

> Otherwise it looks like we have some inconsistent spacing/indentation.

Inherited from the superblock feature bit definitions. I'll clean it
up.

> 
> > +#define XFS_FEAT_PROJID32	(1ULL << 12)	/* 32 bit project id */
> > +#define XFS_FEAT_CRC		(1ULL << 13)	/* metadata CRCs */
> > +#define XFS_FEAT_PQUOTINO	(1ULL << 14)	/* non-shared proj/grp quotas */
> > +#define XFS_FEAT_FTYPE		(1ULL << 15)	/* inode type in dir */
> > +#define XFS_FEAT_FINOBT		(1ULL << 16)	/* free inode btree */
> > +#define XFS_FEAT_RMAPBT		(1ULL << 17)	/* reverse map btree */
> > +#define XFS_FEAT_REFLINK	(1ULL << 18)	/* reflinked files */
> > +#define XFS_FEAT_SPINODES	(1ULL << 19)	/* sparse inode chunks */
> > +#define XFS_FEAT_META_UUID	(1ULL << 20)	/* metadata UUID */
> > +#define XFS_FEAT_REALTIME	(1ULL << 21)	/* realtime device present */
> > +
> > +#define __XFS_HAS_FEAT(name, NAME) \
> > +static inline bool xfs_has_ ## name (struct xfs_mount *mp) \
> > +{ \
> 
> I'm assuming these xfs_has_*() calls will end up replacing the current
> xfs_sb_version_has*() calls throughout the code. Could we start using a

Yes.

> consistent function name prefix across the helpers? E.g., I think using
> 'xfs_feat_ ## name' here (instead of xfs_has_) would be fine, and
> consistent with the other helpers.

I'd prefer to drop the _feat_ prefix for the couple of add/remove
wrappers that exist, not add it to every feature check.

> I don't want to get into bikeshedding too much but tbh I've always found
> the xfs_sb_has_* thing kind of weird where the "has" text seems
> superfluous. It's just not been worth changing. It would be nice if we
> could stop propagating it here and define a consistently used prefix.

Yet we use "is" all over the place when checking if an object is
<something> (e.g. xfs_btree_ptr_is_null(), xfs_iext_rec_is_empty(),
xfs_rmap_is_mergeable(), etc) because it makes the code more
readable. The old sbversion namespace format is the problem, not the
use of "has" to indicate we are checking if the filesystem has a
feature...

I omitted the "_feat_" part of the name because it's effectively
redundant when you read it. i.e. if (xfs_has_pquotaino(mp)) reads as
a well composed sentence: "if XFS has project quotas inodes on this
mount". Doing s/has/feature/ does not improve the readbility of the
code, just makes it unnecessarily verbose.




> > +	return mp->m_features & XFS_FEAT_ ## NAME; \
> > +}
> > +
> > +/* Some features can be added dynamically so they need a set wrapper, too. */
> > +#define __XFS_HAS_ADDFEAT(name, NAME) \
> > +	__XFS_HAS_FEAT(name, NAME); \
> > +static inline void xfs_feat_add_ ## name (struct xfs_mount *mp) \
> > +{ \
> > +	mp->m_features |= XFS_FEAT_ ## NAME; \
> > +	xfs_sb_version_add ## name(&mp->m_sb); \
> > +}
> > +
> > +/* Some features can be cleared dynamically so they need a clear wrapper */
> > +#define __XFS_HAS_REMOVEFEAT(name, NAME) \
> > +	__XFS_HAS_ADDFEAT(name, NAME); \
> > +static inline void xfs_feat_remove_ ## name (struct xfs_mount *mp) \
> > +{ \
> > +	mp->m_features &= ~XFS_FEAT_ ## NAME; \
> > +	xfs_sb_version_remove ## name(&mp->m_sb); \
> > +}
> > +
> 
> In addition to the above, we use HAS_FEAT for an underlying xfs_has_
> (i.e., no "feat") helper above, yet for some reason also use the HAS for
> the underlying xfs_feat_add/remove() helpers that don't use the "has"
> wording. On top of that, HAS_ADD implies HAS and HAS_REMOVE implies
> HAS_ADD, which isn't really clear from the naming.

I'll remove the HAS from them - that was just a side effect of a
quick copy-n-paste.

> I think this would all be much clearer if we defined something like the
> following:
> 
> 	__XFS_FEAT -> xfs_feat_*
> 	__XFS_FEAT_ADD -> xfs_feat_add_*
> 	__XFS_FEAT_REMOVE -> xfs_feat_remove_*

Adding and removing features dynamically is the exception rather
than the common case. I want the common case to be simple and easy,
and I'd much prefer we special case the implementation of the rare,
unusual corner cases rather than make the common case more complex.

> ... for the individual helpers (i.e., no implicit dependencies) and then
> just open-coded the appropriate calls in the list below. Alternatively,
> we could create another set of macros like
> XFS_FEAT_[FIXED|ADDONLY|ADDRM]() for the particular combinations that we
> happen to use today. Either way would make it self-evident from the list
> itself what helpers are defined without having to dig into all of the
> macros.

I'll rename the constructor macros so it's clearer.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com