| Message ID | 20240522124754.9599-1-wegao@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] msync04: Check disk content if dirty bit check failed | expand |
Hi Jan, 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... @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? Kind regards, Petr [1] 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; > + 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"); > + } 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"); > clean: > SAFE_MUNMAP(mmaped_area, pagesize);
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);
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
> 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
On Tue, May 28, 2024 at 02:39:45PM +0200, Petr Vorel wrote: > > Hi Petr! > > > 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. Thanks Petr and Jan's feedback, i will continue do improve on this case such as add direct IO firstly. > > Kind regards, > Petr > > > Honza
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"); + } 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"); clean: SAFE_MUNMAP(mmaped_area, pagesize);
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... Signed-off-by: Wei Gao <wegao@suse.com> --- testcases/kernel/syscalls/msync/msync04.c | 34 ++++++++++++++--------- 1 file changed, 21 insertions(+), 13 deletions(-)