* [PATCH 1/3] xfs: only lock the rt bitmap inode once per allocation
2011-01-25 9:06 [PATCH 0/3] resurrect realtime subvolume support Christoph Hellwig
@ 2011-01-25 9:06 ` Christoph Hellwig
2011-01-25 9:06 ` [PATCH 2/3] xfs: fix xfs_get_extsz_hint for a zero extent size hint Christoph Hellwig
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2011-01-25 9:06 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-fix-rt-inode-locking --]
[-- Type: text/plain, Size: 4262 bytes --]
Currently both xfs_rtpick_extent and xfs_rtallocate_extent call
xfs_trans_iget to grab and lock the rt bitmap inode, which results in a
deadlock since the removal of the lock recursion counters in commit
"xfs: simplify inode to transaction joining"
Fix this by acquiring and locking the inode in xfs_bmap_rtalloc before
calling into xfs_rtpick_extent and xfs_rtallocate_extent.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_bmap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap.c 2011-01-24 11:36:00.541016631 +0100
+++ xfs/fs/xfs/xfs_bmap.c 2011-01-24 16:02:40.581003990 +0100
@@ -2316,6 +2316,7 @@ xfs_bmap_rtalloc(
xfs_extlen_t prod = 0; /* product factor for allocators */
xfs_extlen_t ralen = 0; /* realtime allocation length */
xfs_extlen_t align; /* minimum allocation alignment */
+ xfs_inode_t *ip; /* bitmap incore inode */
xfs_rtblock_t rtb;
mp = ap->ip->i_mount;
@@ -2348,6 +2349,16 @@ xfs_bmap_rtalloc(
*/
if (ralen * mp->m_sb.sb_rextsize >= MAXEXTLEN)
ralen = MAXEXTLEN / mp->m_sb.sb_rextsize;
+
+ /*
+ * Lock out other modifications to the RT bitmap inode.
+ */
+ error = xfs_trans_iget(mp, ap->tp, mp->m_sb.sb_rbmino, 0,
+ XFS_ILOCK_EXCL, &ip);
+ if (error)
+ return error;
+ ASSERT(ip == mp->m_rbmip);
+
/*
* If it's an allocation to an empty file at offset 0,
* pick an extent that will space things out in the rt area.
Index: xfs/fs/xfs/xfs_rtalloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_rtalloc.c 2011-01-24 11:36:00.519026479 +0100
+++ xfs/fs/xfs/xfs_rtalloc.c 2011-01-24 16:02:30.086004688 +0100
@@ -2075,15 +2075,15 @@ xfs_rtallocate_extent(
xfs_extlen_t prod, /* extent product factor */
xfs_rtblock_t *rtblock) /* out: start block allocated */
{
+ xfs_mount_t *mp = tp->t_mountp;
int error; /* error value */
- xfs_inode_t *ip; /* inode for bitmap file */
- xfs_mount_t *mp; /* file system mount structure */
xfs_rtblock_t r; /* result allocated block */
xfs_fsblock_t sb; /* summary file block number */
xfs_buf_t *sumbp; /* summary file block buffer */
+ ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
ASSERT(minlen > 0 && minlen <= maxlen);
- mp = tp->t_mountp;
+
/*
* If prod is set then figure out what to do to minlen and maxlen.
*/
@@ -2099,12 +2099,7 @@ xfs_rtallocate_extent(
return 0;
}
}
- /*
- * Lock out other callers by grabbing the bitmap inode lock.
- */
- if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0,
- XFS_ILOCK_EXCL, &ip)))
- return error;
+
sumbp = NULL;
/*
* Allocate by size, or near another block, or exactly at some block.
@@ -2123,11 +2118,12 @@ xfs_rtallocate_extent(
len, &sumbp, &sb, prod, &r);
break;
default:
+ error = EIO;
ASSERT(0);
}
- if (error) {
+ if (error)
return error;
- }
+
/*
* If it worked, update the superblock.
*/
@@ -2306,20 +2302,16 @@ xfs_rtpick_extent(
xfs_rtblock_t *pick) /* result rt extent */
{
xfs_rtblock_t b; /* result block */
- int error; /* error return value */
- xfs_inode_t *ip; /* bitmap incore inode */
int log2; /* log of sequence number */
__uint64_t resid; /* residual after log removed */
__uint64_t seq; /* sequence number of file creation */
__uint64_t *seqp; /* pointer to seqno in inode */
- if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0,
- XFS_ILOCK_EXCL, &ip)))
- return error;
- ASSERT(ip == mp->m_rbmip);
- seqp = (__uint64_t *)&ip->i_d.di_atime;
- if (!(ip->i_d.di_flags & XFS_DIFLAG_NEWRTBM)) {
- ip->i_d.di_flags |= XFS_DIFLAG_NEWRTBM;
+ ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
+
+ seqp = (__uint64_t *)&mp->m_rbmip->i_d.di_atime;
+ if (!(mp->m_rbmip->i_d.di_flags & XFS_DIFLAG_NEWRTBM)) {
+ mp->m_rbmip->i_d.di_flags |= XFS_DIFLAG_NEWRTBM;
*seqp = 0;
}
seq = *seqp;
@@ -2335,7 +2327,7 @@ xfs_rtpick_extent(
b = mp->m_sb.sb_rextents - len;
}
*seqp = seq + 1;
- xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+ xfs_trans_log_inode(tp, mp->m_rbmip, XFS_ILOG_CORE);
*pick = b;
return 0;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 2/3] xfs: fix xfs_get_extsz_hint for a zero extent size hint
2011-01-25 9:06 [PATCH 0/3] resurrect realtime subvolume support Christoph Hellwig
2011-01-25 9:06 ` [PATCH 1/3] xfs: only lock the rt bitmap inode once per allocation Christoph Hellwig
@ 2011-01-25 9:06 ` Christoph Hellwig
2011-01-25 9:06 ` [PATCH 3/3] xfs: add lockdep annotations for the rt inodes Christoph Hellwig
2011-02-02 1:58 ` [PATCH 0/3] resurrect realtime subvolume support Alex Elder
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2011-01-25 9:06 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-fix-zero-extent-size-hint --]
[-- Type: text/plain, Size: 1297 bytes --]
We can easily set the extsize flag without setting an extent size
hint, or one that evaluates to zero. Historically the di_extsize
field was only used when it was non-zero, but the commit
"Cleanup inode extent size hint extraction"
broke this. Restore the old behaviour, thus fixing xfsqa 090 with
a debug kernel.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_rw.c
===================================================================
--- xfs.orig/fs/xfs/xfs_rw.c 2011-01-24 14:42:50.432004339 +0100
+++ xfs/fs/xfs/xfs_rw.c 2011-01-24 15:03:17.211016282 +0100
@@ -173,17 +173,9 @@ xfs_extlen_t
xfs_get_extsz_hint(
struct xfs_inode *ip)
{
- xfs_extlen_t extsz;
-
- if (unlikely(XFS_IS_REALTIME_INODE(ip))) {
- extsz = (ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)
- ? ip->i_d.di_extsize
- : ip->i_mount->m_sb.sb_rextsize;
- ASSERT(extsz);
- } else {
- extsz = (ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)
- ? ip->i_d.di_extsize : 0;
- }
-
- return extsz;
+ if ((ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE) && ip->i_d.di_extsize)
+ return ip->i_d.di_extsize;
+ if (XFS_IS_REALTIME_INODE(ip))
+ return ip->i_mount->m_sb.sb_rextsize;
+ return 0;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 3/3] xfs: add lockdep annotations for the rt inodes
2011-01-25 9:06 [PATCH 0/3] resurrect realtime subvolume support Christoph Hellwig
2011-01-25 9:06 ` [PATCH 1/3] xfs: only lock the rt bitmap inode once per allocation Christoph Hellwig
2011-01-25 9:06 ` [PATCH 2/3] xfs: fix xfs_get_extsz_hint for a zero extent size hint Christoph Hellwig
@ 2011-01-25 9:06 ` Christoph Hellwig
2011-02-02 1:58 ` [PATCH 0/3] resurrect realtime subvolume support Alex Elder
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2011-01-25 9:06 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-fix-rtinode-lockdep --]
[-- Type: text/plain, Size: 4670 bytes --]
The rt bitmap and summary inodes do not participate in the normal inode
locking protocol. Instead the rt bitmap inode can be locked in any
transaction involving rt allocations, and the both of the rt inodes can
be locked at the same time. Add specific lockdep subclasses for the rt
inodes to prevent lockdep from blowing up.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_bmap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap.c 2011-01-24 16:02:40.000000000 +0100
+++ xfs/fs/xfs/xfs_bmap.c 2011-01-24 16:03:03.416004409 +0100
@@ -2354,7 +2354,7 @@ xfs_bmap_rtalloc(
* Lock out other modifications to the RT bitmap inode.
*/
error = xfs_trans_iget(mp, ap->tp, mp->m_sb.sb_rbmino, 0,
- XFS_ILOCK_EXCL, &ip);
+ XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP, &ip);
if (error)
return error;
ASSERT(ip == mp->m_rbmip);
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h 2011-01-24 16:02:30.000000000 +0100
+++ xfs/fs/xfs/xfs_inode.h 2011-01-24 16:02:46.885004130 +0100
@@ -409,28 +409,35 @@ static inline void xfs_ifunlock(xfs_inod
/*
* Flags for lockdep annotations.
*
- * XFS_I[O]LOCK_PARENT - for operations that require locking two inodes
- * (ie directory operations that require locking a directory inode and
- * an entry inode). The first inode gets locked with this flag so it
- * gets a lockdep subclass of 1 and the second lock will have a lockdep
- * subclass of 0.
+ * XFS_LOCK_PARENT - for directory operations that require locking a
+ * parent directory inode and a child entry inode. The parent gets locked
+ * with this flag so it gets a lockdep subclass of 1 and the child entry
+ * lock will have a lockdep subclass of 0.
+ *
+ * XFS_LOCK_RTBITMAP/XFS_LOCK_RTSUM - the realtime device bitmap and summary
+ * inodes do not participate in the normal lock order, and thus have their
+ * own subclasses.
*
* XFS_LOCK_INUMORDER - for locking several inodes at the some time
* with xfs_lock_inodes(). This flag is used as the starting subclass
* and each subsequent lock acquired will increment the subclass by one.
- * So the first lock acquired will have a lockdep subclass of 2, the
- * second lock will have a lockdep subclass of 3, and so on. It is
+ * So the first lock acquired will have a lockdep subclass of 4, the
+ * second lock will have a lockdep subclass of 5, and so on. It is
* the responsibility of the class builder to shift this to the correct
* portion of the lock_mode lockdep mask.
*/
#define XFS_LOCK_PARENT 1
-#define XFS_LOCK_INUMORDER 2
+#define XFS_LOCK_RTBITMAP 2
+#define XFS_LOCK_RTSUM 3
+#define XFS_LOCK_INUMORDER 4
#define XFS_IOLOCK_SHIFT 16
#define XFS_IOLOCK_PARENT (XFS_LOCK_PARENT << XFS_IOLOCK_SHIFT)
#define XFS_ILOCK_SHIFT 24
#define XFS_ILOCK_PARENT (XFS_LOCK_PARENT << XFS_ILOCK_SHIFT)
+#define XFS_ILOCK_RTBITMAP (XFS_LOCK_RTBITMAP << XFS_ILOCK_SHIFT)
+#define XFS_ILOCK_RTSUM (XFS_LOCK_RTSUM << XFS_ILOCK_SHIFT)
#define XFS_IOLOCK_DEP_MASK 0x00ff0000
#define XFS_ILOCK_DEP_MASK 0xff000000
Index: xfs/fs/xfs/xfs_rtalloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_rtalloc.c 2011-01-24 16:02:30.000000000 +0100
+++ xfs/fs/xfs/xfs_rtalloc.c 2011-01-24 16:02:46.889004409 +0100
@@ -1972,8 +1972,10 @@ xfs_growfs_rt(
/*
* Lock out other callers by grabbing the bitmap inode lock.
*/
- if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0,
- XFS_ILOCK_EXCL, &ip)))
+ error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0,
+ XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP,
+ &ip);
+ if (error)
goto error_cancel;
ASSERT(ip == mp->m_rbmip);
/*
@@ -1986,8 +1988,9 @@ xfs_growfs_rt(
/*
* Get the summary inode into the transaction.
*/
- if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rsumino, 0,
- XFS_ILOCK_EXCL, &ip)))
+ error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rsumino, 0,
+ XFS_ILOCK_EXCL | XFS_ILOCK_RTSUM, &ip);
+ if (error)
goto error_cancel;
ASSERT(ip == mp->m_rsumip);
/*
@@ -2160,8 +2163,9 @@ xfs_rtfree_extent(
/*
* Synchronize by locking the bitmap inode.
*/
- if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0,
- XFS_ILOCK_EXCL, &ip)))
+ error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0,
+ XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP, &ip);
+ if (error)
return error;
#if defined(__KERNEL__) && defined(DEBUG)
/*
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 0/3] resurrect realtime subvolume support
2011-01-25 9:06 [PATCH 0/3] resurrect realtime subvolume support Christoph Hellwig
` (2 preceding siblings ...)
2011-01-25 9:06 ` [PATCH 3/3] xfs: add lockdep annotations for the rt inodes Christoph Hellwig
@ 2011-02-02 1:58 ` Alex Elder
3 siblings, 0 replies; 5+ messages in thread
From: Alex Elder @ 2011-02-02 1:58 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Tue, 2011-01-25 at 04:06 -0500, Christoph Hellwig wrote:
> Fix two regressions that broke using the realtime subvolume, and
> add lockdep annotations for the ilocks of the two rt metadata inodes.
I reviewed all three patches in this series, i.e.:
Reviewed-by: Alex Elder <aelder@sgi.com>
I'll give it another day or so to give others a
chance to review, but otherwise I will incorporate
all three from your posted patches.
-Alex
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread