[LTP] [PATCH v1] msync04.c: Use direct IO to verify the data is stored on disk

[LTP] [PATCH v1] msync04.c: Use direct IO to verify the data is stored on disk

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
Suggested-by: Jan Kara <jack@suse.cz>
---
 testcases/kernel/syscalls/msync/msync04.c | 56 +++++++++++++----------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/testcases/kernel/syscalls/msync/msync04.c b/testcases/kernel/syscalls/msync/msync04.c
index 1718bd7dc..c0580d1b0 100644
--- a/testcases/kernel/syscalls/msync/msync04.c
+++ b/testcases/kernel/syscalls/msync/msync04.c
@@ -11,6 +11,7 @@
  * is no longer dirty after msync() call.
  */
 
+#define _GNU_SOURCE
 #include <errno.h>
 #include "tst_test.h"
 
@@ -43,10 +44,35 @@ static uint64_t get_dirty_bit(void *data)
 	return pageflag_entry & (1ULL << 4);
 }
 
-static void test_msync(void)
+static void verify_mmaped(void)
+{
+	void *buffer = SAFE_MEMALIGN(getpagesize(), getpagesize());
+
+	tst_res(TINFO, "Not see dirty bit so we check content of file instead");
+	test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY | O_DIRECT);
+	SAFE_READ(0, test_fd, buffer, getpagesize());
+
+	char *char_buffer = (char *)buffer;
+
+	if (char_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");
+}
+
+static void verify_dirty(void)
 {
-	char buffer[20];
+	TST_EXP_PASS_SILENT(msync(mmaped_area, pagesize, MS_SYNC));
+
+	if (TST_RET == 0 && !get_dirty_bit(mmaped_area))
+		tst_res(TPASS, "msync() verify dirty page ok");
+	else
+		tst_res(TFAIL, "msync() verify dirty page failed");
+}
 
+static void test_msync(void)
+{
 	test_fd = SAFE_OPEN("msync04/testfile", O_CREAT | O_TRUNC | O_RDWR,
 		0644);
 	SAFE_WRITE(SAFE_WRITE_ANY, test_fd, STRING_TO_WRITE, sizeof(STRING_TO_WRITE) - 1);
@@ -55,27 +81,11 @@ static void test_msync(void)
 	SAFE_CLOSE(test_fd);
 	mmaped_area[8] = 'B';
 
-	if (!get_dirty_bit(mmaped_area)) {
-		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");
-			goto clean;
-		}
-		if (get_dirty_bit(mmaped_area))
-			tst_res(TFAIL, "msync() failed to write dirty page despite succeeding");
-		else
-			tst_res(TPASS, "msync() working correctly");
-	}
-
-clean:
+	if (!get_dirty_bit(mmaped_area))
+		verify_mmaped();
+	else
+		verify_dirty();
+
 	SAFE_MUNMAP(mmaped_area, pagesize);
 	mmaped_area = NULL;
 }
-- 
2.35.3


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

Re: [LTP] [PATCH v1] msync04.c: Use direct IO to verify the data is stored on disk

[Jan Kara]

On Thu 30-05-24 00:40:29, Wei Gao wrote:
> Signed-off-by: Wei Gao <wegao@suse.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Looks good to me! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  testcases/kernel/syscalls/msync/msync04.c | 56 +++++++++++++----------
>  1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/msync/msync04.c b/testcases/kernel/syscalls/msync/msync04.c
> index 1718bd7dc..c0580d1b0 100644
> --- a/testcases/kernel/syscalls/msync/msync04.c
> +++ b/testcases/kernel/syscalls/msync/msync04.c
> @@ -11,6 +11,7 @@
>   * is no longer dirty after msync() call.
>   */
>  
> +#define _GNU_SOURCE
>  #include <errno.h>
>  #include "tst_test.h"
>  
> @@ -43,10 +44,35 @@ static uint64_t get_dirty_bit(void *data)
>  	return pageflag_entry & (1ULL << 4);
>  }
>  
> -static void test_msync(void)
> +static void verify_mmaped(void)
> +{
> +	void *buffer = SAFE_MEMALIGN(getpagesize(), getpagesize());
> +
> +	tst_res(TINFO, "Not see dirty bit so we check content of file instead");
> +	test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY | O_DIRECT);
> +	SAFE_READ(0, test_fd, buffer, getpagesize());
> +
> +	char *char_buffer = (char *)buffer;
> +
> +	if (char_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");
> +}
> +
> +static void verify_dirty(void)
>  {
> -	char buffer[20];
> +	TST_EXP_PASS_SILENT(msync(mmaped_area, pagesize, MS_SYNC));
> +
> +	if (TST_RET == 0 && !get_dirty_bit(mmaped_area))
> +		tst_res(TPASS, "msync() verify dirty page ok");
> +	else
> +		tst_res(TFAIL, "msync() verify dirty page failed");
> +}
>  
> +static void test_msync(void)
> +{
>  	test_fd = SAFE_OPEN("msync04/testfile", O_CREAT | O_TRUNC | O_RDWR,
>  		0644);
>  	SAFE_WRITE(SAFE_WRITE_ANY, test_fd, STRING_TO_WRITE, sizeof(STRING_TO_WRITE) - 1);
> @@ -55,27 +81,11 @@ static void test_msync(void)
>  	SAFE_CLOSE(test_fd);
>  	mmaped_area[8] = 'B';
>  
> -	if (!get_dirty_bit(mmaped_area)) {
> -		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");
> -			goto clean;
> -		}
> -		if (get_dirty_bit(mmaped_area))
> -			tst_res(TFAIL, "msync() failed to write dirty page despite succeeding");
> -		else
> -			tst_res(TPASS, "msync() working correctly");
> -	}
> -
> -clean:
> +	if (!get_dirty_bit(mmaped_area))
> +		verify_mmaped();
> +	else
> +		verify_dirty();
> +
>  	SAFE_MUNMAP(mmaped_area, pagesize);
>  	mmaped_area = NULL;
>  }
> -- 
> 2.35.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

