xfs: truncate_setsize should be outside transactions
PATCH 1/1 xfs: truncate_setsize should be outside transactions
From: Dave Chinner <[email protected]>
truncate_setsize() removes pages from the page cache, and hence requires page locks to be held. It is not valid to lock a page cache page inside a transaction context as we can hold page locks when we we reserve space for a transaction. If we do, then we expose an ABBA deadlock between log space reservation and page locks.
That is, both the write path and writeback lock a page, then start a transaction for block allocation, which means they can block waiting for a log reservation with the page lock held. If we hold a log reservation and then do something that locks a page (e.g. truncate_setsize in xfs_setattr_size) then that page lock can block on the page locked and waiting for a log reservation. If the transaction that is waiting for the page lock is the only active transaction in the system that can free log space via a commit, then writeback will never make progress and so log space will never free up.
This issue with xfs_setattr_size() was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what was xfs_itruncate_data()) to inside the transaction context as a call to truncate_setsize().
--- fs/xfs/xfs_iops.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
fs/xfs/xfs_iops.c +7 -4
@@ -808,22 +808,25 @@ xfs_setattr_size(*/ inode_dio_wait (inode); +/* +* Do all the page cache truncate work outside the transaction +* context as the "lock" order is page lock->log space reservation. +* i.e. locking pages inside the transaction can ABBA deadlock with +* writeback. +*/ error = -block_truncate_page (inode->i_mapping, newsize, xfs_get_blocks);if (error)return error; +truncate_setsize (inode, newsize); tp =xfs_trans_alloc (mp,XFS_TRANS_SETATTR_SIZE ); error =xfs_trans_reserve (tp, &M_RES (mp)->tr_itruncate,0 ,0 );if (error) goto out_trans_cancel; -truncate_setsize (inode, newsize); - commit_flags =XFS_TRANS_RELEASE_LOG_RES ; lock_flags |=XFS_ILOCK_EXCL ; -xfs_ilock (ip,XFS_ILOCK_EXCL ); -xfs_trans_ijoin (tp, ip,0 );/*
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 5 lines · Dave Chinner
This issue with
xfs_setattr_size()was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what wasxfs_itruncate_data()) to inside the transaction context as a call totruncate_setsize().
And it was moved because we should only call truncate_setsize once the truncate can't fail any more. So to move it out of transaction context it needs to move after the commit of the transaction(s).
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 10 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner
This issue with
xfs_setattr_size()was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what wasxfs_itruncate_data()) to inside the transaction context as a call totruncate_setsize().And it was moved because we should only call
truncate_setsizeonce the truncate can't fail any more. So to move it out of transaction context it needs to move after the commit of the transaction(s).
Actually that's only true for the i_size update. So I guess we need to call truncate_pagecache were you put the truncate_setsize now, and then update i_size later, together with the updates of the XFS di_size.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner
This issue with
xfs_setattr_size()was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what wasxfs_itruncate_data()) to inside the transaction context as a call totruncate_setsize().
Quoted 8 lines
Quoted 3 lines
And it was moved because we should only call
truncate_setsizeonce the truncate can't fail any more. So to move it out of transaction context it needs to move after the commit of the transaction(s).Actually that's only true for the
i_sizeupdate. So I guess we need to calltruncate_pagecachewere you put thetruncate_setsizenow, and then updatei_sizelater, together with the updates of the XFSdi_size.
OK, that seems reasonable. I'll add a comment to ensure that we don't break it in future ;)
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/1 xfs: truncate_setsize should be outside transactions
From: Dave Chinner <[email protected]>
truncate_setsize() removes pages from the page cache, and hence requires page locks to be held. It is not valid to lock a page cache page inside a transaction context as we can hold page locks when we we reserve space for a transaction. If we do, then we expose an ABBA deadlock between log space reservation and page locks.
That is, both the write path and writeback lock a page, then start a transaction for block allocation, which means they can block waiting for a log reservation with the page lock held. If we hold a log reservation and then do something that locks a page (e.g. truncate_setsize in xfs_setattr_size) then that page lock can block on the page locked and waiting for a log reservation. If the transaction that is waiting for the page lock is the only active transaction in the system that can free log space via a commit, then writeback will never make progress and so log space will never free up.
This issue with xfs_setattr_size() was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what was xfs_itruncate_data()) to inside the transaction context as a call to truncate_setsize().
The reason truncate_setsize() was located where in this place was that we can't change the file size until after we are in the transaction context and the operation will either succeed or shut down the filesystem on failure. Hence we have to split truncate_setsize() back into a pagecache operation that occurs before the transaction context, and a i_size_write() call that happens within the transaction context.
--- fs/xfs/xfs_iops.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
fs/xfs/xfs_iops.c +10 -4
@@ -808,22 +808,27 @@ xfs_setattr_size(*/ inode_dio_wait (inode); +/* +* Do all the page cache truncate work outside the transaction +* context as the "lock" order is page lock->log space reservation. +* i.e. locking pages inside the transaction can ABBA deadlock with +* writeback. We have to do the inode size update inside the +* transaction, however, as xfs_trans_reserve() can fail with ENOMEM +* and we can't make user visible changes on non-fatal errors. +*/ error = -block_truncate_page (inode->i_mapping, newsize, xfs_get_blocks);if (error)return error; +truncate_pagecache (inode, newsize); tp =xfs_trans_alloc (mp,XFS_TRANS_SETATTR_SIZE ); error =xfs_trans_reserve (tp, &M_RES (mp)->tr_itruncate,0 ,0 );if (error) goto out_trans_cancel; -truncate_setsize (inode, newsize); - commit_flags =XFS_TRANS_RELEASE_LOG_RES ; lock_flags |=XFS_ILOCK_EXCL ; -xfs_ilock (ip,XFS_ILOCK_EXCL ); -xfs_trans_ijoin (tp, ip,0 );/*
@@ -856,6 +861,7 @@ xfs_setattr_size(* they get written to. */ ip->i_d.di_size = newsize; +i_size_write (inode, newsize);xfs_trans_log_inode (tp, ip,XFS_ILOG_CORE );if (newsize <= oldsize) {
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines · Dave Chinner
The reason
truncate_setsize()was located where in this place was that we can't change the file size until after we are in the transaction context and the operation will either succeed or shut down the filesystem on failure. Hence we have to splittruncate_setsize()back into a pagecache operation that occurs before the transaction context, and ai_size_write()call that happens within the transaction context.
Further updating myself earlier on the comment next to truncate_pagecache claims that the file size must have been updated before, but I can't see a reason for that.
This version looks fine to me:
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · Dave Chinner
The reason
truncate_setsize()was located where in this place was that we can't change the file size until after we are in the transaction context and the operation will either succeed or shut down the filesystem on failure. Hence we have to splittruncate_setsize()back into a pagecache operation that occurs before the transaction context, and ai_size_write()call that happens within the transaction context.
Quoted 3 lines
Further updating myself earlier on the comment next to
truncate_pagecacheclaims that the file size must have been updated before, but I can't see a reason for that.
Oh, I can, and that reminds me of why - racing with mmap page faults, which aren't serialised against truncate except by an indirect combination of the page locks and i_size updates. hence if we remove the pages before updating the inode size, then a page fault can re-instantiate a page after the truncation beyond the new EOF when, in fact, it should SEGV.
So, no, we can't split truncate_setsize() like this.
As it is, we've already made a user visible data change in the truncate process before we get to the transaction that can fail:block_truncate_page() zeroes the tail of the page cache page. Hence if the transaction reservation fails, we've already trashed the file data - we may as well finish off the job and at least make it look like the truncate succeeded from a user point of view. They then get a ENOMEM error (only non-fatal error that can come from xfs_trans_reserve) and try the truncate again....
So I now think the first version of the patch is better than this one..
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines · Dave Chinner
before we get to the transaction that can fail:
block_truncate_page()zeroes the tail of the page cache page. Hence if the transaction reservation fails, we've already trashed the file data - we may as well finish off the job and at least make it look like the truncate succeeded from a user point of view. They then get a ENOMEM error (only non-fatal error that can come fromxfs_trans_reserve) and try the truncate again....
I don't think we can even get the ENOMEM. But yeah, I guess we want something like the old version, with comments explaining exactly we we have this order.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 10 lines · Christoph Hellwig
Quoted 7 lines · Dave Chinner
before we get to the transaction that can fail:
block_truncate_page()zeroes the tail of the page cache page. Hence if the transaction reservation fails, we've already trashed the file data - we may as well finish off the job and at least make it look like the truncate succeeded from a user point of view. They then get a ENOMEM error (only non-fatal error that can come fromxfs_trans_reserve) and try the truncate again....I don't think we can even get the ENOMEM.
We can - we pass KM_MAYFAIL to xlog_ticket_alloc() from xfs_log_reserve().
Quoted 3 lines
But yeah, I guess
we want something like the old version, with comments explaining exactly we we have this order.
I'll send another version of the first patch with an expanded comment.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v3 1/1 xfs: truncate_setsize should be outside transactions
From: Dave Chinner <[email protected]>
truncate_setsize() removes pages from the page cache, and hence requires page locks to be held. It is not valid to lock a page cache page inside a transaction context as we can hold page locks when we we reserve space for a transaction. If we do, then we expose an ABBA deadlock between log space reservation and page locks.
That is, both the write path and writeback lock a page, then start a transaction for block allocation, which means they can block waiting for a log reservation with the page lock held. If we hold a log reservation and then do something that locks a page (e.g. truncate_setsize in xfs_setattr_size) then that page lock can block on the page locked and waiting for a log reservation. If the transaction that is waiting for the page lock is the only active transaction in the system that can free log space via a commit, then writeback will never make progress and so log space will never free up.
This issue with xfs_setattr_size() was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what was xfs_itruncate_data()) to inside the transaction context as a call to truncate_setsize().
The reason truncate_setsize() was located where in this place was that we can't shouldn't change the file size until after we are in the transaction context and the operation will either succeed or shut down the filesystem on failure. However, block_truncate_page() already modifies the file contents before we enter the transaction context, so we can't really fulfill this guarantee in any way. Hence we may as well ensure that on success or failure, the in-memory inode and data is truncated away and that the application cleans up the mess appropriately.
---
V3 - revert back to original fix but expand the comment to explain why the truncate is done this way.
fs/xfs/xfs_iops.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
fs/xfs/xfs_iops.c +16 -4
@@ -808,22 +808,34 @@ xfs_setattr_size(*/ inode_dio_wait (inode); +/* +* Do all the page cache truncate work outside the transaction context +* as the "lock" order is page lock->log space reservation. i.e. +* locking pages inside the transaction can ABBA deadlock with +* writeback. We have to do the VFS inode size update before we truncate +* the pagecache, however, to avoid racing with page faults beyond the +* new EOF they are not serialised against truncate operations except by +* page locks and size updates. +* +* Hence we are in a situation where a truncate can fail with ENOMEM +* from xfs_trans_reserve(), but having already truncated the in-memory +* version of the file (i.e. made user visible changes). There's not +* much we can do about this, except to hope that the caller sees ENOMEM +* and retries the truncate operation. +*/ error = -block_truncate_page (inode->i_mapping, newsize, xfs_get_blocks);if (error)return error; +truncate_setsize (inode, newsize); tp =xfs_trans_alloc (mp,XFS_TRANS_SETATTR_SIZE ); error =xfs_trans_reserve (tp, &M_RES (mp)->tr_itruncate,0 ,0 );if (error) goto out_trans_cancel; -truncate_setsize (inode, newsize); - commit_flags =XFS_TRANS_RELEASE_LOG_RES ; lock_flags |=XFS_ILOCK_EXCL ; -xfs_ilock (ip,XFS_ILOCK_EXCL ); -xfs_trans_ijoin (tp, ip,0 );/*
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 36 lines · Dave Chinner
From: Dave Chinner <[email protected]>
truncate_setsize()removes pages from the page cache, and hence requires page locks to be held. It is not valid to lock a page cache page inside a transaction context as we can hold page locks when we we reserve space for a transaction. If we do, then we expose an ABBA deadlock between log space reservation and page locks.That is, both the write path and writeback lock a page, then start a transaction for block allocation, which means they can block waiting for a log reservation with the page lock held. If we hold a log reservation and then do something that locks a page (e.g.
truncate_setsizeinxfs_setattr_size) then that page lock can block on the page locked and waiting for a log reservation. If the transaction that is waiting for the page lock is the only active transaction in the system that can free log space via a commit, then writeback will never make progress and so log space will never free up.This issue with
xfs_setattr_size()was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what wasxfs_itruncate_data()) to inside the transaction context as a call totruncate_setsize().The reason
truncate_setsize()was located where in this place was that we can't shouldn't change the file size until after we are in the transaction context and the operation will either succeed or shut down the filesystem on failure. However,block_truncate_page()already modifies the file contents before we enter the transaction context, so we can't really fulfill this guarantee in any way. Hence we may as well ensure that on success or failure, the in-memory inode and data is truncated away and that the application cleans up the mess appropriately.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 34 lines · Dave Chinner
From: Dave Chinner <[email protected]>
truncate_setsize()removes pages from the page cache, and hence requires page locks to be held. It is not valid to lock a page cache page inside a transaction context as we can hold page locks when we we reserve space for a transaction. If we do, then we expose an ABBA deadlock between log space reservation and page locks.That is, both the write path and writeback lock a page, then start a transaction for block allocation, which means they can block waiting for a log reservation with the page lock held. If we hold a log reservation and then do something that locks a page (e.g.
truncate_setsizeinxfs_setattr_size) then that page lock can block on the page locked and waiting for a log reservation. If the transaction that is waiting for the page lock is the only active transaction in the system that can free log space via a commit, then writeback will never make progress and so log space will never free up.This issue with
xfs_setattr_size()was introduced back in 2010 by commit fa9b227 ("xfs: new truncate sequence") which moved the page cache truncate from outside the transaction context (what wasxfs_itruncate_data()) to inside the transaction context as a call totruncate_setsize().The reason
truncate_setsize()was located where in this place was that we can't change the file size until after we are in the transaction context and the operation will either succeed or shut down the filesystem on failure. Hence we have to splittruncate_setsize()back into a pagecache operation that occurs before the transaction context, and ai_size_write()call that happens within the transaction context.
Quoted 2 lines
Signed-off-by Dave Chinner <[email protected]>---
The manifestation of this that we have seen has writeback blocked on log reservation, and a thread sitting on this:
#0 [ffff88022e153b18] __schedule at ffffffff815f137d
#1 [ffff88022e153b80] io_schedule at ffffffff815f1bdd
#2 [ffff88022e153b98] sleep_on_page at ffffffff811410be #3 [ffff88022e153ba8] __wait_on_bit at ffffffff815ef940 #4 [ffff88022e153be8] wait_on_page_bit at ffffffff81140e46 #5 [ffff88022e153c38] truncate_inode_pages_range at ffffffff81150d03 #6 [ffff88022e153d88] truncate_pagecache at ffffffff81151027 #7 [ffff88022e153db0] truncate_setsize at ffffffff81151059 #8 [ffff88022e153dc0] xfs_setattr_size at ffffffffa01f3594 [xfs] #9 [ffff88022e153e10] xfs_vn_setattr at ffffffffa01f37e0 [xfs]
#10 [ffff88022e153e30] notify_change at ffffffff811cc349 #11 [ffff88022e153e78] do_truncate at ffffffff811adb43
#12 [ffff88022e153ef0] vfs_truncate at ffffffff811adcf1 #13 [ffff88022e153f28] do_sys_truncate at ffffffff811add9c #14 [ffff88022e153f70] sys_truncate at ffffffff811adf3e #15 [ffff88022e153f80] system_call_fastpath at ffffffff815fc819
That wait_on_page_bit() call maps to this bit of code:
0xffffffff81150cef <truncate_inode_pages_range+879>: mov %rdx,%rdi 0xffffffff81150cf2 <truncate_inode_pages_range+882>: mov $0xd,%esi 0xffffffff81150cf7 <truncate_inode_pages_range+887>: mov %rdx,-0x130(%rbp) 0xffffffff81150cfe <truncate_inode_pages_range+894>: callq 0xffffffff81140dc0 <wait_on_page_bit>
So this thread has basically come in, reserved log space, attempted a truncate and is sitting blocked on writeback. xfs_vm_writepage() has the page, set the writeback bit and attempted a log reservation for the file size update transaction. Therefore, no progress can be made.
Ordered properly, the truncate should either wait on writeback without holding the log space hostage or grab the page lock before writeback is set, allowing either path to proceed once the page is acquired.
Makes sense, thanks for tracking this down...
Quoted 48 lines
fs/xfs/xfs_iops.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)fs/xfs/xfs_iops.c +10
-4@@ -808,22 +808,27 @@ xfs_setattr_size(*/ inode_dio_wait (inode); +/* +* Do all the page cache truncate work outside the transaction +* context as the "lock" order is page lock->log space reservation. +* i.e. locking pages inside the transaction can ABBA deadlock with +* writeback. We have to do the inode size update inside the +* transaction, however, as xfs_trans_reserve() can fail with ENOMEM +* and we can't make user visible changes on non-fatal errors. +*/ error = -block_truncate_page (inode->i_mapping, newsize, xfs_get_blocks);if (error)return error; +truncate_pagecache (inode, newsize); tp =xfs_trans_alloc (mp,XFS_TRANS_SETATTR_SIZE ); error =xfs_trans_reserve (tp, &M_RES (mp)->tr_itruncate,0 ,0 );if (error) goto out_trans_cancel; -truncate_setsize (inode, newsize); - commit_flags =XFS_TRANS_RELEASE_LOG_RES ; lock_flags |=XFS_ILOCK_EXCL ; -xfs_ilock (ip,XFS_ILOCK_EXCL ); -xfs_trans_ijoin (tp, ip,0 );/* @@ -856,6 +861,7 @@ xfs_setattr_size(* they get written to. */ ip->i_d.di_size = newsize; +i_size_write (inode, newsize);xfs_trans_log_inode (tp, ip,XFS_ILOG_CORE );if (newsize <= oldsize) {
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs