linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/16 v2] xfs: rework feature flags
@ 2021-08-10  5:24 Dave Chinner
  2021-08-10  5:24 ` [PATCH 01/16] xfs: sb verifier doesn't handle uncached sb buffer Dave Chinner
                   ` (15 more replies)
  0 siblings, 16 replies; 38+ messages in thread
From: Dave Chinner @ 2021-08-10  5:24 UTC (permalink / raw)
  To: linux-xfs

With the shutdown rework, it became very clear that we needed to
make atomic state changes to the mount so that shutdown would be run
once and once only. To do this, we used the mp->m_sb_lock as a
serialisation mechanism rather than introduce a new single use
spinlock for this purpose.

However, what we really need to is to separate the operational state
changes from static feature flag information kept in the mp->m_flags
field. This would allow the m_flags field to remain largely
read-only, and we can make the operational state use atomic bit
operations to set, check and clear the current state.

This separation between state and features was done for the log as
part of the shutdown cleanup work. Reworking the way the xfs mount
feature flags are used is a much bigger undertaking, hence this
separate patchset.

One of the big things we need to address is that features for the
xfs mount can come from multiple sources. They can come from on-disk
state via flags in the superblock, mount options, proc and sysfs
variables, and so on. Each different mechanism has it's own special
way of setting what is effectively read-only boolean state in the
xfs mount that is then checked at runtime by executing code.

In some cases, these boolean checks can be expensive because we have
to do multiple cheaks and mask variables in the superblock to get
the flag information. The naming can be verbose, and the combination
of open coded flag checks vs wrapper based flag checks does little
to improve the readability of the code.

To clean all this up, introduce a m_features field and a m_opstate
field. The m_features field holds all the features the filesysetm
has enabled or disabled, and the m_opstate field holds all the
atomic operational state. m_features is currently a 64 bit variable
with on-disk features starting at bit 0 counting up and mount
features starting at bit 63 and counting down. At the end of the
series, we have roughly 26 on-disk and 16 mount feature flags used,
so there's still plenty of flag space available for future
additions.

The result of moving all the feature flags to the mount is that we
get rid of all the xfs_sb_version_has() wrappers in
libxfs/xfs_format.h. We really want this file to contain the on-disk
format defintion, not code used to access or interpret it. This gets
rid of a large amount of boiler plate wrappers from this file and
replaces them with mount features checks which are much simpler and
lower overhead.

Getting rid of all the sueprblock feature checks reduces the code
size by about 5kB on x86-64. There are about 400
xfs_sb_version_has() feature checks in the code, so saving a few
instructions on every check ends up making a substantial difference
to code size. It also means this patchset is rather large....

There are a few cleanups needed before the patch set starts. We need
to fix up the attr2/noattr2 mount option/superblock bit issues, as
well as properly namespace some internal attribute code so we can
use the global "xfs_" namespace for global feature and operational
state functions.

There are many further cleanups that can be done following on from
this patch set. e.g the xfs_mount has several boolean state/flag
fields that can be moved into the m_opstate and/or m_features
variables, we can shadow state and or features into the log fields
so the log doesn't need to access the xfs_mount to check current
state, runtime quota state can move into the m_opstate field instead
of needing separate flags, etc.

This passes fstests for all the simple configurations (defaults,
quotas, different directory block sizes, etc) without regressions,
so nothing has been obviously broken. There may be corner cases that
I haven't exercises where issues are present, but so far I haven't
found anything.

Version 2:
- rebased on 5.14-rc4 + for-next + "xfs: strictly order log start records"
- added comment about not using bp->b_bn directly for the buffer disk address.
- fix typos in commit messages
- Fixed incorrect feature flag macro definitions in original introduction patch
  rather than fixing them in a subsequent patch.
- Updated open coded superblock checks to check sparse inodes directly instead
  of via xfs_sb_has_incompat_feature().
- use xfs_has_v3inodes() consistently in inode size/version checks.

Version 1:
- https://lore.kernel.org/linux-xfs/20210714041912.2625692-1-david@fromorbit.com/
- based on 5.14-rc1 + "xfs: strictly order log start records"