Re: [LTP] [PATCH v1] msync04.c: Use direct IO to verify the data is stored on disk

[Cyril Hrubis]

Hi!
> Signed-off-by: Wei Gao <wegao@suse.com>
> Suggested-by: Jan Kara <jack@suse.cz>
> ---
>  testcases/kernel/syscalls/msync/msync04.c | 56 +++++++++++++----------
>  1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/msync/msync04.c b/testcases/kernel/syscalls/msync/msync04.c
> index 1718bd7dc..c0580d1b0 100644
> --- a/testcases/kernel/syscalls/msync/msync04.c
> +++ b/testcases/kernel/syscalls/msync/msync04.c
> @@ -11,6 +11,7 @@
>   * is no longer dirty after msync() call.
>   */
>  
> +#define _GNU_SOURCE
>  #include <errno.h>
>  #include "tst_test.h"
>  
> @@ -43,10 +44,35 @@ static uint64_t get_dirty_bit(void *data)
>  	return pageflag_entry & (1ULL << 4);
>  }
>  
> -static void test_msync(void)
> +static void verify_mmaped(void)
> +{
> +	void *buffer = SAFE_MEMALIGN(getpagesize(), getpagesize());
> +
> +	tst_res(TINFO, "Not see dirty bit so we check content of file instead");
                         ^
			Haven't seen dirty...

> +	test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY | O_DIRECT);
> +	SAFE_READ(0, test_fd, buffer, getpagesize());
> +
> +	char *char_buffer = (char *)buffer;

You can declare the buffer directly as char * instead, there is no need
for this indirection.

> +	if (char_buffer[8] == 'B')
> +		tst_res(TCONF, "write file ok but msync couldn't be tested"
> +				" because the storage was written to too quickly");

This could be shorter and stil to the point:

"Write was too fast, couldn't test msync()"

> +	else
> +		tst_res(TFAIL, "write file failed");

We should free() allocated the memory here.


Other than these minor issues the rest looks good to me.

You can add my Reviewed-by: once these issues are fixed.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH v2] msync04.c: Use direct IO to verify the data is stored on disk

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
Suggested-by: Jan Kara <jack@suse.cz>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/syscalls/msync/msync04.c | 55 +++++++++++++----------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/testcases/kernel/syscalls/msync/msync04.c b/testcases/kernel/syscalls/msync/msync04.c
index 1718bd7dc..e3478f690 100644
--- a/testcases/kernel/syscalls/msync/msync04.c
+++ b/testcases/kernel/syscalls/msync/msync04.c
@@ -11,6 +11,7 @@
  * is no longer dirty after msync() call.
  */
 
+#define _GNU_SOURCE
 #include <errno.h>
 #include "tst_test.h"
 
@@ -43,10 +44,34 @@ static uint64_t get_dirty_bit(void *data)
 	return pageflag_entry & (1ULL << 4);
 }
 
