[LTP] [PATCH] mmap16: fix ETIMEDOUT error if test device too large

[LTP] [PATCH] mmap16: fix ETIMEDOUT error if test device too large

[Zorro Lang]

mmap16 will wait DEFAULT_MSEC_TIMEOUT=10000 msec, for parent
process full the test device(-b $DEVICE). But if the device
size is too large, the case will hit ETIMEDOUT error.

For reproduce the bug of mmap16, it don't need too large device.
So I limit the fs size in 100M, for save time.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

Hi maintainers,

This's a demo patch for a problem I meet recently. I always hit mmap16
failed as:

mmap16      0  TINFO  :  Using test device LTP_DEV='/dev/loop0'
mmap16      0  TINFO  :  Formatting /dev/loop0 with ext4 extra opts='-b 1024'
mke2fs 1.41.12 (17-May-2010)
mmap16      1  TBROK  :  tst_checkpoint.c:130: mmap16.c:222: tst_checkpoint_wait(0, 10000): errno=ETIMEDOUT(110): Connection timed out
mmap16      2  TBROK  :  tst_checkpoint.c:130: Remaining cases broken
mmap16      1  TBROK  :  tst_checkpoint.c:143: mmap16.c:124: tst_checkpoint_wake(0, 1, 10000): errno=ETIMEDOUT(110): Connection timed out
mmap16      2  TBROK  :  tst_checkpoint.c:143: Remaining cases broken

I think that's due to my test device LTP_DEV='/dev/loop0'. I use a
1G loop device, sometimes mmap16 will failed as above. If I use a
bigger device, it will failed 100%.

I know that's my test device problem. I should choose a suitable device
for mmap16. But on the other side, I think if mmap16 don't need a large
fs, it can limit the fs size when mkfs.

I thought a lot of idea to make "tst_mkfs()" can support a limited fs
size. But it's difficult for ext4, because ext4's "[fs-size]" parameter
must behind "device". So I can't add a "fs-size" into fs_opts[] simply.

For change the tst_mkfs() function, I thought 3 different ways:
1. Add a new parameter "const char *fssize".
2. Add a new parameter "const *char fs_opts2[]", to store options which
need to behind the dev.
3. As below, use an useless character "|" to separate options need to
behind the dev.

Maybe there're some better ways to fix this problem. And maybe there're
some others cases have the same problem? I try to report this problem, and
submit a V1 patch. I have tested this patch, mmap16 can run normally.

Please help to review. I'm glad to hear your suggestions about how to
deal with this better:)

Thanks,
Zorro

 lib/tst_mkfs.c                          | 11 ++++++++---
 testcases/kernel/syscalls/mmap/mmap16.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/tst_mkfs.c b/lib/tst_mkfs.c
index 5f959a4..2948e25 100644
--- a/lib/tst_mkfs.c
+++ b/lib/tst_mkfs.c
@@ -27,6 +27,7 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
 	int i, pos = 3;
 	const char *argv[OPTS_MAX] = {"mkfs", "-t", fs_type};
 	char fs_opts_str[1024] = "";
+	int tailed = 0;
 
 	if (!fs_type)
 		tst_brkm(TBROK, cleanup_fn, "No fs_type specified");
