On Thu, Oct 22, 2020 at 11:34:32PM -0700, Allison Henderson wrote:
> This patch adds a new feature bit XFS_SB_FEAT_INCOMPAT_LOG_DELATTR which
> can be used to control turning on/off delayed attributes
>
> Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_format.h | 8 ++++++--
> fs/xfs/libxfs/xfs_fs.h | 1 +
> fs/xfs/libxfs/xfs_sb.c | 2 ++
> fs/xfs/xfs_super.c | 3 +++
> 4 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index d419c34..18b41a7 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -483,7 +483,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_DELATTR (1 << 0) /* Delayed Attributes */
> +#define XFS_SB_FEAT_INCOMPAT_LOG_ALL \
> + (XFS_SB_FEAT_INCOMPAT_LOG_DELATTR)
> #define XFS_SB_FEAT_INCOMPAT_LOG_UNKNOWN ~XFS_SB_FEAT_INCOMPAT_LOG_ALL
> static inline bool
> xfs_sb_has_incompat_log_feature(
> @@ -586,7 +588,9 @@ static inline bool xfs_sb_version_hasinobtcounts(struct xfs_sb *sbp)
>
> static inline bool xfs_sb_version_hasdelattr(struct xfs_sb *sbp)
Soooo, something Dave pointed out on IRC this evening --
Log incompat flags exist /only/ to protect the contents of a dirty
journal. They're supposed to get set when you save something to the log
(like the delayed xattr log items) and cleared when the log is replayed
or unmounted cleanly or goes idle. If a new feature changes the disk
format then you'll have to protect that with a new rocompat / compat /
incompat flag, but never a log incompat flag.
Therefore, you can't use log incompat flags to gate higher level
functionality. I don't think delayed attrs themselves have any user
visible effect on the ondisk format outside of the log, right? So I
guess the good news is that's one less hurdle to getting people to use
this feature.
(Aside: in another part of this patchset review I asked if this means we
could drop the INCOMPLETE flag from attr keys. I think you could do
that without needing to add a rocompat / compat / incompat flag, since
an old kernel works fine if it never sees an incomplete flag; and
presumably the new kernel will continue to know how to delete those
things.)
The downside is that no code exists to support log incompat flags. I
guess every time you want to use them you'd potentially have to check
the superblock and log a new superblock to disk with the feature turned
on. I'll have to think about that more later.
I guess for now we'd want to retain the predicate function so that we
could enable it via a seeecret mount option while we stabilize the
feature. Later if we add a new ondisk feature flag that uses the log
item we can change the predicate to return true if that feature flag is
set (e.g. xfs_sb_version_hasdelattr always returns true if parent
pointers are enabled).
Atomic file range swapping falls into the same category, so I guess we
both have things to rework. On the plus side it means that both of our
new features aren't going to require people to upgrade or reformat. :)
--D
> {
> - return false;
> + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
> + (sbp->sb_features_log_incompat &
> + XFS_SB_FEAT_INCOMPAT_LOG_DELATTR));
> }
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index 2a2e3cf..f703d95 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -250,6 +250,7 @@ typedef struct xfs_fsop_resblks {
> #define XFS_FSOP_GEOM_FLAGS_RMAPBT (1 << 19) /* reverse mapping btree */
> #define XFS_FSOP_GEOM_FLAGS_REFLINK (1 << 20) /* files can share blocks */
> #define XFS_FSOP_GEOM_FLAGS_BIGTIME (1 << 21) /* 64-bit nsec timestamps */
> +#define XFS_FSOP_GEOM_FLAGS_DELATTR (1 << 22) /* delayed attributes */
>
> /*
> * Minimum and maximum sizes need for growth checks.
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 5aeafa5..a0ec327 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -1168,6 +1168,8 @@ xfs_fs_geometry(
> geo->flags |= XFS_FSOP_GEOM_FLAGS_REFLINK;
> if (xfs_sb_version_hasbigtime(sbp))
> geo->flags |= XFS_FSOP_GEOM_FLAGS_BIGTIME;
> + if (xfs_sb_version_hasdelattr(sbp))
> + geo->flags |= XFS_FSOP_GEOM_FLAGS_DELATTR;
> if (xfs_sb_version_hassector(sbp))
> geo->logsectsize = sbp->sb_logsectsize;
> else
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d1b5f2d..bb85884 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1580,6 +1580,9 @@ xfs_fc_fill_super(
> if (xfs_sb_version_hasinobtcounts(&mp->m_sb))
> xfs_warn(mp,
> "EXPERIMENTAL inode btree counters feature in use. Use at your own risk!");
> + if (xfs_sb_version_hasdelattr(&mp->m_sb))
> + xfs_alert(mp,
> + "EXPERIMENTAL delayed attrs feature enabled. Use at your own risk!");
>
> error = xfs_mountfs(mp);
> if (error)
> --
> 2.7.4
>
