Re: [PATCH v13 07/10] xfs: Add feature bit XFS_SB_FEAT_INCOMPAT_LOG_DELATTR

Allison Henderson
2020-11-19T04:02:22Z

On 11/18/20 7:36 PM, Darrick J. Wong wrote:
> 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@oracle.com>
>> ---
>> 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.
>
Yeah, I saw the scroll back history later, but it looked like folks
might have retired for the evening. So maybe I can set this bit in the
item create/committed call backs then.

> (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.)
Yes, I saw it. I figure I could just sort of add a check to see if
delattrs are on/off, and skip over it if delayed attrs are on.

>
> 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.
oh ok then. maybe we should make a stand alone patch since we'll both
need it

>
> 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).
Ok, i will look into a secret mount opt then :-)

>
> 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. :)
Yeah, at least we got it figured out before we did that tho :-)

Allison
>
> --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
>>