@@ -54,7 +55,11 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
 
 	if (fs_opts) {
 		for (i = 0; fs_opts[i]; i++) {
-			argv[pos++] = fs_opts[i];
+			if (!strcmp(fs_opts[i], "|")) {
+				argv[pos++] = dev;
+				tailed = 1;
+			} else
+				argv[pos++] = fs_opts[i];
 
 			if (pos + 2 > OPTS_MAX) {
 				tst_brkm(TBROK, cleanup_fn,
@@ -66,8 +71,8 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
 			strcat(fs_opts_str, fs_opts[i]);
 		}
 	}
-
-	argv[pos++] = dev;
+	if (tailed == 0)
+		argv[pos++] = dev;
 	argv[pos] = NULL;
 
 	tst_resm(TINFO, "Formatting %s with %s extra opts='%s'",
diff --git a/testcases/kernel/syscalls/mmap/mmap16.c b/testcases/kernel/syscalls/mmap/mmap16.c
index c5828ea..66db447 100644
--- a/testcases/kernel/syscalls/mmap/mmap16.c
+++ b/testcases/kernel/syscalls/mmap/mmap16.c
@@ -143,7 +143,7 @@ static void do_test(void)
 
 static void setup(void)
 {
-	const char *fs_opts[3] = {"-b", "1024", NULL};
+	const char *fs_opts[5] = {"-b", "1024", "|", "102400", NULL};
 
 	tst_sig(FORK, DEF_HANDLER, NULL);
 	tst_require_root();
-- 
2.5.0

[LTP] [PATCH] mmap16: fix ETIMEDOUT error if test device too large

[Cyril Hrubis]

Hi!
>  lib/tst_mkfs.c                          | 11 ++++++++---
>  testcases/kernel/syscalls/mmap/mmap16.c |  2 +-
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/tst_mkfs.c b/lib/tst_mkfs.c
> index 5f959a4..2948e25 100644
> --- a/lib/tst_mkfs.c
> +++ b/lib/tst_mkfs.c
> @@ -27,6 +27,7 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
>  	int i, pos = 3;
>  	const char *argv[OPTS_MAX] = {"mkfs", "-t", fs_type};
>  	char fs_opts_str[1024] = "";
> +	int tailed = 0;
>  
>  	if (!fs_type)
>  		tst_brkm(TBROK, cleanup_fn, "No fs_type specified");
> @@ -54,7 +55,11 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
>  
>  	if (fs_opts) {
>  		for (i = 0; fs_opts[i]; i++) {
> -			argv[pos++] = fs_opts[i];
> +			if (!strcmp(fs_opts[i], "|")) {
> +				argv[pos++] = dev;
> +				tailed = 1;
> +			} else
> +				argv[pos++] = fs_opts[i];
>  
>  			if (pos + 2 > OPTS_MAX) {
>  				tst_brkm(TBROK, cleanup_fn,
> @@ -66,8 +71,8 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
>  			strcat(fs_opts_str, fs_opts[i]);
>  		}
>  	}
> -
> -	argv[pos++] = dev;
> +	if (tailed == 0)
> +		argv[pos++] = dev;
>  	argv[pos] = NULL;
>  
>  	tst_resm(TINFO, "Formatting %s with %s extra opts='%s'",

This is ugly. If you want to pass the filesystem size you should add an
paramter to the mkfs function instead of inventing special syntax for
what has been simple list of parameters. We may add a second tst_mkfs2()
function with extra parameter for the fs size.

Also you should make sure that the device is large enough for the
requested size. For this use we may call the paramter max_size and
simply ignore it if the device is smaller than that.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] mmap16: fix ETIMEDOUT error if test device too large

[Zirong Lang]

Hi Cyril,

----- 原始邮件 -----
> 发件人: "Cyril Hrubis" <chrubis@suse.cz>
> 收件人: "Zorro Lang" <zlang@redhat.com>
> 抄送: ltp@lists.linux.it
> 发送时间: 星期二, 2016年 3 月 08日 上午 12:13:07
> 主题: Re: [LTP] [PATCH] mmap16: fix ETIMEDOUT error if test device too large
> 
> Hi!
> >  lib/tst_mkfs.c                          | 11 ++++++++---
> >  testcases/kernel/syscalls/mmap/mmap16.c |  2 +-
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/tst_mkfs.c b/lib/tst_mkfs.c
> > index 5f959a4..2948e25 100644
> > --- a/lib/tst_mkfs.c
> > +++ b/lib/tst_mkfs.c
> > @@ -27,6 +27,7 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
> >  	int i, pos = 3;
> >  	const char *argv[OPTS_MAX] = {"mkfs", "-t", fs_type};
> >  	char fs_opts_str[1024] = "";
> > +	int tailed = 0;
> >  
> >  	if (!fs_type)
> >  		tst_brkm(TBROK, cleanup_fn, "No fs_type specified");
> > @@ -54,7 +55,11 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
> >  
> >  	if (fs_opts) {
> >  		for (i = 0; fs_opts[i]; i++) {
> > -			argv[pos++] = fs_opts[i];
> > +			if (!strcmp(fs_opts[i], "|")) {
> > +				argv[pos++] = dev;
> > +				tailed = 1;
> > +			} else
> > +				argv[pos++] = fs_opts[i];
> >  
> >  			if (pos + 2 > OPTS_MAX) {
> >  				tst_brkm(TBROK, cleanup_fn,
> > @@ -66,8 +71,8 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
> >  			strcat(fs_opts_str, fs_opts[i]);
> >  		}
> >  	}
> > -
> > -	argv[pos++] = dev;
> > +	if (tailed == 0)
> > +		argv[pos++] = dev;
> >  	argv[pos] = NULL;
> >  
> >  	tst_resm(TINFO, "Formatting %s with %s extra opts='%s'",
> 
> This is ugly. If you want to pass the filesystem size you should add an
> paramter to the mkfs function instead of inventing special syntax for
> what has been simple list of parameters. We may add a second tst_mkfs2()
> function with extra parameter for the fs size.
> 
> Also you should make sure that the device is large enough for the
> requested size. For this use we may call the paramter max_size and
> simply ignore it if the device is smaller than that.

Yeah, I know that's ugly:) I try to find a better way which suit LTP by talk with
you. So I sent a simple patch only for describe what problem I met.

My original idea it's:

1. change tst_mkfs(...) to tst_mkfs_sized(..., const char *fssize, const char *blocksize)
2. add a function cvtnum() which can transform a string about size to a number, e.g. "512M" to 512*1024*1024, "1k" to 1024 ...
3. In tst_mkfs_sized(), if fssize != NULL, check fs type(extX and some others fs need to know blocksize):
   XFS can use "mkfs -t xfs -d size=fssize"
   extX need to use "mkfs -t extX .... dev cvtnum(fssize)/cvtnum(blocksize)"
   ...
   We need to add more different fs support(We can support xfs, ext4 and btrfs at first, add
   more later)
4. #define tst_mkfs(...) tst_mkfs_size(..., NULL, NULL)

I think we don't need to make sure the device is large enough. The cases which use
tst_mkfs_sized() should make sure to use a appropriate size(maybe the smallest it need)
as the parameter. If the test device size not suit the case requested size, we directly
return ERROR which mkfs return to us.

What do you think? If you feel it's OK, I'll write a normal patch as that.

Thanks,
Zorro

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