| Message ID | 1327091686-23177-5-git-send-email-jack@suse.cz |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Jan 20, 2012 at 09:34:42PM +0100, Jan Kara wrote: > In xfs we first take ilock and start transaction afterwards. The correct order is to allocate the transaction, reserve the space for it and then take the ilock. We cannot hold the ilock over the transaction reservation because that can deadlock the journal. That is, to make space for the new transaction reservation, we may need to take the ilock to flush the inode and allow the journal tail to move forwards to make space for the new transaction. If we already hold the ilock, then it can't be flushed, we can't make space available in the journal and hence deadlock. Maybe you confused the ilock vs the iolock. We can hold the iolock over the trans alloc/reserve because that lock is not required to move the tail of the journal, so the deadlock doesn't exist. > We should obey > this order in all places because otherwise we can create the following deadlock > with filesystem freezing: One process holds ilock and blocks on s_frozen == > SB_FREEZE_TRANS in xfs_trans_alloc(), another process has a transaction started > (thus blocking freezing) and blocks on ilock. So we have to take ilock earlier > in xfs_setattr_size(). Where are we taking the ilock and then calling xfs_trans_alloc()? That's the caller needs to be fixed, not the 40-odd that do the right thing by taking the ilock -after- the trans alloc/reserve calls. Cheers, Dave.
On Tue 24-01-12 17:59:45, Dave Chinner wrote: > On Fri, Jan 20, 2012 at 09:34:42PM +0100, Jan Kara wrote: > > In xfs we first take ilock and start transaction afterwards. > > The correct order is to allocate the transaction, reserve the space > for it and then take the ilock. We cannot hold the ilock over the > transaction reservation because that can deadlock the journal. > > That is, to make space for the new transaction reservation, we may > need to take the ilock to flush the inode and allow the journal tail > to move forwards to make space for the new transaction. If we > already hold the ilock, then it can't be flushed, we can't make > space available in the journal and hence deadlock. Thanks for clarification! > Maybe you confused the ilock vs the iolock. We can hold the iolock > over the trans alloc/reserve because that lock is not required to > move the tail of the journal, so the deadlock doesn't exist. Ups! I now had a look at what xfs_rw_ilock() does. I always thought it's just a plain rw semaphore and now I see it takes several locks depending on the argument. Ugh, a bit surprising for XFS newcomer as me ;) But now things become clearer so I fix my patches with this new knowledge in mind. So just disregard my locking comments. They were likely bogus. Honza
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 23ce927..3579bc8 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -850,6 +850,9 @@ xfs_setattr_size( if (error) goto out_unlock; + xfs_ilock(ip, XFS_ILOCK_EXCL); + lock_flags |= XFS_ILOCK_EXCL; + tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_SIZE); error = xfs_trans_reserve(tp, 0, XFS_ITRUNCATE_LOG_RES(mp), 0, XFS_TRANS_PERM_LOG_RES, @@ -860,9 +863,6 @@ xfs_setattr_size( truncate_setsize(inode, iattr->ia_size); commit_flags = XFS_TRANS_RELEASE_LOG_RES; - lock_flags |= XFS_ILOCK_EXCL; - - xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, 0);
In xfs we first take ilock and start transaction afterwards. We should obey this order in all places because otherwise we can create the following deadlock with filesystem freezing: One process holds ilock and blocks on s_frozen == SB_FREEZE_TRANS in xfs_trans_alloc(), another process has a transaction started (thus blocking freezing) and blocks on ilock. So we have to take ilock earlier in xfs_setattr_size(). CC: Ben Myers <bpm@sgi.com> CC: Alex Elder <elder@kernel.org> Signed-off-by: Jan Kara <jack@suse.cz> --- fs/xfs/xfs_iops.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)