-static void test_msync(void)
+static void verify_mmaped(void)
+{
+	char *buffer = SAFE_MEMALIGN(getpagesize(), getpagesize());
+
+	tst_res(TINFO, "Haven't seen dirty bit so we check content of file instead");
+	test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY | O_DIRECT);
+	SAFE_READ(0, test_fd, buffer, getpagesize());
+
+	if (buffer[8] == 'B')
+		tst_res(TCONF, "Write was too fast, couldn't test msync()");
+	else
+		tst_res(TFAIL, "write file failed");
+
+	free(buffer);
+}
+
+static void verify_dirty(void)
 {
-	char buffer[20];
+	TST_EXP_PASS_SILENT(msync(mmaped_area, pagesize, MS_SYNC));
+
+	if (TST_RET == 0 && !get_dirty_bit(mmaped_area))
+		tst_res(TPASS, "msync() verify dirty page ok");
+	else
+		tst_res(TFAIL, "msync() verify dirty page failed");
+}
 
+static void test_msync(void)
+{
 	test_fd = SAFE_OPEN("msync04/testfile", O_CREAT | O_TRUNC | O_RDWR,
 		0644);
 	SAFE_WRITE(SAFE_WRITE_ANY, test_fd, STRING_TO_WRITE, sizeof(STRING_TO_WRITE) - 1);
@@ -55,27 +80,11 @@ static void test_msync(void)
 	SAFE_CLOSE(test_fd);
 	mmaped_area[8] = 'B';
 
-	if (!get_dirty_bit(mmaped_area)) {
-		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");
-			goto clean;
-		}
-		if (get_dirty_bit(mmaped_area))
-			tst_res(TFAIL, "msync() failed to write dirty page despite succeeding");
-		else
-			tst_res(TPASS, "msync() working correctly");
-	}
-
-clean:
+	if (!get_dirty_bit(mmaped_area))
+		verify_mmaped();
+	else
+		verify_dirty();
+
 	SAFE_MUNMAP(mmaped_area, pagesize);
 	mmaped_area = NULL;
 }
-- 
2.35.3


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

Re: [LTP] [PATCH v2] msync04.c: Use direct IO to verify the data is stored on disk

[Petr Vorel]

Hi Wei,

...
> +static void verify_mmaped(void)
> +{
> +	char *buffer = SAFE_MEMALIGN(getpagesize(), getpagesize());
> +
> +	tst_res(TINFO, "Haven't seen dirty bit so we check content of file instead");
> +	test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY | O_DIRECT);
If this fails we have memory leak, because free() is not run.

> +	SAFE_READ(0, test_fd, buffer, getpagesize());
Also here. Maybe create buf as static and have conditional free() also at the
cleanup?

Kind regards,
Petr

> +
> +	if (buffer[8] == 'B')
> +		tst_res(TCONF, "Write was too fast, couldn't test msync()");
> +	else
> +		tst_res(TFAIL, "write file failed");
> +
> +	free(buffer);
> +}
> +

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

Re: [LTP] [PATCH v2] msync04.c: Use direct IO to verify the data is stored on disk

[Cyril Hrubis]

Hi!
> > +static void verify_mmaped(void)
> > +{
> > +	char *buffer = SAFE_MEMALIGN(getpagesize(), getpagesize());
> > +
> > +	tst_res(TINFO, "Haven't seen dirty bit so we check content of file instead");
> > +	test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY | O_DIRECT);
> If this fails we have memory leak, because free() is not run.
> 
> > +	SAFE_READ(0, test_fd, buffer, getpagesize());
> Also here. Maybe create buf as static and have conditional free() also at the
> cleanup?

If these fail we exit the test, which frees memory anyways...

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2] msync04.c: Use direct IO to verify the data is stored on disk

[Petr Vorel]

Hi all,

thanks, merged!

Kind regards,
Petr

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

Re: [LTP] [PATCH v2] msync04.c: Use direct IO to verify the data is stored on disk

[Petr Vorel]

> Hi!
> > > +static void verify_mmaped(void)
> > > +{
> > > +	char *buffer = SAFE_MEMALIGN(getpagesize(), getpagesize());
> > > +
> > > +	tst_res(TINFO, "Haven't seen dirty bit so we check content of file instead");
> > > +	test_fd = SAFE_OPEN("msync04/testfile", O_RDONLY | O_DIRECT);
> > If this fails we have memory leak, because free() is not run.

> > > +	SAFE_READ(0, test_fd, buffer, getpagesize());
> > Also here. Maybe create buf as static and have conditional free() also at the
> > cleanup?

> If these fail we exit the test, which frees memory anyways...

Thanks for correcting me.  I always forget the basis that exit() frees the
memory (unlike return 0 and unless overwritten). And we call exit() in
tst_brk().

Kind regards,
Petr

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