Re: [LTP] [PATCH v1] msync04: Check disk content if dirty bit check failed

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> msync04 test is inherently racy and nothing guarantees that the page
> is not written out before get_dirty_page() manages to read the page state.
> Hence the test should be reworked to verify the page contents is on disk
> when it finds dirty bit isn't set anymore...

It's nice habit to add Jan's credit :)

He did it [1], thus one would add:
Suggested-by: Jan Kara <jack@suse.cz>

+ Add a ticket:
Implements: https://github.com/linux-test-project/ltp/issues/1157

[1] https://bugzilla.suse.com/show_bug.cgi?id=1224201#c13
[2] https://lore.kernel.org/ltp/20220125121746.wrs4254pfs2mwexb@quack3.lan/

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/syscalls/msync/msync04.c | 34 ++++++++++++++---------
>  1 file changed, 21 insertions(+), 13 deletions(-)

> diff --git a/testcases/kernel/syscalls/msync/msync04.c b/testcases/kernel/syscalls/msync/msync04.c
> index 72ddc27a4..c296c8b20 100644
> --- a/testcases/kernel/syscalls/msync/msync04.c
> +++ b/testcases/kernel/syscalls/msync/msync04.c
> @@ -46,6 +46,7 @@ uint64_t get_dirty_bit(void *data)
>  static void test_msync(void)
>  {
>  	uint64_t dirty;
While at it, could you please remove dirty variable and use get_dirty_bit(...)
directly?

> +	char buffer[20];

>  	test_fd = SAFE_OPEN("msync04/testfile", O_CREAT | O_TRUNC | O_RDWR,
>  		0644);
> @@ -56,20 +57,27 @@ static void test_msync(void)
>  	mmaped_area[8] = 'B';
>  	dirty = get_dirty_bit(mmaped_area);
>  	if (!dirty) {
 	if (!get_dirty_bit(mmaped_area)) {

> -		tst_res(TFAIL, "Expected dirty bit to be set after writing to"
> -				" mmap()-ed area");
> -		goto clean;
> -	}
> -	if (msync(mmaped_area, pagesize, MS_SYNC) < 0) {
> -		tst_res(TFAIL | TERRNO, "msync() failed");
> -		goto clean;
> +		tst_res(TINFO, "Not see dirty bit so we check content of file instead");
> +		test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY);
> +		SAFE_READ(0, test_fd, buffer, 9);
> +		if (buffer[8] == 'B')
> +			tst_res(TCONF, "write file ok but msync couldn't be tested"
> +				" because the storage was written to too quickly");
> +		else
> +			tst_res(TFAIL, "write file failed");
> +	} else {
> +		if (msync(mmaped_area, pagesize, MS_SYNC) < 0) {
> +			tst_res(TFAIL | TERRNO, "msync() failed");
I know you copy old code, but it would deserve to fix: -1 is failure, anything
different than 0 or -1 is invalid value.  And you get this for free if you use
TST_EXP_PASS_SILENT().

> +			goto clean;

nit: Having if and else part in it's own functions (verify_mmaped(),
verify_dirty() would allow to get rid of goto and produced slightly nicer code
(less indentation => less split lines):

	if (!get_dirty_bit(mmaped_area))
		verify_mmaped();
	else
		verify_dirty();

	SAFE_MUNMAP(mmaped_area, pagesize);
	mmaped_area = NULL;

> +		}
> +		dirty = get_dirty_bit(mmaped_area);
> +		if (dirty)
		if (get_dirty_bit(mmaped_area)) {

> +			tst_res(TFAIL, "msync() failed to write dirty page despite"
> +					" succeeding");
I would keep this string separate
> +		else
> +			tst_res(TPASS, "msync() working correctly");
> +

>  	}
> -	dirty = get_dirty_bit(mmaped_area);
> -	if (dirty)
> -		tst_res(TFAIL, "msync() failed to write dirty page despite"
> -				" succeeding");
> -	else
> -		tst_res(TPASS, "msync() working correctly");

>  clean:
>  	SAFE_MUNMAP(mmaped_area, pagesize);

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