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

[Jan Kara]

Hi Petr!

On Tue 28-05-24 01:16:36, Petr Vorel wrote:
> > 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...
> 
> @Jan some time ago [1] you asked to:
> 
> 	mmap file, write to mmap, msync, abort fs, mount fs again, check the data is
> 	there.
> 
> Could you please have a look if this is enough? Or is it aborting fs and mounting
> again necessary?

Well, it depends on what functionality exactly you want to test... See
below.

> > 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;
> > +	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) {
> > -		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");

So this will read the very same page in the page cache that mmap(2) has
been using for write. As such it isn't very interesting verification. I'd
at least use direct IO to verify the data is stored on disk now.

> > +	} else {
> > +		if (msync(mmaped_area, pagesize, MS_SYNC) < 0) {
> > +			tst_res(TFAIL | TERRNO, "msync() failed");
> > +			goto clean;
> > +		}
> > +		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");
> > +
> >  	}
> > -	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");

Overall this looks correct. But what this test really does is that it
verifies msync(2) is clearing dirty page bits. That is not very useful
verification from userspace point of view (as IMO it leans too much on
internal kernel implementation details). msync(2) is really a data
integrity operation so ideally its tests would verify data integrity
guarantees are met after a power failure - that is what userspace is
interested in. But probably these test are more within the realm of fstests
(where we do similar tests) as LTP lacks necessary infrastructure to do
this low-level filesystem manipulation so I guess what you have is fine.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR