[PATCH] xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

[PATCH] xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

[Alain Renaud]

xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

When trying to do preallocation that exceed the the maximum size of
an extent, the extsize alignment can exceed this value.
We are already trying to solve this issue for delay allocation but we
have the same with prealloc. So I think the simple solution is to limit
the size in xfs_bmap_extsize_align() and remove the code specific to
delay allocation.

We do have a simple test case to confirm that the problem exist.

# cd /xfs_dir/
# xfs_io -c 'extsize 4m' .
# xfs_io -f -c 'resvsp 0 8g' test_file
XFS_IOC_RESVSP64: No space left on device

Signed-off-by: Alain Renaud <arenaud@sgi.com>

---
  fs/xfs/xfs_bmap.c |   12 +++++-------
  1 file changed, 5 insertions(+), 7 deletions(-)

Index: b/fs/xfs/xfs_bmap.c
===================================================================
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1912,6 +1912,11 @@ xfs_bmap_extsize_align(
  		align_alen += extsz - temp;
  	}
  	/*
+	 * Make sure we do not exceed the maximum len of an extent.
+	 */
+	align_alen = XFS_FILBLKS_MIN(align_alen,
+				     MAXEXTLEN - (MAXEXTLEN % extsz));
+	/*
  	 * If the previous block overlaps with this proposed allocation
  	 * then move the start forward without adjusting the length.
  	 */
