[LTP] [PATCH v2 1/2] syscalls/sync_file_range: add partial file sync test-case

[LTP] [PATCH v2 1/2] syscalls/sync_file_range: add partial file sync test-case

[Sumit Garg]

Add partial file sync test as part of sync_file_range02 test-case.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---

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, 45 insertions(+), 2 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..2d2ed2f 100644
--- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
+++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
@@ -27,7 +27,7 @@
 #define FILE_SIZE (FILE_SIZE_MB * TST_MB)
 #define MODE			0644
 
-static void verify_sync_file_range(void)
+static void verify_sync_full_file(void)
 {
 	int fd;
 	unsigned long written;
@@ -56,6 +56,48 @@ static void verify_sync_file_range(void)
 		tst_res(TFAIL, "Synced %li, expected %i", written, FILE_SIZE);
 }
 
+static void verify_sync_partial_file(void)
+{
+	int fd;
+	unsigned long written;
+
+	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
+
+	tst_dev_bytes_written(tst_device->dev);
+
+	tst_fill_fd(fd, 0xff, TST_MB, FILE_SIZE_MB);
+
+	TEST(sync_file_range(fd, FILE_SIZE/4, FILE_SIZE/2,
+			     SYNC_FILE_RANGE_WAIT_BEFORE |
+			     SYNC_FILE_RANGE_WRITE |
+			     SYNC_FILE_RANGE_WAIT_AFTER));
+
+	if (TST_RET)
+		tst_brk(TFAIL | TTERRNO, "sync_file_range() failed");
+
+	written = tst_dev_bytes_written(tst_device->dev);
+
+	SAFE_CLOSE(fd);
+
+	if (written >= FILE_SIZE/2)
+		tst_res(TPASS, "Test file range synced to device");
+	else
+		tst_res(TFAIL, "Synced %li, expected %i", written,
+			FILE_SIZE/2);
+}
+
+static struct tcase {
+	void (*tfunc)(void);
+} tcases[] = {
+	{&verify_sync_full_file},
+	{&verify_sync_partial_file}
+};
+
+static void run(unsigned int i)
+{
+	tcases[i].tfunc();
+}
+
 static void setup(void)
 {
 	if (!check_sync_file_range())
@@ -63,10 +105,11 @@ static void setup(void)
 }
 
 static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
 	.needs_root = 1,
 	.mount_device = 1,
 	.all_filesystems = 1,
 	.mntpoint = MNTPOINT,
 	.setup = setup,
-	.test_all = verify_sync_file_range,
+	.test = run,
 };
-- 
2.7.4

[LTP] [PATCH v2 1/2] syscalls/sync_file_range: add partial file sync test-case

[Cyril Hrubis]

Hi!
> +static void verify_sync_partial_file(void)
> +{
> +	int fd;
> +	unsigned long written;
> +
> +	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
> +
> +	tst_dev_bytes_written(tst_device->dev);
> +
> +	tst_fill_fd(fd, 0xff, TST_MB, FILE_SIZE_MB);
> +
> +	TEST(sync_file_range(fd, FILE_SIZE/4, FILE_SIZE/2,
> +			     SYNC_FILE_RANGE_WAIT_BEFORE |
> +			     SYNC_FILE_RANGE_WRITE |
> +			     SYNC_FILE_RANGE_WAIT_AFTER));
> +
> +	if (TST_RET)
> +		tst_brk(TFAIL | TTERRNO, "sync_file_range() failed");
> +
> +	written = tst_dev_bytes_written(tst_device->dev);
> +
> +	SAFE_CLOSE(fd);
> +
> +	if (written >= FILE_SIZE/2)
> +		tst_res(TPASS, "Test file range synced to device");
> +	else
> +		tst_res(TFAIL, "Synced %li, expected %i", written,
> +			FILE_SIZE/2);
> +}


Looking at this the function is nearly the same as the other one, I
guess that we may as well define the function as:

static void verify_sync_file_range(off64_t off, off64_t size, char byte)
{
	...
}

Also I'm not sure I was clear enough, but I was suggesting to check for
upper bound for the synced size as well, which is why I suggested to do
full write, sync only part of it, then check that the size was within
bounds, i.e. >= size and  <= size + epsilon.

I guess that we can even extend this to call the sync over a range that
has been only partially written, but for that we would have to be
careful and make sure all the data has been either synced@the end of
the test function or use a different file for each test.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] syscalls/sync_file_range: add partial file sync test-case

[Sumit Garg]

On Tue, 5 Mar 2019 at 19:49, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > +static void verify_sync_partial_file(void)
> > +{
> > +     int fd;
> > +     unsigned long written;
> > +
> > +     fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
> > +
> > +     tst_dev_bytes_written(tst_device->dev);
> > +
> > +     tst_fill_fd(fd, 0xff, TST_MB, FILE_SIZE_MB);
> > +
> > +     TEST(sync_file_range(fd, FILE_SIZE/4, FILE_SIZE/2,
> > +                          SYNC_FILE_RANGE_WAIT_BEFORE |
> > +                          SYNC_FILE_RANGE_WRITE |
> > +                          SYNC_FILE_RANGE_WAIT_AFTER));
> > +
> > +     if (TST_RET)
> > +             tst_brk(TFAIL | TTERRNO, "sync_file_range() failed");
> > +
> > +     written = tst_dev_bytes_written(tst_device->dev);
> > +
> > +     SAFE_CLOSE(fd);
> > +
> > +     if (written >= FILE_SIZE/2)
> > +             tst_res(TPASS, "Test file range synced to device");
> > +     else
> > +             tst_res(TFAIL, "Synced %li, expected %i", written,
> > +                     FILE_SIZE/2);
> > +}
>
>
> Looking at this the function is nearly the same as the other one, I
> guess that we may as well define the function as:
>
> static void verify_sync_file_range(off64_t off, off64_t size, char byte)
> {
>         ...
> }
>

Yeah it could be made common. But we may not be able to reuse it for
third case you mentioned below.

> Also I'm not sure I was clear enough, but I was suggesting to check for
> upper bound for the synced size as well, which is why I suggested to do
> full write, sync only part of it, then check that the size was within
> bounds, i.e. >= size and  <= size + epsilon.
>

Do you see any value add of upper bound check? AFAIK, device writes
continue in back-end and we might not be sure about appropriate value
for "epsilon".

> I guess that we can even extend this to call the sync over a range that
> has been only partially written, but for that we would have to be
> careful and make sure all the data has been either synced at the end of
> the test function or use a different file for each test.
>

I think using different file for each case looks more appropriate.

-Sumit

> --
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH v2 1/2] syscalls/sync_file_range: add partial file sync test-case

[Cyril Hrubis]

Hi!
> > Looking at this the function is nearly the same as the other one, I
> > guess that we may as well define the function as:
> >
> > static void verify_sync_file_range(off64_t off, off64_t size, char byte)
> > {
> >         ...
> > }
> >
> 
> Yeah it could be made common. But we may not be able to reuse it for
> third case you mentioned below.

We may as well add offset and size for the write as well.

> > Also I'm not sure I was clear enough, but I was suggesting to check for
> > upper bound for the synced size as well, which is why I suggested to do
> > full write, sync only part of it, then check that the size was within
> > bounds, i.e. >= size and  <= size + epsilon.
> >
> 
> Do you see any value add of upper bound check? AFAIK, device writes
> continue in back-end and we might not be sure about appropriate value
> for "epsilon".

AFAIK this makes sense as far the file written fits into RAM buffers.
We do write followed by a sync that asks particular range to be written
to the device, so unless we manage to write significant portion of the
file while we are calling the write syscall in a loop we will get quite
close to the expectations. I guess that we can also check how much data
was written right after we finished writing to the file and if that
number is neglectible it would be fairly safe to check for upper bound.
Does that sound reasonable to you?

> > I guess that we can even extend this to call the sync over a range that
> > has been only partially written, but for that we would have to be
> > careful and make sure all the data has been either synced at the end of
> > the test function or use a different file for each test.
> >
> 
> I think using different file for each case looks more appropriate.

Agreed.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] syscalls/sync_file_range: add partial file sync test-case

[Sumit Garg]

On Wed, 6 Mar 2019 at 14:24, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > > Looking at this the function is nearly the same as the other one, I
> > > guess that we may as well define the function as:
> > >
> > > static void verify_sync_file_range(off64_t off, off64_t size, char byte)
> > > {
> > >         ...
> > > }
> > >
> >
> > Yeah it could be made common. But we may not be able to reuse it for
> > third case you mentioned below.
>
> We may as well add offset and size for the write as well.
>

Ok.

> > > Also I'm not sure I was clear enough, but I was suggesting to check for
> > > upper bound for the synced size as well, which is why I suggested to do
> > > full write, sync only part of it, then check that the size was within
> > > bounds, i.e. >= size and  <= size + epsilon.
> > >
> >
> > Do you see any value add of upper bound check? AFAIK, device writes
> > continue in back-end and we might not be sure about appropriate value
> > for "epsilon".
>
> AFAIK this makes sense as far the file written fits into RAM buffers.
> We do write followed by a sync that asks particular range to be written
> to the device, so unless we manage to write significant portion of the
> file while we are calling the write syscall in a loop we will get quite
> close to the expectations. I guess that we can also check how much data
> was written right after we finished writing to the file and if that
> number is neglectible it would be fairly safe to check for upper bound.
> Does that sound reasonable to you?
>

Yes, I see number is negligible after write to file. So will add this
upper bound check with epsilon = 10%.

-Sumit

> > > I guess that we can even extend this to call the sync over a range that
> > > has been only partially written, but for that we would have to be
> > > careful and make sure all the data has been either synced at the end of
> > > the test function or use a different file for each test.
> > >
> >
> > I think using different file for each case looks more appropriate.
>
> Agreed.
>
> --
> Cyril Hrubis
> chrubis@suse.cz