All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH 0/1] Use real FS block size in fallocate05
@ 2019-11-28  9:36 Martin Doucha
  2019-11-28  9:36 ` [LTP] [PATCH 1/1] " Martin Doucha
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2019-11-28  9:36 UTC (permalink / raw)
  To: ltp

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've also found some XFS and Btrfs quirks which are documented in the patch.
The XFS behavior appears to be intentional. I'm still waiting for reply
whether the Btrfs quirk with deallocating blocks is a bug or not.

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

Martin Doucha (1):
  Use real FS block size in fallocate05

 .../kernel/syscalls/fallocate/fallocate05.c   | 75 ++++++++++++-------
 1 file changed, 50 insertions(+), 25 deletions(-)

-- 
2.24.0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-11-28  9:36 [LTP] [PATCH 0/1] Use real FS block size in fallocate05 Martin Doucha
@ 2019-11-28  9:36 ` Martin Doucha
  2019-11-28 17:47   ` Petr Vorel
  2019-11-29 12:01   ` Jan Stancek
  0 siblings, 2 replies; 24+ messages in thread
From: Martin Doucha @ 2019-11-28  9:36 UTC (permalink / raw)
  To: ltp

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 add notes about
some file system quirks that may or may not be bugs.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 .../kernel/syscalls/fallocate/fallocate05.c   | 75 ++++++++++++-------
 1 file changed, 50 insertions(+), 25 deletions(-)

diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c
index 17034e5b1..040f176b9 100644
--- a/testcases/kernel/syscalls/fallocate/fallocate05.c
+++ b/testcases/kernel/syscalls/fallocate/fallocate05.c
@@ -11,65 +11,87 @@
 
 #include <stdio.h>
 #include <stdlib.h>
-#include <errno.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 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;
+	size_t bufsize, i;
+	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);
+	bufsize = FALLOCATE_BLOCKS * statbuf.st_blksize;
+	buf = realloc(buf, bufsize);
+
+	if (!buf) {
+		tst_brk(TBROK, "Buffer allocation failed");
+		SAFE_CLOSE(fd);
+		return;
+	}
+
+	TEST(fallocate(fd, 0, 0, bufsize));
+
+	if (TST_RET) {
+		if (errno == ENOTSUP) {
+			tst_res(TCONF | TTERRNO, "fallocate() not supported");
 			SAFE_CLOSE(fd);
 			return;
 		}
 
-		tst_brk(TBROK | TERRNO,
-			"fallocate(fd, 0, 0, %i)", FALLOCATE_SIZE);
+		tst_brk(TBROK | TTERRNO, "fallocate(fd, 0, 0, %i)", 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);
 
-	ret = fallocate(fd, 0, FALLOCATE_SIZE, FALLOCATE_SIZE);
-	if (ret != -1)
+	// fallocate(1 block) may pass here on XFS. Original test allocated
+	// 8KB (2 blocks on x86) so keep the original behavior.
+	TEST(fallocate(fd, 0, bufsize, 2 * statbuf.st_blksize));
+	if (TST_RET != -1)
 		tst_brk(TFAIL, "fallocate() succeeded unexpectedly");
 
-	if (errno != ENOSPC)
-		tst_brk(TFAIL | TERRNO, "fallocate() should fail with ENOSPC");
+	if (TST_ERR != ENOSPC)
+		tst_brk(TFAIL | TTERRNO, "fallocate() should fail with ENOSPC");
 
-	tst_res(TPASS | TERRNO, "fallocate() on full FS");
+	tst_res(TPASS | TTERRNO, "fallocate() on full FS");
 
-	ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE);
-	if (ret == -1) {
-		if (errno == EOPNOTSUPP)
+	// btrfs won't release any space unless the whole file has been
+	// deallocated. Original test did that, keep the original behavior.
+	TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0,
+		bufsize));
+	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,6 +102,9 @@ static void cleanup(void)
 {
 	if (fd > 0)
 		SAFE_CLOSE(fd);
+
+	if (buf)
+		free(buf);
 }
 
 static struct tst_test test = {
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-11-28  9:36 ` [LTP] [PATCH 1/1] " Martin Doucha
@ 2019-11-28 17:47   ` Petr Vorel
  2019-11-29  9:54     ` Martin Doucha
  2019-11-29 12:01   ` Jan Stancek
  1 sibling, 1 reply; 24+ messages in thread
From: Petr Vorel @ 2019-11-28 17:47 UTC (permalink / raw)
  To: ltp

Hi Martin,

Looks ok to me.

BTW there is change on results on some of my VM:

Old version:
tst_test.c:1169: INFO: Testing on ext4
tst_mkfs.c:89: INFO: Formatting /dev/loop0 with ext4 opts='' extra opts=''
mke2fs 1.43.8 (1-Jan-2018)
tst_test.c:1106: INFO: Timeout per run is 0h 05m 00s
tst_fill_fs.c:29: INFO: Creating file mntpoint/file0 size 21710183
tst_fill_fs.c:29: INFO: Creating file mntpoint/file1 size 8070086
tst_fill_fs.c:29: INFO: Creating file mntpoint/file2 size 3971177
tst_fill_fs.c:29: INFO: Creating file mntpoint/file3 size 36915315
tst_fill_fs.c:29: INFO: Creating file mntpoint/file4 size 70310993
tst_fill_fs.c:29: INFO: Creating file mntpoint/file5 size 4807935
tst_fill_fs.c:29: INFO: Creating file mntpoint/file6 size 90739786
tst_fill_fs.c:29: INFO: Creating file mntpoint/file7 size 76896492
tst_fill_fs.c:49: INFO: write(): ENOSPC
fallocate05.c:50: PASS: write() wrote 8192 bytes
fallocate05.c:59: PASS: fallocate() on full FS: ENOSPC
fallocate05.c:68: PASS: fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)
fallocate05.c:74: PASS: write()

