Re: [PATCH] generic: test lseek with seek data mode on a one byte file

[David Disseldorp <ddiss@suse.de>]

Looks fine:
Reviewed-by: David Disseldorp <ddiss@suse.de>

Minor nit below...

On Mon,  9 Jan 2023 10:34:15 +0000, fdmanana@kernel.org wrote:

> From: Filipe Manana <fdmanana@suse.com>
> 
> Test that seeking for data on a 1 byte file works correctly, the returned
> offset should be 0 if the start offset is 0.
> 
> This is a regression test motivated by a btrfs bug introduced in kernel
> 6.1, which got recently fixed by the following kernel commit:
> 
>   2f2e84ca6066 ("btrfs: fix off-by-one in delalloc search during lseek")
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  src/seek_sanity_test.c | 36 ++++++++++++++++++++++++++++++++++++
>  tests/generic/706      | 36 ++++++++++++++++++++++++++++++++++++
>  tests/generic/706.out  |  2 ++
>  3 files changed, 74 insertions(+)
>  create mode 100755 tests/generic/706
>  create mode 100644 tests/generic/706.out
> 
> diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> index 8a586f74..48b3ccc0 100644
> --- a/src/seek_sanity_test.c
> +++ b/src/seek_sanity_test.c
> @@ -292,6 +292,41 @@ out:
>  	return ret;
>  }
>  
> +/*
> + * Test seeking for data on a 1 byte file, both when there's delalloc and also
> + * after delalloc is flushed.
> + */
> +static int test22(int fd, int testnum)
> +{
> +	const char buf = 'X';
> +	int ret;
> +
> +	ret = do_pwrite(fd, &buf, 1, 0);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Our file as a size of 1 byte and that byte is in delalloc. Seeking
> +	 * for data, with a start offset of 0, should return file offset 0.
> +	 */
> +	ret = do_lseek(testnum, 1, fd, 1, SEEK_DATA, 0, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Flush all delalloc. */
> +	ret = fsync(fd);
> +	if (ret) {
> +		fprintf(stderr, "fsync failed: %s (%d)\n", strerror(errno), errno);
> +		return ret;
> +	}
> +
> +	/*
> +	 * We should get the same result we got when we had delalloc, 0 is the
> +	 * offset with data.
> +	 */
> +	return do_lseek(testnum, 2, fd, 1, SEEK_DATA, 0, 0);
> +}
> +
>  /*
>   * Make sure hole size is properly reported when punched in the middle of a file
>   */
> @@ -1131,6 +1166,7 @@ struct testrec seek_tests[] = {
>         { 19, test19, "Test file SEEK_DATA from middle of a large hole" },
>         { 20, test20, "Test file SEEK_DATA from middle of a huge hole" },
>         { 21, test21, "Test file SEEK_HOLE that was created by PUNCH_HOLE" },
> +       { 22, test22, "Test a 1 byte file" },
>  };
>  
>  static int run_test(struct testrec *tr)
> diff --git a/tests/generic/706 b/tests/generic/706
> new file mode 100755
> index 00000000..b3e7aa7c
> --- /dev/null
> +++ b/tests/generic/706
> @@ -0,0 +1,36 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2023 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 706
> +#
> +# Test that seeking for data on a 1 byte file works correctly, the returned
> +# offset should be 0 if the start offset is 0.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick seek
> +
> +[ $FSTYP == "btrfs" ] &&
> +	_fixed_by_kernel_commit 2f2e84ca6066 \
> +	"btrfs: fix off-by-one in delalloc search during lseek"
> +
> +_supported_fs generic
> +_require_test
> +_require_seek_data_hole
> +_require_test_program "seek_sanity_test"
> +
> +test_file=$TEST_DIR/seek_sanity_testfile.$seq

Nit: test_file set and use below should be quoted. 

> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +	rm -f $test_file
> +}
> +
> +_run_seek_sanity_test -s 22 -e 22 $test_file > $seqres.full 2>&1 ||
> +	_fail "seek sanity check failed!"

Nit: I think it'd be nicer to avoid the redirect and _fail here, instead
filtering the FS magic / alloc size strings. I'm okay with it as-is
though.

Cheers, David

> +
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/706.out b/tests/generic/706.out
> new file mode 100644
> index 00000000..577697c6
> --- /dev/null
> +++ b/tests/generic/706.out
> @@ -0,0 +1,2 @@
> +QA output created by 706
> +Silence is golden