2.6.33-rcX candidate fixes
cover 2.6.33-rcX candidate fixes
The first three of these patches are the respun inode reclaim fixes adressing all of Christoph's concerns. These should be OK for merge now, and once merged canbe backported to .31/.32.
The last patch is a fix for a new lockdep warning since 2.6.32 that occurs during mount of an XFS filesystem. The ASSERT for the rwsem being held is redundant now that lockdep checks for the lock already being held/in use during lock initialisation that happens directly after the ASSERT in XFS.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/4 xfs: reclaim inodes under a write lock
Make the inode tree reclaim walk exclusive to avoid races with concurrent sync walkers and lookups. This is a version of a patch posted by Christoph Hellwig that avoids all the code duplication.
--- fs/xfs/linux-2.6/xfs_sync.c | 150 ++++++++++++++++++---------------------- fs/xfs/linux-2.6/xfs_sync.h | 2 +- fs/xfs/quota/xfs_qm_syscalls.c | 2 +- 3 files changed, 69 insertions(+), 85 deletions(-)
fs/xfs/linux-2.6/xfs_sync.c +67 -83
@@ -65,7 +65,6 @@ xfs_inode_ag_lookup(* as the tree is sparse and a gang lookup walks to find * the number of objects requested. */ -read_lock (&pag->pag_ici_lock);if (tag ==XFS_ICI_NO_TAG ) { nr_found =radix_tree_gang_lookup (&pag->pag_ici_root, (void **)&ip, *first_index,1 );
@@ -74,7 +73,7 @@ xfs_inode_ag_lookup( (void **)&ip, *first_index, 1, tag); }if (!nr_found) - goto unlock; +return NULL ;/* * Update the index for the next lookup. Catch overflows
@@ -84,13 +83,8 @@ xfs_inode_ag_lookup(*/ *first_index =XFS_INO_TO_AGINO (mp, ip->i_ino +1 );if (*first_index <XFS_INO_TO_AGINO (mp, ip->i_ino)) - goto unlock; - +return NULL ;return ip; - -unlock: -read_unlock (&pag->pag_ici_lock); -return NULL ; }STATIC int
@@ -100,7 +94,8 @@ xfs_inode_ag_walk(int (*execute)(struct xfs_inode *ip,struct xfs_perag *pag,int flags), int flags, - int tag) + int tag, + int exclusive) { struct xfs_perag *pag = &mp->m_perag[ag];uint32_t first_index;
@@ -114,10 +109,20 @@ restart:int error =0 ;xfs_inode_t *ip; +if (exclusive) +write_lock (&pag->pag_ici_lock); +else +read_lock (&pag->pag_ici_lock); ip =xfs_inode_ag_lookup (mp, pag, &first_index, tag); -if (!ip) +if (!ip) { +if (exclusive) +write_unlock (&pag->pag_ici_lock); +else +read_unlock (&pag->pag_ici_lock);break ; + } +/* execute releases pag->pag_ici_lock */ error =execute (ip, pag, flags);if (error ==EAGAIN ) { skipped++;
@@ -125,9 +130,8 @@ restart: }if (error) last_error = error; -/* -* bail out if the filesystem is corrupted. -*/ + +/* bail out if the filesystem is corrupted. */ if (error ==EFSCORRUPTED )break ;
@@ -148,7 +152,8 @@ xfs_inode_ag_iterator(int (*execute)(struct xfs_inode *ip,struct xfs_perag *pag,int flags), int flags, - int tag) + int tag, + int exclusive) { int error = 0;int last_error =0 ;
@@ -157,7 +162,8 @@ xfs_inode_ag_iterator(for (ag =0 ; ag < mp->m_sb.sb_agcount; ag++) {if (!mp->m_perag[ag].pag_ici_init)continue ; - error =xfs_inode_ag_walk (mp, ag, execute, flags, tag); + error =xfs_inode_ag_walk (mp, ag, execute, flags, tag, + exclusive);if (error) { last_error = error;if (error ==EFSCORRUPTED )
@@ -181,11 +187,7 @@ xfs_sync_inode_valid(return EFSCORRUPTED ; } -/* -* If we can't get a reference on the inode, it must be in reclaim. -* Leave it for the reclaim code to flush. Also avoid inodes that -* haven't been fully initialised. -*/ +/* If we can't get a reference on the inode, it must be in reclaim. */ if (!igrab (inode)) {read_unlock (&pag->pag_ici_lock);return ENOENT ;
@@ -282,7 +284,7 @@ xfs_sync_data(ASSERT ((flags & ~(SYNC_TRYLOCK |SYNC_WAIT )) ==0 ); error =xfs_inode_ag_iterator (mp, xfs_sync_inode_data, flags, -XFS_ICI_NO_TAG ); +XFS_ICI_NO_TAG ,0 );if (error)return XFS_ERROR (error);
@@ -304,7 +306,7 @@ xfs_sync_attr(ASSERT ((flags & ~SYNC_WAIT ) ==0 );return xfs_inode_ag_iterator (mp, xfs_sync_inode_attr, flags, -XFS_ICI_NO_TAG ); +XFS_ICI_NO_TAG ,0 ); }STATIC int
@@ -664,60 +666,6 @@ xfs_syncd_stop(kthread_stop (mp->m_sync_task); } -STATIC int -xfs_reclaim_inode ( -xfs_inode_t *ip, -int sync_mode) -{ -xfs_perag_t *pag =xfs_get_perag (ip->i_mount, ip->i_ino); - -/* The hash lock here protects a thread in xfs_iget_core from -* racing with us on linking the inode back with a vnode. -* Once we have the XFS_IRECLAIM flag set it will not touch -* us. -*/ -write_lock (&pag->pag_ici_lock); -spin_lock (&ip->i_flags_lock); -if (__xfs_iflags_test (ip,XFS_IRECLAIM ) || - !__xfs_iflags_test (ip,XFS_IRECLAIMABLE )) { -spin_unlock (&ip->i_flags_lock); -write_unlock (&pag->pag_ici_lock); -return -EAGAIN ; - } -__xfs_iflags_set (ip,XFS_IRECLAIM ); -spin_unlock (&ip->i_flags_lock); -write_unlock (&pag->pag_ici_lock); -xfs_put_perag (ip->i_mount, pag); - -/* -* If the inode is still dirty, then flush it out. If the inode -* is not in the AIL, then it will be OK to flush it delwri as -* long as xfs_iflush() does not keep any references to the inode. -* We leave that decision up to xfs_iflush() since it has the -* knowledge of whether it's OK to simply do a delwri flush of -* the inode or whether we need to wait until the inode is -* pulled from the AIL. -* We get the flush lock regardless, though, just to make sure -* we don't free it while it is being flushed. -*/ -xfs_ilock (ip,XFS_ILOCK_EXCL ); -xfs_iflock (ip); - -/* -* In the case of a forced shutdown we rely on xfs_iflush() to -* wait for the inode to be unpinned before returning an error. -*/ -if (!is_bad_inode (VFS_I (ip)) &&xfs_iflush (ip, sync_mode) ==0 ) { -/* synchronize with xfs_iflush_done */ -xfs_iflock (ip); -xfs_ifunlock (ip); - } - -xfs_iunlock (ip,XFS_ILOCK_EXCL ); -xfs_ireclaim (ip); -return 0 ; -} - void __xfs_inode_set_reclaim_tag( struct xfs_perag *pag,
@@ -763,16 +711,52 @@ STATIC intxfs_reclaim_inode_now ( struct xfs_inode *ip, struct xfs_perag *pag, - int flags) + int sync_mode) { - /* ignore if already under reclaim */ - if (xfs_iflags_test(ip, XFS_IRECLAIM)) { - read_unlock(&pag->pag_ici_lock); +/* +* The radix tree lock here protects a thread in xfs_iget from racing +* with us starting reclaim on the inode. Once we have the +* XFS_IRECLAIM flag set it will not touch us. +*/ + spin_lock(&ip->i_flags_lock); +ASSERT_ALWAYS (__xfs_iflags_test (ip,XFS_IRECLAIMABLE )); +if (__xfs_iflags_test (ip,XFS_IRECLAIM )) { +/* ignore as it is already under reclaim */ +spin_unlock (&ip->i_flags_lock); +write_unlock (&pag->pag_ici_lock);return 0 ; } -read_unlock (&pag->pag_ici_lock); +__xfs_iflags_set (ip,XFS_IRECLAIM ); +spin_unlock (&ip->i_flags_lock); +write_unlock (&pag->pag_ici_lock); -return xfs_reclaim_inode (ip, flags); +/* +* If the inode is still dirty, then flush it out. If the inode +* is not in the AIL, then it will be OK to flush it delwri as +* long as xfs_iflush() does not keep any references to the inode. +* We leave that decision up to xfs_iflush() since it has the +* knowledge of whether it's OK to simply do a delwri flush of +* the inode or whether we need to wait until the inode is +* pulled from the AIL. +* We get the flush lock regardless, though, just to make sure +* we don't free it while it is being flushed. +*/ +xfs_ilock (ip,XFS_ILOCK_EXCL ); +xfs_iflock (ip); + +/* +* In the case of a forced shutdown we rely on xfs_iflush() to +* wait for the inode to be unpinned before returning an error. +*/ +if (!is_bad_inode (VFS_I (ip)) &&xfs_iflush (ip, sync_mode) ==0 ) { +/* synchronize with xfs_iflush_done */ +xfs_iflock (ip); +xfs_ifunlock (ip); + } + +xfs_iunlock (ip,XFS_ILOCK_EXCL ); +xfs_ireclaim (ip); +return 0 ; }int
@@ -781,5 +765,5 @@ xfs_reclaim_inodes( int mode) {return xfs_inode_ag_iterator (mp, xfs_reclaim_inode_now, mode, -XFS_ICI_RECLAIM_TAG ); +XFS_ICI_RECLAIM_TAG ,1 ); }
fs/xfs/linux-2.6/xfs_sync.h +1 -1
@@ -54,6 +54,6 @@ void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,int xfs_sync_inode_valid (struct xfs_inode *ip,struct xfs_perag *pag);int xfs_inode_ag_iterator (struct xfs_mount *mp,int (*execute)(struct xfs_inode *ip,struct xfs_perag *pag,int flags), -int flags,int tag); +int flags,int tag,int write_lock);#endif
fs/xfs/quota/xfs_qm_syscalls.c +1 -1
@@ -891,7 +891,7 @@ xfs_qm_dqrele_all_inodes( uint flags) {ASSERT (mp->m_quotainfo); -xfs_inode_ag_iterator (mp, xfs_dqrele_inode, flags,XFS_ICI_NO_TAG ); +xfs_inode_ag_iterator (mp, xfs_dqrele_inode, flags,XFS_ICI_NO_TAG ,0 ); }/*------------------------------------------------------------------------*/
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/4] xfs: reclaim inodes under a write lock
Quoted 4 lines · Dave Chinner
Make the inode tree reclaim walk exclusive to avoid races with concurrent sync walkers and lookups. This is a version of a patch posted by Christoph Hellwig that avoids all the code duplication.
looks good,
FYI: I would rename xfs_reclaim_inode_now to just xfs_reclaim_inode, but unless Alex wants to edit it let's just get the patch in for now.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/4 xfs: Avoid inodes in reclaim when flushing from inode cache
In reply to Dave ChinnerTHe reclaim code will handle flushing of dirty inodes before reclaim occurs, so avoid them when determining whether an inode is a candidate for flushing to disk when walking the radix trees. This is based on a test patch from Christoph Hellwig.
--- fs/xfs/linux-2.6/xfs_sync.c | 31 ++++++++++++++++++------------- 1 files changed, 18 insertions(+), 13 deletions(-)
fs/xfs/linux-2.6/xfs_sync.c +18 -13
@@ -180,26 +180,31 @@ xfs_sync_inode_valid( struct xfs_perag *pag) {struct inode *inode =VFS_I (ip); +int error =EFSCORRUPTED ;/* nothing to sync during shutdown */ -if (XFS_FORCED_SHUTDOWN (ip->i_mount)) { -read_unlock (&pag->pag_ici_lock); -return EFSCORRUPTED ; - } +if (XFS_FORCED_SHUTDOWN (ip->i_mount)) + goto out_unlock; -/* If we can't get a reference on the inode, it must be in reclaim. */ -if (!igrab (inode)) { -read_unlock (&pag->pag_ici_lock); -return ENOENT ; - } -read_unlock (&pag->pag_ici_lock); +/* avoid new or reclaimable inodes. Leave for reclaim code to flush */ + error =ENOENT ; +if (xfs_iflags_test (ip,XFS_INEW |XFS_IRECLAIMABLE |XFS_IRECLAIM )) + goto out_unlock; -if (is_bad_inode (inode) ||xfs_iflags_test (ip,XFS_INEW )) { +/* If we can't grab the inode, it must on it's way to reclaim. */ +if (!igrab (inode)) + goto out_unlock; + +if (is_bad_inode (inode)) {IRELE (ip); -return ENOENT ; + goto out_unlock; } -return 0 ; +/* inode is valid */ + error =0 ; +out_unlock: +read_unlock (&pag->pag_ici_lock); +return error; }STATIC int
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/4] xfs: Avoid inodes in reclaim when flushing from inode cache
Quoted 6 lines · Dave Chinner
THe reclaim code will handle flushing of dirty inodes before reclaim occurs, so avoid them when determining whether an inode is a candidate for flushing to disk when walking the radix trees. This is based on a test patch from Christoph Hellwig.
Signed-off-by Dave Chinner <[email protected]>
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/4 xfs: reclaim all inodes by background tree walks
In reply to Dave ChinnerWe cannot do direct inode reclaim without taking the flush lock to ensure that we do not reclaim an inode under IO. We check the inode is clean before doing direct reclaim, but this is not good enough because the inode flush code marks the inode clean once it has copied the in-core dirty state to the backing buffer.
It is the flush lock that determines whether the inode is still under IO, even though it is marked clean, and the inode is still required at IO completion so we can't reclaim it even though it is clean in core. Hence the requirement that we need to take the flush lock even on clean inodes because this guarantees that the inode writeback IO has completed and it is safe to reclaim the inode.
With delayed write inode flushing, we coul dend up waiting a long time on the flush lock even for a clean inode. The background reclaim already handles this efficiently, so avoid all the problems by killing the direct reclaim path altogether.
--- fs/xfs/linux-2.6/xfs_super.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-)
fs/xfs/linux-2.6/xfs_super.c +6 -8
@@ -954,16 +954,14 @@ xfs_fs_destroy_inode(ASSERT_ALWAYS (!xfs_iflags_test (ip,XFS_IRECLAIM ));/* -* If we have nothing to flush with this inode then complete the -* teardown now, otherwise delay the flush operation. +* we always use background reclaim here because even if the +* inode is clean, it still may be under IO and hence we have +* to take the flush lock. The background reclaim path handles +* this more efficiently than we can here, so simply let background +* reclaim tear down all inodes. */ -if (!xfs_inode_clean (ip)) { -xfs_inode_set_reclaim_tag (ip); -return ; - } - out_reclaim: -xfs_ireclaim (ip); +xfs_inode_set_reclaim_tag (ip); }/*
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/4] xfs: reclaim all inodes by background tree walks
Quoted 14 lines · Dave Chinner
We cannot do direct inode reclaim without taking the flush lock to ensure that we do not reclaim an inode under IO. We check the inode is clean before doing direct reclaim, but this is not good enough because the inode flush code marks the inode clean once it has copied the in-core dirty state to the backing buffer.
It is the flush lock that determines whether the inode is still under IO, even though it is marked clean, and the inode is still required at IO completion so we can't reclaim it even though it is clean in core. Hence the requirement that we need to take the flush lock even on clean inodes because this guarantees that the inode writeback IO has completed and it is safe to reclaim the inode.
Quoted 4 lines
With delayed write inode flushing, we coul dend up waiting a long time on the flush lock even for a clean inode. The background reclaim already handles this efficiently, so avoid all the problems by killing the direct reclaim path altogether.
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/4 xfs: Remove inode iolock held check during allocation
In reply to Dave Chinnerlockdep complains about a the lock not being initialised as we do an ASSERT based check that the lock is not held before we initialise it to catch inodes freed with the lock held.
lockdep does this check for us in the lock initialisation code, so remove the ASSERT to stop the lockdep warning.
--- fs/xfs/xfs_iget.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
fs/xfs/xfs_iget.c +0 -1
@@ -73,7 +73,6 @@ xfs_inode_alloc(ASSERT (atomic_read (&ip->i_pincount) ==0 );ASSERT (!spin_is_locked (&ip->i_flags_lock));ASSERT (completion_done (&ip->i_flush)); -ASSERT (!rwsem_is_locked (&ip->i_iolock.mr_lock));mrlock_init (&ip->i_iolock,MRLOCK_BARRIER ,"xfsio" , ip->i_ino);
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/4] xfs: Remove inode iolock held check during allocation
Quoted 6 lines · Dave Chinner
lockdep complains about a the lock not being initialised as we do an ASSERT based check that the lock is not held before we initialise it to catch inodes freed with the lock held.
lockdep does this check for us in the lock initialisation code, so remove the ASSERT to stop the lockdep warning.
Yes, checking things on the lock before it's initialized is always a bad idea. I think this is my fault because I moved the iolock initialization from the inode_init_always path into the inode_init_once path.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs