[LTP] [PATCH v3 1/7] lib: Add library functions for sync related syscalls

Sumit Garg
2019-02-20T07:57:24Z

On Tue, 19 Feb 2019 at 19:36, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> > include/tst_sync_device.h | 17 ++++++++++
> > lib/tst_sync_device.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 97 insertions(+)
> > create mode 100644 include/tst_sync_device.h
> > create mode 100644 lib/tst_sync_device.c
> >
> > diff --git a/include/tst_sync_device.h b/include/tst_sync_device.h
> > new file mode 100644
> > index 0000000..b07c490
> > --- /dev/null
> > +++ b/include/tst_sync_device.h
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2019 Linaro Limited. All rights reserved.
> > + * Author: Sumit Garg <sumit.garg@linaro.org>
> > + */
> > +
> > +#ifndef TST_SYNC_DEVICE_H__
> > +#define TST_SYNC_DEVICE_H__
> > +
> > +#include <stdbool.h>
> > +
> > +void tst_sync_device_init(const char *dev);
> > +int tst_sync_device_write(const char *file, unsigned int size_mb);
> > +bool tst_sync_device_check(unsigned int size_mb);
> > +void tst_sync_device_cleanup(void);
> > +
> > +#endif
> > diff --git a/lib/tst_sync_device.c b/lib/tst_sync_device.c
> > new file mode 100644
> > index 0000000..5a0a17c
> > --- /dev/null
> > +++ b/lib/tst_sync_device.c
> > @@ -0,0 +1,80 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2019 Linaro Limited. All rights reserved.
> > + * Author: Sumit Garg <sumit.garg@linaro.org>
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <sys/types.h>
> > +
> > +#define TST_NO_DEFAULT_MAIN
> > +#include "tst_test.h"
> > +#include "lapi/stat.h"
> > +#include "tst_sync_device.h"
> > +
> > +#define SIZE_MB (1024*1024)
> > +#define MODE 0644
> > +
> > +static char dev_stat_path[1024];
> > +static char *buffer;
> > +static int fd;
> > +static unsigned long prev_write_sec;
> > +
> > +void tst_sync_device_init(const char *dev)
> > +{
> > + struct stat st;
> > +
> > + snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat",
> > + strrchr(dev, '/') + 1);
> > +
> > + if (stat(dev_stat_path, &st) != 0)
> > + tst_brk(TCONF, "Test device stat file: %s not found",
> > + dev_stat_path);
> > +
> > + buffer = SAFE_MALLOC(SIZE_MB);
> > +
> > + memset(buffer, 0, SIZE_MB);
> > +}
> > +
> > +int tst_sync_device_write(const char *file, unsigned int size_mb)
> > +{
> > + unsigned int counter;
> ^
> It's kind of strange to name this variable anything
> else but i.
>

Will use i instead.

> > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %lu",
> > + &prev_write_sec);
> > +
> > + fd = SAFE_OPEN(file, O_RDWR|O_CREAT, MODE);
> ^
> Extra space.
>

Will fix.

> > + /* Filling the test file */
> ^
> Useless comment, it's pretty clear what we do here.
>

Will remove this comment.

> > + for (counter = 0; counter < size_mb; counter++)
> > + SAFE_WRITE(1, fd, buffer, SIZE_MB);
> > +
> > + return fd;
> > +}
> > +
> > +bool tst_sync_device_check(unsigned int size_mb)
> > +{
> > + unsigned long write_sec = 0;
> > + bool res = false;
> > +
> > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %lu",
> > + &write_sec);
> > +
> > + if ((write_sec - prev_write_sec) * 512 >=
> > + (size_mb * SIZE_MB))
> > + res = true;
> > +
> > + SAFE_CLOSE(fd);
> > +
> > + return res;
> > +}
>
> I do not like that this API is tailored just to this specific usecase.
>
> I would rather see the code that writes the file separated from the code
> that measures the bytes written.
>

Agree. Actually my initial idea was to tailor it for maximal code re-use.

> Maybe we just need a tst_dev_bytes_written() function that would return
> the bytes written since the last call of the function so that we could
> do:
>
> fd = SAFE_OPEN(...);
>
> tst_dev_blocks_written(tst_device.dev);
>
> tst_fill_fd(fd, 0, TST_MB, size_mb);
>
> TEST(fdsync(fd));
>
> if (TST_RET)
> tst_brk(TFAIL | TTERRNO, "syncfd() failed with %i", TST_RET);
>
> written = tst_dev_blocks_written(tst_device.dev);
>
> SAFE_CLOSE(fd);
>
> if (written >= size_mb * TST_DEV_BLOCKS_IN_MB)
> tst_res(TPASS, ...);
> else
> tst_res(TFAIL, ...);
>

Seems to be nicer approach. So rather than new file should I add
tst_dev_bytes_written() function in "lib/tst_device.c" file?

>
> The test a bit longer, but the library functions are more reusable, if
> you do 'git grep -B 1 SAFE_WRITE' you can see that the tst_fill_fd
> function that loops over SAFE_WRITE could be used in a few places
> already.
>

I am not able to locate tst_fill_fd function. Are you referring to
tst_fill_file function? If yes, then we could split that function to
create tst_fill_fd function.

-Sumit

> > +void tst_sync_device_cleanup(void)
> > +{
> > + if (buffer)
> > + free(buffer);
> > +
> > + if (fd > 0)
> > + SAFE_CLOSE(fd);
> > +}
> > --
> > 2.7.4
> >
>
> --
> Cyril Hrubis
> chrubis@suse.cz