[LTP] [PATCH 1/2] fallocate05: Check that deallocated file range is marked as a hole

[LTP] [PATCH 1/2] fallocate05: Check that deallocated file range is marked as a hole

[Martin Doucha]

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

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);
 }
-- 
2.46.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/2] fallocate05: Deallocate whole file on bcachefs

[Martin Doucha]

The default deallocation size is likely too small for bcachefs
to actually release the blocks. Since it is also a copy-on-write
filesystem, deallocated the whole file like on Btrfs.

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

AFAICT Bcachefs uses 512 byte data blocks by default but 256KB inode
blocks. The whole file will be 128KB and 32KB gets deallocated which may
be too small. However, I'm not entirely sure whether this is the best
solution.

See also https://bugzilla.suse.com/show_bug.cgi?id=1230155

 testcases/kernel/syscalls/fallocate/fallocate05.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c
index 979c70d6e..732a2f15d 100644
--- a/testcases/kernel/syscalls/fallocate/fallocate05.c
+++ b/testcases/kernel/syscalls/fallocate/fallocate05.c
@@ -114,7 +114,8 @@ static void run(void)
 	tst_res(TPASS, "fallocate() on full FS");
 
 	/* Btrfs deallocates only complete extents, not individual blocks */
-	if (!strcmp(tst_device->fs_type, "btrfs"))
+	if (!strcmp(tst_device->fs_type, "btrfs") ||
+		!strcmp(tst_device->fs_type, "bcachefs"))
 		holesize = bufsize + extsize;
 	else
 		holesize = DEALLOCATE_BLOCKS * blocksize;
-- 
2.46.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/2] fallocate05: Deallocate whole file on bcachefs

[Li Wang]

On Thu, Sep 5, 2024 at 9:45 PM Martin Doucha <mdoucha@suse.cz> wrote:

> The default deallocation size is likely too small for bcachefs
> to actually release the blocks. Since it is also a copy-on-write
> filesystem, deallocated the whole file like on Btrfs.
>
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>

---
>
> AFAICT Bcachefs uses 512 byte data blocks by default but 256KB inode
> blocks. The whole file will be 128KB and 32KB gets deallocated which may
> be too small. However, I'm not entirely sure whether this is the best
> solution.
>

I think this solution is correct.

Even deallocating a hole size (32KB) that is larger than a block size
512bytes,
but that does not mean the Bcachefs can satisfy enough size for following
written
behavior, because the allocation of new data blocks (hole) might involve
modifying
metadata structures that need more space(extends more inode blocks 256KB).

If the filesystem cannot accommodate these changes, it could lead to an
ENOSPC error.

Reviewed-by: Li Wang <liwang@redhat.com>


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/2] fallocate05: Deallocate whole file on bcachefs

[Petr Vorel]

Hi Martin, all,

[ Cc Kent and Bcachefs ML ]

> The default deallocation size is likely too small for bcachefs
> to actually release the blocks. Since it is also a copy-on-write
> filesystem, deallocated the whole file like on Btrfs.

Make sense.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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

> AFAICT Bcachefs uses 512 byte data blocks by default but 256KB inode
> blocks. The whole file will be 128KB and 32KB gets deallocated which may
> be too small. However, I'm not entirely sure whether this is the best
> solution.

> See also https://bugzilla.suse.com/show_bug.cgi?id=1230155

>  testcases/kernel/syscalls/fallocate/fallocate05.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

> diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c
> index 979c70d6e..732a2f15d 100644
> --- a/testcases/kernel/syscalls/fallocate/fallocate05.c
> +++ b/testcases/kernel/syscalls/fallocate/fallocate05.c
> @@ -114,7 +114,8 @@ static void run(void)
>  	tst_res(TPASS, "fallocate() on full FS");

>  	/* Btrfs deallocates only complete extents, not individual blocks */
> -	if (!strcmp(tst_device->fs_type, "btrfs"))
> +	if (!strcmp(tst_device->fs_type, "btrfs") ||
> +		!strcmp(tst_device->fs_type, "bcachefs"))
>  		holesize = bufsize + extsize;
>  	else
>  		holesize = DEALLOCATE_BLOCKS * blocksize;

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/2] fallocate05: Deallocate whole file on bcachefs

[Kent Overstreet]

On Fri, Sep 06, 2024 at 10:02:00AM GMT, Petr Vorel wrote:
> Hi Martin, all,
> 
> [ Cc Kent and Bcachefs ML ]
> 
> > The default deallocation size is likely too small for bcachefs
> > to actually release the blocks. Since it is also a copy-on-write
> > filesystem, deallocated the whole file like on Btrfs.
> 
> Make sense.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>

I haven't looked at this specific test, but one thing that we run into
with the weird CoW behaviour tests in xfstests is that bcachefs btree
nodes are 256k by default - you're going to see weird effects from that
if tests are looking at disk usage.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/2] fallocate05: Deallocate whole file on bcachefs

[Li Wang]

On Fri, Sep 6, 2024 at 6:18 PM Kent Overstreet <kent.overstreet@linux.dev>
wrote:

> On Fri, Sep 06, 2024 at 10:02:00AM GMT, Petr Vorel wrote:
> > Hi Martin, all,
> >
> > [ Cc Kent and Bcachefs ML ]
> >
> > > The default deallocation size is likely too small for bcachefs
> > > to actually release the blocks. Since it is also a copy-on-write
> > > filesystem, deallocated the whole file like on Btrfs.
> >
> > Make sense.
> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> I haven't looked at this specific test, but one thing that we run into
> with the weird CoW behaviour tests in xfstests is that bcachefs btree
> nodes are 256k by default - you're going to see weird effects from that
> if tests are looking at disk usage.
>

Thank you. Patch merged.


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/2] fallocate05: Check that deallocated file range is marked as a hole

[Li Wang]

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>


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/2] fallocate05: Check that deallocated file range is marked as a hole

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/2] fallocate05: Check that deallocated file range is marked as a hole

[Martin Doucha]

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.

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


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/2] fallocate05: Check that deallocated file range is marked as a hole

[Petr Vorel]

> 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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp