* [PATCH 1/5] xfs: clean up xfs_inactive
2012-07-04 15:13 [PATCH 0/5] do not take the iolock in inode reclaim context Christoph Hellwig
@ 2012-07-04 15:13 ` Christoph Hellwig
2012-07-26 15:30 ` Rich Johnston
2012-07-04 15:13 ` [PATCH 2/5] xfs: remove xfs_inactive_attrs Christoph Hellwig
` (6 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2012-07-04 15:13 UTC (permalink / raw)
To: xfs; +Cc: sage
[-- Attachment #1: xfs-cleanup-evict-transactions --]
[-- Type: text/plain, Size: 7094 bytes --]
The code to reserve log space and join the inode to the transaction is
common for all cases, so don't duplicate it. Also remove the trivial
xfs_inactive_symlink_local helper which can simply be opencode now.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_vnodeops.c | 171 ++++++++++++--------------------------------------
1 file changed, 43 insertions(+), 128 deletions(-)
Index: xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-07-04 09:01:24.733722862 +0200
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-07-04 09:50:20.683705323 +0200
@@ -282,23 +282,15 @@ xfs_inactive_symlink_rmt(
* free them all in one bunmapi call.
*/
ASSERT(ip->i_d.di_nextents > 0 && ip->i_d.di_nextents <= 2);
- if ((error = xfs_trans_reserve(tp, 0, XFS_ITRUNCATE_LOG_RES(mp), 0,
- XFS_TRANS_PERM_LOG_RES, XFS_ITRUNCATE_LOG_COUNT))) {
- ASSERT(XFS_FORCED_SHUTDOWN(mp));
- xfs_trans_cancel(tp, 0);
- *tpp = NULL;
- return error;
- }
+
/*
* Lock the inode, fix the size, and join it to the transaction.
* Hold it so in the normal path, we still have it locked for
* the second transaction. In the error paths we need it
* held so the cancel won't rele it, see below.
*/
- xfs_ilock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
size = (int)ip->i_d.di_size;
ip->i_d.di_size = 0;
- xfs_trans_ijoin(tp, ip, 0);
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
/*
* Find the block(s) so we can inval and unmap them.
@@ -385,67 +377,15 @@ xfs_inactive_symlink_rmt(
ASSERT(XFS_FORCED_SHUTDOWN(mp));
goto error0;
}
- /*
- * Return with the inode locked but not joined to the transaction.
- */
+
+ xfs_trans_ijoin(tp, ip, 0);
*tpp = tp;
return 0;
error1:
xfs_bmap_cancel(&free_list);
error0:
- /*
- * Have to come here with the inode locked and either
- * (held and in the transaction) or (not in the transaction).
- * If the inode isn't held then cancel would iput it, but
- * that's wrong since this is inactive and the vnode ref
- * count is 0 already.
- * Cancel won't do anything to the inode if held, but it still
- * needs to be locked until the cancel is done, if it was
- * joined to the transaction.
- */
- xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
- *tpp = NULL;
return error;
-
-}
-
-STATIC int
-xfs_inactive_symlink_local(
- xfs_inode_t *ip,
- xfs_trans_t **tpp)
-{
- int error;
-
- ASSERT(ip->i_d.di_size <= XFS_IFORK_DSIZE(ip));
- /*
- * We're freeing a symlink which fit into
- * the inode. Just free the memory used
- * to hold the old symlink.
- */
- error = xfs_trans_reserve(*tpp, 0,
- XFS_ITRUNCATE_LOG_RES(ip->i_mount),
- 0, XFS_TRANS_PERM_LOG_RES,
- XFS_ITRUNCATE_LOG_COUNT);
-
- if (error) {
- xfs_trans_cancel(*tpp, 0);
- *tpp = NULL;
- return error;
- }
- xfs_ilock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
-
- /*
- * Zero length symlinks _can_ exist.
- */
- if (ip->i_df.if_bytes > 0) {
- xfs_idata_realloc(ip,
- -(ip->i_df.if_bytes),
- XFS_DATA_FORK);
- ASSERT(ip->i_df.if_bytes == 0);
- }
- return 0;
}
STATIC int
@@ -604,7 +544,7 @@ xfs_inactive(
xfs_trans_t *tp;
xfs_mount_t *mp;
int error;
- int truncate;
+ int truncate = 0;
/*
* If the inode is already free, then there can be nothing
@@ -616,17 +556,6 @@ xfs_inactive(
return VN_INACTIVE_CACHE;
}
- /*
- * Only do a truncate if it's a regular file with
- * some actual space in it. It's OK to look at the
- * inode's fields without the lock because we're the
- * only one with a reference to the inode.
- */
- truncate = ((ip->i_d.di_nlink == 0) &&
- ((ip->i_d.di_size != 0) || XFS_ISIZE(ip) != 0 ||
- (ip->i_d.di_nextents > 0) || (ip->i_delayed_blks > 0)) &&
- S_ISREG(ip->i_d.di_mode));
-
mp = ip->i_mount;
error = 0;
@@ -650,72 +579,54 @@ xfs_inactive(
goto out;
}
- ASSERT(ip->i_d.di_nlink == 0);
+ if (S_ISREG(ip->i_d.di_mode) &&
+ (ip->i_d.di_size != 0 || XFS_ISIZE(ip) != 0 ||
+ ip->i_d.di_nextents > 0 || ip->i_delayed_blks > 0))
+ truncate = 1;
error = xfs_qm_dqattach(ip, 0);
if (error)
return VN_INACTIVE_CACHE;
tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
- if (truncate) {
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
-
- error = xfs_trans_reserve(tp, 0,
- XFS_ITRUNCATE_LOG_RES(mp),
- 0, XFS_TRANS_PERM_LOG_RES,
- XFS_ITRUNCATE_LOG_COUNT);
- if (error) {
- /* Don't call itruncate_cleanup */
- ASSERT(XFS_FORCED_SHUTDOWN(mp));
- xfs_trans_cancel(tp, 0);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
- return VN_INACTIVE_CACHE;
- }
+ error = xfs_trans_reserve(tp, 0,
+ (truncate || S_ISLNK(ip->i_d.di_mode)) ?
+ XFS_ITRUNCATE_LOG_RES(mp) :
+ XFS_IFREE_LOG_RES(mp),
+ 0,
+ XFS_TRANS_PERM_LOG_RES,
+ XFS_ITRUNCATE_LOG_COUNT);
+ if (error) {
+ ASSERT(XFS_FORCED_SHUTDOWN(mp));
+ xfs_trans_cancel(tp, 0);
+ return VN_INACTIVE_CACHE;
+ }
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- xfs_trans_ijoin(tp, ip, 0);
+ xfs_ilock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, ip, 0);
+ if (S_ISLNK(ip->i_d.di_mode)) {
+ /*
+ * Zero length symlinks _can_ exist.
+ */
+ if (ip->i_d.di_size > XFS_IFORK_DSIZE(ip)) {
+ error = xfs_inactive_symlink_rmt(ip, &tp);
+ if (error)
+ goto out_cancel;
+ } else if (ip->i_df.if_bytes > 0) {
+ xfs_idata_realloc(ip, -(ip->i_df.if_bytes),
+ XFS_DATA_FORK);
+ ASSERT(ip->i_df.if_bytes == 0);
+ }
+ } else if (truncate) {
ip->i_d.di_size = 0;
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
- if (error) {
- xfs_trans_cancel(tp,
- XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
- return VN_INACTIVE_CACHE;
- }
+ if (error)
+ goto out_cancel;
ASSERT(ip->i_d.di_nextents == 0);
- } else if (S_ISLNK(ip->i_d.di_mode)) {
-
- /*
- * If we get an error while cleaning up a
- * symlink we bail out.
- */
- error = (ip->i_d.di_size > XFS_IFORK_DSIZE(ip)) ?
- xfs_inactive_symlink_rmt(ip, &tp) :
- xfs_inactive_symlink_local(ip, &tp);
-
- if (error) {
- ASSERT(tp == NULL);
- return VN_INACTIVE_CACHE;
- }
-
- xfs_trans_ijoin(tp, ip, 0);
- } else {
- error = xfs_trans_reserve(tp, 0,
- XFS_IFREE_LOG_RES(mp),
- 0, XFS_TRANS_PERM_LOG_RES,
- XFS_INACTIVE_LOG_COUNT);
- if (error) {
- ASSERT(XFS_FORCED_SHUTDOWN(mp));
- xfs_trans_cancel(tp, 0);
- return VN_INACTIVE_CACHE;
- }
-
- xfs_ilock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
- xfs_trans_ijoin(tp, ip, 0);
}
/*
@@ -781,7 +692,11 @@ xfs_inactive(
xfs_qm_dqdetach(ip);
xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
- out:
+out:
+ return VN_INACTIVE_CACHE;
+out_cancel:
+ xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
return VN_INACTIVE_CACHE;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/5] xfs: clean up xfs_inactive
2012-07-04 15:13 ` [PATCH 1/5] xfs: clean up xfs_inactive Christoph Hellwig
@ 2012-07-26 15:30 ` Rich Johnston
0 siblings, 0 replies; 15+ messages in thread
From: Rich Johnston @ 2012-07-26 15:30 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 07/04/2012 10:13 AM, Christoph Hellwig wrote:
> The code to reserve log space and join the inode to the transaction is
> common for all cases, so don't duplicate it. Also remove the trivial
> xfs_inactive_symlink_local helper which can simply be opencode now.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> ---
> fs/xfs/xfs_vnodeops.c | 171 ++++++++++++--------------------------------------
> 1 file changed, 43 insertions(+), 128 deletions(-)
>
As stated this patch cleans up duplicate code and removed local helper
function. Looks good.
Reviewed-by: Rich Johnston <rjohnston@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] xfs: remove xfs_inactive_attrs
2012-07-04 15:13 [PATCH 0/5] do not take the iolock in inode reclaim context Christoph Hellwig
2012-07-04 15:13 ` [PATCH 1/5] xfs: clean up xfs_inactive Christoph Hellwig
@ 2012-07-04 15:13 ` Christoph Hellwig
2012-07-26 15:31 ` Rich Johnston
2012-07-04 15:13 ` [PATCH 3/5] xfs: do not take the iolock in xfs_inactive Christoph Hellwig
` (5 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2012-07-04 15:13 UTC (permalink / raw)
To: xfs; +Cc: sage
[-- Attachment #1: xfs-cleanup-evict-transactions-2 --]
[-- Type: text/plain, Size: 3616 bytes --]
Remove this helper as the code flow is a lot more obvious when it gets
merged into its only caller.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_vnodeops.c | 97 ++++++++++++++++++--------------------------------
1 file changed, 36 insertions(+), 61 deletions(-)
Index: xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-07-04 09:50:20.683705323 +0200
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-07-04 09:51:01.347038413 +0200
@@ -388,54 +388,6 @@ xfs_inactive_symlink_rmt(
return error;
}
-STATIC int
-xfs_inactive_attrs(
- xfs_inode_t *ip,
- xfs_trans_t **tpp)
-{
- xfs_trans_t *tp;
- int error;
- xfs_mount_t *mp;
-
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
- tp = *tpp;
- mp = ip->i_mount;
- ASSERT(ip->i_d.di_forkoff != 0);
- error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
- if (error)
- goto error_unlock;
-
- error = xfs_attr_inactive(ip);
- if (error)
- goto error_unlock;
-
- tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
- error = xfs_trans_reserve(tp, 0,
- XFS_IFREE_LOG_RES(mp),
- 0, XFS_TRANS_PERM_LOG_RES,
- XFS_INACTIVE_LOG_COUNT);
- if (error)
- goto error_cancel;
-
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- xfs_trans_ijoin(tp, ip, 0);
- xfs_idestroy_fork(ip, XFS_ATTR_FORK);
-
- ASSERT(ip->i_d.di_anextents == 0);
-
- *tpp = tp;
- return 0;
-
-error_cancel:
- ASSERT(XFS_FORCED_SHUTDOWN(mp));
- xfs_trans_cancel(tp, 0);
-error_unlock:
- *tpp = NULL;
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
- return error;
-}
-
int
xfs_release(
xfs_inode_t *ip)
@@ -630,24 +582,40 @@ xfs_inactive(
}
/*
- * If there are attributes associated with the file
- * then blow them away now. The code calls a routine
- * that recursively deconstructs the attribute fork.
- * We need to just commit the current transaction
+ * If there are attributes associated with the file then blow them away
+ * now. The code calls a routine that recursively deconstructs the
+ * attribute fork. We need to just commit the current transaction
* because we can't use it for xfs_attr_inactive().
*/
if (ip->i_d.di_anextents > 0) {
- error = xfs_inactive_attrs(ip, &tp);
- /*
- * If we got an error, the transaction is already
- * cancelled, and the inode is unlocked. Just get out.
- */
- if (error)
- return VN_INACTIVE_CACHE;
- } else if (ip->i_afp) {
- xfs_idestroy_fork(ip, XFS_ATTR_FORK);
+ ASSERT(ip->i_d.di_forkoff != 0);
+
+ error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ if (error)
+ goto error_unlock;
+
+ error = xfs_attr_inactive(ip);
+ if (error)
+ goto error_unlock;
+
+ tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
+ error = xfs_trans_reserve(tp, 0,
+ XFS_IFREE_LOG_RES(mp),
+ 0, XFS_TRANS_PERM_LOG_RES,
+ XFS_INACTIVE_LOG_COUNT);
+ if (error)
+ goto error_cancel;
+
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, ip, 0);
}
+ if (ip->i_afp)
+ xfs_idestroy_fork(ip, XFS_ATTR_FORK);
+
+ ASSERT(ip->i_d.di_anextents == 0);
+
/*
* Free the inode.
*/
@@ -698,6 +666,13 @@ out_cancel:
xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
return VN_INACTIVE_CACHE;
+
+error_cancel:
+ ASSERT(XFS_FORCED_SHUTDOWN(mp));
+ xfs_trans_cancel(tp, 0);
+error_unlock:
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ return VN_INACTIVE_CACHE;
}
/*
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 3/5] xfs: do not take the iolock in xfs_inactive
2012-07-04 15:13 [PATCH 0/5] do not take the iolock in inode reclaim context Christoph Hellwig
2012-07-04 15:13 ` [PATCH 1/5] xfs: clean up xfs_inactive Christoph Hellwig
2012-07-04 15:13 ` [PATCH 2/5] xfs: remove xfs_inactive_attrs Christoph Hellwig
@ 2012-07-04 15:13 ` Christoph Hellwig
2012-07-26 15:31 ` Rich Johnston
2012-07-04 15:13 ` [PATCH 4/5] xfs: avoid the iolock in xfs_free_eofblocks for evicted inodes Christoph Hellwig
` (4 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2012-07-04 15:13 UTC (permalink / raw)
To: xfs; +Cc: sage
[-- Attachment #1: xfs-dont-hold-iolock-during-eviction --]
[-- Type: text/plain, Size: 3127 bytes --]
An inode that enters xfs_inactive has been removed from all global
lists but the inode hash, and can't be recycled in xfs_iget before
it has been marked reclaimable. Thus taking the iolock in here
is not nessecary at all, and given the amount of lockdep false
positives it has triggered already I'd rather remove the locking.
The only change outside of xfs_inactive is relaxing an assert in
xfs_itruncate_extents.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_inode.c | 4 +++-
fs/xfs/xfs_vnodeops.c | 29 ++++++++++++-----------------
2 files changed, 15 insertions(+), 18 deletions(-)
Index: xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-07-04 09:51:01.347038413 +0200
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-07-04 09:51:03.913705064 +0200
@@ -554,7 +554,7 @@ xfs_inactive(
return VN_INACTIVE_CACHE;
}
- xfs_ilock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
if (S_ISLNK(ip->i_d.di_mode)) {
@@ -591,21 +591,24 @@ xfs_inactive(
ASSERT(ip->i_d.di_forkoff != 0);
error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
if (error)
- goto error_unlock;
+ goto out_unlock;
+
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
error = xfs_attr_inactive(ip);
if (error)
- goto error_unlock;
+ goto out;
tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
error = xfs_trans_reserve(tp, 0,
XFS_IFREE_LOG_RES(mp),
0, XFS_TRANS_PERM_LOG_RES,
XFS_INACTIVE_LOG_COUNT);
- if (error)
- goto error_cancel;
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ goto out;
+ }
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
@@ -658,21 +661,13 @@ xfs_inactive(
* Release the dquots held by inode, if any.
*/
xfs_qm_dqdetach(ip);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
-
+out_unlock:
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
out:
return VN_INACTIVE_CACHE;
out_cancel:
xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
- return VN_INACTIVE_CACHE;
-
-error_cancel:
- ASSERT(XFS_FORCED_SHUTDOWN(mp));
- xfs_trans_cancel(tp, 0);
-error_unlock:
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
- return VN_INACTIVE_CACHE;
+ goto out_unlock;
}
/*
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c 2012-07-04 09:40:04.413709004 +0200
+++ xfs/fs/xfs/xfs_inode.c 2012-07-04 09:51:03.913705064 +0200
@@ -1123,7 +1123,9 @@ xfs_itruncate_extents(
int error = 0;
int done = 0;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
+ xfs_isilocked(ip, XFS_IOLOCK_EXCL));
ASSERT(new_size <= XFS_ISIZE(ip));
ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
ASSERT(ip->i_itemp != NULL);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/5] xfs: do not take the iolock in xfs_inactive
2012-07-04 15:13 ` [PATCH 3/5] xfs: do not take the iolock in xfs_inactive Christoph Hellwig
@ 2012-07-26 15:31 ` Rich Johnston
0 siblings, 0 replies; 15+ messages in thread
From: Rich Johnston @ 2012-07-26 15:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 07/04/2012 10:13 AM, Christoph Hellwig wrote:
> An inode that enters xfs_inactive has been removed from all global
> lists but the inode hash, and can't be recycled in xfs_iget before
> it has been marked reclaimable. Thus taking the iolock in here
> is not nessecary at all, and given the amount of lockdep false
> positives it has triggered already I'd rather remove the locking.
>
> The only change outside of xfs_inactive is relaxing an assert in
> xfs_itruncate_extents.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> ---
> fs/xfs/xfs_inode.c | 4 +++-
> fs/xfs/xfs_vnodeops.c | 29 ++++++++++++-----------------
> 2 files changed, 15 insertions(+), 18 deletions(-)
>
> Index: xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-07-04 09:51:01.347038413 +0200
> +++ xfs/fs/xfs/xfs_vnodeops.c 2012-07-04 09:51:03.913705064 +0200
> @@ -554,7 +554,7 @@ xfs_inactive(
> return VN_INACTIVE_CACHE;
> }
>
> - xfs_ilock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> xfs_trans_ijoin(tp, ip, 0);
>
> if (S_ISLNK(ip->i_d.di_mode)) {
> @@ -591,21 +591,24 @@ xfs_inactive(
> ASSERT(ip->i_d.di_forkoff != 0);
>
> error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> if (error)
> - goto error_unlock;
> + goto out_unlock;
> +
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
>
> error = xfs_attr_inactive(ip);
> if (error)
> - goto error_unlock;
> + goto out;
>
> tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> error = xfs_trans_reserve(tp, 0,
> XFS_IFREE_LOG_RES(mp),
> 0, XFS_TRANS_PERM_LOG_RES,
> XFS_INACTIVE_LOG_COUNT);
> - if (error)
> - goto error_cancel;
> + if (error) {
> + xfs_trans_cancel(tp, 0);
> + goto out;
> + }
>
> xfs_ilock(ip, XFS_ILOCK_EXCL);
> xfs_trans_ijoin(tp, ip, 0);
> @@ -658,21 +661,13 @@ xfs_inactive(
> * Release the dquots held by inode, if any.
> */
> xfs_qm_dqdetach(ip);
> - xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> -
> +out_unlock:
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> out:
> return VN_INACTIVE_CACHE;
> out_cancel:
> xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
> - xfs_iunlock(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> - return VN_INACTIVE_CACHE;
> -
> -error_cancel:
> - ASSERT(XFS_FORCED_SHUTDOWN(mp));
> - xfs_trans_cancel(tp, 0);
> -error_unlock:
> - xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> - return VN_INACTIVE_CACHE;
> + goto out_unlock;
> }
Although I am not a fan of goto statements, this code would be very ugly
without it.
>
> /*
> Index: xfs/fs/xfs/xfs_inode.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode.c 2012-07-04 09:40:04.413709004 +0200
> +++ xfs/fs/xfs/xfs_inode.c 2012-07-04 09:51:03.913705064 +0200
> @@ -1123,7 +1123,9 @@ xfs_itruncate_extents(
> int error = 0;
> int done = 0;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
> + xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> ASSERT(new_size <= XFS_ISIZE(ip));
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> ASSERT(ip->i_itemp != NULL);
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
This is a good first step to removing the iolock. Looks good.
Reviewed-by: Rich Johnston <rjohnston@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] xfs: avoid the iolock in xfs_free_eofblocks for evicted inodes
2012-07-04 15:13 [PATCH 0/5] do not take the iolock in inode reclaim context Christoph Hellwig
` (2 preceding siblings ...)
2012-07-04 15:13 ` [PATCH 3/5] xfs: do not take the iolock in xfs_inactive Christoph Hellwig
@ 2012-07-04 15:13 ` Christoph Hellwig
2012-07-26 15:31 ` Rich Johnston
2012-07-04 15:13 ` [PATCH 5/5] xfs: remove iolock lock classes Christoph Hellwig
` (3 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2012-07-04 15:13 UTC (permalink / raw)
To: xfs; +Cc: sage
[-- Attachment #1: xfs-dont-hold-iolock-during-eviction-2 --]
[-- Type: text/plain, Size: 2482 bytes --]
Same rational as the last patch - these inodes are not reachable, so
don't bother with locking.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_vnodeops.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
Index: xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-07-04 09:51:03.913705064 +0200
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-07-04 09:57:07.987036223 +0200
@@ -146,11 +146,6 @@ xfs_readlink(
}
/*
- * Flags for xfs_free_eofblocks
- */
-#define XFS_FREE_EOF_TRYLOCK (1<<0)
-
-/*
* This is called by xfs_inactive to free any blocks beyond eof
* when the link count isn't zero and by xfs_dm_punch_hole() when
* punching a hole to EOF.
@@ -159,7 +154,7 @@ STATIC int
xfs_free_eofblocks(
xfs_mount_t *mp,
xfs_inode_t *ip,
- int flags)
+ bool need_iolock)
{
xfs_trans_t *tp;
int error;
@@ -201,13 +196,11 @@ xfs_free_eofblocks(
*/
tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
- if (flags & XFS_FREE_EOF_TRYLOCK) {
+ if (need_iolock) {
if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
xfs_trans_cancel(tp, 0);
return 0;
}
- } else {
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
}
error = xfs_trans_reserve(tp, 0,
@@ -217,7 +210,8 @@ xfs_free_eofblocks(
if (error) {
ASSERT(XFS_FORCED_SHUTDOWN(mp));
xfs_trans_cancel(tp, 0);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ if (need_iolock)
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL);
return error;
}
@@ -244,7 +238,10 @@ xfs_free_eofblocks(
error = xfs_trans_commit(tp,
XFS_TRANS_RELEASE_LOG_RES);
}
- xfs_iunlock(ip, XFS_IOLOCK_EXCL|XFS_ILOCK_EXCL);
+
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ if (need_iolock)
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL);
}
return error;
}
@@ -466,8 +463,7 @@ xfs_release(
if (xfs_iflags_test(ip, XFS_IDIRTY_RELEASE))
return 0;
- error = xfs_free_eofblocks(mp, ip,
- XFS_FREE_EOF_TRYLOCK);
+ error = xfs_free_eofblocks(mp, ip, true);
if (error)
return error;
@@ -524,7 +520,7 @@ xfs_inactive(
(!(ip->i_d.di_flags &
(XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) ||
ip->i_delayed_blks != 0))) {
- error = xfs_free_eofblocks(mp, ip, 0);
+ error = xfs_free_eofblocks(mp, ip, false);
if (error)
return VN_INACTIVE_CACHE;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 5/5] xfs: remove iolock lock classes
2012-07-04 15:13 [PATCH 0/5] do not take the iolock in inode reclaim context Christoph Hellwig
` (3 preceding siblings ...)
2012-07-04 15:13 ` [PATCH 4/5] xfs: avoid the iolock in xfs_free_eofblocks for evicted inodes Christoph Hellwig
@ 2012-07-04 15:13 ` Christoph Hellwig
2012-07-26 15:31 ` Rich Johnston
2012-07-06 0:05 ` [PATCH 0/5] do not take the iolock in inode reclaim context Sage Weil
` (2 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2012-07-04 15:13 UTC (permalink / raw)
To: xfs; +Cc: sage
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: xfs-remove-iolock-classes --]
[-- Type: text/plain, Size: 3820 bytes --]
Now that we never take the iolock during inode reclaim we don't need
to play games with lock classes.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_iget.c | 15 ---------------
fs/xfs/xfs_inode.h | 2 --
fs/xfs/xfs_super.c | 18 ++----------------
3 files changed, 2 insertions(+), 33 deletions(-)
Index: xfs/fs/xfs/xfs_iget.c
===================================================================
--- xfs.orig/fs/xfs/xfs_iget.c 2012-06-04 13:45:36.000000000 +0200
+++ xfs/fs/xfs/xfs_iget.c 2012-07-04 15:08:58.517051204 +0200
@@ -41,17 +41,6 @@
/*
- * Define xfs inode iolock lockdep classes. We need to ensure that all active
- * inodes are considered the same for lockdep purposes, including inodes that
- * are recycled through the XFS_IRECLAIMABLE state. This is the the only way to
- * guarantee the locks are considered the same when there are multiple lock
- * initialisation siteѕ. Also, define a reclaimable inode class so it is
- * obvious in lockdep reports which class the report is against.
- */
-static struct lock_class_key xfs_iolock_active;
-struct lock_class_key xfs_iolock_reclaimable;
-
-/*
* Allocate and initialise an xfs_inode.
*/
STATIC struct xfs_inode *
@@ -80,8 +69,6 @@ xfs_inode_alloc(
ASSERT(ip->i_ino == 0);
mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
- lockdep_set_class_and_name(&ip->i_iolock.mr_lock,
- &xfs_iolock_active, "xfs_iolock_active");
/* initialise the xfs inode */
ip->i_ino = ino;
@@ -250,8 +237,6 @@ xfs_iget_cache_hit(
ASSERT(!rwsem_is_locked(&ip->i_iolock.mr_lock));
mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
- lockdep_set_class_and_name(&ip->i_iolock.mr_lock,
- &xfs_iolock_active, "xfs_iolock_active");
spin_unlock(&ip->i_flags_lock);
spin_unlock(&pag->pag_ici_lock);
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h 2012-07-03 20:31:33.533706239 +0200
+++ xfs/fs/xfs/xfs_inode.h 2012-07-04 15:08:51.413717911 +0200
@@ -487,8 +487,6 @@ static inline int xfs_isiflocked(struct
#define XFS_IOLOCK_DEP(flags) (((flags) & XFS_IOLOCK_DEP_MASK) >> XFS_IOLOCK_SHIFT)
#define XFS_ILOCK_DEP(flags) (((flags) & XFS_ILOCK_DEP_MASK) >> XFS_ILOCK_SHIFT)
-extern struct lock_class_key xfs_iolock_reclaimable;
-
/*
* For multiple groups support: if S_ISGID bit is set in the parent
* directory, group of new file is set to that of the parent, and
Index: xfs/fs/xfs/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/xfs_super.c 2012-07-02 12:11:56.435779819 +0200
+++ xfs/fs/xfs/xfs_super.c 2012-07-04 15:08:44.667051287 +0200
@@ -929,6 +929,8 @@ xfs_fs_evict_inode(
{
xfs_inode_t *ip = XFS_I(inode);
+ ASSERT(!rwsem_is_locked(&ip->i_iolock.mr_lock));
+
trace_xfs_evict_inode(ip);
truncate_inode_pages(&inode->i_data, 0);
@@ -937,22 +939,6 @@ xfs_fs_evict_inode(
XFS_STATS_INC(vn_remove);
XFS_STATS_DEC(vn_active);
- /*
- * The iolock is used by the file system to coordinate reads,
- * writes, and block truncates. Up to this point the lock
- * protected concurrent accesses by users of the inode. But
- * from here forward we're doing some final processing of the
- * inode because we're done with it, and although we reuse the
- * iolock for protection it is really a distinct lock class
- * (in the lockdep sense) from before. To keep lockdep happy
- * (and basically indicate what we are doing), we explicitly
- * re-init the iolock here.
- */
- ASSERT(!rwsem_is_locked(&ip->i_iolock.mr_lock));
- mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
- lockdep_set_class_and_name(&ip->i_iolock.mr_lock,
- &xfs_iolock_reclaimable, "xfs_iolock_reclaimable");
-
xfs_inactive(ip);
}
[-- Attachment #2: Type: text/plain, Size: 121 bytes --]
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 5/5] xfs: remove iolock lock classes
2012-07-04 15:13 ` [PATCH 5/5] xfs: remove iolock lock classes Christoph Hellwig
@ 2012-07-26 15:31 ` Rich Johnston
0 siblings, 0 replies; 15+ messages in thread
From: Rich Johnston @ 2012-07-26 15:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 07/04/2012 10:13 AM, Christoph Hellwig wrote:
> > Now that we never take the iolock during inode reclaim we don't need
> to play games with lock classes.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> ---
> fs/xfs/xfs_iget.c | 15 ---------------
> fs/xfs/xfs_inode.h | 2 --
> fs/xfs/xfs_super.c | 18 ++----------------
> 3 files changed, 2 insertions(+), 33 deletions(-)
>
Last patch in this series which removes the unneeded lock classes.
Looks good.
Reviewed-by: Rich Johnston <rjohnston@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] do not take the iolock in inode reclaim context
2012-07-04 15:13 [PATCH 0/5] do not take the iolock in inode reclaim context Christoph Hellwig
` (4 preceding siblings ...)
2012-07-04 15:13 ` [PATCH 5/5] xfs: remove iolock lock classes Christoph Hellwig
@ 2012-07-06 0:05 ` Sage Weil
[not found] ` <20120717071923.GD15473@infradead.org>
2012-07-26 15:30 ` Rich Johnston
7 siblings, 0 replies; 15+ messages in thread
From: Sage Weil @ 2012-07-06 0:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sage, xfs
Hi Christoph,
On Wed, 4 Jul 2012, Christoph Hellwig wrote:
> This series should fix the (false-positive) lockdep warnings Sage
> has seen while testing ceph workloads with heavy attr usage.
I tried this out (minus the last patch, which didn't apply to
linus/master) and it does make the warnings go away, thanks! I'll run it
through a larger test run to make sure nothing else comes up.
> Btw, you probably should create the XFS filesystems for Ceph usage
> with large inodes to avoid going out of line for the attributes.
Oh, right. The qa stuff is still just doing mkfs.xfs defaults.
Thanks!
sage
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread[parent not found: <20120717071923.GD15473@infradead.org>]
* Re: [PATCH 0/5] do not take the iolock in inode reclaim context
[not found] ` <20120717071923.GD15473@infradead.org>
@ 2012-07-17 15:46 ` Sage Weil
2012-07-17 17:27 ` Mark Tinguely
0 siblings, 1 reply; 15+ messages in thread
From: Sage Weil @ 2012-07-17 15:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sage, xfs
On Tue, 17 Jul 2012, Christoph Hellwig wrote:
> ping/ I'd really like to get this queued up for 3.6
I forget if I mentioned this before, but I pulled this series into our
testing branch and have had no problems (aside from the last patch not
applying to my tree) in qa (ceph on xfs) over the last couple of weeks.
sage
>
> On Wed, Jul 04, 2012 at 11:13:28AM -0400, Christoph Hellwig wrote:
> > This series should fix the (false-positive) lockdep warnings Sage
> > has seen while testing ceph workloads with heavy attr usage.
> >
> > Btw, you probably should create the XFS filesystems for Ceph usage
> > with large inodes to avoid going out of line for the attributes.
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> ---end quoted text---
>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/5] do not take the iolock in inode reclaim context
2012-07-17 15:46 ` Sage Weil
@ 2012-07-17 17:27 ` Mark Tinguely
0 siblings, 0 replies; 15+ messages in thread
From: Mark Tinguely @ 2012-07-17 17:27 UTC (permalink / raw)
To: Sage Weil; +Cc: Christoph Hellwig, sage, xfs
On 07/17/12 10:46, Sage Weil wrote:
> On Tue, 17 Jul 2012, Christoph Hellwig wrote:
>> ping/ I'd really like to get this queued up for 3.6
>
> I forget if I mentioned this before, but I pulled this series into our
> testing branch and have had no problems (aside from the last patch not
> applying to my tree) in qa (ceph on xfs) over the last couple of weeks.
>
> sage
Sage,
The patch "5-5-xfs-remove-iolock-lock-classes.patch" does not cleanly
apply because the comment that the patch is trying to remove in
xfs_iget.c has the following character sequence "<D1><95>" that the
mailer converted to a "?". It is easy enough to hand patch:
/*
* Define xfs inode iolock lockdep classes. We need to ensure that all
active
* inodes are considered the same for lockdep purposes, including
inodes that
* are recycled through the XFS_IRECLAIMABLE state. This is the the
only way to
* guarantee the locks are considered the same when there are multiple lock
* initialisation site<D1><95>. Also, define a reclaimable inode class
so it is
^^^^^^^^
--Mark Tinguely.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] do not take the iolock in inode reclaim context
2012-07-04 15:13 [PATCH 0/5] do not take the iolock in inode reclaim context Christoph Hellwig
` (6 preceding siblings ...)
[not found] ` <20120717071923.GD15473@infradead.org>
@ 2012-07-26 15:30 ` Rich Johnston
7 siblings, 0 replies; 15+ messages in thread
From: Rich Johnston @ 2012-07-26 15:30 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 07/04/2012 10:13 AM, Christoph Hellwig wrote:
> This series should fix the (false-positive) lockdep warnings Sage
> has seen while testing ceph workloads with heavy attr usage.
>
> Btw, you probably should create the XFS filesystems for Ceph usage
> with large inodes to avoid going out of line for the attributes.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
I really liked the way this patch series was broken up. As I am new to
the XFS group, this was very educational to review. Nice job Christoph.
--Rich
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 15+ messages in thread