^ permalink raw reply	[flat|nested] 38+ messages in thread
* [PATCH 00/16 v3] xfs: rework feature flags
@ 2021-08-18 23:59 Dave Chinner
  2021-08-18 23:59 ` [PATCH 02/16] xfs: rename xfs_has_attr() Dave Chinner
  0 siblings, 1 reply; 38+ messages in thread
From: Dave Chinner @ 2021-08-18 23:59 UTC (permalink / raw)
  To: linux-xfs

With the shutdown rework, it became very clear that we needed to
make atomic state changes to the mount so that shutdown would be run
once and once only. To do this, we used the mp->m_sb_lock as a
serialisation mechanism rather than introduce a new single use
spinlock for this purpose.

However, what we really need to is to separate the operational state
changes from static feature flag information kept in the mp->m_flags
field. This would allow the m_flags field to remain largely
read-only, and we can make the operational state use atomic bit
operations to set, check and clear the current state.

This separation between state and features was done for the log as
part of the shutdown cleanup work. Reworking the way the xfs mount
feature flags are used is a much bigger undertaking, hence this
separate patchset.

One of the big things we need to address is that features for the
xfs mount can come from multiple sources. They can come from on-disk
state via flags in the superblock, mount options, proc and sysfs
variables, and so on. Each different mechanism has it's own special
way of setting what is effectively read-only boolean state in the
xfs mount that is then checked at runtime by executing code.

In some cases, these boolean checks can be expensive because we have
to do multiple cheaks and mask variables in the superblock to get
the flag information. The naming can be verbose, and the combination
of open coded flag checks vs wrapper based flag checks does little
to improve the readability of the code. 

To clean all this up, introduce a m_features field and a m_opstate
field. The m_features field holds all the features the filesysetm
has enabled or disabled, and the m_opstate field holds all the
atomic operational state. m_features is currently a 64 bit variable
with on-disk features starting at bit 0 counting up and mount
features starting at bit 63 and counting down. At the end of the
series, we have roughly 26 on-disk and 16 mount feature flags used,
so there's still plenty of flag space available for future
additions.

The result of moving all the feature flags to the mount is that we
get rid of all the xfs_sb_version_has() wrappers in
libxfs/xfs_format.h. We really want this file to contain the on-disk
format defintion, not code used to access or interpret it. This gets
rid of a large amount of boiler plate wrappers from this file and
replaces them with mount features checks which are much simpler and
lower overhead.

Getting rid of all the sueprblock feature checks reduces the code
size by about 5kB on x86-64. There are about 400
xfs_sb_version_has() feature checks in the code, so saving a few
instructions on every check ends up making a substantial difference
to code size. It also means this patchset is rather large....

There are a few cleanups needed before the patch set starts. We need
to fix up the attr2/noattr2 mount option/superblock bit issues, as
well as properly namespace some internal attribute code so we can
use the global "xfs_" namespace for global feature and operational
state functions.

There are many further cleanups that can be done following on from
this patch set. e.g the xfs_mount has several boolean state/flag
fields that can be moved into the m_opstate and/or m_features
variables, we can shadow state and or features into the log fields
so the log doesn't need to access the xfs_mount to check current
state, runtime quota state can move into the m_opstate field instead
of needing separate flags, etc.

This passes fstests for all the simple configurations (defaults,
quotas, different directory block sizes, etc) without regressions,
so nothing has been obviously broken. The only obvious issue is
xfs/187, which expects "-o noattr2" to remove feature bits from the
superblock and this behaviour is explicitly removed during this
patchset. It is not clear if xfs/187 is a valid test to perform
anymore now that noattr2 behaves this way.

Version 3:
- only need a single call to xfs_attr_lookup()
- fix rebase mis-merge of attr2 mount flag setting.
- set the realtime feature flag correctly when adding a rt device via growfs
- fixed attr2 upgrade behaviour regressions.

Version 2:
- https://lore.kernel.org/linux-xfs/20210810052451.41578-1-david@fromorbit.com/
- rebased on 5.14-rc4 + for-next + "xfs: strictly order log start records"
- added comment about not using bp->b_bn directly for the buffer disk address.
- fix typos in commit messages
- Fixed incorrect feature flag macro definitions in original introduction patch
  rather than fixing them in a subsequent patch.
- Updated open coded superblock checks to check sparse inodes directly instead
  of via xfs_sb_has_incompat_feature().
- use xfs_has_v3inodes() consistently in inode size/version checks.

Version 1:
- https://lore.kernel.org/linux-xfs/20210714041912.2625692-1-david@fromorbit.com/
- based on 5.14-rc1 + "xfs: strictly order log start records"


^ permalink raw reply	[flat|nested] 38+ messages in thread
* [PATCH 00/16] xfs: rework feature flags
@ 2021-07-14  4:18 Dave Chinner
  2021-07-14  4:18 ` [PATCH 02/16] xfs: rename xfs_has_attr() Dave Chinner
  0 siblings, 1 reply; 38+ messages in thread
From: Dave Chinner @ 2021-07-14  4:18 UTC (permalink / raw)
  To: linux-xfs

With the shutdown rework, it became very clear that we needed to
make atomic state changes to the mount so that shutdown would be run
once and once only. To do this, we used the mp->m_sb_lock as a
serialisation mechanism rather than introduce a new single use
spinlock for this purpose.

However, what we really need to is to separate the operational state
changes from static feature flag information kept in the mp->m_flags
field. This would allow the m_flags field to remain largely
read-only, and we can make the operational state use atomic bit
operations to set, check and clear the current state.

This separation between state and features was done for the log as
part of the shutdown cleanup work. Reworking the way the xfs mount
feature flags are used is a much bigger undertaking, hence this
separate patchset.

One of the big things we need to address is that features for the
xfs mount can come from multiple sources. They can come from on-disk
state via flags in the superblock, mount options, proc and sysfs
variables, and so on. Each different mechanism has it's own special
way of setting what is effectively read-only boolean state in the
xfs mount that is then checked at runtime by executing code.

In some cases, these boolean checks can be expensive because we have
to do multiple cheaks and mask variables in the superblock to get
the flag information. The naming can be verbose, and the combination
of open coded flag checks vs wrapper based flag checks does little
to improve the readability of the code. 

To clean all this up, introduce a m_features field and a m_opstate
field. The m_features field holds all the features the filesysetm
has enabled or disabled, and the m_opstate field holds all the
atomic operational state. m_features is currently a 64 bit variable
with on-disk features starting at bit 0 counting up and mount
features starting at bit 63 and counting down. At the end of the
series, we have roughly 26 on-disk and 16 mount feature flags used,
so there's still plenty of flag space available for future
additions.

The result of moving all the feature flags to the mount is that we
get rid of all the xfs_sb_version_has() wrappers in
libxfs/xfs_format.h. We really want this file to contain the on-disk
format defintion, not code used to access or interpret it. This gets
rid of a large amount of boiler plate wrappers from this file and
replaces them with mount features checks which are much simpler and
lower overhead.

Getting rid of all the sueprblock feature checks reduces the code
size by about 5kB on x86-64. There are about 400
xfs_sb_version_has() feature checks in the code, so saving a few
instructions on every check ends up making a substantial difference
to code size. It also means this patchset is rather large....

There are a few cleanups needed before the patch set starts. We need
to fix up the attr2/noattr2 mount option/superblock bit issues, as
well as properly namespace some internal attribute code so we can
use the global "xfs_" namespace for global feature and operational
state functions.

There are many further cleanups that can be done following on from
this patch set. e.g the xfs_mount has several boolean state/flag
fields that can be moved into the m_opstate and/or m_features
variables, we can shadow state and or features into the log fields
so the log doesn't need to access the xfs_mount to check current
state, runtime quota state can move into the m_opstate field instead
of needing separate flags, etc.

This passes fstests for all the simple configurations (defaults,
quotas, different directory block sizes, etc) without regressions,
so nothing has been obviously broken. There may be corner cases that
I haven't exercises where issues are present, but so far I haven't
found anything.

Version 1:
- based on 5.14-rc1 + "xfs: strictly order log start records"


^ permalink raw reply	[flat|nested] 38+ messages in thread
end of thread, other threads:[~2021-08-18 23:59 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10  5:24 [PATCH 00/16 v2] xfs: rework feature flags Dave Chinner
2021-08-10  5:24 ` [PATCH 01/16] xfs: sb verifier doesn't handle uncached sb buffer Dave Chinner
2021-08-11  0:34   ` Darrick J. Wong
2021-08-12  7:52   ` Christoph Hellwig
2021-08-10  5:24 ` [PATCH 02/16] xfs: rename xfs_has_attr() Dave Chinner
2021-08-12  8:03   ` Christoph Hellwig
2021-08-18  0:56     ` Dave Chinner
2021-08-18  2:56       ` Darrick J. Wong
2021-08-10  5:24 ` [PATCH 03/16] xfs: rework attr2 feature and mount options Dave Chinner
2021-08-11 23:04   ` Darrick J. Wong
2021-08-18  1:18     ` Dave Chinner
2021-08-12  0:27   ` Darrick J. Wong
2021-08-18  1:25     ` Dave Chinner
2021-08-10  5:24 ` [PATCH 04/16] xfs: reflect sb features in xfs_mount Dave Chinner
2021-08-10  5:24 ` [PATCH 05/16] xfs: replace xfs_sb_version checks with feature flag checks Dave Chinner
2021-08-11 22:13   ` Darrick J. Wong
2021-08-18  1:33     ` Dave Chinner
2021-08-10  5:24 ` [PATCH 06/16] xfs: consolidate mount option features in m_features Dave Chinner
2021-08-12  8:07   ` Christoph Hellwig
2021-08-10  5:24 ` [PATCH 07/16] xfs: convert mount flags to features Dave Chinner
2021-08-11  0:38   ` Darrick J. Wong
2021-08-11 23:28     ` Darrick J. Wong
2021-08-18  2:25       ` Dave Chinner
2021-08-10  5:24 ` [PATCH 08/16] xfs: convert remaining mount flags to state flags Dave Chinner
2021-08-10  5:24 ` [PATCH 09/16] xfs: replace XFS_FORCED_SHUTDOWN with xfs_is_shutdown Dave Chinner
2021-08-10  5:24 ` [PATCH 10/16] xfs: convert xfs_fs_geometry to use mount feature checks Dave Chinner
2021-08-11  0:39   ` Darrick J. Wong
2021-08-10  5:24 ` [PATCH 11/16] xfs: open code sb verifier " Dave Chinner
2021-08-11  0:48   ` Darrick J. Wong
2021-08-10  5:24 ` [PATCH 12/16] xfs: convert scrub to use mount-based " Dave Chinner
2021-08-10  5:24 ` [PATCH 13/16] xfs: convert xfs_sb_version_has checks to use mount features Dave Chinner
2021-08-10  5:24 ` [PATCH 14/16] xfs: remove unused xfs_sb_version_has wrappers Dave Chinner
2021-08-10  5:24 ` [PATCH 15/16] xfs: introduce xfs_sb_is_v5 helper Dave Chinner
2021-08-10  5:24 ` [PATCH 16/16] xfs: kill xfs_sb_version_has_v3inode() Dave Chinner
  -- strict thread matches above, loose matches on Subject: below --
2021-08-18 23:59 [PATCH 00/16 v3] xfs: rework feature flags Dave Chinner
2021-08-18 23:59 ` [PATCH 02/16] xfs: rename xfs_has_attr() Dave Chinner
2021-07-14  4:18 [PATCH 00/16] xfs: rework feature flags Dave Chinner
2021-07-14  4:18 ` [PATCH 02/16] xfs: rename xfs_has_attr() Dave Chinner
2021-07-14  6:49   ` Christoph Hellwig
2021-07-14 22:46   ` Darrick J. Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).