[PATCH] xfs: fix indlen accounting error on partial delalloc conversion

[PATCH] xfs: fix indlen accounting error on partial delalloc conversion

[Brian Foster]

The delalloc -> real block conversion path uses an incorrect
calculation in the case where the middle part of a delalloc extent
is being converted. This is documented as a rare situation because
XFS generally attempts to maximize contiguity by converting as much
of a delalloc extent as possible.

If this situation does occur, the indlen reservation for the two new
delalloc extents left behind by the conversion of the middle range
is calculated and compared with the original reservation. If more
blocks are required, the delta is allocated from the global block
pool. This delta value can be characterized as the difference
between the new total requirement (temp + temp2) and the currently
available reservation minus those blocks that have already been
allocated (startblockval(PREV.br_startblock) - allocated).

The problem is that the current code does not account for previously
allocated blocks correctly. It subtracts the current allocation
count from the (new - old) delta rather than the old indlen
reservation. This means that more indlen blocks than have been
allocated end up stashed in the remaining extents and free space
accounting is broken as a result.

Fix up the calculation to subtract the allocated block count from
the original extent indlen and thus correctly allocate the
reservation delta based on the difference between the new total
requirement and the unused blocks from the original reservation.
Also remove a bogus assert that contradicts the fact that the new
indlen reservation can be larger than the original indlen
reservation.

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

Hi all,

So while I fortunately was able to reproduce this problem and verify the
patch, I'm not able to do so in any way that facilitates a generic test
case. To reproduce, I had to hack up the allocation path to always do
single physical block allocations and otherwise not attempt to convert
whole delalloc extents, hack up an ioctl to do a
filemap_write_and_wait_range(), and run a test that repeatedly does 1M
buffered writes followed by fragmented, page-sized writebacks of that 1M
extent. This forces partial (middle) extent conversions until the extent
tree grows enough to require a bmbt block allocation. When that occurs,
I always end up with the following free space accounting inconsistency
reported via xfs_repair:

Phase 1 - find and verify superblock...     
Phase 2 - using internal log
        - zero log...               
        - scan filesystem freespace and inode maps...
sb_fdblocks 3929316, counted 3929314             
...

With this patch applied, the same test no longer reproduces the
corruption. This otherwise survives my xfstests testing (without the
aforementioned hacks). Thoughts, reviews, flames appreciated.

Brian

 fs/xfs/libxfs/xfs_bmap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index f02eb76..8adb91b 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -2065,8 +2065,10 @@ xfs_bmap_add_extent_delay_real(
 		}
 		temp = xfs_bmap_worst_indlen(bma->ip, temp);
 		temp2 = xfs_bmap_worst_indlen(bma->ip, temp2);
-		diff = (int)(temp + temp2 - startblockval(PREV.br_startblock) -
-			(bma->cur ? bma->cur->bc_private.b.allocated : 0));
+		diff = (int)(temp + temp2 -
+			     (startblockval(PREV.br_startblock) -
+			      (bma->cur ?
+			       bma->cur->bc_private.b.allocated : 0)));
 		if (diff > 0) {
 			error = xfs_mod_fdblocks(bma->ip->i_mount,
 						 -((int64_t)diff), false);
@@ -2123,7 +2125,6 @@ xfs_bmap_add_extent_delay_real(
 		temp = da_new;
 		if (bma->cur)
 			temp += bma->cur->bc_private.b.allocated;
-		ASSERT(temp <= da_old);
 		if (temp < da_old)
 			xfs_mod_fdblocks(bma->ip->i_mount,
 					(int64_t)(da_old - temp), false);
-- 
2.7.4

Re: [PATCH] xfs: fix indlen accounting error on partial delalloc conversion

[Darrick J. Wong]

On Wed, May 03, 2017 at 02:46:26PM -0400, Brian Foster wrote:
> The delalloc -> real block conversion path uses an incorrect
> calculation in the case where the middle part of a delalloc extent
> is being converted. This is documented as a rare situation because
> XFS generally attempts to maximize contiguity by converting as much
> of a delalloc extent as possible.
> 
> If this situation does occur, the indlen reservation for the two new
> delalloc extents left behind by the conversion of the middle range
> is calculated and compared with the original reservation. If more
> blocks are required, the delta is allocated from the global block
> pool. This delta value can be characterized as the difference
> between the new total requirement (temp + temp2) and the currently
> available reservation minus those blocks that have already been
> allocated (startblockval(PREV.br_startblock) - allocated).
> 
> The problem is that the current code does not account for previously
> allocated blocks correctly. It subtracts the current allocation
> count from the (new - old) delta rather than the old indlen
> reservation. This means that more indlen blocks than have been
> allocated end up stashed in the remaining extents and free space
> accounting is broken as a result.
> 
> Fix up the calculation to subtract the allocated block count from
> the original extent indlen and thus correctly allocate the
> reservation delta based on the difference between the new total
> requirement and the unused blocks from the original reservation.
> Also remove a bogus assert that contradicts the fact that the new
> indlen reservation can be larger than the original indlen
> reservation.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Hi all,
> 
> So while I fortunately was able to reproduce this problem and verify the
> patch, I'm not able to do so in any way that facilitates a generic test
> case. To reproduce, I had to hack up the allocation path to always do
> single physical block allocations and otherwise not attempt to convert
> whole delalloc extents, hack up an ioctl to do a
> filemap_write_and_wait_range(), and run a test that repeatedly does 1M
> buffered writes followed by fragmented, page-sized writebacks of that 1M
> extent. This forces partial (middle) extent conversions until the extent
> tree grows enough to require a bmbt block allocation. When that occurs,
> I always end up with the following free space accounting inconsistency
> reported via xfs_repair:
> 
> Phase 1 - find and verify superblock...     
> Phase 2 - using internal log
>         - zero log...               
>         - scan filesystem freespace and inode maps...
> sb_fdblocks 3929316, counted 3929314             
> ...
> 
> With this patch applied, the same test no longer reproduces the
> corruption. This otherwise survives my xfstests testing (without the
> aforementioned hacks). Thoughts, reviews, flames appreciated.
> 
> Brian
> 
>  fs/xfs/libxfs/xfs_bmap.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index f02eb76..8adb91b 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -2065,8 +2065,10 @@ xfs_bmap_add_extent_delay_real(
>  		}
>  		temp = xfs_bmap_worst_indlen(bma->ip, temp);
>  		temp2 = xfs_bmap_worst_indlen(bma->ip, temp2);
> -		diff = (int)(temp + temp2 - startblockval(PREV.br_startblock) -
> -			(bma->cur ? bma->cur->bc_private.b.allocated : 0));
> +		diff = (int)(temp + temp2 -
> +			     (startblockval(PREV.br_startblock) -
> +			      (bma->cur ?
> +			       bma->cur->bc_private.b.allocated : 0)));

<thinking aloud again>

Prior to this operation we made a delalloc reservation in which we
reserved a number of blocks for bmbt expansion by subtracting that
quantity from incore fdblocks; that is startblockval(PREV.br_startblock).
Then we did some bmbt magic which used up cur->bc_private.b.allocated
blocks from the reservation.  Since those blocks were already subtracted
from the incore fdblocks, we don't need to do that now.  After the magic
was over, we now have two smaller delalloc reservations (with a real
extent in the middle), each with their own reservations for bmbt shape
changing expansion.

Now we aim to make the accounting of incore fdblocks square again by
checking if we are now hanging on to more blocks than we were before and
subtracting the difference from fdblocks if so.  (We deal separately with
the possibility of hanging onto /fewer/ blocks than before.)

Ok, so first we reserved startblockval(PREV.br_startblock).

After splitting the da extent in the middle we now have two da extents;
the effective indlen reservation of the whole range we just touched is:
xfs_bmap_worst_indlen(bma->ip, temp) + xfs_bmap_worst_indlen(bma->ip, temp2);

Then we (may have) fed bma->cur->bc_private.b.allocated blocks to the bmbt.
The number of blocks we now know about is:

xfs_bmap_worst_indlen(bma->ip, temp) +
xfs_bmap_worst_indlen(bma->ip, temp2) +
bma->cur->bc_private.b.allocated

Therefore, the number of blocks we are over (if at all) is:

xfs_bmap_worst_indlen(bma->ip, temp) +
xfs_bmap_worst_indlen(bma->ip, temp2) +
bma->cur->bc_private.b.allocated -
startblockval(PREV.br_startblock)

Or, put another way:

(xfs_bmap_worst_indlen(bma->ip, temp) +
 xfs_bmap_worst_indlen(bma->ip, temp2)) -
(startblockval(PREV.br_startblock) -
 bma->cur->bc_private.b.allocated)

(Exactly what Brian wrote above.)

If that quantity is positive, we're now hanging on to more blocks than
we were before, so we subtract that many from fdblocks.  That part looks
ok, though I have one question -- why is diff an int?  temp/temp2 are
both xfs_filblks_t (__uint64_t), so the whole expression is promoted to
a 64-bit operation, then the results are squashed down into a 32-bit
int, which is then cast back to int64_t.  With our short MAXEXTLEN I
think it's impossible ever to have diff > 2^32, but the casting back and
forth caught my eye.

Aside from that, it looks ok...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  		if (diff > 0) {
>  			error = xfs_mod_fdblocks(bma->ip->i_mount,
>  						 -((int64_t)diff), false);
> @@ -2123,7 +2125,6 @@ xfs_bmap_add_extent_delay_real(
>  		temp = da_new;
>  		if (bma->cur)
>  			temp += bma->cur->bc_private.b.allocated;
> -		ASSERT(temp <= da_old);
>  		if (temp < da_old)
>  			xfs_mod_fdblocks(bma->ip->i_mount,
>  					(int64_t)(da_old - temp), false);
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: fix indlen accounting error on partial delalloc conversion

[Brian Foster]

On Tue, May 09, 2017 at 09:09:48AM -0700, Darrick J. Wong wrote:
> On Wed, May 03, 2017 at 02:46:26PM -0400, Brian Foster wrote:
> > The delalloc -> real block conversion path uses an incorrect
> > calculation in the case where the middle part of a delalloc extent
> > is being converted. This is documented as a rare situation because
> > XFS generally attempts to maximize contiguity by converting as much
> > of a delalloc extent as possible.
> > 
> > If this situation does occur, the indlen reservation for the two new
> > delalloc extents left behind by the conversion of the middle range
> > is calculated and compared with the original reservation. If more
> > blocks are required, the delta is allocated from the global block
> > pool. This delta value can be characterized as the difference
> > between the new total requirement (temp + temp2) and the currently
> > available reservation minus those blocks that have already been
> > allocated (startblockval(PREV.br_startblock) - allocated).
> > 
> > The problem is that the current code does not account for previously
> > allocated blocks correctly. It subtracts the current allocation
> > count from the (new - old) delta rather than the old indlen
> > reservation. This means that more indlen blocks than have been
> > allocated end up stashed in the remaining extents and free space
> > accounting is broken as a result.
> > 
> > Fix up the calculation to subtract the allocated block count from
> > the original extent indlen and thus correctly allocate the
> > reservation delta based on the difference between the new total
> > requirement and the unused blocks from the original reservation.
> > Also remove a bogus assert that contradicts the fact that the new
> > indlen reservation can be larger than the original indlen
> > reservation.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > Hi all,
> > 
> > So while I fortunately was able to reproduce this problem and verify the
> > patch, I'm not able to do so in any way that facilitates a generic test
> > case. To reproduce, I had to hack up the allocation path to always do
> > single physical block allocations and otherwise not attempt to convert
> > whole delalloc extents, hack up an ioctl to do a
> > filemap_write_and_wait_range(), and run a test that repeatedly does 1M
> > buffered writes followed by fragmented, page-sized writebacks of that 1M
> > extent. This forces partial (middle) extent conversions until the extent
> > tree grows enough to require a bmbt block allocation. When that occurs,
> > I always end up with the following free space accounting inconsistency
> > reported via xfs_repair:
> > 
> > Phase 1 - find and verify superblock...     
> > Phase 2 - using internal log
> >         - zero log...               
> >         - scan filesystem freespace and inode maps...
> > sb_fdblocks 3929316, counted 3929314             
> > ...
> > 
> > With this patch applied, the same test no longer reproduces the
> > corruption. This otherwise survives my xfstests testing (without the
> > aforementioned hacks). Thoughts, reviews, flames appreciated.
> > 
> > Brian
> > 
> >  fs/xfs/libxfs/xfs_bmap.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index f02eb76..8adb91b 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -2065,8 +2065,10 @@ xfs_bmap_add_extent_delay_real(
> >  		}
> >  		temp = xfs_bmap_worst_indlen(bma->ip, temp);
> >  		temp2 = xfs_bmap_worst_indlen(bma->ip, temp2);
> > -		diff = (int)(temp + temp2 - startblockval(PREV.br_startblock) -
> > -			(bma->cur ? bma->cur->bc_private.b.allocated : 0));
> > +		diff = (int)(temp + temp2 -
> > +			     (startblockval(PREV.br_startblock) -
> > +			      (bma->cur ?
> > +			       bma->cur->bc_private.b.allocated : 0)));
> 
> <thinking aloud again>
> 
> Prior to this operation we made a delalloc reservation in which we
> reserved a number of blocks for bmbt expansion by subtracting that
> quantity from incore fdblocks; that is startblockval(PREV.br_startblock).
> Then we did some bmbt magic which used up cur->bc_private.b.allocated
> blocks from the reservation.  Since those blocks were already subtracted
> from the incore fdblocks, we don't need to do that now.  After the magic
> was over, we now have two smaller delalloc reservations (with a real
> extent in the middle), each with their own reservations for bmbt shape
> changing expansion.
> 
> Now we aim to make the accounting of incore fdblocks square again by
> checking if we are now hanging on to more blocks than we were before and
> subtracting the difference from fdblocks if so.  (We deal separately with
> the possibility of hanging onto /fewer/ blocks than before.)
> 
> Ok, so first we reserved startblockval(PREV.br_startblock).
> 
> After splitting the da extent in the middle we now have two da extents;
> the effective indlen reservation of the whole range we just touched is:
> xfs_bmap_worst_indlen(bma->ip, temp) + xfs_bmap_worst_indlen(bma->ip, temp2);
> 
> Then we (may have) fed bma->cur->bc_private.b.allocated blocks to the bmbt.
> The number of blocks we now know about is:
> 
> xfs_bmap_worst_indlen(bma->ip, temp) +
> xfs_bmap_worst_indlen(bma->ip, temp2) +
> bma->cur->bc_private.b.allocated
> 
> Therefore, the number of blocks we are over (if at all) is:
> 
> xfs_bmap_worst_indlen(bma->ip, temp) +
> xfs_bmap_worst_indlen(bma->ip, temp2) +
> bma->cur->bc_private.b.allocated -
> startblockval(PREV.br_startblock)
> 
> Or, put another way:
> 
> (xfs_bmap_worst_indlen(bma->ip, temp) +
>  xfs_bmap_worst_indlen(bma->ip, temp2)) -
> (startblockval(PREV.br_startblock) -
>  bma->cur->bc_private.b.allocated)
> 
> (Exactly what Brian wrote above.)
> 
> If that quantity is positive, we're now hanging on to more blocks than
> we were before, so we subtract that many from fdblocks.  That part looks
> ok, though I have one question -- why is diff an int?  temp/temp2 are
> both xfs_filblks_t (__uint64_t), so the whole expression is promoted to
> a 64-bit operation, then the results are squashed down into a 32-bit
> int, which is then cast back to int64_t.  With our short MAXEXTLEN I
> think it's impossible ever to have diff > 2^32, but the casting back and
> forth caught my eye.
> 

As to why it is signed, I'd guess that either it was coded that way to
avoid a separate conditional or perhaps diff was previously used
elsewhere in this function..? I'm not really sure why it uses 32-bit vs.
64-bit tbh. (I don't think it matters, but I'm fine if you want to use
an int64_t or something and clean up the casting).

> Aside from that, it looks ok...
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 

Thanks for review.

Brian

> --D
> 
> >  		if (diff > 0) {
> >  			error = xfs_mod_fdblocks(bma->ip->i_mount,
> >  						 -((int64_t)diff), false);
> > @@ -2123,7 +2125,6 @@ xfs_bmap_add_extent_delay_real(
> >  		temp = da_new;
> >  		if (bma->cur)
> >  			temp += bma->cur->bc_private.b.allocated;
> > -		ASSERT(temp <= da_old);
> >  		if (temp < da_old)
> >  			xfs_mod_fdblocks(bma->ip->i_mount,
> >  					(int64_t)(da_old - temp), false);
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html