With your patch:
...
tst_fill_fs.c:49: INFO: write(): ENOSPC (28)
fallocate05.c:67: PASS: write() wrote 16384 bytes
fallocate05.c:73: FAIL: fallocate() succeeded unexpectedly

Maybe it's correct (previous version didn't catch a problem),
not really sure.

+ there are few simple warnings:
../../../../include/tst_test.h:70:41: warning: format ?%i? expects argument of type ?int?, but argument 5 has type ?size_t? {aka ?long unsigned int?} [-Wformat=]
   70 |   tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
      |                                         ^~~~~~~~~
fallocate05.c:53:3: note: in expansion of macro ?tst_brk?
   53 |   tst_brk(TBROK | TTERRNO, "fallocate(fd, 0, 0, %i)", bufsize);
      |   ^~~~~~~
fallocate05.c:62:19: warning: comparison of integer expressions of different signedness: ?long int? and ?size_t? {aka ?long unsigned int?} [-Wsign-compare]
   62 |  else if (TST_RET != bufsize)
      |                   ^~
fallocate05.c:27:18: warning: unused variable ?i? [-Wunused-variable]
   27 |  size_t bufsize, i;
      |                  ^

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-11-28 17:47   ` Petr Vorel
@ 2019-11-29  9:54     ` Martin Doucha
  0 siblings, 0 replies; 24+ messages in thread
From: Martin Doucha @ 2019-11-29  9:54 UTC (permalink / raw)
  To: ltp

On 11/28/19 6:47 PM, Petr Vorel wrote:
> Hi Martin,
> 
> Looks ok to me.
> 
> BTW there is change on results on some of my VM:
> 
> Old version:
> tst_test.c:1169: INFO: Testing on ext4
> tst_mkfs.c:89: INFO: Formatting /dev/loop0 with ext4 opts='' extra opts=''
> mke2fs 1.43.8 (1-Jan-2018)
> tst_test.c:1106: INFO: Timeout per run is 0h 05m 00s
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file0 size 21710183
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file1 size 8070086
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file2 size 3971177
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file3 size 36915315
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file4 size 70310993
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file5 size 4807935
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file6 size 90739786
> tst_fill_fs.c:29: INFO: Creating file mntpoint/file7 size 76896492
> tst_fill_fs.c:49: INFO: write(): ENOSPC
> fallocate05.c:50: PASS: write() wrote 8192 bytes
> fallocate05.c:59: PASS: fallocate() on full FS: ENOSPC
> fallocate05.c:68: PASS: fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)
> fallocate05.c:74: PASS: write()
> 
> With your patch:
> ...
> tst_fill_fs.c:49: INFO: write(): ENOSPC (28)
> fallocate05.c:67: PASS: write() wrote 16384 bytes
> fallocate05.c:73: FAIL: fallocate() succeeded unexpectedly
> 
> Maybe it's correct (previous version didn't catch a problem),
> not really sure.

Your VM seems to use 1KB blocks in the ext4 test partition. It's
probably the same quirk as I've documented for XFS in the patch. I'd
like some more opinions on how to deal with it:
- improve tst_fill_fs() and make sure that fallocate(1 block) always
fails after it?
- set some reasonable minimum size where fallocate() must fail on full
FS but allow fallocate() to pass with smaller allocation requests?
- drop that one fallocate() fail check entirely because it's too unreliable?

> + there are few simple warnings:

I'll resubmit after we agree on a solution to the above problem.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-11-28  9:36 ` [LTP] [PATCH 1/1] " Martin Doucha
  2019-11-28 17:47   ` Petr Vorel
@ 2019-11-29 12:01   ` Jan Stancek
  2019-11-29 15:25     ` Martin Doucha
  1 sibling, 1 reply; 24+ messages in thread
From: Jan Stancek @ 2019-11-29 12:01 UTC (permalink / raw)
  To: ltp

Hi,

<snip>

>  
>  static void run(void)
>  {
> -	char buf[FALLOCATE_SIZE];
> -	ssize_t ret;
> +	size_t bufsize, i;
> +	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

Style guide favors c-style comments.

> +	SAFE_FSTAT(fd, &statbuf);
> +	bufsize = FALLOCATE_BLOCKS * statbuf.st_blksize;
> +	buf = realloc(buf, bufsize);
> +
> +	if (!buf) {
> +		tst_brk(TBROK, "Buffer allocation failed");
> +		SAFE_CLOSE(fd);
> +		return;

Anything after TBROK will be unreachable.

> +	}
> +
> +	TEST(fallocate(fd, 0, 0, bufsize));
> +
> +	if (TST_RET) {
> +		if (errno == ENOTSUP) {

TEST_ERR

> +			tst_res(TCONF | TTERRNO, "fallocate() not supported");

tst_brk would make more sense here. If we fail here we can end the test.

>  			SAFE_CLOSE(fd);
>  			return;
>  		}
>  
> -		tst_brk(TBROK | TERRNO,
> -			"fallocate(fd, 0, 0, %i)", FALLOCATE_SIZE);
> +		tst_brk(TBROK | TTERRNO, "fallocate(fd, 0, 0, %i)", 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);
>  
> -	ret = fallocate(fd, 0, FALLOCATE_SIZE, FALLOCATE_SIZE);
> -	if (ret != -1)
> +	// fallocate(1 block) may pass here on XFS. Original test allocated
> +	// 8KB (2 blocks on x86) so keep the original behavior.
> +	TEST(fallocate(fd, 0, bufsize, 2 * statbuf.st_blksize));

I don't understand why there is need to find minimum value that can
satisfy this check. It looks like we are testing tst_fill_fs() more
than fallocate().

In other words, what is wrong with current test? Is the problem that
FALLOCATE_SIZE (1M) is not aligned on all platforms? Or is the test
invalid with FALLOCATE_SIZE that big? Or both?

<snip>

>  
> @@ -80,6 +102,9 @@ static void cleanup(void)
>  {
>  	if (fd > 0)
>  		SAFE_CLOSE(fd);
> +
> +	if (buf)

Check is not needed, free() can handle NULL.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-11-29 12:01   ` Jan Stancek
@ 2019-11-29 15:25     ` Martin Doucha
  2019-11-29 16:17       ` Jan Stancek
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2019-11-29 15:25 UTC (permalink / raw)
  To: ltp

On 11/29/19 1:01 PM, Jan Stancek wrote:
>> +			tst_res(TCONF | TTERRNO, "fallocate() not supported");
> 
> tst_brk would make more sense here. If we fail here we can end the test.

