[LTP] [PATCH 1/2] lib: limit the size of tmpfs in LTP

[LTP] [PATCH 1/2] lib: limit the size of tmpfs in LTP

[Li Wang]

LTP mount and make use of the whole tmpfs of the test system,
generally, it's fine. But if a test (e.g fallocate06) try to
fill full in the filesystem, it takes too long to complete
testing on a large memory system.

This patch adds a new function limit_tmpfs_mount_size with
appending '-o size=xxM' to the mount options in prepare_device()
which helps limit the tmpfs mount size.

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Li Wang <liwang@redhat.com>
---
 include/tst_device.h |  7 +++++++
 lib/tst_device.c     | 15 +++++++++++++++
 lib/tst_test.c       | 10 ++++++++++
 3 files changed, 32 insertions(+)

diff --git a/include/tst_device.h b/include/tst_device.h
index 1d1246e82..51bde4190 100644
--- a/include/tst_device.h
+++ b/include/tst_device.h
@@ -31,6 +31,13 @@ int tst_umount(const char *path);
 int tst_is_mounted(const char *path);
 int tst_is_mounted_at_tmpdir(const char *path);
 
+/*
+ * Limit the tmpfs mount size for LTP test
+ * @mnt_data: mount options from tst_test->mnt_data
+ * @size: tmpfs size to be mounted
+ */
+char *limit_tmpfs_mount_size(const char *mnt_data, unsigned int size);
+
 /*
  * Clears a first few blocks of the device. This is needed when device has
  * already been formatted with a filesystems, subset of mkfs.foo utils aborts
diff --git a/lib/tst_device.c b/lib/tst_device.c
index c096b418b..66a830b7b 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -45,6 +45,7 @@
 #define DEV_FILE "test_dev.img"
 #define DEV_SIZE_MB 256u
 
+static char tmpfs_buf[1024];
 static char dev_path[1024];
 static int device_acquired;
 static unsigned long prev_dev_sec_write;
@@ -368,6 +369,20 @@ int tst_clear_device(const char *dev)
 	return 0;
 }
 
+char *limit_tmpfs_mount_size(const char *mnt_data, unsigned int size)
+{
+	unsigned int dev_size = MAX(size, DEV_SIZE_MB);
+
+	if (mnt_data)
+		snprintf(tmpfs_buf, sizeof(tmpfs_buf), "%s,size=%uM", mnt_data, dev_size);
+	else
+		snprintf(tmpfs_buf, sizeof(tmpfs_buf), "size=%uM", dev_size);
+
+	tst_resm(TINFO, "Limiting tmpfs size to %uMB", dev_size);
+
+	return tmpfs_buf;
+}
+
 int tst_umount(const char *path)
 {
 	int err, ret, i;
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 55449c80b..27766fbfd 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -896,9 +896,19 @@ static void prepare_device(void)
 	}
 
 	if (tst_test->mount_device) {
+		char *mnt_data = tst_test->mnt_data;
+
+		if (!strcmp(tdev.fs_type, "tmpfs")) {
+			tst_test->mnt_data = limit_tmpfs_mount_size(tst_test->mnt_data,
+					tst_test->dev_min_size);
+		}
+
 		SAFE_MOUNT(tdev.dev, tst_test->mntpoint, tdev.fs_type,
 			   tst_test->mnt_flags, tst_test->mnt_data);
 		mntpoint_mounted = 1;
+
+		if (!strcmp(tdev.fs_type, "tmpfs"))
+			tst_test->mnt_data = mnt_data;
 	}
 }
 
-- 
2.31.1

[LTP] [PATCH 2/2] lib: mount tmpfs name as ltp-tmpfs

[Li Wang]

Given a specific name as "ltp-tmpfs" instead of the "/dev/loopX"
string in order to make "tmpfs" filesystem more evident.

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Li Wang <liwang@redhat.com>
---
 lib/tst_test.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 27766fbfd..ab343e593 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -896,19 +896,23 @@ static void prepare_device(void)
 	}
 
 	if (tst_test->mount_device) {
+		const char *path_name = tdev.dev;
 		char *mnt_data = tst_test->mnt_data;
 
 		if (!strcmp(tdev.fs_type, "tmpfs")) {
 			tst_test->mnt_data = limit_tmpfs_mount_size(tst_test->mnt_data,
 					tst_test->dev_min_size);
+			tdev.dev = "ltp-tmpfs";
 		}
 
 		SAFE_MOUNT(tdev.dev, tst_test->mntpoint, tdev.fs_type,
 			   tst_test->mnt_flags, tst_test->mnt_data);
 		mntpoint_mounted = 1;
 
-		if (!strcmp(tdev.fs_type, "tmpfs"))
+		if (!strcmp(tdev.fs_type, "tmpfs")) {
 			tst_test->mnt_data = mnt_data;
+			tdev.dev = path_name;
+		}
 	}
 }
 
-- 
2.31.1

[LTP] [PATCH 1/2] lib: limit the size of tmpfs in LTP

[Cyril Hrubis]

Hi!
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  include/tst_device.h |  7 +++++++
>  lib/tst_device.c     | 15 +++++++++++++++
>  lib/tst_test.c       | 10 ++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/include/tst_device.h b/include/tst_device.h
> index 1d1246e82..51bde4190 100644
> --- a/include/tst_device.h
> +++ b/include/tst_device.h
> @@ -31,6 +31,13 @@ int tst_umount(const char *path);
>  int tst_is_mounted(const char *path);
>  int tst_is_mounted_at_tmpdir(const char *path);
>  
> +/*
> + * Limit the tmpfs mount size for LTP test
> + * @mnt_data: mount options from tst_test->mnt_data
> + * @size: tmpfs size to be mounted
> + */
> +char *limit_tmpfs_mount_size(const char *mnt_data, unsigned int size);

If we want this function to be public it has to be prefixed with 'tst_'.

Also do we really need this to be public?

>  /*
>   * Clears a first few blocks of the device. This is needed when device has
>   * already been formatted with a filesystems, subset of mkfs.foo utils aborts
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index c096b418b..66a830b7b 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -45,6 +45,7 @@
>  #define DEV_FILE "test_dev.img"
>  #define DEV_SIZE_MB 256u
>  
> +static char tmpfs_buf[1024];

Can we please, instead of adding a global variable, pass the buffer and
it's size to the limit_tmpfs_mount size, and then create the path on the
stack in the prepare device function?

>  static char dev_path[1024];
>  static int device_acquired;
>  static unsigned long prev_dev_sec_write;
> @@ -368,6 +369,20 @@ int tst_clear_device(const char *dev)
>  	return 0;
>  }
>  
> +char *limit_tmpfs_mount_size(const char *mnt_data, unsigned int size)
> +{
> +	unsigned int dev_size = MAX(size, DEV_SIZE_MB);
> +
> +	if (mnt_data)
> +		snprintf(tmpfs_buf, sizeof(tmpfs_buf), "%s,size=%uM", mnt_data, dev_size);
> +	else
> +		snprintf(tmpfs_buf, sizeof(tmpfs_buf), "size=%uM", dev_size);
> +
> +	tst_resm(TINFO, "Limiting tmpfs size to %uMB", dev_size);
> +
> +	return tmpfs_buf;
> +}

If we passed the filesystem type to this function here as well we could
do:

	if (!strcmp(fs_type, "tmpfs"))
		return mnt_data;

As a first thing in this function and then we could pass the return
value from this function to the SAFE_MOUNT() unconditionally.

>  int tst_umount(const char *path)
>  {
>  	int err, ret, i;
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 55449c80b..27766fbfd 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -896,9 +896,19 @@ static void prepare_device(void)
>  	}
>  
>  	if (tst_test->mount_device) {
> +		char *mnt_data = tst_test->mnt_data;
> +
> +		if (!strcmp(tdev.fs_type, "tmpfs")) {
> +			tst_test->mnt_data = limit_tmpfs_mount_size(tst_test->mnt_data,
> +					tst_test->dev_min_size);
> +		}
> +
>  		SAFE_MOUNT(tdev.dev, tst_test->mntpoint, tdev.fs_type,
>  			   tst_test->mnt_flags, tst_test->mnt_data);
>  		mntpoint_mounted = 1;
> +
> +		if (!strcmp(tdev.fs_type, "tmpfs"))
> +			tst_test->mnt_data = mnt_data;

I guess that we are doing this in order to export the changes in the
mnt_data to the test, right?

Is that needed for something or are you doing this just in a case that
somebody will use that?

>  	}
>  }
>  
> -- 
> 2.31.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] lib: mount tmpfs name as ltp-tmpfs

[Cyril Hrubis]

Hi!
> Given a specific name as "ltp-tmpfs" instead of the "/dev/loopX"
> string in order to make "tmpfs" filesystem more evident.
> 
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  lib/tst_test.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 27766fbfd..ab343e593 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -896,19 +896,23 @@ static void prepare_device(void)
>  	}
>  
>  	if (tst_test->mount_device) {
> +		const char *path_name = tdev.dev;
>  		char *mnt_data = tst_test->mnt_data;
>  
>  		if (!strcmp(tdev.fs_type, "tmpfs")) {
>  			tst_test->mnt_data = limit_tmpfs_mount_size(tst_test->mnt_data,
>  					tst_test->dev_min_size);
> +			tdev.dev = "ltp-tmpfs";
>  		}
>  
>  		SAFE_MOUNT(tdev.dev, tst_test->mntpoint, tdev.fs_type,
>  			   tst_test->mnt_flags, tst_test->mnt_data);
>  		mntpoint_mounted = 1;
>  
> -		if (!strcmp(tdev.fs_type, "tmpfs"))
> +		if (!strcmp(tdev.fs_type, "tmpfs")) {
>  			tst_test->mnt_data = mnt_data;
> +			tdev.dev = path_name;

Here as well, do we need to change the tdev.dev?

Can't we just create a function get_device_name(const char *fs_type)
that would return either tdev.dev pointer or pointer to static
"ltp-tmpfs" string based on the fs_type argument?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] lib: limit the size of tmpfs in LTP

[Cyril Hrubis]

Hi!
> > > +
> > > +             if (!strcmp(tdev.fs_type, "tmpfs"))
> > > +                     tst_test->mnt_data = mnt_data;
> >
> > I guess that we are doing this in order to export the changes in the
> > mnt_data to the test, right?
> >
> > Is that needed for something or are you doing this just in a case that
> > somebody will use that?
> >
> 
> No, you probably mis-read this part.
> 
> In contrast, this is just to restore it to the original value,
> because we don't want to export the changed tst_test->mnt_data
> take effect on other filesystems.

I'm just asking why we are setting it in the first place?

If we do not change it there is no need to restore the value, so the
real question is, do we need to change the tst_test->mnt_data at all?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] lib: limit the size of tmpfs in LTP

[Li Wang]

Cyril Hrubis <chrubis@suse.cz> wrote:



> > +char *limit_tmpfs_mount_size(const char *mnt_data, unsigned int size);
>
> If we want this function to be public it has to be prefixed with 'tst_'.
>
> Also do we really need this to be public?
>

No, we don't. I put it in the tst_device.c only hope to make use of the
DEV_SIZE_MB definition, and now seems this is a bad idea.

Looks like we should move it back to tst_test.c and state it as a static
function.



>
> > +static char tmpfs_buf[1024];
>
> Can we please, instead of adding a global variable, pass the buffer and
> it's size to the limit_tmpfs_mount size, and then create the path on the
> stack in the prepare device function?
>
>
Sure.


>
> > +char *limit_tmpfs_mount_size(const char *mnt_data, unsigned int size)
> > +{
> > +     unsigned int dev_size = MAX(size, DEV_SIZE_MB);
> > +
> > +     if (mnt_data)
> > +             snprintf(tmpfs_buf, sizeof(tmpfs_buf), "%s,size=%uM",
> mnt_data, dev_size);
> > +     else
> > +             snprintf(tmpfs_buf, sizeof(tmpfs_buf), "size=%uM",
> dev_size);
> > +
> > +     tst_resm(TINFO, "Limiting tmpfs size to %uMB", dev_size);
> > +
> > +     return tmpfs_buf;
> > +}
>
> If we passed the filesystem type to this function here as well we could
> do:
>
>         if (!strcmp(fs_type, "tmpfs"))
>                 return mnt_data;
>
> As a first thing in this function and then we could pass the return
> value from this function to the SAFE_MOUNT() unconditionally.
>

+1 This sounds good.


> > +
> > +             if (!strcmp(tdev.fs_type, "tmpfs"))
> > +                     tst_test->mnt_data = mnt_data;
>
> I guess that we are doing this in order to export the changes in the
> mnt_data to the test, right?
>
> Is that needed for something or are you doing this just in a case that
> somebody will use that?
>

No, you probably mis-read this part.

In contrast, this is just to restore it to the original value,
because we don't want to export the changed tst_test->mnt_data
take effect on other filesystems.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210702/b0f9c08e/attachment.htm>

[LTP] [PATCH 1/2] lib: limit the size of tmpfs in LTP

[Li Wang]

On Fri, Jul 2, 2021 at 8:06 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > > +
> > > > +             if (!strcmp(tdev.fs_type, "tmpfs"))
> > > > +                     tst_test->mnt_data = mnt_data;
> > >
> > > I guess that we are doing this in order to export the changes in the
> > > mnt_data to the test, right?
> > >
> > > Is that needed for something or are you doing this just in a case that
> > > somebody will use that?
> > >
> >
> > No, you probably mis-read this part.
> >
> > In contrast, this is just to restore it to the original value,
> > because we don't want to export the changed tst_test->mnt_data
> > take effect on other filesystems.
>
> I'm just asking why we are setting it in the first place?
>
> If we do not change it there is no need to restore the value, so the
> real question is, do we need to change the tst_test->mnt_data at all?
>

Alright, we can just return a new pointer buf, and do nothing
for the tst_test->mnt_data itself.

Seems the misread person is me (I need more coffee now:).

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210702/32ade573/attachment.htm>