[PATCH] xfs: truncate_setsize should be outside transactions

[PATCH] xfs: truncate_setsize should be outside transactions

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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().

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_iops.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ef1ca01..84db577 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -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);
 
 	/*
-- 
1.9.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: truncate_setsize should be outside transactions

[Christoph Hellwig]

On Fri, May 02, 2014 at 08:39:39AM +1000, Dave Chinner wrote:
> 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().

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: truncate_setsize should be outside transactions

[Christoph Hellwig]

On Thu, May 01, 2014 at 09:54:43PM -0700, Christoph Hellwig wrote:
> On Fri, May 02, 2014 at 08:39:39AM +1000, Dave Chinner wrote:
> > 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().
> 
> 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).

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: truncate_setsize should be outside transactions

[Dave Chinner]

On Thu, May 01, 2014 at 10:00:53PM -0700, Christoph Hellwig wrote:
> On Thu, May 01, 2014 at 09:54:43PM -0700, Christoph Hellwig wrote:
> > On Fri, May 02, 2014 at 08:39:39AM +1000, Dave Chinner wrote:
> > > 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().
> > 
> > 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).
> 
> 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.

OK, that seems reasonable. I'll add a comment to ensure that we
don't break it in future ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V2] xfs: truncate_setsize should be outside transactions

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_iops.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ef1ca01..ab2dc47 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: truncate_setsize should be outside transactions

[Christoph Hellwig]

On Fri, May 02, 2014 at 05:00:54PM +1000, Dave Chinner wrote:
> 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.

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:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: truncate_setsize should be outside transactions

[Brian Foster]

On Fri, May 02, 2014 at 05:00:54PM +1000, Dave Chinner wrote:
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

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...

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_iops.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ef1ca01..ab2dc47 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: truncate_setsize should be outside transactions

[Dave Chinner]

On Fri, May 02, 2014 at 03:08:02AM -0700, Christoph Hellwig wrote:
> On Fri, May 02, 2014 at 05:00:54PM +1000, Dave Chinner wrote:
> > 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.
> 
> 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.

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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: truncate_setsize should be outside transactions

[Christoph Hellwig]

On Sat, May 03, 2014 at 09:23:39AM +1000, Dave Chinner wrote:
> 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....

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: truncate_setsize should be outside transactions

[Dave Chinner]

On Sat, May 03, 2014 at 08:16:01AM -0700, Christoph Hellwig wrote:
> On Sat, May 03, 2014 at 09:23:39AM +1000, Dave Chinner wrote:
> > 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....
> 
> I don't think we can even get the ENOMEM.

We can - we pass KM_MAYFAIL to xlog_ticket_alloc() from
xfs_log_reserve().

> 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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V3] xfs: truncate_setsize should be outside transactions

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---

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(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ef1ca01..9ef6394 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V3] xfs: truncate_setsize should be outside transactions

[Christoph Hellwig]

On Mon, May 05, 2014 at 03:19:42PM +1000, Dave Chinner wrote:
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs