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

[Petr Vorel]

> 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.

+1

> > > +	} 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.

Thanks for having a deeper look and explanation! I guess we take it and we can
think how hard would be to implement the hard way (abort fs), as Cyril wanted we
do it.  Or we at least add direct IO. (update that in the ticket:
https://github.com/linux-test-project/ltp/issues/1157).
And sure, if anybody adds msync() support to fstests, that would be the best.

But for now I merged it with your RBT (I'm sorry if I was too inventive to add
it) to get some improvement (hope that without direct IO we did not actually
turned it into false negative).

@Wei: feel free to further improve it.

Kind regards,
Petr

> 								Honza