* [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: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
* [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
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.