@@ -4450,13 +4455,6 @@ xfs_bmapi_reserve_delalloc(
  	/* Figure out the extent size, adjust alen */
  	extsz = xfs_get_extsz_hint(ip);
  	if (extsz) {
-		/*
-		 * Make sure we don't exceed a single extent length when we
-		 * align the extent by reducing length we are going to
-		 * allocate by the maximum amount extent size aligment may
-		 * require.
-		 */
-		alen = XFS_FILBLKS_MIN(len, MAXEXTLEN - (2 * extsz - 1));
  		error = xfs_bmap_extsize_align(mp, got, prev, extsz, rt, eof,
  					       1, 0, &aoff, &alen);
  		ASSERT(!error);

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

[PATCH V2] xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

[Alain Renaud]

[-- Attachment #1.1: Type: text/plain, Size: 2497 bytes --]

Here is a second revision. Needed to remove an ASSERT and added a new one.

xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

When trying to do preallocation that exceed the the maximum size of
an extent, the extsize alignment can exceed this value.

We are already trying to solve this issue for delay allocation but we
have the same with prealloc. So I think the simple solution is to limit
the size in xfs_bmap_extsize_align() and remove the code specific to
delay allocation.

We do have a simple test case to confirm that the problem exist.

# cd/xfs_dir/
# xfs_io -c 'extsize 4m' .
# xfs_io -f -c 'resvsp 0 8g' test_file
XFS_IOC_RESVSP64: No space left on device

Signed-off-by: Alain Renaud<arenaud@sgi.com>

---
  fs/xfs/xfs_bmap.c |   14 ++++++--------
  1 file changed, 6 insertions(+), 8 deletions(-)

Index: b/fs/xfs/xfs_bmap.c
===================================================================
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1912,6 +1912,11 @@ xfs_bmap_extsize_align(
  		align_alen += extsz - temp;
  	}
  	/*
+	 * Make sure we do not exceed the maximum len of an extent.
+	 */
+	align_alen = XFS_FILBLKS_MIN(align_alen,
+				     MAXEXTLEN - (MAXEXTLEN % extsz));
+	/*
  	 * If the previous block overlaps with this proposed allocation
  	 * then move the start forward without adjusting the length.
  	 */
@@ -1999,7 +2004,6 @@ xfs_bmap_extsize_align(
  			return XFS_ERROR(EINVAL);
  	} else {
  		ASSERT(orig_off >= align_off);
-		ASSERT(orig_end <= align_off + align_alen);
  	}
  
  #ifdef DEBUG
@@ -2009,6 +2013,7 @@ xfs_bmap_extsize_align(
  		ASSERT(align_off >= prevp->br_startoff + prevp->br_blockcount);
  #endif
  
+	ASSERT(align_alen <= MAXEXTLEN);
  	*lenp = align_alen;
  	*offp = align_off;
  	return 0;
@@ -4450,13 +4455,6 @@ xfs_bmapi_reserve_delalloc(
  	/* Figure out the extent size, adjust alen */
  	extsz = xfs_get_extsz_hint(ip);
  	if (extsz) {
-		/*
-		 * Make sure we don't exceed a single extent length when we
-		 * align the extent by reducing length we are going to
-		 * allocate by the maximum amount extent size aligment may
-		 * require.
-		 */
-		alen = XFS_FILBLKS_MIN(len, MAXEXTLEN - (2 * extsz - 1));
  		error = xfs_bmap_extsize_align(mp, got, prev, extsz, rt, eof,
  					       1, 0, &aoff, &alen);
  		ASSERT(!error);


-- 
===============================================
Alain Renaud     - Cluster File System Engineer
arenaud@sgi.com  - SGI
===============================================




[-- Attachment #1.2: Type: text/html, Size: 3044 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

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

Re: [PATCH V2] xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

[Dave Chinner]

On Tue, Jul 17, 2012 at 06:35:03PM +0200, Alain Renaud wrote:
> Here is a second revision. Needed to remove an ASSERT and added a new one.

FWIW, the change log goes in the section after the first --- (i.e.
where the diffstat is) as it is not needed in the commit message...

> xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.
> 
> When trying to do preallocation that exceed the the maximum size of
> an extent, the extsize alignment can exceed this value.
> 
> We are already trying to solve this issue for delay allocation but we
> have the same with prealloc. So I think the simple solution is to limit
> the size in xfs_bmap_extsize_align() and remove the code specific to
> delay allocation.
> 
> We do have a simple test case to confirm that the problem exist.
> 
> # cd/xfs_dir/
> # xfs_io -c 'extsize 4m' .
> # xfs_io -f -c 'resvsp 0 8g' test_file
> XFS_IOC_RESVSP64: No space left on device

simpler: xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' test_file

I was able to reproduce this, for a short while. Then it went away:

$ /usr/sbin/xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' test_file
XFS_IOC_RESVSP64: No space left on device
$ /usr/sbin/xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' test_file
XFS_IOC_RESVSP64: No space left on device
$ /usr/sbin/xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' -c bmap test_file
XFS_IOC_RESVSP64: No space left on device
test_file: no extents
$ /usr/sbin/xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' -c "bmap -vp" test_file
test_file:
 EXT: FILE-OFFSET           BLOCK-RANGE          AG AG-OFFSET               TOTAL FLAGS
   0: [0..16732159]:        238228480..254960639  8 (11596800..28328959) 16732160 10011
   1: [16732160..16781311]: 445662208..445711359 15 (20727808..20776959)    49152 10011
$ 
$ rm test_file
$ /usr/sbin/xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' -c "bmap -vp" test_file
test_file:
 EXT: FILE-OFFSET           BLOCK-RANGE          AG AG-OFFSET               TOTAL FLAGS
   0: [0..16732159]:        238228480..254960639  8 (11596800..28328959) 16732160 10011
   1: [16732160..16781311]: 445662208..445711359 15 (20727808..20776959)    49152 10011
$ rm test_file
$ /usr/sbin/xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' -c "bmap -vp" test_file
test_file:
 EXT: FILE-OFFSET           BLOCK-RANGE          AG AG-OFFSET               TOTAL FLAGS
   0: [0..16732655]:        238227984..254960639  8 (11596304..28328959) 16732656 11011
   1: [16732656..16781807]: 445662208..445711359 15 (20727808..20776959)    49152 10011
$ rm test_file
$ /usr/sbin/xfs_io -f -c 'extsize 4m' -c 'resvsp 0 8g' -c "bmap -vp" test_file
test_file:
 EXT: FILE-OFFSET           BLOCK-RANGE          AG AG-OFFSET               TOTAL FLAGS
   0: [0..16732655]:        238227984..254960639  8 (11596304..28328959) 16732656 11011
   1: [16732656..16781807]: 445662208..445711359 15 (20727808..20776959)    49152 10011
$ uname -a
Linux disappointment 3.4-trunk-amd64 #1 SMP Tue Jun 26 17:23:03 UTC 2012 x86_64 GNU/Linux


And it appears to be realted to the fact I don't have an AG with an
8G free extent in it. Initially, the trace was indicating the
allocation was asking for a signle 8GB extent, and failing because
it didn't find one, hence ENOSPC. I'd see this:

     kworker/4:0-22    [004] 1223944.610192: xfs_alloc_vextent_loopfailed: dev 9:0 agno 2 agbno 1213074 minlen 2097152 maxlen 2097152 mod 0 prod 1024 minleft 1 total 0 alignment 128 minalignslop 0 len 2661202812 type NEAR_BNO otype START_BNO wasdel 0 wasfromfl 136 isfl 0 userdata 2 firstblock 0xffffffffffffffff

where the minlen/maxlen values indicate the acceptible extent range.

When it started succeeding, I'd see this:

kworker/5:1-49    [005] 1225590.604825: xfs_alloc_vextent_loopfailed: dev 9:0 agno 15 agbno 471304 minlen 1348384 maxlen 2097152 mod 0 prod 1024 minleft 1 total 0 alignment 1 minalignslop 0 len 2348686204 type NEAR_BNO otype START_BNO wasdel 0 wasfromfl 136 isfl 0 userdata 2 firstblock 0xffffffffffffffff

Note the minlen? It's been shortened to reflect the fact I don't
have 8GB freespace extents in the filesystem.

Firstly, can you reproduce this on your test setup, Alain? What is
the size (and number) of the AGs in the filesystem? If you do see
this behaviour, then the alignment code is not the problem...

> +        * Make sure we do not exceed the maximum len of an extent.
> +        */
> +       align_alen = XFS_FILBLKS_MIN(align_alen,
> +                                    MAXEXTLEN - (MAXEXTLEN % extsz));

I don't think that is correct. alignment happens at both the start
and end of the extent, so you need to take into account two lots of
(extsz - 1) blocks of range extension for alignment.

> +	/*
>  	 * If the previous block overlaps with this proposed allocation
>  	 * then move the start forward without adjusting the length.
>  	 */
> @@ -1999,7 +2004,6 @@ xfs_bmap_extsize_align(
>  			return XFS_ERROR(EINVAL);
>  	} else {
>  		ASSERT(orig_off >= align_off);
> -		ASSERT(orig_end <= align_off + align_alen);

Why is this removed? What it is checking is that the aligned end
covers the entire range of the original space requested.

>  	}
>  #ifdef DEBUG
> @@ -2009,6 +2013,7 @@ xfs_bmap_extsize_align(
>  		ASSERT(align_off >= prevp->br_startoff + prevp->br_blockcount);
>  #endif
> +	ASSERT(align_alen <= MAXEXTLEN);
>  	*lenp = align_alen;
>  	*offp = align_off;
>  	return 0;
> @@ -4450,13 +4455,6 @@ xfs_bmapi_reserve_delalloc(
>  	/* Figure out the extent size, adjust alen */
>  	extsz = xfs_get_extsz_hint(ip);
>  	if (extsz) {
> -		/*
> -		 * Make sure we don't exceed a single extent length when we
> -		 * align the extent by reducing length we are going to
> -		 * allocate by the maximum amount extent size aligment may
> -		 * require.
> -		 */
> -		alen = XFS_FILBLKS_MIN(len, MAXEXTLEN - (2 * extsz - 1));

This code is allowing for both head and tail extent alignment....

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: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

[Alain Renaud]

On 12-07-19 05:30 AM, Dave Chinner wrote:
>
> Note the minlen? It's been shortened to reflect the fact I don't
> have 8GB freespace extents in the filesystem.
>
> Firstly, can you reproduce this on your test setup, Alain? What is
> the size (and number) of the AGs in the filesystem? If you do see
> this behaviour, then the alignment code is not the problem...

Note that I did provide an xfstest case in a seperate patch and YES you do 
need to make sure that the AG size is bigger then 8Gig for a filesystem 
with 4K block size you can reduce that by using a filesystem with a 512 
block size and 1Gig AG. look for this patch that I sent to the list.

[PATCH] xfstest: preallocate file space greater then MAXEXTLEN

>
>> +        * Make sure we do not exceed the maximum len of an extent.
>> +        */
>> +       align_alen = XFS_FILBLKS_MIN(align_alen,
>> +                                    MAXEXTLEN - (MAXEXTLEN % extsz));
> I don't think that is correct. alignment happens at both the start
> and end of the extent, so you need to take into account two lots of
> (extsz - 1) blocks of range extension for alignment.

If you look in the function where I put this code is it AFTER the head/len 
alignment so the HEAD is already align so I do not need to account for it. 
And if you look at the rest of the function we can only decrease the length 
and we do return the start and the length not the start and the end.

>
>> +	/*
>>   	 * If the previous block overlaps with this proposed allocation
>>   	 * then move the start forward without adjusting the length.
>>   	 */
>> @@ -1999,7 +2004,6 @@ xfs_bmap_extsize_align(
>>   			return XFS_ERROR(EINVAL);
>>   	} else {
>>   		ASSERT(orig_off >= align_off);
>> -		ASSERT(orig_end <= align_off + align_alen);
> Why is this removed? What it is checking is that the aligned end
> covers the entire range of the original space requested.

This is the logic I am following if the original start was at zero and the 
length we passed in was 9G the orig_end would be at 0+9G however the since 
we will truncate the 8Gig the align_off + align_alen will be smaller then 9G
>
>>   	}
>>   #ifdef DEBUG
>> @@ -2009,6 +2013,7 @@ xfs_bmap_extsize_align(
>>   		ASSERT(align_off >= prevp->br_startoff + prevp->br_blockcount);
>>   #endif
>> +	ASSERT(align_alen <= MAXEXTLEN);
>>   	*lenp = align_alen;
>>   	*offp = align_off;
>>   	return 0;
>> @@ -4450,13 +4455,6 @@ xfs_bmapi_reserve_delalloc(
>>   	/* Figure out the extent size, adjust alen */
>>   	extsz = xfs_get_extsz_hint(ip);
>>   	if (extsz) {
>> -		/*
>> -		 * Make sure we don't exceed a single extent length when we
>> -		 * align the extent by reducing length we are going to
>> -		 * allocate by the maximum amount extent size aligment may
>> -		 * require.
>> -		 */
>> -		alen = XFS_FILBLKS_MIN(len, MAXEXTLEN - (2 * extsz - 1));
> This code is allowing for both head and tail extent alignment....

Like I said I think my code does the same thing since when we truncate the 
length we already have aligned the head.


>
> Cheers,
>
> Dave.

-- 
===============================================
Alain Renaud     - Cluster File System Engineer
arenaud@sgi.com  - SGI
===============================================



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

Re: [PATCH V2] xfs: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

[Dave Chinner]

On Thu, Jul 19, 2012 at 10:20:11AM +0200, Alain Renaud wrote:
> 
> On 12-07-19 05:30 AM, Dave Chinner wrote:
> >
> >Note the minlen? It's been shortened to reflect the fact I don't
> >have 8GB freespace extents in the filesystem.
> >
> >Firstly, can you reproduce this on your test setup, Alain? What is
> >the size (and number) of the AGs in the filesystem? If you do see
> >this behaviour, then the alignment code is not the problem...
> 
> Note that I did provide an xfstest case in a seperate patch and YES
> you do need to make sure that the AG size is bigger then 8Gig for a
> filesystem with 4K block size you can reduce that by using a
> filesystem with a 512 block size and 1Gig AG. look for this patch
> that I sent to the list.
> 
> [PATCH] xfstest: preallocate file space greater then MAXEXTLEN
> 
> >
> >>+        * Make sure we do not exceed the maximum len of an extent.
> >>+        */
> >>+       align_alen = XFS_FILBLKS_MIN(align_alen,
> >>+                                    MAXEXTLEN - (MAXEXTLEN % extsz));
> >I don't think that is correct. alignment happens at both the start
> >and end of the extent, so you need to take into account two lots of
> >(extsz - 1) blocks of range extension for alignment.
> 
> If you look in the function where I put this code is it AFTER the
> head/len alignment so the HEAD is already align so I do not need to
> account for it.

No need to shout. I did look at the code, and IMO you do need to
account for both head and tail extension and you definitely can't do
it there.....

> And if you look at the rest of the function we can
> only decrease the length and we do return the start and the length
> not the start and the end.

Sure, but that's not the problem.  You're passing in {orig_off,
orig_len}, and the idea is to extend it -outwards- to {align_off,
align_len}. In other words, correct behaviour is this:

	extsize:	|-----|-----|-----|-----|-----|-----|
	orig:		  o---|-----|-----|-----|-----|--o
	aligned:	a-----|-----|-----|-----|-----|-----a

If orig_len > (MAXEXTLEN - extsize), then the existing code
overflows and causes ENOSPC, but your code now results in this:

	extsize:	|-----|-----|-----|-----|-----|-----|
	orig:		  o---|-----|-----|-----|-----|--o
	new:		n-----|-----|-----|-----|-----n

which is incorrect as it does not cover the entire range of the
original request. That's why it fired that assert that you removed -
the newly aligned offset+len does not extend past the original
extent end. Alignment has well defined behavioural semantics, and we
do not want to add a special case here because we use the same
"aligment extends outwards" semantics all throughout the allocation
code. 

Fundamentally, you can't fix the extent length alignment overflow
problem in the alignment function as it knows nothing of the calling
context. It aligns outwards only and hence the caller has to limit
the incoming extent length to prevent the alignment extension length
overflows.

> >>@@ -1999,7 +2004,6 @@ xfs_bmap_extsize_align(
> >>  			return XFS_ERROR(EINVAL);
> >>  	} else {
> >>  		ASSERT(orig_off >= align_off);
> >>-		ASSERT(orig_end <= align_off + align_alen);
> >Why is this removed? What it is checking is that the aligned end
> >covers the entire range of the original space requested.
> 
> This is the logic I am following if the original start was at zero
> and the length we passed in was 9G the orig_end would be at 0+9G
> however the since we will truncate the 8Gig the align_off +
> align_alen will be smaller then 9G

I don't think you can pass a length of greater than MAXEXTLEN to
xfs_bmap_extsize_align() anymore. I've spent a fair bit of time
debugging problems as a result of missing length limiting in the
calling functions. IOWs, anything coming through
xfs_bmapi_allocate() will already be limited to MAXEXTLEN. from
xfs_bmapi_write():

                        /*
                         * There's a 32/64 bit type mismatch between the
                         * allocation length request (which can be 64 bits in
                         * length) and the bma length request, which is
                         * xfs_extlen_t and therefore 32 bits. Hence we have to
                         * check for 32-bit overflows and handle them here.
                         */
                        if (len > (xfs_filblks_t)MAXEXTLEN)
                                bma.length = MAXEXTLEN;
                        else
                                bma.length = len;

                        ASSERT(len > 0);
                        ASSERT(bma.length > 0);
                        error = xfs_bmapi_allocate(&bma, flags);

xfs_bmapi_allocate() has other range limits to MAXEXTLEN as well,
and we know that xfs_bmapi_delalloc_reserve() ensures this as well.
Hence no-one should be asking to allocate more than MAXEXTLEN at at
time through any path to xfs_bmap_extsize_align(). Hence I don't
think your logic is valid as it assumes a condition that shouldn't
exist (i.e. is a bug) occurs.

Alain, I'm really glad you're trying to understand this code well
enough to fix problems in it,  but please understand that it took me
5 years of digging in this code before I had any confidence that I
knew how it worked.  I went through this same "i think I've fixed
it, but it fires this strange ASSERT" process many times, and the
result of an ASSERT firing -always- was that my logic was flawed and
I didn't understand the code correctly, so the fix was wrong. I
expect anyone else trying to fix bugs in this code is going to have
exactly the same learning curve I had. Hence when I see changes to
assert statements along with the fix to this code, it raises a great
big red flag....

Remember, the xfs_bmap.c code is the most complex and most critical
code in XFS, and so my review process reflects that. I need to 1)
reproduce it reliably 2) understand what the problem is, and 3) test
the fix before I can say I've reviewed it. Right now I haven't got
past 1) as all my test machines show it to be a transient
condition and that needs to be understood first....

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: prevent xfs_bmap_extsize_align() to exceed maximum extent size.

[Alain Renaud]

On 12-07-20 03:44 AM, Dave Chinner wrote:
>
>>>
>> If you look in the function where I put this code is it AFTER the
>> head/len alignment so the HEAD is already align so I do not need to
>> account for it.
> No need to shout. I did look at the code, and IMO you do need to
> account for both head and tail extension and you definitely can't do
> it there.....
Sorry for the cap I did not intend to shout.

However the problem does exist. the len limitation in xfs_bmapi_write is 
not sufficient
if you set the extsize.

Here is what I see.

xfs_bmapi_write() <= lenght limited to MAXEXTLEN;
     xfs_bmapi_allocate()
           xfs_bmap_alloc()
                 xfs_bmap_btalloc()
                       xfs_bmap_extsize_align()  <== Align the offest / len

Since the alignment is outward like you explain bellow we return len > 
MAXEXTLEN
causing failure to report ENOSPC.

You can reproduce the problem the following way.

# uname -r
3.5.0-rc1-0.2-default+

# mkfs.xfs -f -b size=512 -d agcount=1,size=4294967296 /dev/sdb5

meta-data=/dev/sdb5              isize=256    agcount=1, agsize=8388608 blks
          =                       sectsz=512   attr=2, projid32bit=0
data     =                       bsize=512    blocks=8388608, imaxpct=25
          =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0
log      =internal log           bsize=512    blocks=20480, version=2
          =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0

# mount /dev/sdb5 /a

# xfs_io -f -c "extsize 2m" -c "resvsp 0 1g" /a/data1
XFS_IOC_RESVSP64: No space left on device


So Maybe the fix is to limit the bma.length in xfs_bmapi_write to MAXEXTLEN 
- (2 * extsz - 1) like you do in xfs_bmapi_reserve_delalloc().


Hope this is making sense.




-- 
===============================================
Alain Renaud     - Cluster File System Engineer
arenaud@sgi.com  - SGI
===============================================



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

[PATCH V3] xfs: prevent xfs_bmapi_write() to exceed maximum extent size.

[Alain Renaud]

[-- Attachment #1.1: Type: text/plain, Size: 3021 bytes --]

xfs: prevent xfs_bmapi_write() to exceed maximum extent size.


When trying to do preallocation that exceed the the maximum size of
an extent, the extsize alignment can exceed the MAXEXTLEN.

We are already solving this issue for delay allocation but we
have the same with prealloc. So the Solution is to limit the bma.length
we send to xfs_bmapi_allocate() in xfs_bmapi_write().

We do have a simple test case to confirm that the problem exist.

# mkfs.xfs -f -b size=512 -d agcount=1,size=4294967296  /dev/sdb1
meta-data=/dev/sdb1              isize=256    agcount=1, agsize=8388608 blks
          =                       sectsz=512   attr=2, projid32bit=0
data     =                       bsize=512    blocks=8388608, imaxpct=25
          =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0
log      =internal log           bsize=512    blocks=20480, version=2
          =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
# mount /dev/sdb1 /xfs_dir
# cd/xfs_dir/
# xfs_io -c 'extsize 4m' .
# xfs_io -f -c 'resvsp 0 1g' test_file
XFS_IOC_RESVSP64: No space left on device

Signed-off-by: Alain Renaud<arenaud@sgi.com>

---
    This is the 3rd version of the patch. In the previous version we were
making the change in xfs_bmap_extsize_align() but like Dave mention
we were breaking the function definition.

    Limiting the length in xfs_bmapi_write() seem more logical and should
avoid any unexpected side effect.

  fs/xfs/xfs_bmap.c |   27 ++++++++++++++++-----------
  1 file changed, 16 insertions(+), 11 deletions(-)

Index: b/fs/xfs/xfs_bmap.c
===================================================================
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -4915,22 +4915,27 @@ xfs_bmapi_write(
  		 * that we found, if any.
  		 */
  		if (inhole || wasdelay) {
+			xfs_extlen_t	extsz;
  			bma.eof = eof;
  			bma.conv = !!(flags & XFS_BMAPI_CONVERT);
  			bma.wasdel = wasdelay;
  			bma.offset = bno;
  
-			/*
-			 * There's a 32/64 bit type mismatch between the
-			 * allocation length request (which can be 64 bits in
-			 * length) and the bma length request, which is
-			 * xfs_extlen_t and therefore 32 bits. Hence we have to
-			 * check for 32-bit overflows and handle them here.
-			 */
-			if (len > (xfs_filblks_t)MAXEXTLEN)
-				bma.length = MAXEXTLEN;
-			else
-				bma.length = len;
+			/* Figure out the extent size, set bma.length */
+			extsz = xfs_get_extsz_hint(ip);
+			if (extsz) {
+				/*
+				 * Make sure we don't exceed the maximum extent
+				 * length when we align the extent.  Reduce the
+				 * length we are going to allocate by the
+				 * maximum adjustment extent size aligment may
+				 * require.
+				 */
+				bma.length = XFS_FILBLKS_MIN(len,
+					     MAXEXTLEN - (2 * extsz - 1));
+			} else {
+				bma.length = XFS_FILBLKS_MIN(len, MAXEXTLEN);
+			}
  
  			ASSERT(len > 0);
  			ASSERT(bma.length > 0);


Alain Renaud


[-- Attachment #1.2: Type: text/html, Size: 3523 bytes --]

[-- Attachment #2: xfs:avoid_bmap_len_overflow_xfs_bmap_extsize_align.patch --]
[-- Type: text/x-patch, Size: 2926 bytes --]

When trying to do preallocation that exceed the the maximum size of
an extent, the extsize alignment can exceed the MAXEXTLEN. 

We are already solving this issue for delay allocation but we
have the same with prealloc. So the Solution is to limit the bma.length
we send to xfs_bmapi_allocate() in xfs_bmapi_write().

We do have a simple test case to confirm that the problem exist.

# mkfs.xfs -f -b size=512 -d agcount=1,size=4294967296  /dev/sdb1
meta-data=/dev/sdb1              isize=256    agcount=1, agsize=8388608 blks
         =                       sectsz=512   attr=2, projid32bit=0
data     =                       bsize=512    blocks=8388608, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0
log      =internal log           bsize=512    blocks=20480, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
# mount /dev/sdb1 /xfs_dir
# cd /xfs_dir/
# xfs_io -c 'extsize 4m' .
# xfs_io -f -c 'resvsp 0 1g' test_file
XFS_IOC_RESVSP64: No space left on device

Signed-off-by: Alain Renaud <arenaud@sgi.com>

---
   This is the 3rd version of the patch. In the previous version we were
making the change in xfs_bmap_extsize_align() but like Dave mention
we were breaking the function definition.

   Limiting the length in xfs_bmapi_write() seem more logical and should
avoid any unexpected side effect.

 fs/xfs/xfs_bmap.c |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Index: b/fs/xfs/xfs_bmap.c
===================================================================
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -4915,22 +4915,27 @@ xfs_bmapi_write(
 		 * that we found, if any.
 		 */
 		if (inhole || wasdelay) {
+			xfs_extlen_t	extsz;
 			bma.eof = eof;
 			bma.conv = !!(flags & XFS_BMAPI_CONVERT);
 			bma.wasdel = wasdelay;
 			bma.offset = bno;
 
-			/*
-			 * There's a 32/64 bit type mismatch between the
-			 * allocation length request (which can be 64 bits in
-			 * length) and the bma length request, which is
-			 * xfs_extlen_t and therefore 32 bits. Hence we have to
-			 * check for 32-bit overflows and handle them here.
-			 */
-			if (len > (xfs_filblks_t)MAXEXTLEN)
-				bma.length = MAXEXTLEN;
-			else
-				bma.length = len;
+			/* Figure out the extent size, set bma.length */
+			extsz = xfs_get_extsz_hint(ip);
+			if (extsz) {
+				/*
+				 * Make sure we don't exceed the maximum extent
+				 * length when we align the extent.  Reduce the
+				 * length we are going to allocate by the
+				 * maximum adjustment extent size aligment may
+				 * require.
+				 */
+				bma.length = XFS_FILBLKS_MIN(len,
+					     MAXEXTLEN - (2 * extsz - 1));
+			} else {
+				bma.length = XFS_FILBLKS_MIN(len, MAXEXTLEN);
+			}
 
 			ASSERT(len > 0);
 			ASSERT(bma.length > 0);

[-- Attachment #3: Type: text/plain, Size: 121 bytes --]

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