| Message ID | 20240905134502.33759-1-mdoucha@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] fallocate05: Check that deallocated file range is marked as a hole | expand |
On Thu, Sep 5, 2024 at 9:45 PM Martin Doucha <mdoucha@suse.cz> wrote: > Signed-off-by: Martin Doucha <mdoucha@suse.cz> > --- > > The new lseek(SEEK_HOLE/SEEK_DATA) checks will be useful for diagnosing > why the final write() check fails with ENOSPC. If the hole doesn't get > created at all, the lseek() checks will fail. > Make sense! Reviewed-by: Li Wang <liwang@redhat.com>
Hi Martin, > The new lseek(SEEK_HOLE/SEEK_DATA) checks will be useful for diagnosing > why the final write() check fails with ENOSPC. If the hole doesn't get > created at all, the lseek() checks will fail. Thank you! ... > + /* Check that the deallocated file range is marked as a hole */ > + TEST(lseek(fd, 0, SEEK_HOLE)); > + > + if (TST_RET == 0) { > + tst_res(TPASS, "Test file contains hole at offset 0"); > + } else if (TST_RET == -1) { > + tst_res(TFAIL | TTERRNO, "lseek(SEEK_HOLE) failed"); > + } else { > + tst_res(TFAIL | TTERRNO, > + "Unexpected lseek(SEEK_HOLE) return value %ld", > + TST_RET); > + } nit: maybe just using SAFE_LSEEK()? > + > + TEST(lseek(fd, 0, SEEK_DATA)); > + > + if (TST_RET == holesize) { > + tst_res(TPASS, "Test file data start at offset %ld", TST_RET); > + } else if (TST_RET == -1) { > + tst_res(TFAIL | TTERRNO, "lseek(SEEK_DATA) failed"); > + } else { > + tst_res(TFAIL | TTERRNO, > + "Unexpected lseek(SEEK_DATA) return value %ld", > + TST_RET); > + } nit: and here just: TEST(SAFE_LSEEK(fd, 0, SEEK_DATA)); if (TST_RET == holesize) tst_res(TPASS, "Test file data start at offset %ld", TST_RET); Whatever you choose, LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
On 06. 09. 24 10:17, Petr Vorel wrote: > Hi Martin, > >> The new lseek(SEEK_HOLE/SEEK_DATA) checks will be useful for diagnosing >> why the final write() check fails with ENOSPC. If the hole doesn't get >> created at all, the lseek() checks will fail. > > Thank you! > > ... >> + /* Check that the deallocated file range is marked as a hole */ >> + TEST(lseek(fd, 0, SEEK_HOLE)); >> + >> + if (TST_RET == 0) { >> + tst_res(TPASS, "Test file contains hole at offset 0"); >> + } else if (TST_RET == -1) { >> + tst_res(TFAIL | TTERRNO, "lseek(SEEK_HOLE) failed"); >> + } else { >> + tst_res(TFAIL | TTERRNO, >> + "Unexpected lseek(SEEK_HOLE) return value %ld", >> + TST_RET); >> + } > nit: maybe just using SAFE_LSEEK()? Definitely no SAFE_LSEEK() here because I want to continue to the second lseek() even if the first one fails. > >> + >> + TEST(lseek(fd, 0, SEEK_DATA)); >> + >> + if (TST_RET == holesize) { >> + tst_res(TPASS, "Test file data start at offset %ld", TST_RET); >> + } else if (TST_RET == -1) { >> + tst_res(TFAIL | TTERRNO, "lseek(SEEK_DATA) failed"); >> + } else { >> + tst_res(TFAIL | TTERRNO, >> + "Unexpected lseek(SEEK_DATA) return value %ld", >> + TST_RET); >> + } > > nit: and here just: > > TEST(SAFE_LSEEK(fd, 0, SEEK_DATA)); > if (TST_RET == holesize) > tst_res(TPASS, "Test file data start at offset %ld", TST_RET); We could use SAFE_LSEEK() here at least until someone decides to add another sanity check below it. But we still need the "else" branch to report wrong hole size. I'd say it's slightly better to keep it as is for the more descriptive error messages.
> On 06. 09. 24 10:17, Petr Vorel wrote: > > Hi Martin, > > > The new lseek(SEEK_HOLE/SEEK_DATA) checks will be useful for diagnosing > > > why the final write() check fails with ENOSPC. If the hole doesn't get > > > created at all, the lseek() checks will fail. > > Thank you! > > ... > > > + /* Check that the deallocated file range is marked as a hole */ > > > + TEST(lseek(fd, 0, SEEK_HOLE)); > > > + > > > + if (TST_RET == 0) { > > > + tst_res(TPASS, "Test file contains hole at offset 0"); > > > + } else if (TST_RET == -1) { > > > + tst_res(TFAIL | TTERRNO, "lseek(SEEK_HOLE) failed"); > > > + } else { > > > + tst_res(TFAIL | TTERRNO, > > > + "Unexpected lseek(SEEK_HOLE) return value %ld", > > > + TST_RET); > > > + } > > nit: maybe just using SAFE_LSEEK()? > Definitely no SAFE_LSEEK() here because I want to continue to the second > lseek() even if the first one fails. OK, fair enough. > > > + > > > + TEST(lseek(fd, 0, SEEK_DATA)); > > > + > > > + if (TST_RET == holesize) { > > > + tst_res(TPASS, "Test file data start at offset %ld", TST_RET); > > > + } else if (TST_RET == -1) { > > > + tst_res(TFAIL | TTERRNO, "lseek(SEEK_DATA) failed"); > > > + } else { > > > + tst_res(TFAIL | TTERRNO, > > > + "Unexpected lseek(SEEK_DATA) return value %ld", > > > + TST_RET); > > > + } > > nit: and here just: > > TEST(SAFE_LSEEK(fd, 0, SEEK_DATA)); > > if (TST_RET == holesize) > > tst_res(TPASS, "Test file data start at offset %ld", TST_RET); > We could use SAFE_LSEEK() here at least until someone decides to add another > sanity check below it. But we still need the "else" branch to report wrong > hole size. I'd say it's slightly better to keep it as is for the more > descriptive error messages. Fair enough, thx for info. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c index af6bf9e8c..979c70d6e 100644 --- a/testcases/kernel/syscalls/fallocate/fallocate05.c +++ b/testcases/kernel/syscalls/fallocate/fallocate05.c @@ -55,7 +55,7 @@ static void setup(void) static void run(void) { int fd; - long extsize, tmp; + long extsize, holesize, tmp; fd = SAFE_OPEN(MNTPOINT "/test_file", O_WRONLY | O_CREAT | O_TRUNC, 0644); @@ -115,11 +115,12 @@ static void run(void) /* Btrfs deallocates only complete extents, not individual blocks */ if (!strcmp(tst_device->fs_type, "btrfs")) - tmp = bufsize + extsize; + holesize = bufsize + extsize; else - tmp = DEALLOCATE_BLOCKS * blocksize; + holesize = DEALLOCATE_BLOCKS * blocksize; - TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, tmp)); + TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, + holesize)); if (TST_RET == -1) { if (TST_ERR == ENOTSUP) @@ -135,6 +136,31 @@ static void run(void) else tst_res(TPASS, "write()"); + /* Check that the deallocated file range is marked as a hole */ + TEST(lseek(fd, 0, SEEK_HOLE)); + + if (TST_RET == 0) { + tst_res(TPASS, "Test file contains hole at offset 0"); + } else if (TST_RET == -1) { + tst_res(TFAIL | TTERRNO, "lseek(SEEK_HOLE) failed"); + } else { + tst_res(TFAIL | TTERRNO, + "Unexpected lseek(SEEK_HOLE) return value %ld", + TST_RET); + } + + TEST(lseek(fd, 0, SEEK_DATA)); + + if (TST_RET == holesize) { + tst_res(TPASS, "Test file data start at offset %ld", TST_RET); + } else if (TST_RET == -1) { + tst_res(TFAIL | TTERRNO, "lseek(SEEK_DATA) failed"); + } else { + tst_res(TFAIL | TTERRNO, + "Unexpected lseek(SEEK_DATA) return value %ld", + TST_RET); + } + SAFE_CLOSE(fd); tst_purge_dir(MNTPOINT); }
Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- The new lseek(SEEK_HOLE/SEEK_DATA) checks will be useful for diagnosing why the final write() check fails with ENOSPC. If the hole doesn't get created at all, the lseek() checks will fail. .../kernel/syscalls/fallocate/fallocate05.c | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-)