[v3] Use real FS block size in fallocate05
diff mbox series

Message ID 20191218131533.15323-1-mdoucha@suse.cz
State Superseded
Headers show
Series
  • [v3] Use real FS block size in fallocate05
Related show

Commit Message

Martin Doucha Dec. 18, 2019, 1:15 p.m. UTC
fallocate() behavior depends on whether the file range is aligned to full
blocks. Make sure that the test always uses aligned file range so that
the test is consistent across platforms.

Also use the TEST() macro to prevent errno pollution and increase test device
size to avoid weird edge cases that don't happen in the real world.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Using fixed-size buffer in fallocate05 caused some failures in the past
due to allocation requests being misaligned with actual file system blocks.
Btrfs in particular will treat misaligned allocation as regular write()
and apply copy-on-write to partially allocated blocks even on the first real
write().

While that behavior is somewhat surprising, it does make sense. Fix the error
by using multiples of real block size in fallocate() and write().

I'll also write another fallocate() test later for checking FS behavior
on intentionally misaligned allocation. But this fix can be committed before
that.

Changes since v1:
- XFS keeps some free blocks even when write() failed with ENOSPC. Repeat
  fallocate() until it gets ENOSPC, too.
- Deallocate only part of the file.
- Add description of test scenario in the header comment.
- Increase test device size to 1GB to avoid unrealistic Btrfs edge cases.

Changes since v2:
- Deallocate whole file on Btrfs, otherwise the PUNCH_HOLE check will fail.
  Btrfs deallocates only complete file extents by design.

 .../kernel/syscalls/fallocate/fallocate05.c   | 116 ++++++++++++++----
 1 file changed, 89 insertions(+), 27 deletions(-)

Comments

Jan Stancek Jan. 2, 2020, 10:01 a.m. UTC | #1
----- Original Message -----
> fallocate() behavior depends on whether the file range is aligned to full
> blocks. Make sure that the test always uses aligned file range so that
> the test is consistent across platforms.
> 
> Also use the TEST() macro to prevent errno pollution and increase test device
> size to avoid weird edge cases that don't happen in the real world.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>

Acked-by: Jan Stancek <jstancek@redhat.com>
Cyril Hrubis Jan. 7, 2020, 3:21 p.m. UTC | #2
Hi!
> Changes since v1:
> - XFS keeps some free blocks even when write() failed with ENOSPC. Repeat
>   fallocate() until it gets ENOSPC, too.
> - Deallocate only part of the file.
> - Add description of test scenario in the header comment.
> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases.

Do we really need 1GB here? That quadruples the runtime. Aren't we good
with just 512MB, that would just double it?

> Changes since v2:
> - Deallocate whole file on Btrfs, otherwise the PUNCH_HOLE check will fail.
>   Btrfs deallocates only complete file extents by design.
> 
>  .../kernel/syscalls/fallocate/fallocate05.c   | 116 ++++++++++++++----
>  1 file changed, 89 insertions(+), 27 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c
> index 17034e5b1..34faabbe8 100644
> --- a/testcases/kernel/syscalls/fallocate/fallocate05.c
> +++ b/testcases/kernel/syscalls/fallocate/fallocate05.c
> @@ -1,75 +1,134 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) 2017 Cyril Hrubis <chrubis@suse.cz>
> + * Copyright (c) 2019 SUSE LLC <mdoucha@suse.cz>
>   */
>  
>  /*
>   * Tests that writing to fallocated file works when filesystem is full.
> + * Test scenario:
> + * - fallocate() some empty blocks
> + * - fill the filesystem
> + * - write() into the preallocated space
> + * - try to fallocate() more blocks until we get ENOSPC
> + * - write() into the extra allocated space
> + * - deallocate part of the file
> + * - write() to the end of file to check that some blocks were freed
>   */
>  
>  #define _GNU_SOURCE
>  
>  #include <stdio.h>
>  #include <stdlib.h>
> -#include <errno.h>
> +#include <string.h>
>  #include <fcntl.h>
>  #include "tst_test.h"
>  #include "lapi/fallocate.h"
>  
>  #define MNTPOINT "mntpoint"
> -#define FALLOCATE_SIZE (1024*1024)
> +#define FALLOCATE_BLOCKS 16
> +#define DEALLOCATE_BLOCKS 4
>  #define TESTED_FLAGS "fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)"
>  
>  static int fd;
> +static char *buf = NULL;

There is no point in setting global variable to zero/NULL.

>  static void run(void)
>  {
> -	char buf[FALLOCATE_SIZE];
> -	ssize_t ret;
> +	long bufsize, extsize, tmp;
> +	blksize_t blocksize;
> +	struct stat statbuf;
>  
>  	fd = SAFE_OPEN(MNTPOINT "/test_file", O_WRONLY | O_CREAT);
>  
> -	if (fallocate(fd, 0, 0, FALLOCATE_SIZE)) {
> -		if (errno == EOPNOTSUPP) {
> -			tst_res(TCONF | TERRNO, "fallocate() not supported");
> +	/*
> +	 * Use real FS block size, otherwise fallocate() call will test
> +	 * different things on different platforms
> +	 */
> +	SAFE_FSTAT(fd, &statbuf);
> +	blocksize = statbuf.st_blksize;
> +	bufsize = FALLOCATE_BLOCKS * blocksize;
> +	buf = realloc(buf, bufsize);
> +
> +	if (!buf) {
> +		SAFE_CLOSE(fd);
> +		tst_brk(TBROK, "Buffer allocation failed");
> +	}

Why realloc()? Each filesystem is tested in separately forked process so
buf can't be anything but NULL here.

So this should just simply be SAFE_MALLOC() and this piece of code, the
part that gets the blocksize and allocates the buffer should be moved
into the test setup() function that is executed also once per
filesystem. And the free should be in the test cleanup().

That way we would allocate the buffer only once if the test was executed
with -i option.

> +	TEST(fallocate(fd, 0, 0, bufsize));
> +
> +	if (TST_RET) {
> +		if (TST_ERR == ENOTSUP) {
>  			SAFE_CLOSE(fd);
> -			return;
> +			tst_brk(TCONF | TTERRNO, "fallocate() not supported");
>  		}
>  
> -		tst_brk(TBROK | TERRNO,
> -			"fallocate(fd, 0, 0, %i)", FALLOCATE_SIZE);
> +		tst_brk(TBROK | TTERRNO, "fallocate(fd, 0, 0, %ld)", bufsize);
>  	}
>  
>  	tst_fill_fs(MNTPOINT, 1);
>  
> -	ret = write(fd, buf, sizeof(buf));
> +	TEST(write(fd, buf, bufsize));
>  
> -	if (ret < 0)
> -		tst_res(TFAIL | TERRNO, "write() failed unexpectedly");
> +	if (TST_RET < 0)
> +		tst_res(TFAIL | TTERRNO, "write() failed unexpectedly");
> +	else if (TST_RET != bufsize)
> +		tst_res(TFAIL,
> +			"Short write(): %ld bytes (expected %zu)",
> +			TST_RET, bufsize);
>  	else
> -		tst_res(TPASS, "write() wrote %zu bytes", ret);
> +		tst_res(TPASS, "write() wrote %ld bytes", TST_RET);
> +
> +	/*
> +	 * Some file systems may still have a few extra blocks that can be
> +	 * allocated.
> +	 */
> +	for (TST_RET = 0, extsize = 0; !TST_RET; extsize += blocksize) {
> +		TEST(fallocate(fd, 0, bufsize + extsize, blocksize));
> +	}

This is minor, but LKML prefers not to have curly braces around single
line blocks.

> +	if (TST_RET != -1)
> +		tst_brk(TFAIL, "Invalid fallocate() return value %ld",
> +			TST_RET);

Isn't this line under 80 chars even with the TST_RET); at the end?

> -	ret = fallocate(fd, 0, FALLOCATE_SIZE, FALLOCATE_SIZE);
> -	if (ret != -1)
> -		tst_brk(TFAIL, "fallocate() succeeded unexpectedly");
> +	if (TST_ERR != ENOSPC)
> +		tst_brk(TFAIL | TTERRNO, "fallocate() should fail with ENOSPC");
>  
> -	if (errno != ENOSPC)
> -		tst_brk(TFAIL | TERRNO, "fallocate() should fail with ENOSPC");
> +	/* The loop above always counts 1 more block than it should. */
> +	extsize -= blocksize;
> +	tst_res(TINFO, "fallocate()d %ld extra blocks on full FS",
> +		extsize / blocksize);
>  
> -	tst_res(TPASS | TERRNO, "fallocate() on full FS");
> +	for (tmp = extsize; tmp > 0; tmp -= TST_RET) {
> +		TEST(write(fd, buf, MIN(bufsize, tmp)));
>  
> -	ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE);
> -	if (ret == -1) {
> -		if (errno == EOPNOTSUPP)
> +		if (TST_RET <= 0)
> +			tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly");

tst_brk(TFAIL, is not allowed at the moment, see:

https://github.com/linux-test-project/ltp/issues/462

The only current solution is to tst_res() + return

Also shouldn't we check for the write size here as well?

> +	}
> +
> +	tst_res(TPASS, "fallocate() on full FS");
> +
> +	/* Btrfs deallocates only complete extents, not individual blocks */
> +	if (!strcmp(tst_device->fs_type, "btrfs")) {
> +		tmp = bufsize + extsize;
> +	} else {
> +		tmp = DEALLOCATE_BLOCKS * blocksize;
> +	}
> +
> +	TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0,
> +		tmp));

Here as well, isn't the line under 80 chars?



Other than these minor things the changes looks good to me.
Martin Doucha Jan. 7, 2020, 3:50 p.m. UTC | #3
On 1/7/20 4:21 PM, Cyril Hrubis wrote:
> Hi!
>> Changes since v1:
>> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases.
> 
> Do we really need 1GB here? That quadruples the runtime. Aren't we good
> with just 512MB, that would just double it?

I guess that's a question for Btrfs devs, so let's ask them.

We're trying to test fallocate()/write() on various file systems (both
space allocation and deallocation). What's the minimum block device size
where Btrfs will use the same code paths as in real-world use cases?
mkfs.btrfs is called without --mixed.
Martin Doucha Jan. 7, 2020, 4:09 p.m. UTC | #4
On 1/7/20 4:21 PM, Cyril Hrubis wrote:
>> +	bufsize = FALLOCATE_BLOCKS * blocksize;
>> +	buf = realloc(buf, bufsize);
>> +
>> +	if (!buf) {
>> +		SAFE_CLOSE(fd);
>> +		tst_brk(TBROK, "Buffer allocation failed");
>> +	}
> 
> Why realloc()? Each filesystem is tested in separately forked process so
> buf can't be anything but NULL here.
> 
> So this should just simply be SAFE_MALLOC() and this piece of code, the
> part that gets the blocksize and allocates the buffer should be moved
> into the test setup() function that is executed also once per
> filesystem. And the free should be in the test cleanup().
> 
> That way we would allocate the buffer only once if the test was executed
> with -i option.

Where is this control flow documented? When some behavior is not
documented, I assume it may change without notice and write my code so
that it will work in every case.

>> -	tst_res(TPASS | TERRNO, "fallocate() on full FS");
>> +	for (tmp = extsize; tmp > 0; tmp -= TST_RET) {
>> +		TEST(write(fd, buf, MIN(bufsize, tmp)));
>>  
>> -	ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE);
>> -	if (ret == -1) {
>> -		if (errno == EOPNOTSUPP)
>> +		if (TST_RET <= 0)
>> +			tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly");
> 
> tst_brk(TFAIL, is not allowed at the moment, see:
> 
> https://github.com/linux-test-project/ltp/issues/462
> 
> The only current solution is to tst_res() + return
> 
> Also shouldn't we check for the write size here as well?

I'll fix the tst_brk().

The code above will either fill the extra allocated space to the last
byte, or hit the tst_brk(). No other result is possible. I don't want to
pedantically check for short writes because we're not testing write() here.

I'll implement the rest of your suggestions and resubmit when we get a
reply from Btrfs devs.
Cyril Hrubis Jan. 7, 2020, 4:29 p.m. UTC | #5
Hi!
> > Why realloc()? Each filesystem is tested in separately forked process so
> > buf can't be anything but NULL here.
> > 
> > So this should just simply be SAFE_MALLOC() and this piece of code, the
> > part that gets the blocksize and allocates the buffer should be moved
> > into the test setup() function that is executed also once per
> > filesystem. And the free should be in the test cleanup().
> > 
> > That way we would allocate the buffer only once if the test was executed
> > with -i option.
> 
> Where is this control flow documented? When some behavior is not
> documented, I assume it may change without notice and write my code so
> that it will work in every case.

Unfortunately at the moment only in the lib/tst_test.c source code.

I want to write down a design document for the library, that would
explain the more complicated parts and decisions, but I'm not sure when
I will get to that.

> >> -	tst_res(TPASS | TERRNO, "fallocate() on full FS");
> >> +	for (tmp = extsize; tmp > 0; tmp -= TST_RET) {
> >> +		TEST(write(fd, buf, MIN(bufsize, tmp)));
> >>  
> >> -	ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE);
> >> -	if (ret == -1) {
> >> -		if (errno == EOPNOTSUPP)
> >> +		if (TST_RET <= 0)
> >> +			tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly");
> > 
> > tst_brk(TFAIL, is not allowed at the moment, see:
> > 
> > https://github.com/linux-test-project/ltp/issues/462
> > 
> > The only current solution is to tst_res() + return
> > 
> > Also shouldn't we check for the write size here as well?
> 
> I'll fix the tst_brk().
> 
> The code above will either fill the extra allocated space to the last
> byte, or hit the tst_brk(). No other result is possible. I don't want to
> pedantically check for short writes because we're not testing write() here.
> 
> I'll implement the rest of your suggestions and resubmit when we get a
> reply from Btrfs devs.

Ok, thanks.
Martin Doucha Jan. 13, 2020, 12:16 p.m. UTC | #6
Hello, dear Btrfs devs,
we know that you're busy fixing bugs and implementing new features but
could you spare a minute to answer a question that'll help us improve
Btrfs test coverage in LTP?

We'd like to include the improved fallocate() test in the new LTP
release which is planned for early next week. See the question below:

On 1/7/20 4:50 PM, Martin Doucha wrote:
> On 1/7/20 4:21 PM, Cyril Hrubis wrote:
>> Hi!
>>> Changes since v1:
>>> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases.
>>
>> Do we really need 1GB here? That quadruples the runtime. Aren't we good
>> with just 512MB, that would just double it?
> 
> I guess that's a question for Btrfs devs, so let's ask them.
> 
> We're trying to test fallocate()/write() on various file systems (both
> space allocation and deallocation). What's the minimum block device size
> where Btrfs will use the same code paths as in real-world use cases?
> mkfs.btrfs is called without --mixed.
Qu WenRuo Jan. 13, 2020, 1:16 p.m. UTC | #7
On 2020/1/13 下午8:16, Martin Doucha wrote:
> Hello, dear Btrfs devs,
> we know that you're busy fixing bugs and implementing new features but
> could you spare a minute to answer a question that'll help us improve
> Btrfs test coverage in LTP?
> 
> We'd like to include the improved fallocate() test in the new LTP
> release which is planned for early next week. See the question below:
> 
> On 1/7/20 4:50 PM, Martin Doucha wrote:
>> On 1/7/20 4:21 PM, Cyril Hrubis wrote:
>>> Hi!
>>>> Changes since v1:
>>>> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases.
>>>
>>> Do we really need 1GB here? That quadruples the runtime. Aren't we good
>>> with just 512MB, that would just double it?
>>
>> I guess that's a question for Btrfs devs, so let's ask them.
>>
>> We're trying to test fallocate()/write() on various file systems (both
>> space allocation and deallocation).

Just a small tip, btrfs defaults to data CoW, and unlike other CoW fs
(like xfs), btrfs has an extent booking behavior, that even only part of
a large extent (e.g 128MiB) is referred, the whole extent will not be freed.

So for the following case, it may cause problem for used space accounting:

# xfs_io -f -c "fallocate 0 128M" -c sync -c "fpunch 0 127M" \
  /mnt/btrfs/file1

That 128M will still be considered as used, that 127M punch won't free
any space.

>> What's the minimum block device size
>> where Btrfs will use the same code paths as in real-world use cases?

Mkfs.btrfs no longer enables --mixed for small fs.

But btrfs still has a pretty complex minimal device size, it depends on
profile (-m and -d options).

If LTP guys want to be safe for single device, it needs 256MiB for
`mkfs.btrfs -m dup -d dup` to run successfully.

If only default case (-m dup -d single) is needed, then 128MiB is enough.

Thanks,
Qu

>> mkfs.btrfs is called without --mixed.
>
Martin Doucha Jan. 13, 2020, 1:25 p.m. UTC | #8
On 1/13/20 2:16 PM, Qu WenRuo wrote:
> Just a small tip, btrfs defaults to data CoW, and unlike other CoW fs
> (like xfs), btrfs has an extent booking behavior, that even only part of
> a large extent (e.g 128MiB) is referred, the whole extent will not be freed.

I know, I reported the bug where we discussed this.

>>> What's the minimum block device size
>>> where Btrfs will use the same code paths as in real-world use cases?
> 
> Mkfs.btrfs no longer enables --mixed for small fs.
> 
> But btrfs still has a pretty complex minimal device size, it depends on
> profile (-m and -d options).
> 
> If LTP guys want to be safe for single device, it needs 256MiB for
> `mkfs.btrfs -m dup -d dup` to run successfully.
> 
> If only default case (-m dup -d single) is needed, then 128MiB is enough.

Sorry but my question was not about the minimum for mkfs. My question
was about the minimum device size so that the kernel driver will use the
same block allocation logic as on a 100GB+ partition (instead of some
special case allocation logic for tiny block devices).
Qu WenRuo Jan. 13, 2020, 1:30 p.m. UTC | #9
On 2020/1/13 下午9:25, Martin Doucha wrote:
> On 1/13/20 2:16 PM, Qu WenRuo wrote:
>> Just a small tip, btrfs defaults to data CoW, and unlike other CoW fs
>> (like xfs), btrfs has an extent booking behavior, that even only part of
>> a large extent (e.g 128MiB) is referred, the whole extent will not be freed.
> 
> I know, I reported the bug where we discussed this.
> 
>>>> What's the minimum block device size
>>>> where Btrfs will use the same code paths as in real-world use cases?
>>
>> Mkfs.btrfs no longer enables --mixed for small fs.
>>
>> But btrfs still has a pretty complex minimal device size, it depends on
>> profile (-m and -d options).
>>
>> If LTP guys want to be safe for single device, it needs 256MiB for
>> `mkfs.btrfs -m dup -d dup` to run successfully.
>>
>> If only default case (-m dup -d single) is needed, then 128MiB is enough.
> 
> Sorry but my question was not about the minimum for mkfs. My question
> was about the minimum device size so that the kernel driver will use the
> same block allocation logic as on a 100GB+ partition (instead of some
> special case allocation logic for tiny block devices).

Then the meaningful boundary is 50G.

Smaller 50G, metadata chunk is limited to 256M, otherwise metadata chunk
can be 1G sized.
Despite chunk size, there shouldn't be much difference in the code path.

I guess you're talking about the old `mixed` behavior, which is no
longer the default option for any fs size.

Thanks,
Qu

Patch
diff mbox series

diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c
index 17034e5b1..34faabbe8 100644
--- a/testcases/kernel/syscalls/fallocate/fallocate05.c
+++ b/testcases/kernel/syscalls/fallocate/fallocate05.c
@@ -1,75 +1,134 @@ 
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2017 Cyril Hrubis <chrubis@suse.cz>
+ * Copyright (c) 2019 SUSE LLC <mdoucha@suse.cz>
  */
 
 /*
  * Tests that writing to fallocated file works when filesystem is full.
+ * Test scenario:
+ * - fallocate() some empty blocks
+ * - fill the filesystem
+ * - write() into the preallocated space
+ * - try to fallocate() more blocks until we get ENOSPC
+ * - write() into the extra allocated space
+ * - deallocate part of the file
+ * - write() to the end of file to check that some blocks were freed
  */
 
 #define _GNU_SOURCE
 
 #include <stdio.h>
 #include <stdlib.h>
-#include <errno.h>
+#include <string.h>
 #include <fcntl.h>
 #include "tst_test.h"
 #include "lapi/fallocate.h"
 
 #define MNTPOINT "mntpoint"
-#define FALLOCATE_SIZE (1024*1024)
+#define FALLOCATE_BLOCKS 16
+#define DEALLOCATE_BLOCKS 4
 #define TESTED_FLAGS "fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)"
 
 static int fd;
+static char *buf = NULL;
 
 static void run(void)
 {
-	char buf[FALLOCATE_SIZE];
-	ssize_t ret;
+	long bufsize, extsize, tmp;
+	blksize_t blocksize;
+	struct stat statbuf;
 
 	fd = SAFE_OPEN(MNTPOINT "/test_file", O_WRONLY | O_CREAT);
 
-	if (fallocate(fd, 0, 0, FALLOCATE_SIZE)) {
-		if (errno == EOPNOTSUPP) {
-			tst_res(TCONF | TERRNO, "fallocate() not supported");
+	/*
+	 * Use real FS block size, otherwise fallocate() call will test
+	 * different things on different platforms
+	 */
+	SAFE_FSTAT(fd, &statbuf);
+	blocksize = statbuf.st_blksize;
+	bufsize = FALLOCATE_BLOCKS * blocksize;
+	buf = realloc(buf, bufsize);
+
+	if (!buf) {
+		SAFE_CLOSE(fd);
+		tst_brk(TBROK, "Buffer allocation failed");
+	}
+
+	TEST(fallocate(fd, 0, 0, bufsize));
+
+	if (TST_RET) {
+		if (TST_ERR == ENOTSUP) {
 			SAFE_CLOSE(fd);
-			return;
+			tst_brk(TCONF | TTERRNO, "fallocate() not supported");
 		}
 
-		tst_brk(TBROK | TERRNO,
-			"fallocate(fd, 0, 0, %i)", FALLOCATE_SIZE);
+		tst_brk(TBROK | TTERRNO, "fallocate(fd, 0, 0, %ld)", bufsize);
 	}
 
 	tst_fill_fs(MNTPOINT, 1);
 
-	ret = write(fd, buf, sizeof(buf));
+	TEST(write(fd, buf, bufsize));
 
-	if (ret < 0)
-		tst_res(TFAIL | TERRNO, "write() failed unexpectedly");
+	if (TST_RET < 0)
+		tst_res(TFAIL | TTERRNO, "write() failed unexpectedly");
+	else if (TST_RET != bufsize)
+		tst_res(TFAIL,
+			"Short write(): %ld bytes (expected %zu)",
+			TST_RET, bufsize);
 	else
-		tst_res(TPASS, "write() wrote %zu bytes", ret);
+		tst_res(TPASS, "write() wrote %ld bytes", TST_RET);
+
+	/*
+	 * Some file systems may still have a few extra blocks that can be
+	 * allocated.
+	 */
+	for (TST_RET = 0, extsize = 0; !TST_RET; extsize += blocksize) {
+		TEST(fallocate(fd, 0, bufsize + extsize, blocksize));
+	}
+
+	if (TST_RET != -1)
+		tst_brk(TFAIL, "Invalid fallocate() return value %ld",
+			TST_RET);
 
-	ret = fallocate(fd, 0, FALLOCATE_SIZE, FALLOCATE_SIZE);
-	if (ret != -1)
-		tst_brk(TFAIL, "fallocate() succeeded unexpectedly");
+	if (TST_ERR != ENOSPC)
+		tst_brk(TFAIL | TTERRNO, "fallocate() should fail with ENOSPC");
 
-	if (errno != ENOSPC)
-		tst_brk(TFAIL | TERRNO, "fallocate() should fail with ENOSPC");
+	/* The loop above always counts 1 more block than it should. */
+	extsize -= blocksize;
+	tst_res(TINFO, "fallocate()d %ld extra blocks on full FS",
+		extsize / blocksize);
 
-	tst_res(TPASS | TERRNO, "fallocate() on full FS");
+	for (tmp = extsize; tmp > 0; tmp -= TST_RET) {
+		TEST(write(fd, buf, MIN(bufsize, tmp)));
 
-	ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE);
-	if (ret == -1) {
-		if (errno == EOPNOTSUPP)
+		if (TST_RET <= 0)
+			tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly");
+	}
+
+	tst_res(TPASS, "fallocate() on full FS");
+
+	/* Btrfs deallocates only complete extents, not individual blocks */
+	if (!strcmp(tst_device->fs_type, "btrfs")) {
+		tmp = bufsize + extsize;
+	} else {
+		tmp = DEALLOCATE_BLOCKS * blocksize;
+	}
+
+	TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0,
+		tmp));
+
+	if (TST_RET == -1) {
+		if (TST_ERR == ENOTSUP)
 			tst_brk(TCONF, TESTED_FLAGS);
 
-		tst_brk(TBROK | TERRNO, TESTED_FLAGS);
+		tst_brk(TBROK | TTERRNO, TESTED_FLAGS);
 	}
 	tst_res(TPASS, TESTED_FLAGS);
 
-	ret = write(fd, buf, 10);
-	if (ret == -1)
-		tst_res(TFAIL | TERRNO, "write()");
+	TEST(write(fd, buf, 10));
+	if (TST_RET == -1)
+		tst_res(TFAIL | TTERRNO, "write()");
 	else
 		tst_res(TPASS, "write()");
 
@@ -80,12 +139,15 @@  static void cleanup(void)
 {
 	if (fd > 0)
 		SAFE_CLOSE(fd);
+
+	free(buf);
 }
 
 static struct tst_test test = {
 	.needs_root = 1,
 	.needs_tmpdir = 1,
 	.mount_device = 1,
+	.dev_min_size = 1024,
 	.mntpoint = MNTPOINT,
 	.all_filesystems = 1,
 	.cleanup = cleanup,