| Message ID | 1551962651-22261-1-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] syscalls/sync_file_range: add partial file sync test-cases | expand |
Hi!
Sorry for the long delay.
This is altmost perfect, the only problem is that the third test fails
on vfat. As far as I can tell the reason is that vfat does not support
sparse files, hence seeking to the middle of file and writing there also
schedulles I/O to write zeros from the start of the file to the offset
we started writing to.
Following ugly patch solves the problem:
diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
index 334ea5e88..774524c2f 100644
--- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
+++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
@@ -45,6 +45,12 @@ static void verify_sync_file_range(struct testcase *tc)
fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE);
+ if (!strcmp(tst_device->fs_type, "vfat")) {
+ tst_res(TINFO, "Pre-filling file");
+ tst_fill_fd(fd, 0, tc->write_off, 1);
+ fsync(fd);
+ }
+
lseek(fd, tc->write_off, SEEK_SET);
So either we limit the tests so that the sync region does not overlap with the
possible hole at the start of the file and loose some test coverage.
Or we can add a function to the test library that would return true/false if
sparse files are supported for a given FS.
On Wed, 27 Mar 2019 at 20:18, Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > Sorry for the long delay. > > This is altmost perfect, the only problem is that the third test fails > on vfat. As far as I can tell the reason is that vfat does not support > sparse files, hence seeking to the middle of file and writing there also > schedulles I/O to write zeros from the start of the file to the offset > we started writing to. > Hmm, I see. > Following ugly patch solves the problem: > > diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > index 334ea5e88..774524c2f 100644 > --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > @@ -45,6 +45,12 @@ static void verify_sync_file_range(struct testcase *tc) > > fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE); > > + if (!strcmp(tst_device->fs_type, "vfat")) { > + tst_res(TINFO, "Pre-filling file"); > + tst_fill_fd(fd, 0, tc->write_off, 1); > + fsync(fd); > + } > + > lseek(fd, tc->write_off, SEEK_SET); > > > So either we limit the tests so that the sync region does not overlap with the > possible hole at the start of the file and loose some test coverage. > > Or we can add a function to the test library that would return true/false if > sparse files are supported for a given FS. > My initial thought behind this test-case was to run sync over a range which is partially written. The other partial region not being written could either be a hole or already synced data. So pre-fill file in case of vfat looks sane option, but how about if we add pre-fill as part of setup? Something like: --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c @@ -86,6 +86,12 @@ static void setup(void) { if (!check_sync_file_range()) tst_brk(TCONF, "sync_file_range() not supported"); + + if (!strcmp(tst_device->fs_type, "vfat")) { + tst_res(TINFO, "Pre-filling file"); + tst_fill_file(FNAME3, 0, TST_MB, FILE_SZ_MB); + sync(); + } } -Sumit > -- > Cyril Hrubis > chrubis@suse.cz
Hi Sumit, On Thu, Mar 7, 2019 at 8:44 PM Sumit Garg <sumit.garg@linaro.org> wrote: > Add partial file sync tests as part of sync_file_range02 test-case. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > > Changes in v3: > 1. Add upper bound check for synced size to device. > 2. Refactor tests for more code reuse. > 3. Add another test to check sync over partial write. > > Changes in v2: > 1. Do full file write instead of partial and test sync partial file. > > .../syscalls/sync_file_range/sync_file_range02.c | 47 > +++++++++++++++++----- > 1 file changed, 37 insertions(+), 10 deletions(-) > > diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > index 82d77f7..334ea5e 100644 > --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > @@ -22,23 +22,36 @@ > #include "check_sync_file_range.h" > > #define MNTPOINT "mnt_point" > -#define FNAME MNTPOINT"/test" > -#define FILE_SIZE_MB 32 > -#define FILE_SIZE (FILE_SIZE_MB * TST_MB) > +#define FNAME1 MNTPOINT"/test1" > +#define FNAME2 MNTPOINT"/test2" > +#define FNAME3 MNTPOINT"/test3" > +#define FILE_SZ_MB 32 > +#define FILE_SZ (FILE_SZ_MB * TST_MB) > #define MODE 0644 > > -static void verify_sync_file_range(void) > +struct testcase { > + char *fname; > + off64_t sync_off; > + off64_t sync_size; > + size_t exp_sync_size; > + off64_t write_off; > + size_t write_size_mb; > +}; > + > +static void verify_sync_file_range(struct testcase *tc) > { > int fd; > unsigned long written; > > - fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); > + fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE); > + > + lseek(fd, tc->write_off, SEEK_SET); > > tst_dev_bytes_written(tst_device->dev); > > - tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB); > + tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb); > I'm just thinking that is probably more precise if we reverse the order of tst_fill_fd() and tst_dev_bytes_written()? Because that does counting the dev_bytes_writen only before and after sync_file_range(), we cann't garantee system does not wirte back to deviece when do fill_fd(), isn't it? > > - TEST(sync_file_range(fd, 0, FILE_SIZE, > + TEST(sync_file_range(fd, tc->sync_off, tc->sync_size, > SYNC_FILE_RANGE_WAIT_BEFORE | > SYNC_FILE_RANGE_WRITE | > SYNC_FILE_RANGE_WAIT_AFTER)); > @@ -50,10 +63,23 @@ static void verify_sync_file_range(void) > > SAFE_CLOSE(fd); > > - if (written >= FILE_SIZE) > + if ((written >= tc->exp_sync_size) && > + (written <= (tc->exp_sync_size + tc->exp_sync_size/10))) > tst_res(TPASS, "Test file range synced to device"); > else > - tst_res(TFAIL, "Synced %li, expected %i", written, > FILE_SIZE); > + tst_res(TFAIL, "Synced %li, expected %li", written, > + tc->exp_sync_size); > +} > + > +static struct testcase testcases[] = { > + { FNAME1, 0, FILE_SZ, FILE_SZ, 0, FILE_SZ_MB }, > + { FNAME2, FILE_SZ/4, FILE_SZ/2, FILE_SZ/2, 0, FILE_SZ_MB }, > + { FNAME3, FILE_SZ/4, FILE_SZ/2, FILE_SZ/4, FILE_SZ/2, FILE_SZ_MB/4 > }, > +}; > + > +static void run(unsigned int i) > +{ > + verify_sync_file_range(&testcases[i]); > } > > static void setup(void) > @@ -63,10 +89,11 @@ static void setup(void) > } > > static struct tst_test test = { > + .tcnt = ARRAY_SIZE(testcases), > .needs_root = 1, > .mount_device = 1, > .all_filesystems = 1, > .mntpoint = MNTPOINT, > .setup = setup, > - .test_all = verify_sync_file_range, > + .test = run, > }; > -- > 2.7.4 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > -- Regards, Li Wang <div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi Sumit,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 7, 2019 at 8:44 PM Sumit Garg <<a href="mailto:sumit.garg@linaro.org">sumit.garg@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Add partial file sync tests as part of sync_file_range02 test-case.<br> <br> Signed-off-by: Sumit Garg <<a href="mailto:sumit.garg@linaro.org" target="_blank">sumit.garg@linaro.org</a>><br> ---<br> <br> Changes in v3:<br> 1. Add upper bound check for synced size to device.<br> 2. Refactor tests for more code reuse.<br> 3. Add another test to check sync over partial write.<br> <br> Changes in v2:<br> 1. Do full file write instead of partial and test sync partial file.<br> <br> .../syscalls/sync_file_range/sync_file_range02.c | 47 +++++++++++++++++-----<br> 1 file changed, 37 insertions(+), 10 deletions(-)<br> <br> diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> index 82d77f7..334ea5e 100644<br> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> @@ -22,23 +22,36 @@<br> #include "check_sync_file_range.h"<br> <br> #define MNTPOINT "mnt_point"<br> -#define FNAME MNTPOINT"/test"<br> -#define FILE_SIZE_MB 32<br> -#define FILE_SIZE (FILE_SIZE_MB * TST_MB)<br> +#define FNAME1 MNTPOINT"/test1"<br> +#define FNAME2 MNTPOINT"/test2"<br> +#define FNAME3 MNTPOINT"/test3"<br> +#define FILE_SZ_MB 32<br> +#define FILE_SZ (FILE_SZ_MB * TST_MB)<br> #define MODE 0644<br> <br> -static void verify_sync_file_range(void)<br> +struct testcase {<br> + char *fname;<br> + off64_t sync_off;<br> + off64_t sync_size;<br> + size_t exp_sync_size;<br> + off64_t write_off;<br> + size_t write_size_mb;<br> +};<br> +<br> +static void verify_sync_file_range(struct testcase *tc)<br> {<br> int fd;<br> unsigned long written;<br> <br> - fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);<br> + fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE);<br> +<br> + lseek(fd, tc->write_off, SEEK_SET);<br> <br> tst_dev_bytes_written(tst_device->dev);<br> <br> - tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);<br> + tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb);<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I'm just thinking that is probably more precise if we reverse the order of tst_fill_fd() and tst_dev_bytes_written()? Because that does counting the dev_bytes_writen only before and after sync_file_range(), we cann't garantee system does not wirte back to deviece when do fill_fd(), isn't it? </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> - TEST(sync_file_range(fd, 0, FILE_SIZE,<br> + TEST(sync_file_range(fd, tc->sync_off, tc->sync_size,<br> SYNC_FILE_RANGE_WAIT_BEFORE |<br> SYNC_FILE_RANGE_WRITE |<br> SYNC_FILE_RANGE_WAIT_AFTER));<br> @@ -50,10 +63,23 @@ static void verify_sync_file_range(void)<br> <br> SAFE_CLOSE(fd);<br> <br> - if (written >= FILE_SIZE)<br> + if ((written >= tc->exp_sync_size) &&<br> + (written <= (tc->exp_sync_size + tc->exp_sync_size/10)))<br> tst_res(TPASS, "Test file range synced to device");<br> else<br> - tst_res(TFAIL, "Synced %li, expected %i", written, FILE_SIZE);<br> + tst_res(TFAIL, "Synced %li, expected %li", written,<br> + tc->exp_sync_size);<br> +}<br> +<br> +static struct testcase testcases[] = {<br> + { FNAME1, 0, FILE_SZ, FILE_SZ, 0, FILE_SZ_MB },<br> + { FNAME2, FILE_SZ/4, FILE_SZ/2, FILE_SZ/2, 0, FILE_SZ_MB },<br> + { FNAME3, FILE_SZ/4, FILE_SZ/2, FILE_SZ/4, FILE_SZ/2, FILE_SZ_MB/4 },<br> +};<br> +<br> +static void run(unsigned int i)<br> +{<br> + verify_sync_file_range(&testcases[i]);<br> }<br> <br> static void setup(void)<br> @@ -63,10 +89,11 @@ static void setup(void)<br> }<br> <br> static struct tst_test test = {<br> + .tcnt = ARRAY_SIZE(testcases),<br> .needs_root = 1,<br> .mount_device = 1,<br> .all_filesystems = 1,<br> .mntpoint = MNTPOINT,<br> .setup = setup,<br> - .test_all = verify_sync_file_range,<br> + .test = run,<br> };<br> -- <br> 2.7.4<br> <br> <br> -- <br> Mailing list info: <a href="https://lists.linux.it/listinfo/ltp" rel="noreferrer" target="_blank">https://lists.linux.it/listinfo/ltp</a><br> </blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div></div>
Hi Li, Firstly apologies for the late reply due to travelling for Linaro Connect BKK19. On Mon, 1 Apr 2019 at 13:54, Li Wang <liwang@redhat.com> wrote: > > Hi Sumit, > > On Thu, Mar 7, 2019 at 8:44 PM Sumit Garg <sumit.garg@linaro.org> wrote: >> >> Add partial file sync tests as part of sync_file_range02 test-case. >> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org> >> --- >> >> Changes in v3: >> 1. Add upper bound check for synced size to device. >> 2. Refactor tests for more code reuse. >> 3. Add another test to check sync over partial write. >> >> Changes in v2: >> 1. Do full file write instead of partial and test sync partial file. >> >> .../syscalls/sync_file_range/sync_file_range02.c | 47 +++++++++++++++++----- >> 1 file changed, 37 insertions(+), 10 deletions(-) >> >> diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> index 82d77f7..334ea5e 100644 >> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> @@ -22,23 +22,36 @@ >> #include "check_sync_file_range.h" >> >> #define MNTPOINT "mnt_point" >> -#define FNAME MNTPOINT"/test" >> -#define FILE_SIZE_MB 32 >> -#define FILE_SIZE (FILE_SIZE_MB * TST_MB) >> +#define FNAME1 MNTPOINT"/test1" >> +#define FNAME2 MNTPOINT"/test2" >> +#define FNAME3 MNTPOINT"/test3" >> +#define FILE_SZ_MB 32 >> +#define FILE_SZ (FILE_SZ_MB * TST_MB) >> #define MODE 0644 >> >> -static void verify_sync_file_range(void) >> +struct testcase { >> + char *fname; >> + off64_t sync_off; >> + off64_t sync_size; >> + size_t exp_sync_size; >> + off64_t write_off; >> + size_t write_size_mb; >> +}; >> + >> +static void verify_sync_file_range(struct testcase *tc) >> { >> int fd; >> unsigned long written; >> >> - fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); >> + fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE); >> + >> + lseek(fd, tc->write_off, SEEK_SET); >> >> tst_dev_bytes_written(tst_device->dev); >> >> - tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB); >> + tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb); > > > I'm just thinking that is probably more precise if we reverse the order of tst_fill_fd() and tst_dev_bytes_written()? Because that does counting the dev_bytes_writen only before and after sync_file_range(), we cann't garantee system does not wirte back to deviece when do fill_fd(), isn't it? There is another aspect to this if we move tst_dev_bytes_written() after tst_fill_fd() then we may miss count for actual data that may be written back during tst_fill_fd() operation. AFAIU, LTP uses test-device for all device specific tests of which it has full control and also the tests run sequentially. So I think there are pretty rare chances to have such scenario that you referred too. -Sumit > >> >> >> - TEST(sync_file_range(fd, 0, FILE_SIZE, >> + TEST(sync_file_range(fd, tc->sync_off, tc->sync_size, >> SYNC_FILE_RANGE_WAIT_BEFORE | >> SYNC_FILE_RANGE_WRITE | >> SYNC_FILE_RANGE_WAIT_AFTER)); >> @@ -50,10 +63,23 @@ static void verify_sync_file_range(void) >> >> SAFE_CLOSE(fd); >> >> - if (written >= FILE_SIZE) >> + if ((written >= tc->exp_sync_size) && >> + (written <= (tc->exp_sync_size + tc->exp_sync_size/10))) >> tst_res(TPASS, "Test file range synced to device"); >> else >> - tst_res(TFAIL, "Synced %li, expected %i", written, FILE_SIZE); >> + tst_res(TFAIL, "Synced %li, expected %li", written, >> + tc->exp_sync_size); >> +} >> + >> +static struct testcase testcases[] = { >> + { FNAME1, 0, FILE_SZ, FILE_SZ, 0, FILE_SZ_MB }, >> + { FNAME2, FILE_SZ/4, FILE_SZ/2, FILE_SZ/2, 0, FILE_SZ_MB }, >> + { FNAME3, FILE_SZ/4, FILE_SZ/2, FILE_SZ/4, FILE_SZ/2, FILE_SZ_MB/4 }, >> +}; >> + >> +static void run(unsigned int i) >> +{ >> + verify_sync_file_range(&testcases[i]); >> } >> >> static void setup(void) >> @@ -63,10 +89,11 @@ static void setup(void) >> } >> >> static struct tst_test test = { >> + .tcnt = ARRAY_SIZE(testcases), >> .needs_root = 1, >> .mount_device = 1, >> .all_filesystems = 1, >> .mntpoint = MNTPOINT, >> .setup = setup, >> - .test_all = verify_sync_file_range, >> + .test = run, >> }; >> -- >> 2.7.4 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp > > > > -- > Regards, > Li Wang
On Wed, Apr 3, 2019 at 7:17 PM Sumit Garg <sumit.garg@linaro.org> wrote: > Hi Li, > > Firstly apologies for the late reply due to travelling for Linaro Connect > BKK19. > > On Mon, 1 Apr 2019 at 13:54, Li Wang <liwang@redhat.com> wrote: > > > > Hi Sumit, > > > > On Thu, Mar 7, 2019 at 8:44 PM Sumit Garg <sumit.garg@linaro.org> wrote: > >> > >> Add partial file sync tests as part of sync_file_range02 test-case. > >> > >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > >> --- > >> > >> Changes in v3: > >> 1. Add upper bound check for synced size to device. > >> 2. Refactor tests for more code reuse. > >> 3. Add another test to check sync over partial write. > >> > >> Changes in v2: > >> 1. Do full file write instead of partial and test sync partial file. > >> > >> .../syscalls/sync_file_range/sync_file_range02.c | 47 > +++++++++++++++++----- > >> 1 file changed, 37 insertions(+), 10 deletions(-) > >> > >> diff --git > a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > >> index 82d77f7..334ea5e 100644 > >> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > >> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > >> @@ -22,23 +22,36 @@ > >> #include "check_sync_file_range.h" > >> > >> #define MNTPOINT "mnt_point" > >> -#define FNAME MNTPOINT"/test" > >> -#define FILE_SIZE_MB 32 > >> -#define FILE_SIZE (FILE_SIZE_MB * TST_MB) > >> +#define FNAME1 MNTPOINT"/test1" > >> +#define FNAME2 MNTPOINT"/test2" > >> +#define FNAME3 MNTPOINT"/test3" > >> +#define FILE_SZ_MB 32 > >> +#define FILE_SZ (FILE_SZ_MB * TST_MB) > >> #define MODE 0644 > >> > >> -static void verify_sync_file_range(void) > >> +struct testcase { > >> + char *fname; > >> + off64_t sync_off; > >> + off64_t sync_size; > >> + size_t exp_sync_size; > >> + off64_t write_off; > >> + size_t write_size_mb; > >> +}; > >> + > >> +static void verify_sync_file_range(struct testcase *tc) > >> { > >> int fd; > >> unsigned long written; > >> > >> - fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); > >> + fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE); > >> + > >> + lseek(fd, tc->write_off, SEEK_SET); > >> > >> tst_dev_bytes_written(tst_device->dev); > >> > >> - tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB); > >> + tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb); > > > > > > I'm just thinking that is probably more precise if we reverse the order > of tst_fill_fd() and tst_dev_bytes_written()? Because that does counting > the dev_bytes_writen only before and after sync_file_range(), we cann't > garantee system does not wirte back to deviece when do fill_fd(), isn't it? > > There is another aspect to this if we move tst_dev_bytes_written() > after tst_fill_fd() then we may miss count for actual data that may be > written back during tst_fill_fd() operation. > > Sounds reasonable, thanks for the clarification. > AFAIU, LTP uses test-device for all device specific tests of which it > has full control and also the tests run sequentially. So I think there > are pretty rare chances to have such scenario that you referred too. > > -Sumit > -- Regards, Li Wang <div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 3, 2019 at 7:17 PM Sumit Garg <<a href="mailto:sumit.garg@linaro.org">sumit.garg@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Li,<br> <br> Firstly apologies for the late reply due to travelling for Linaro Connect BKK19.<br> <br> On Mon, 1 Apr 2019 at 13:54, Li Wang <<a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a>> wrote:<br> ><br> > Hi Sumit,<br> ><br> > On Thu, Mar 7, 2019 at 8:44 PM Sumit Garg <<a href="mailto:sumit.garg@linaro.org" target="_blank">sumit.garg@linaro.org</a>> wrote:<br> >><br> >> Add partial file sync tests as part of sync_file_range02 test-case.<br> >><br> >> Signed-off-by: Sumit Garg <<a href="mailto:sumit.garg@linaro.org" target="_blank">sumit.garg@linaro.org</a>><br> >> ---<br> >><br> >> Changes in v3:<br> >> 1. Add upper bound check for synced size to device.<br> >> 2. Refactor tests for more code reuse.<br> >> 3. Add another test to check sync over partial write.<br> >><br> >> Changes in v2:<br> >> 1. Do full file write instead of partial and test sync partial file.<br> >><br> >> .../syscalls/sync_file_range/sync_file_range02.c | 47 +++++++++++++++++-----<br> >> 1 file changed, 37 insertions(+), 10 deletions(-)<br> >><br> >> diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> >> index 82d77f7..334ea5e 100644<br> >> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> >> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> >> @@ -22,23 +22,36 @@<br> >> #include "check_sync_file_range.h"<br> >><br> >> #define MNTPOINT "mnt_point"<br> >> -#define FNAME MNTPOINT"/test"<br> >> -#define FILE_SIZE_MB 32<br> >> -#define FILE_SIZE (FILE_SIZE_MB * TST_MB)<br> >> +#define FNAME1 MNTPOINT"/test1"<br> >> +#define FNAME2 MNTPOINT"/test2"<br> >> +#define FNAME3 MNTPOINT"/test3"<br> >> +#define FILE_SZ_MB 32<br> >> +#define FILE_SZ (FILE_SZ_MB * TST_MB)<br> >> #define MODE 0644<br> >><br> >> -static void verify_sync_file_range(void)<br> >> +struct testcase {<br> >> + char *fname;<br> >> + off64_t sync_off;<br> >> + off64_t sync_size;<br> >> + size_t exp_sync_size;<br> >> + off64_t write_off;<br> >> + size_t write_size_mb;<br> >> +};<br> >> +<br> >> +static void verify_sync_file_range(struct testcase *tc)<br> >> {<br> >> int fd;<br> >> unsigned long written;<br> >><br> >> - fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);<br> >> + fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE);<br> >> +<br> >> + lseek(fd, tc->write_off, SEEK_SET);<br> >><br> >> tst_dev_bytes_written(tst_device->dev);<br> >><br> >> - tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);<br> >> + tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb);<br> ><br> ><br> > I'm just thinking that is probably more precise if we reverse the order of tst_fill_fd() and tst_dev_bytes_written()? Because that does counting the dev_bytes_writen only before and after sync_file_range(), we cann't garantee system does not wirte back to deviece when do fill_fd(), isn't it?<br> <br> There is another aspect to this if we move tst_dev_bytes_written()<br> after tst_fill_fd() then we may miss count for actual data that may be<br> written back during tst_fill_fd() operation.<br> <br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Sounds reasonable, thanks for the clarification.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> AFAIU, LTP uses test-device for all device specific tests of which it<br> has full control and also the tests run sequentially. So I think there<br> are pretty rare chances to have such scenario that you referred too.<br> <br> -Sumit<br></blockquote><div> </div></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>
On Thu, Mar 28, 2019 at 12:57 PM Sumit Garg <sumit.garg@linaro.org> wrote: > On Wed, 27 Mar 2019 at 20:18, Cyril Hrubis <chrubis@suse.cz> wrote: > > > > Hi! > > Sorry for the long delay. > > > > This is altmost perfect, the only problem is that the third test fails > > on vfat. As far as I can tell the reason is that vfat does not support > > sparse files, hence seeking to the middle of file and writing there also > > schedulles I/O to write zeros from the start of the file to the offset > > we started writing to. > > > > Hmm, I see. > > > Following ugly patch solves the problem: > > > > diff --git > a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > > index 334ea5e88..774524c2f 100644 > > --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > > +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > > @@ -45,6 +45,12 @@ static void verify_sync_file_range(struct testcase > *tc) > > > > fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE); > > > > + if (!strcmp(tst_device->fs_type, "vfat")) { > > + tst_res(TINFO, "Pre-filling file"); > > + tst_fill_fd(fd, 0, tc->write_off, 1); > > + fsync(fd); > > + } > > + > > lseek(fd, tc->write_off, SEEK_SET); > > > > > > So either we limit the tests so that the sync region does not overlap > with the > > possible hole at the start of the file and loose some test coverage. > > > > Or we can add a function to the test library that would return > true/false if > > sparse files are supported for a given FS. > > > > My initial thought behind this test-case was to run sync over a range > which is partially written. The other partial region not being written > could either be a hole or already synced data. So pre-fill file in > case of vfat looks sane option, but how about if we add pre-fill as > part of setup? Something like: > I think this is a bit better. Could u send a new patch version? > --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c > @@ -86,6 +86,12 @@ static void setup(void) > { > if (!check_sync_file_range()) > tst_brk(TCONF, "sync_file_range() not supported"); > + > + if (!strcmp(tst_device->fs_type, "vfat")) { > + tst_res(TINFO, "Pre-filling file"); > + tst_fill_file(FNAME3, 0, TST_MB, FILE_SZ_MB); > + sync(); > + } > } > > -Sumit > > > -- > > Cyril Hrubis > > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > -- Regards, Li Wang <div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 28, 2019 at 12:57 PM Sumit Garg <<a href="mailto:sumit.garg@linaro.org">sumit.garg@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, 27 Mar 2019 at 20:18, Cyril Hrubis <<a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a>> wrote:<br> ><br> > Hi!<br> > Sorry for the long delay.<br> ><br> > This is altmost perfect, the only problem is that the third test fails<br> > on vfat. As far as I can tell the reason is that vfat does not support<br> > sparse files, hence seeking to the middle of file and writing there also<br> > schedulles I/O to write zeros from the start of the file to the offset<br> > we started writing to.<br> ><br> <br> Hmm, I see.<br> <br> > Following ugly patch solves the problem:<br> ><br> > diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> > index 334ea5e88..774524c2f 100644<br> > --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> > +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> > @@ -45,6 +45,12 @@ static void verify_sync_file_range(struct testcase *tc)<br> ><br> > fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE);<br> ><br> > + if (!strcmp(tst_device->fs_type, "vfat")) {<br> > + tst_res(TINFO, "Pre-filling file");<br> > + tst_fill_fd(fd, 0, tc->write_off, 1);<br> > + fsync(fd);<br> > + }<br> > +<br> > lseek(fd, tc->write_off, SEEK_SET);<br> ><br> ><br> > So either we limit the tests so that the sync region does not overlap with the<br> > possible hole at the start of the file and loose some test coverage.<br> ><br> > Or we can add a function to the test library that would return true/false if<br> > sparse files are supported for a given FS.<br> ><br> <br> My initial thought behind this test-case was to run sync over a range<br> which is partially written. The other partial region not being written<br> could either be a hole or already synced data. So pre-fill file in<br> case of vfat looks sane option, but how about if we add pre-fill as<br> part of setup? Something like:<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I think this is a bit better. Could u send a new patch version?</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c<br> @@ -86,6 +86,12 @@ static void setup(void)<br> {<br> if (!check_sync_file_range())<br> tst_brk(TCONF, "sync_file_range() not supported");<br> +<br> + if (!strcmp(tst_device->fs_type, "vfat")) {<br> + tst_res(TINFO, "Pre-filling file");<br> + tst_fill_file(FNAME3, 0, TST_MB, FILE_SZ_MB);<br> + sync();<br> + }<br> }<br> <br> -Sumit<br> <br> > --<br> > Cyril Hrubis<br> > <a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a><br> <br> -- <br> Mailing list info: <a href="https://lists.linux.it/listinfo/ltp" rel="noreferrer" target="_blank">https://lists.linux.it/listinfo/ltp</a><br> </blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>
On Mon, 10 Jun 2019 at 09:03, Li Wang <liwang@redhat.com> wrote: > > > > On Thu, Mar 28, 2019 at 12:57 PM Sumit Garg <sumit.garg@linaro.org> wrote: >> >> On Wed, 27 Mar 2019 at 20:18, Cyril Hrubis <chrubis@suse.cz> wrote: >> > >> > Hi! >> > Sorry for the long delay. >> > >> > This is altmost perfect, the only problem is that the third test fails >> > on vfat. As far as I can tell the reason is that vfat does not support >> > sparse files, hence seeking to the middle of file and writing there also >> > schedulles I/O to write zeros from the start of the file to the offset >> > we started writing to. >> > >> >> Hmm, I see. >> >> > Following ugly patch solves the problem: >> > >> > diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> > index 334ea5e88..774524c2f 100644 >> > --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> > +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> > @@ -45,6 +45,12 @@ static void verify_sync_file_range(struct testcase *tc) >> > >> > fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE); >> > >> > + if (!strcmp(tst_device->fs_type, "vfat")) { >> > + tst_res(TINFO, "Pre-filling file"); >> > + tst_fill_fd(fd, 0, tc->write_off, 1); >> > + fsync(fd); >> > + } >> > + >> > lseek(fd, tc->write_off, SEEK_SET); >> > >> > >> > So either we limit the tests so that the sync region does not overlap with the >> > possible hole at the start of the file and loose some test coverage. >> > >> > Or we can add a function to the test library that would return true/false if >> > sparse files are supported for a given FS. >> > >> >> My initial thought behind this test-case was to run sync over a range >> which is partially written. The other partial region not being written >> could either be a hole or already synced data. So pre-fill file in >> case of vfat looks sane option, but how about if we add pre-fill as >> part of setup? Something like: > > > I think this is a bit better. Could u send a new patch version? > Sure. BTW, apologies for the delay as I was busy with some other tasks. -Sumit >> >> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c >> @@ -86,6 +86,12 @@ static void setup(void) >> { >> if (!check_sync_file_range()) >> tst_brk(TCONF, "sync_file_range() not supported"); >> + >> + if (!strcmp(tst_device->fs_type, "vfat")) { >> + tst_res(TINFO, "Pre-filling file"); >> + tst_fill_file(FNAME3, 0, TST_MB, FILE_SZ_MB); >> + sync(); >> + } >> } >> >> -Sumit >> >> > -- >> > Cyril Hrubis >> > chrubis@suse.cz >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp > > > > -- > Regards, > Li Wang
diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c index 82d77f7..334ea5e 100644 --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c @@ -22,23 +22,36 @@ #include "check_sync_file_range.h" #define MNTPOINT "mnt_point" -#define FNAME MNTPOINT"/test" -#define FILE_SIZE_MB 32 -#define FILE_SIZE (FILE_SIZE_MB * TST_MB) +#define FNAME1 MNTPOINT"/test1" +#define FNAME2 MNTPOINT"/test2" +#define FNAME3 MNTPOINT"/test3" +#define FILE_SZ_MB 32 +#define FILE_SZ (FILE_SZ_MB * TST_MB) #define MODE 0644 -static void verify_sync_file_range(void) +struct testcase { + char *fname; + off64_t sync_off; + off64_t sync_size; + size_t exp_sync_size; + off64_t write_off; + size_t write_size_mb; +}; + +static void verify_sync_file_range(struct testcase *tc) { int fd; unsigned long written; - fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); + fd = SAFE_OPEN(tc->fname, O_RDWR|O_CREAT, MODE); + + lseek(fd, tc->write_off, SEEK_SET); tst_dev_bytes_written(tst_device->dev); - tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB); + tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb); - TEST(sync_file_range(fd, 0, FILE_SIZE, + TEST(sync_file_range(fd, tc->sync_off, tc->sync_size, SYNC_FILE_RANGE_WAIT_BEFORE | SYNC_FILE_RANGE_WRITE | SYNC_FILE_RANGE_WAIT_AFTER)); @@ -50,10 +63,23 @@ static void verify_sync_file_range(void) SAFE_CLOSE(fd); - if (written >= FILE_SIZE) + if ((written >= tc->exp_sync_size) && + (written <= (tc->exp_sync_size + tc->exp_sync_size/10))) tst_res(TPASS, "Test file range synced to device"); else - tst_res(TFAIL, "Synced %li, expected %i", written, FILE_SIZE); + tst_res(TFAIL, "Synced %li, expected %li", written, + tc->exp_sync_size); +} + +static struct testcase testcases[] = { + { FNAME1, 0, FILE_SZ, FILE_SZ, 0, FILE_SZ_MB }, + { FNAME2, FILE_SZ/4, FILE_SZ/2, FILE_SZ/2, 0, FILE_SZ_MB }, + { FNAME3, FILE_SZ/4, FILE_SZ/2, FILE_SZ/4, FILE_SZ/2, FILE_SZ_MB/4 }, +}; + +static void run(unsigned int i) +{ + verify_sync_file_range(&testcases[i]); } static void setup(void) @@ -63,10 +89,11 @@ static void setup(void) } static struct tst_test test = { + .tcnt = ARRAY_SIZE(testcases), .needs_root = 1, .mount_device = 1, .all_filesystems = 1, .mntpoint = MNTPOINT, .setup = setup, - .test_all = verify_sync_file_range, + .test = run, };
Add partial file sync tests as part of sync_file_range02 test-case. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- Changes in v3: 1. Add upper bound check for synced size to device. 2. Refactor tests for more code reuse. 3. Add another test to check sync over partial write. Changes in v2: 1. Do full file write instead of partial and test sync partial file. .../syscalls/sync_file_range/sync_file_range02.c | 47 +++++++++++++++++----- 1 file changed, 37 insertions(+), 10 deletions(-)