| Message ID | 20240318225406.3378998-3-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: drop SB_I_VERSION support | expand |
On Tue, Mar 19, 2024 at 09:51:01AM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Now that we don't support SB_I_VERSION, completely internalise the > remaining usage of inode->i_version. We use our own internal change > counter now, and leave inode->i_version completely unused. This > grows the xfs_inode by 8 bytes, but also allows us to use a normal > uint64_t rather than an expensive atomic64_t for the counter. > > This clears the way for implementing different inode->i_version > functionality in the future whilst still maintaining the internal > XFS change counters as they currently stand. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_inode_buf.c | 7 ++----- > fs/xfs/libxfs/xfs_trans_inode.c | 5 +---- > fs/xfs/xfs_icache.c | 4 ---- > fs/xfs/xfs_inode.c | 4 +--- > fs/xfs/xfs_inode.h | 1 + > fs/xfs/xfs_inode_item.c | 4 +--- > fs/xfs/xfs_iops.c | 1 - > 7 files changed, 6 insertions(+), 20 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > index 68989f4bf793..cadd8be83cc4 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.c > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > @@ -20,8 +20,6 @@ > #include "xfs_dir2.h" > #include "xfs_health.h" > > -#include <linux/iversion.h> > - > /* > * If we are doing readahead on an inode buffer, we might be in log recovery > * reading an inode allocation buffer that hasn't yet been replayed, and hence > @@ -244,8 +242,7 @@ xfs_inode_from_disk( > xfs_iflags_set(ip, XFS_IPRESERVE_DM_FIELDS); > > if (xfs_has_v3inodes(ip->i_mount)) { > - inode_set_iversion_queried(inode, > - be64_to_cpu(from->di_changecount)); > + ip->i_changecount = be64_to_cpu(from->di_changecount); > ip->i_crtime = xfs_inode_from_disk_ts(from, from->di_crtime); > ip->i_diflags2 = be64_to_cpu(from->di_flags2); > ip->i_cowextsize = be32_to_cpu(from->di_cowextsize); > @@ -339,7 +336,7 @@ xfs_inode_to_disk( > > if (xfs_has_v3inodes(ip->i_mount)) { > to->di_version = 3; > - to->di_changecount = cpu_to_be64(inode_peek_iversion(inode)); > + to->di_changecount = cpu_to_be64(ip->i_changecount); > to->di_crtime = xfs_inode_to_disk_ts(ip, ip->i_crtime); > to->di_flags2 = cpu_to_be64(ip->i_diflags2); > to->di_cowextsize = cpu_to_be32(ip->i_cowextsize); > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c > index b82f9c7ff2d5..f9196eff6bab 100644 > --- a/fs/xfs/libxfs/xfs_trans_inode.c > +++ b/fs/xfs/libxfs/xfs_trans_inode.c > @@ -15,8 +15,6 @@ > #include "xfs_trans_priv.h" > #include "xfs_inode_item.h" > > -#include <linux/iversion.h> > - > /* > * Add a locked inode to the transaction. > * > @@ -87,7 +85,6 @@ xfs_trans_log_inode( > uint flags) > { > struct xfs_inode_log_item *iip = ip->i_itemp; > - struct inode *inode = VFS_I(ip); > > ASSERT(iip); > xfs_assert_ilocked(ip, XFS_ILOCK_EXCL); > @@ -101,7 +98,7 @@ xfs_trans_log_inode( > */ > if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) && > xfs_has_crc(ip->i_mount)) { > - atomic64_inc(&inode->i_version); > + ip->i_changecount++; > flags |= XFS_ILOG_IVERSION; > } > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index 74f1812b03cb..6c87b90754c4 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -26,8 +26,6 @@ > #include "xfs_log_priv.h" > #include "xfs_health.h" > > -#include <linux/iversion.h> > - > /* Radix tree tags for incore inode tree. */ > > /* inode is to be reclaimed */ > @@ -309,7 +307,6 @@ xfs_reinit_inode( > int error; > uint32_t nlink = inode->i_nlink; > uint32_t generation = inode->i_generation; > - uint64_t version = inode_peek_iversion(inode); > umode_t mode = inode->i_mode; > dev_t dev = inode->i_rdev; > kuid_t uid = inode->i_uid; > @@ -319,7 +316,6 @@ xfs_reinit_inode( > > set_nlink(inode, nlink); > inode->i_generation = generation; > - inode_set_iversion_queried(inode, version); > inode->i_mode = mode; > inode->i_rdev = dev; > inode->i_uid = uid; > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index e7a724270423..3ca8e905dbd4 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -3,8 +3,6 @@ > * Copyright (c) 2000-2006 Silicon Graphics, Inc. > * All Rights Reserved. > */ > -#include <linux/iversion.h> > - > #include "xfs.h" > #include "xfs_fs.h" > #include "xfs_shared.h" > @@ -828,7 +826,7 @@ xfs_init_new_inode( > ip->i_diflags = 0; > > if (xfs_has_v3inodes(mp)) { > - inode_set_iversion(inode, 1); > + ip->i_changecount = 1; > ip->i_cowextsize = 0; > ip->i_crtime = tv; > } > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index ab46ffb3ac19..0f9d32cbae72 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -42,6 +42,7 @@ typedef struct xfs_inode { > struct rw_semaphore i_lock; /* inode lock */ > atomic_t i_pincount; /* inode pin count */ > struct llist_node i_gclist; /* deferred inactivation list */ > + uint64_t i_changecount; /* # of attribute changes */ Now that we've separated this from i_version, should we export this via bulkstat or something? The code itself looks fine so Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > /* > * Bitsets of inode metadata that have been checked and/or are sick. > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > index f28d653300d1..9ec88a84edfa 100644 > --- a/fs/xfs/xfs_inode_item.c > +++ b/fs/xfs/xfs_inode_item.c > @@ -21,8 +21,6 @@ > #include "xfs_error.h" > #include "xfs_rtbitmap.h" > > -#include <linux/iversion.h> > - > struct kmem_cache *xfs_ili_cache; /* inode log item */ > > static inline struct xfs_inode_log_item *INODE_ITEM(struct xfs_log_item *lip) > @@ -546,7 +544,7 @@ xfs_inode_to_log_dinode( > > if (xfs_has_v3inodes(ip->i_mount)) { > to->di_version = 3; > - to->di_changecount = inode_peek_iversion(inode); > + to->di_changecount = ip->i_changecount; > to->di_crtime = xfs_inode_to_log_dinode_ts(ip, ip->i_crtime); > to->di_flags2 = ip->i_diflags2; > to->di_cowextsize = ip->i_cowextsize; > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index 3940ad1ee66e..8a145ca7d380 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -28,7 +28,6 @@ > > #include <linux/posix_acl.h> > #include <linux/security.h> > -#include <linux/iversion.h> > #include <linux/fiemap.h> > > /* > -- > 2.43.0 > >
Is there much of a good reason to do this now vs oing it whenverer we actually sort out i_version semantics? It adds size to the inode and I don't see how the atomics actually cost us much. The patch itself does looks good, though.
On Tue, Mar 19, 2024 at 04:06:28PM -0700, Christoph Hellwig wrote: > Is there much of a good reason to do this now vs oing it whenverer > we actually sort out i_version semantics? It adds size to the inode > and I don't see how the atomics actually cost us much. The patch > itself does looks good, though. I /think/ it's safe because all the callers I found gated their inode*inc_iversion calls on IS_I_VERSION(), but as di_changecount is now a totally internal inode attribute, I don't think it should be out where the VFS can mess with it. --D
On Tue, Mar 19, 2024 at 04:06:28PM -0700, Christoph Hellwig wrote: > Is there much of a good reason to do this now vs oing it whenverer > we actually sort out i_version semantics? It adds size to the inode > and I don't see how the atomics actually cost us much. The patch > itself does looks good, though. I'm kinda with Darrick on this - yes, it burns an extra 8 bytes per inode, but it doesn't leave us exposed to someone misusing the inode->i_version field. I also don't like the idea of leaving the disablement half done - that just makes it more work for whoever comes later to do something with this again... Cheers, Dave.
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index 68989f4bf793..cadd8be83cc4 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -20,8 +20,6 @@ #include "xfs_dir2.h" #include "xfs_health.h" -#include <linux/iversion.h> - /* * If we are doing readahead on an inode buffer, we might be in log recovery * reading an inode allocation buffer that hasn't yet been replayed, and hence @@ -244,8 +242,7 @@ xfs_inode_from_disk( xfs_iflags_set(ip, XFS_IPRESERVE_DM_FIELDS); if (xfs_has_v3inodes(ip->i_mount)) { - inode_set_iversion_queried(inode, - be64_to_cpu(from->di_changecount)); + ip->i_changecount = be64_to_cpu(from->di_changecount); ip->i_crtime = xfs_inode_from_disk_ts(from, from->di_crtime); ip->i_diflags2 = be64_to_cpu(from->di_flags2); ip->i_cowextsize = be32_to_cpu(from->di_cowextsize); @@ -339,7 +336,7 @@ xfs_inode_to_disk( if (xfs_has_v3inodes(ip->i_mount)) { to->di_version = 3; - to->di_changecount = cpu_to_be64(inode_peek_iversion(inode)); + to->di_changecount = cpu_to_be64(ip->i_changecount); to->di_crtime = xfs_inode_to_disk_ts(ip, ip->i_crtime); to->di_flags2 = cpu_to_be64(ip->i_diflags2); to->di_cowextsize = cpu_to_be32(ip->i_cowextsize); diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c index b82f9c7ff2d5..f9196eff6bab 100644 --- a/fs/xfs/libxfs/xfs_trans_inode.c +++ b/fs/xfs/libxfs/xfs_trans_inode.c @@ -15,8 +15,6 @@ #include "xfs_trans_priv.h" #include "xfs_inode_item.h" -#include <linux/iversion.h> - /* * Add a locked inode to the transaction. * @@ -87,7 +85,6 @@ xfs_trans_log_inode( uint flags) { struct xfs_inode_log_item *iip = ip->i_itemp; - struct inode *inode = VFS_I(ip); ASSERT(iip); xfs_assert_ilocked(ip, XFS_ILOCK_EXCL); @@ -101,7 +98,7 @@ xfs_trans_log_inode( */ if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) && xfs_has_crc(ip->i_mount)) { - atomic64_inc(&inode->i_version); + ip->i_changecount++; flags |= XFS_ILOG_IVERSION; } diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index 74f1812b03cb..6c87b90754c4 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -26,8 +26,6 @@ #include "xfs_log_priv.h" #include "xfs_health.h" -#include <linux/iversion.h> - /* Radix tree tags for incore inode tree. */ /* inode is to be reclaimed */ @@ -309,7 +307,6 @@ xfs_reinit_inode( int error; uint32_t nlink = inode->i_nlink; uint32_t generation = inode->i_generation; - uint64_t version = inode_peek_iversion(inode); umode_t mode = inode->i_mode; dev_t dev = inode->i_rdev; kuid_t uid = inode->i_uid; @@ -319,7 +316,6 @@ xfs_reinit_inode( set_nlink(inode, nlink); inode->i_generation = generation; - inode_set_iversion_queried(inode, version); inode->i_mode = mode; inode->i_rdev = dev; inode->i_uid = uid; diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index e7a724270423..3ca8e905dbd4 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -3,8 +3,6 @@ * Copyright (c) 2000-2006 Silicon Graphics, Inc. * All Rights Reserved. */ -#include <linux/iversion.h> - #include "xfs.h" #include "xfs_fs.h" #include "xfs_shared.h" @@ -828,7 +826,7 @@ xfs_init_new_inode( ip->i_diflags = 0; if (xfs_has_v3inodes(mp)) { - inode_set_iversion(inode, 1); + ip->i_changecount = 1; ip->i_cowextsize = 0; ip->i_crtime = tv; } diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index ab46ffb3ac19..0f9d32cbae72 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -42,6 +42,7 @@ typedef struct xfs_inode { struct rw_semaphore i_lock; /* inode lock */ atomic_t i_pincount; /* inode pin count */ struct llist_node i_gclist; /* deferred inactivation list */ + uint64_t i_changecount; /* # of attribute changes */ /* * Bitsets of inode metadata that have been checked and/or are sick. diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c index f28d653300d1..9ec88a84edfa 100644 --- a/fs/xfs/xfs_inode_item.c +++ b/fs/xfs/xfs_inode_item.c @@ -21,8 +21,6 @@ #include "xfs_error.h" #include "xfs_rtbitmap.h" -#include <linux/iversion.h> - struct kmem_cache *xfs_ili_cache; /* inode log item */ static inline struct xfs_inode_log_item *INODE_ITEM(struct xfs_log_item *lip) @@ -546,7 +544,7 @@ xfs_inode_to_log_dinode( if (xfs_has_v3inodes(ip->i_mount)) { to->di_version = 3; - to->di_changecount = inode_peek_iversion(inode); + to->di_changecount = ip->i_changecount; to->di_crtime = xfs_inode_to_log_dinode_ts(ip, ip->i_crtime); to->di_flags2 = ip->i_diflags2; to->di_cowextsize = ip->i_cowextsize; diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 3940ad1ee66e..8a145ca7d380 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -28,7 +28,6 @@ #include <linux/posix_acl.h> #include <linux/security.h> -#include <linux/iversion.h> #include <linux/fiemap.h> /*