| Message ID | 172437087470.59588.4171434021531099837.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/24] xfs: clean up the ISVALID macro in xfs_bmap_adjacent | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Aug 22, 2024 at 05:18:02PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Add a dynamic lockdep class key for rtgroup inodes. This will enable > lockdep to deduce inconsistencies in the rtgroup metadata ILOCK locking > order. Each class can have 8 subclasses, and for now we will only have > 2 inodes per group. This enables rtgroup order and inode order checks > when nesting ILOCKs. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_rtgroup.c | 52 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > > diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c > index 51f04cad5227c..ae6d67c673b1a 100644 > --- a/fs/xfs/libxfs/xfs_rtgroup.c > +++ b/fs/xfs/libxfs/xfs_rtgroup.c > @@ -243,3 +243,55 @@ xfs_rtgroup_trans_join( > if (rtglock_flags & XFS_RTGLOCK_BITMAP) > xfs_rtbitmap_trans_join(tp); > } > + > +#ifdef CONFIG_PROVE_LOCKING > +static struct lock_class_key xfs_rtginode_lock_class; > + > +static int > +xfs_rtginode_ilock_cmp_fn( > + const struct lockdep_map *m1, > + const struct lockdep_map *m2) > +{ > + const struct xfs_inode *ip1 = > + container_of(m1, struct xfs_inode, i_lock.dep_map); > + const struct xfs_inode *ip2 = > + container_of(m2, struct xfs_inode, i_lock.dep_map); > + > + if (ip1->i_projid < ip2->i_projid) > + return -1; > + if (ip1->i_projid > ip2->i_projid) > + return 1; > + return 0; > +} What's the project ID of the inode got to do with realtime groups? -Dave.
On Mon, Aug 26, 2024 at 09:58:05AM +1000, Dave Chinner wrote: > On Thu, Aug 22, 2024 at 05:18:02PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Add a dynamic lockdep class key for rtgroup inodes. This will enable > > lockdep to deduce inconsistencies in the rtgroup metadata ILOCK locking > > order. Each class can have 8 subclasses, and for now we will only have > > 2 inodes per group. This enables rtgroup order and inode order checks > > when nesting ILOCKs. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > fs/xfs/libxfs/xfs_rtgroup.c | 52 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 52 insertions(+) > > > > > > diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c > > index 51f04cad5227c..ae6d67c673b1a 100644 > > --- a/fs/xfs/libxfs/xfs_rtgroup.c > > +++ b/fs/xfs/libxfs/xfs_rtgroup.c > > @@ -243,3 +243,55 @@ xfs_rtgroup_trans_join( > > if (rtglock_flags & XFS_RTGLOCK_BITMAP) > > xfs_rtbitmap_trans_join(tp); > > } > > + > > +#ifdef CONFIG_PROVE_LOCKING > > +static struct lock_class_key xfs_rtginode_lock_class; > > + > > +static int > > +xfs_rtginode_ilock_cmp_fn( > > + const struct lockdep_map *m1, > > + const struct lockdep_map *m2) > > +{ > > + const struct xfs_inode *ip1 = > > + container_of(m1, struct xfs_inode, i_lock.dep_map); > > + const struct xfs_inode *ip2 = > > + container_of(m2, struct xfs_inode, i_lock.dep_map); > > + > > + if (ip1->i_projid < ip2->i_projid) > > + return -1; > > + if (ip1->i_projid > ip2->i_projid) > > + return 1; > > + return 0; > > +} > > What's the project ID of the inode got to do with realtime groups? Each rtgroup metadata file stores its group number in i_projid so that mount can detect if there's a corruption in /rtgroup and we just opened the bitmap from the wrong group. We can also use lockdep to detect code that locks rtgroup metadata in the wrong order. Potentially we could use this _cmp_fn to enforce that we always ILOCK in the order bitmap -> summary -> rmap -> refcount based on i_metatype. --D > -Dave. > -- > Dave Chinner > david@fromorbit.com >
On Mon, Aug 26, 2024 at 02:38:27PM -0700, Darrick J. Wong wrote: > On Mon, Aug 26, 2024 at 09:58:05AM +1000, Dave Chinner wrote: > > On Thu, Aug 22, 2024 at 05:18:02PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > Add a dynamic lockdep class key for rtgroup inodes. This will enable > > > lockdep to deduce inconsistencies in the rtgroup metadata ILOCK locking > > > order. Each class can have 8 subclasses, and for now we will only have > > > 2 inodes per group. This enables rtgroup order and inode order checks > > > when nesting ILOCKs. > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > --- > > > fs/xfs/libxfs/xfs_rtgroup.c | 52 +++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 52 insertions(+) > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c > > > index 51f04cad5227c..ae6d67c673b1a 100644 > > > --- a/fs/xfs/libxfs/xfs_rtgroup.c > > > +++ b/fs/xfs/libxfs/xfs_rtgroup.c > > > @@ -243,3 +243,55 @@ xfs_rtgroup_trans_join( > > > if (rtglock_flags & XFS_RTGLOCK_BITMAP) > > > xfs_rtbitmap_trans_join(tp); > > > } > > > + > > > +#ifdef CONFIG_PROVE_LOCKING > > > +static struct lock_class_key xfs_rtginode_lock_class; > > > + > > > +static int > > > +xfs_rtginode_ilock_cmp_fn( > > > + const struct lockdep_map *m1, > > > + const struct lockdep_map *m2) > > > +{ > > > + const struct xfs_inode *ip1 = > > > + container_of(m1, struct xfs_inode, i_lock.dep_map); > > > + const struct xfs_inode *ip2 = > > > + container_of(m2, struct xfs_inode, i_lock.dep_map); > > > + > > > + if (ip1->i_projid < ip2->i_projid) > > > + return -1; > > > + if (ip1->i_projid > ip2->i_projid) > > > + return 1; > > > + return 0; > > > +} > > > > What's the project ID of the inode got to do with realtime groups? > > Each rtgroup metadata file stores its group number in i_projid so that > mount can detect if there's a corruption in /rtgroup and we just opened > the bitmap from the wrong group. > > We can also use lockdep to detect code that locks rtgroup metadata in > the wrong order. Potentially we could use this _cmp_fn to enforce that > we always ILOCK in the order bitmap -> summary -> rmap -> refcount based > on i_metatype. Ok, can we union the i_projid field (both in memory and in the on-disk structure) so that dual use of the field is well documented by the code? -Dave.
On Tue, Aug 27, 2024 at 10:58:59AM +1000, Dave Chinner wrote: > On Mon, Aug 26, 2024 at 02:38:27PM -0700, Darrick J. Wong wrote: > > On Mon, Aug 26, 2024 at 09:58:05AM +1000, Dave Chinner wrote: > > > On Thu, Aug 22, 2024 at 05:18:02PM -0700, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > > > Add a dynamic lockdep class key for rtgroup inodes. This will enable > > > > lockdep to deduce inconsistencies in the rtgroup metadata ILOCK locking > > > > order. Each class can have 8 subclasses, and for now we will only have > > > > 2 inodes per group. This enables rtgroup order and inode order checks > > > > when nesting ILOCKs. > > > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > > --- > > > > fs/xfs/libxfs/xfs_rtgroup.c | 52 +++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 52 insertions(+) > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c > > > > index 51f04cad5227c..ae6d67c673b1a 100644 > > > > --- a/fs/xfs/libxfs/xfs_rtgroup.c > > > > +++ b/fs/xfs/libxfs/xfs_rtgroup.c > > > > @@ -243,3 +243,55 @@ xfs_rtgroup_trans_join( > > > > if (rtglock_flags & XFS_RTGLOCK_BITMAP) > > > > xfs_rtbitmap_trans_join(tp); > > > > } > > > > + > > > > +#ifdef CONFIG_PROVE_LOCKING > > > > +static struct lock_class_key xfs_rtginode_lock_class; > > > > + > > > > +static int > > > > +xfs_rtginode_ilock_cmp_fn( > > > > + const struct lockdep_map *m1, > > > > + const struct lockdep_map *m2) > > > > +{ > > > > + const struct xfs_inode *ip1 = > > > > + container_of(m1, struct xfs_inode, i_lock.dep_map); > > > > + const struct xfs_inode *ip2 = > > > > + container_of(m2, struct xfs_inode, i_lock.dep_map); > > > > + > > > > + if (ip1->i_projid < ip2->i_projid) > > > > + return -1; > > > > + if (ip1->i_projid > ip2->i_projid) > > > > + return 1; > > > > + return 0; > > > > +} > > > > > > What's the project ID of the inode got to do with realtime groups? > > > > Each rtgroup metadata file stores its group number in i_projid so that > > mount can detect if there's a corruption in /rtgroup and we just opened > > the bitmap from the wrong group. > > > > We can also use lockdep to detect code that locks rtgroup metadata in > > the wrong order. Potentially we could use this _cmp_fn to enforce that > > we always ILOCK in the order bitmap -> summary -> rmap -> refcount based > > on i_metatype. > > Ok, can we union the i_projid field (both in memory and in the > on-disk structure) so that dual use of the field is well documented > by the code? Sounds good to me. Does union { xfs_prid_t i_projid; uint32_t i_metagroup; }; sound ok? --D > -Dave. > -- > Dave Chinner > david@fromorbit.com
On Mon, Aug 26, 2024 at 06:56:58PM -0700, Darrick J. Wong wrote: > On Tue, Aug 27, 2024 at 10:58:59AM +1000, Dave Chinner wrote: > > On Mon, Aug 26, 2024 at 02:38:27PM -0700, Darrick J. Wong wrote: > > > On Mon, Aug 26, 2024 at 09:58:05AM +1000, Dave Chinner wrote: > > > > On Thu, Aug 22, 2024 at 05:18:02PM -0700, Darrick J. Wong wrote: > > > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > > > > > Add a dynamic lockdep class key for rtgroup inodes. This will enable > > > > > lockdep to deduce inconsistencies in the rtgroup metadata ILOCK locking > > > > > order. Each class can have 8 subclasses, and for now we will only have > > > > > 2 inodes per group. This enables rtgroup order and inode order checks > > > > > when nesting ILOCKs. > > > > > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > > > --- > > > > > fs/xfs/libxfs/xfs_rtgroup.c | 52 +++++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 52 insertions(+) > > > > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c > > > > > index 51f04cad5227c..ae6d67c673b1a 100644 > > > > > --- a/fs/xfs/libxfs/xfs_rtgroup.c > > > > > +++ b/fs/xfs/libxfs/xfs_rtgroup.c > > > > > @@ -243,3 +243,55 @@ xfs_rtgroup_trans_join( > > > > > if (rtglock_flags & XFS_RTGLOCK_BITMAP) > > > > > xfs_rtbitmap_trans_join(tp); > > > > > } > > > > > + > > > > > +#ifdef CONFIG_PROVE_LOCKING > > > > > +static struct lock_class_key xfs_rtginode_lock_class; > > > > > + > > > > > +static int > > > > > +xfs_rtginode_ilock_cmp_fn( > > > > > + const struct lockdep_map *m1, > > > > > + const struct lockdep_map *m2) > > > > > +{ > > > > > + const struct xfs_inode *ip1 = > > > > > + container_of(m1, struct xfs_inode, i_lock.dep_map); > > > > > + const struct xfs_inode *ip2 = > > > > > + container_of(m2, struct xfs_inode, i_lock.dep_map); > > > > > + > > > > > + if (ip1->i_projid < ip2->i_projid) > > > > > + return -1; > > > > > + if (ip1->i_projid > ip2->i_projid) > > > > > + return 1; > > > > > + return 0; > > > > > +} > > > > > > > > What's the project ID of the inode got to do with realtime groups? > > > > > > Each rtgroup metadata file stores its group number in i_projid so that > > > mount can detect if there's a corruption in /rtgroup and we just opened > > > the bitmap from the wrong group. > > > > > > We can also use lockdep to detect code that locks rtgroup metadata in > > > the wrong order. Potentially we could use this _cmp_fn to enforce that > > > we always ILOCK in the order bitmap -> summary -> rmap -> refcount based > > > on i_metatype. > > > > Ok, can we union the i_projid field (both in memory and in the > > on-disk structure) so that dual use of the field is well documented > > by the code? > > Sounds good to me. Does > > union { > xfs_prid_t i_projid; > uint32_t i_metagroup; > }; > > sound ok? Yup.
diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c index 51f04cad5227c..ae6d67c673b1a 100644 --- a/fs/xfs/libxfs/xfs_rtgroup.c +++ b/fs/xfs/libxfs/xfs_rtgroup.c @@ -243,3 +243,55 @@ xfs_rtgroup_trans_join( if (rtglock_flags & XFS_RTGLOCK_BITMAP) xfs_rtbitmap_trans_join(tp); } + +#ifdef CONFIG_PROVE_LOCKING +static struct lock_class_key xfs_rtginode_lock_class; + +static int +xfs_rtginode_ilock_cmp_fn( + const struct lockdep_map *m1, + const struct lockdep_map *m2) +{ + const struct xfs_inode *ip1 = + container_of(m1, struct xfs_inode, i_lock.dep_map); + const struct xfs_inode *ip2 = + container_of(m2, struct xfs_inode, i_lock.dep_map); + + if (ip1->i_projid < ip2->i_projid) + return -1; + if (ip1->i_projid > ip2->i_projid) + return 1; + return 0; +} + +static inline void +xfs_rtginode_ilock_print_fn( + const struct lockdep_map *m) +{ + const struct xfs_inode *ip = + container_of(m, struct xfs_inode, i_lock.dep_map); + + printk(KERN_CONT " rgno=%u", ip->i_projid); +} + +/* + * Most of the time each of the RTG inode locks are only taken one at a time. + * But when committing deferred ops, more than one of a kind can be taken. + * However, deferred rt ops will be committed in rgno order so there is no + * potential for deadlocks. The code here is needed to tell lockdep about this + * order. + */ +static inline void +xfs_rtginode_lockdep_setup( + struct xfs_inode *ip, + xfs_rgnumber_t rgno, + enum xfs_rtg_inodes type) +{ + lockdep_set_class_and_subclass(&ip->i_lock, &xfs_rtginode_lock_class, + type); + lock_set_cmp_fn(&ip->i_lock, xfs_rtginode_ilock_cmp_fn, + xfs_rtginode_ilock_print_fn); +} +#else +#define xfs_rtginode_lockdep_setup(ip, rgno, type) do { } while (0) +#endif /* CONFIG_PROVE_LOCKING */