| Message ID | 20191128093610.6903-2-mdoucha@suse.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | Use real FS block size in fallocate05 | expand |
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
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.
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.
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.
----- 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));
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.
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.
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 = {
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(-)