No. tst_brk() will terminate the whole test on the first usual test case
(Ext2) and skip all the other file systems that do support fallocate().

> I don't understand why there is need to find minimum value that can
> satisfy this check. It looks like we are testing tst_fill_fs() more
> than fallocate().
> 
> In other words, what is wrong with current test? Is the problem that
> FALLOCATE_SIZE (1M) is not aligned on all platforms? Or is the test
> invalid with FALLOCATE_SIZE that big? Or both?

I don't like to blindly rely on the assumption that block size is always
a power of 2 and smaller than some magic number. Getting the real block
size is trivial. The only real question is how many free blocks do we
allow on a "full" file system in our tests. 1MB is just 16 blocks on
PPC64 so the magic number isn't particularly big anyway.

I've implemented the rest of your suggestions and I'll resubmit later
next week.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-11-29 15:25     ` Martin Doucha
@ 2019-11-29 16:17       ` Jan Stancek
  2019-12-04 10:38         ` Martin Doucha
  0 siblings, 1 reply; 24+ messages in thread
From: Jan Stancek @ 2019-11-29 16:17 UTC (permalink / raw)
  To: ltp



----- Original Message -----
> On 11/29/19 1:01 PM, Jan Stancek wrote:
> >> +			tst_res(TCONF | TTERRNO, "fallocate() not supported");
> > 
> > tst_brk would make more sense here. If we fail here we can end the test.
> 
> No. tst_brk() will terminate the whole test on the first usual test case
> (Ext2) and skip all the other file systems that do support fallocate().

It shouldn't. tst_brk() does call exit() for test process, but
.all_filesystems spawns new process for each fs.

If I add:
   tst_brk(TCONF, "STOP");
to run(), I get STOP for each fs:
  $ sudo ./fallocate05 2>&1 | grep STOP
  fallocate05.c:30: CONF: STOP
  fallocate05.c:30: CONF: STOP
  fallocate05.c:30: CONF: STOP
  fallocate05.c:30: CONF: STOP
  fallocate05.c:30: CONF: STOP
  fallocate05.c:30: CONF: STOP

> 
> > I don't understand why there is need to find minimum value that can
> > satisfy this check. It looks like we are testing tst_fill_fs() more
> > than fallocate().
> > 
> > In other words, what is wrong with current test? Is the problem that
> > FALLOCATE_SIZE (1M) is not aligned on all platforms? Or is the test
> > invalid with FALLOCATE_SIZE that big? Or both?
> 
> I don't like to blindly rely on the assumption that block size is always
> a power of 2 and smaller than some magic number. Getting the real block
> size is trivial. The only real question is how many free blocks do we
> allow on a "full" file system in our tests. 1MB is just 16 blocks on
> PPC64 so the magic number isn't particularly big anyway.

OK, let's assume 16 is enough. Can we use that value also for ENOSPC check?
  TEST(fallocate(fd, 0, bufsize, 2 * statbuf.st_blksize));



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-11-29 16:17       ` Jan Stancek
@ 2019-12-04 10:38         ` Martin Doucha
  2019-12-13 13:40           ` Cyril Hrubis
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2019-12-04 10:38 UTC (permalink / raw)
  To: ltp

On 11/29/19 5:17 PM, Jan Stancek wrote:
>> No. tst_brk() will terminate the whole test on the first usual test case
>> (Ext2) and skip all the other file systems that do support fallocate().
> 
> It shouldn't. tst_brk() does call exit() for test process, but
> .all_filesystems spawns new process for each fs.

You're right, I'll use tst_brk(TCONF, ...) then.

>> I don't like to blindly rely on the assumption that block size is always
>> a power of 2 and smaller than some magic number. Getting the real block
>> size is trivial. The only real question is how many free blocks do we
>> allow on a "full" file system in our tests. 1MB is just 16 blocks on
>> PPC64 so the magic number isn't particularly big anyway.
> 
> OK, let's assume 16 is enough. Can we use that value also for ENOSPC check?
>   TEST(fallocate(fd, 0, bufsize, 2 * statbuf.st_blksize));

I think it might be better to change the test scenario a bit:
1. fallocate(FALLOCATE_BLOCKS * blocksize)
2. tst_fill_fs()
3. write(FALLOCATE_BLOCKS * blocksize)
4. repeat fallocate(blocksize) until we get ENOSPC
5. write() into all blocks allocated in step 4
6. check that another write() will get ENOSPC
7. test fallocate(PUNCH_HOLE | KEEP_SIZE)

This should get us around the issue with tst_fill_fs() and still
properly validate that fallocate() handles full FS gracefully.

The only remaining issue is whether it's correct for Btrfs to only
release blocks when you deallocate the whole file. I still haven't heard
back from our Btrfs dev.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH 1/1] Use real FS block size in fallocate05
  2019-12-04 10:38         ` Martin Doucha
@ 2019-12-13 13:40           ` Cyril Hrubis
  2019-12-17 13:17             ` [LTP] [PATCH v2] " Martin Doucha
  0 siblings, 1 reply; 24+ messages in thread
From: Cyril Hrubis @ 2019-12-13 13:40 UTC (permalink / raw)
  To: ltp

Hi!
> I think it might be better to change the test scenario a bit:
> 1. fallocate(FALLOCATE_BLOCKS * blocksize)
> 2. tst_fill_fs()
> 3. write(FALLOCATE_BLOCKS * blocksize)
> 4. repeat fallocate(blocksize) until we get ENOSPC
> 5. write() into all blocks allocated in step 4
> 6. check that another write() will get ENOSPC
> 7. test fallocate(PUNCH_HOLE | KEEP_SIZE)
> 
> This should get us around the issue with tst_fill_fs() and still
> properly validate that fallocate() handles full FS gracefully.

Looping over the second fallocate until we got ENOSPC sounds reasonable
to me.

> The only remaining issue is whether it's correct for Btrfs to only
> release blocks when you deallocate the whole file. I still haven't heard
> back from our Btrfs dev.

So the punched hole does not free space on Btrfs even if we are FS block
aligned? I was under an impression that it should, but again Btrfs is
copy-on-write filesystem, so it may want to keep a copy of the discarded
blocks anyways.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v2] Use real FS block size in fallocate05
  2019-12-13 13:40           ` Cyril Hrubis
@ 2019-12-17 13:17             ` Martin Doucha
  2019-12-17 21:02               ` Jan Stancek
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2019-12-17 13:17 UTC (permalink / raw)
  To: ltp

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. Btrfs will fail this check because it has
  a bug.
- Add description of test scenario in the header comment.
- Increase test device size to 1GB to avoid unrealistic Btrfs edge cases.

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

diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c
index 17034e5b1..36ca84bdc 100644
--- a/testcases/kernel/syscalls/fallocate/fallocate05.c
+++ b/testcases/kernel/syscalls/fallocate/fallocate05.c
@@ -1,75 +1,126 @@
 // 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 <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;
+	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));
+	}
 
-	ret = fallocate(fd, 0, FALLOCATE_SIZE, FALLOCATE_SIZE);
-	if (ret != -1)
-		tst_brk(TFAIL, "fallocate() succeeded unexpectedly");
+	if (TST_RET != -1)
+		tst_brk(TFAIL, "Invalid fallocate() return value %ld",
+			TST_RET);
 
-	if (errno != ENOSPC)
-		tst_brk(TFAIL | TERRNO, "fallocate() should fail with ENOSPC");
+	if (TST_ERR != ENOSPC)
+		tst_brk(TFAIL | TTERRNO, "fallocate() should fail with ENOSPC");
 
-	tst_res(TPASS | TERRNO, "fallocate() on full FS");
+	/* 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);
 
-	ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE);
-	if (ret == -1) {
-		if (errno == EOPNOTSUPP)
+	for (; extsize > 0; extsize -= TST_RET) {
+		TEST(write(fd, buf, MIN(bufsize, extsize)));
+
+		if (TST_RET <= 0)
+			tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly");
+	}
+
+	tst_res(TPASS, "fallocate() on full FS");
+
+	TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0,
+		DEALLOCATE_BLOCKS * blocksize));
+
+	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 +131,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,
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v2] Use real FS block size in fallocate05
  2019-12-17 13:17             ` [LTP] [PATCH v2] " Martin Doucha
@ 2019-12-17 21:02               ` Jan Stancek
  2019-12-18  9:09                 ` Martin Doucha
  0 siblings, 1 reply; 24+ messages in thread
From: Jan Stancek @ 2019-12-17 21:02 UTC (permalink / raw)
  To: ltp



----- 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>
> ---
> 
> 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. Btrfs will fail this check because it has
>   a bug.
> - Add description of test scenario in the header comment.
> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases.
> 

Looks good to me.

Is there an upstream thread link for that btrfs bug?


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v2] Use real FS block size in fallocate05
  2019-12-17 21:02               ` Jan Stancek
@ 2019-12-18  9:09                 ` Martin Doucha
  2019-12-18 10:01                   ` Martin Doucha
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2019-12-18  9:09 UTC (permalink / raw)
  To: ltp

On 12/17/19 10:02 PM, Jan Stancek wrote:
> Is there an upstream thread link for that btrfs bug?

Not yet. I've reported the bug to SUSE Bugzilla for now so that our
kernel devs can take a closer look. But they may still decide that this
is expected behavior.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v2] Use real FS block size in fallocate05
  2019-12-18  9:09                 ` Martin Doucha
@ 2019-12-18 10:01                   ` Martin Doucha
  2019-12-18 10:07                     ` Jan Stancek
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2019-12-18 10:01 UTC (permalink / raw)
  To: ltp

On 12/18/19 10:09 AM, Martin Doucha wrote:
> On 12/17/19 10:02 PM, Jan Stancek wrote:
>> Is there an upstream thread link for that btrfs bug?
> 
> Not yet. I've reported the bug to SUSE Bugzilla for now so that our
> kernel devs can take a closer look. But they may still decide that this
> is expected behavior.

Update: Our kernel devs just said that this is expected behavior. Btrfs
will only release disk space when you deallocate a whole file extent
that can be up to 128MB in size. I'll have to add an exception for Btrfs
to fallocate05 then. Or would you guys prefer to always deallocate the
whole file before the last write() check?

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v2] Use real FS block size in fallocate05
  2019-12-18 10:01                   ` Martin Doucha
@ 2019-12-18 10:07                     ` Jan Stancek
  2019-12-18 13:15                       ` [LTP] [PATCH v3] " Martin Doucha
  0 siblings, 1 reply; 24+ messages in thread
From: Jan Stancek @ 2019-12-18 10:07 UTC (permalink / raw)
  To: ltp


----- Original Message -----
> On 12/18/19 10:09 AM, Martin Doucha wrote:
> > On 12/17/19 10:02 PM, Jan Stancek wrote:
> >> Is there an upstream thread link for that btrfs bug?
> > 
> > Not yet. I've reported the bug to SUSE Bugzilla for now so that our
> > kernel devs can take a closer look. But they may still decide that this
> > is expected behavior.
> 
> Update: Our kernel devs just said that this is expected behavior. Btrfs
> will only release disk space when you deallocate a whole file extent
> that can be up to 128MB in size. I'll have to add an exception for Btrfs
> to fallocate05 then. Or would you guys prefer to always deallocate the
> whole file before the last write() check?

Since it's expected to work on other file systems, I'd add exception
only for btrfs.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2019-12-18 10:07                     ` Jan Stancek
@ 2019-12-18 13:15                       ` Martin Doucha
  2020-01-02 10:01                         ` Jan Stancek
  2020-01-07 15:21                         ` Cyril Hrubis
  0 siblings, 2 replies; 24+ messages in thread
From: Martin Doucha @ 2019-12-18 13:15 UTC (permalink / raw)
  To: ltp

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

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,
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2019-12-18 13:15                       ` [LTP] [PATCH v3] " Martin Doucha
@ 2020-01-02 10:01                         ` Jan Stancek
  2020-01-07 15:21                         ` Cyril Hrubis
  1 sibling, 0 replies; 24+ messages in thread
From: Jan Stancek @ 2020-01-02 10:01 UTC (permalink / raw)
  To: ltp



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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2019-12-18 13:15                       ` [LTP] [PATCH v3] " Martin Doucha
  2020-01-02 10:01                         ` Jan Stancek
@ 2020-01-07 15:21                         ` Cyril Hrubis
  2020-01-07 15:50                           ` Martin Doucha
  2020-01-07 16:09                           ` Martin Doucha
  1 sibling, 2 replies; 24+ messages in thread
From: Cyril Hrubis @ 2020-01-07 15:21 UTC (permalink / raw)
  To: ltp

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.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2020-01-07 15:21                         ` Cyril Hrubis
@ 2020-01-07 15:50                           ` Martin Doucha
  2020-01-13 12:16                             ` Martin Doucha
  2020-01-07 16:09                           ` Martin Doucha
  1 sibling, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2020-01-07 15:50 UTC (permalink / raw)
  To: ltp

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   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2020-01-07 15:21                         ` Cyril Hrubis
  2020-01-07 15:50                           ` Martin Doucha
@ 2020-01-07 16:09                           ` Martin Doucha
  2020-01-07 16:29                             ` Cyril Hrubis
  1 sibling, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2020-01-07 16:09 UTC (permalink / raw)
  To: ltp

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.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2020-01-07 16:09                           ` Martin Doucha
@ 2020-01-07 16:29                             ` Cyril Hrubis
  0 siblings, 0 replies; 24+ messages in thread
From: Cyril Hrubis @ 2020-01-07 16:29 UTC (permalink / raw)
  To: ltp

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.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2020-01-07 15:50                           ` Martin Doucha
@ 2020-01-13 12:16                             ` Martin Doucha
  2020-01-13 13:16                               ` Qu WenRuo
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2020-01-13 12:16 UTC (permalink / raw)
  To: ltp

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.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2020-01-13 12:16                             ` Martin Doucha
@ 2020-01-13 13:16                               ` Qu WenRuo
  2020-01-13 13:25                                 ` Martin Doucha
  0 siblings, 1 reply; 24+ messages in thread
From: Qu WenRuo @ 2020-01-13 13:16 UTC (permalink / raw)
  To: ltp



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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2020-01-13 13:16                               ` Qu WenRuo
@ 2020-01-13 13:25                                 ` Martin Doucha
  2020-01-13 13:30                                   ` Qu WenRuo
  0 siblings, 1 reply; 24+ messages in thread
From: Martin Doucha @ 2020-01-13 13:25 UTC (permalink / raw)
  To: ltp

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

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [LTP] [PATCH v3] Use real FS block size in fallocate05
  2020-01-13 13:25                                 ` Martin Doucha
@ 2020-01-13 13:30                                   ` Qu WenRuo
  0 siblings, 0 replies; 24+ messages in thread
From: Qu WenRuo @ 2020-01-13 13:30 UTC (permalink / raw)
  To: ltp



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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2020-01-13 13:30 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28  9:36 [LTP] [PATCH 0/1] Use real FS block size in fallocate05 Martin Doucha
2019-11-28  9:36 ` [LTP] [PATCH 1/1] " Martin Doucha
2019-11-28 17:47   ` Petr Vorel
2019-11-29  9:54     ` Martin Doucha
2019-11-29 12:01   ` Jan Stancek
2019-11-29 15:25     ` Martin Doucha
2019-11-29 16:17       ` Jan Stancek
2019-12-04 10:38         ` Martin Doucha
2019-12-13 13:40           ` Cyril Hrubis
2019-12-17 13:17             ` [LTP] [PATCH v2] " Martin Doucha
2019-12-17 21:02               ` Jan Stancek
2019-12-18  9:09                 ` Martin Doucha
2019-12-18 10:01                   ` Martin Doucha
2019-12-18 10:07                     ` Jan Stancek
2019-12-18 13:15                       ` [LTP] [PATCH v3] " Martin Doucha
2020-01-02 10:01                         ` Jan Stancek
2020-01-07 15:21                         ` Cyril Hrubis
2020-01-07 15:50                           ` Martin Doucha
2020-01-13 12:16                             ` Martin Doucha
2020-01-13 13:16                               ` Qu WenRuo
2020-01-13 13:25                                 ` Martin Doucha
2020-01-13 13:30                                   ` Qu WenRuo
2020-01-07 16:09                           ` Martin Doucha
2020-01-07 16:29                             ` Cyril Hrubis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.