[LTP] [PATCH 1/2] syscalls/inotify06: Terminate child process on test error

[LTP] [PATCH 1/2] syscalls/inotify06: Terminate child process on test error

[Martin Doucha]

If the main test process exits early, the child would keep running and
interfere with tmpdir cleanup.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 testcases/kernel/syscalls/inotify/inotify06.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/inotify/inotify06.c b/testcases/kernel/syscalls/inotify/inotify06.c
index 96189b082..f39ab46a1 100644
--- a/testcases/kernel/syscalls/inotify/inotify06.c
+++ b/testcases/kernel/syscalls/inotify/inotify06.c
@@ -38,7 +38,8 @@
 /* Number of files to test (must be > 1) */
 #define FILES 5
 
-char names[FILES][PATH_MAX];
+static char names[FILES][PATH_MAX];
+static pid_t pid;
 
 static void setup(void)
 {
@@ -51,7 +52,6 @@ static void setup(void)
 static void verify_inotify(void)
 {
 	int inotify_fd, fd;
-	pid_t pid;
 	int i, tests;
 
 	pid = SAFE_FORK();
@@ -85,14 +85,24 @@ static void verify_inotify(void)
 
 	/* Kill the child creating / deleting files and wait for it */
 	SAFE_KILL(pid, SIGKILL);
+	pid = 0;
 	SAFE_WAIT(NULL);
 }
 
+static void cleanup(void)
+{
+	if (pid) {
+		SAFE_KILL(pid, SIGKILL);
+		SAFE_WAIT(NULL);
+	}
+}
+
 static struct tst_test test = {
 	.timeout = 600,
 	.needs_tmpdir = 1,
 	.forks_child = 1,
 	.setup = setup,
+	.cleanup = cleanup,
 	.test_all = verify_inotify,
 };
 
-- 
2.31.1

[LTP] [PATCH 2/2] syscalls/inotify06: Raise inotify instance limit in /proc

[Martin Doucha]

inotify_init() sometimes fails with EMFILE because there are too many
partially closed instances waiting for garbage collection. Bump the limit
in /proc/sys/fs/inotify/max_user_instances for the duration of the test.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

I thought about only reading the procfile and calling yield() after every
proc_limit/2 iterations to wait for garbage collection but I'm afraid that
it might reduce the likelihood of triggering the bug. Since I currently have
no system where I could reproduce the race, I've decided to play it safe and
bump the /proc limit.

 testcases/kernel/syscalls/inotify/inotify06.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/testcases/kernel/syscalls/inotify/inotify06.c b/testcases/kernel/syscalls/inotify/inotify06.c
index f39ab46a1..68813769b 100644
--- a/testcases/kernel/syscalls/inotify/inotify06.c
+++ b/testcases/kernel/syscalls/inotify/inotify06.c
@@ -38,8 +38,11 @@
 /* Number of files to test (must be > 1) */
 #define FILES 5
 
+#define PROCFILE "/proc/sys/fs/inotify/max_user_instances"
+
 static char names[FILES][PATH_MAX];
 static pid_t pid;
+static int old_proc_limit;
 
 static void setup(void)
 {
@@ -47,6 +50,11 @@ static void setup(void)
 
 	for (i = 0; i < FILES; i++)
 		sprintf(names[i], "fname_%d", i);
+
+	SAFE_FILE_SCANF(PROCFILE, "%d", &old_proc_limit);
+
+	if (old_proc_limit >= 0 && old_proc_limit < TEARDOWNS)
+		SAFE_FILE_PRINTF(PROCFILE, "%d", TEARDOWNS + 128);
 }
 
 static void verify_inotify(void)
@@ -95,10 +103,13 @@ static void cleanup(void)
 		SAFE_KILL(pid, SIGKILL);
 		SAFE_WAIT(NULL);
 	}
+
+	SAFE_FILE_PRINTF(PROCFILE, "%d", old_proc_limit);
 }
 
 static struct tst_test test = {
 	.timeout = 600,
+	.needs_root = 1,
 	.needs_tmpdir = 1,
 	.forks_child = 1,
 	.setup = setup,
-- 
2.31.1

[LTP] [PATCH 2/2] syscalls/inotify06: Raise inotify instance limit in /proc

[Jan Kara]

On Wed 05-05-21 17:38:47, Martin Doucha wrote:
> inotify_init() sometimes fails with EMFILE because there are too many
> partially closed instances waiting for garbage collection. Bump the limit
> in /proc/sys/fs/inotify/max_user_instances for the duration of the test.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
> 
> I thought about only reading the procfile and calling yield() after every
> proc_limit/2 iterations to wait for garbage collection but I'm afraid that
> it might reduce the likelihood of triggering the bug. Since I currently have
> no system where I could reproduce the race, I've decided to play it safe and
> bump the /proc limit.

So waiting would be fine as well. One process simply creates & deletes
files in a loop until the other performs TEARDOWNS teardowns. It doesn't
really matter how fast teardowns happen for the race to trigger. But I have
no problem with this solution either.

								Honza

> 
>  testcases/kernel/syscalls/inotify/inotify06.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/testcases/kernel/syscalls/inotify/inotify06.c b/testcases/kernel/syscalls/inotify/inotify06.c
> index f39ab46a1..68813769b 100644
> --- a/testcases/kernel/syscalls/inotify/inotify06.c
> +++ b/testcases/kernel/syscalls/inotify/inotify06.c
> @@ -38,8 +38,11 @@
>  /* Number of files to test (must be > 1) */
>  #define FILES 5
>  
> +#define PROCFILE "/proc/sys/fs/inotify/max_user_instances"
> +
>  static char names[FILES][PATH_MAX];
>  static pid_t pid;
> +static int old_proc_limit;
>  
>  static void setup(void)
>  {
> @@ -47,6 +50,11 @@ static void setup(void)
>  
>  	for (i = 0; i < FILES; i++)
>  		sprintf(names[i], "fname_%d", i);
> +
> +	SAFE_FILE_SCANF(PROCFILE, "%d", &old_proc_limit);
> +
> +	if (old_proc_limit >= 0 && old_proc_limit < TEARDOWNS)
> +		SAFE_FILE_PRINTF(PROCFILE, "%d", TEARDOWNS + 128);
>  }
>  
>  static void verify_inotify(void)
> @@ -95,10 +103,13 @@ static void cleanup(void)
>  		SAFE_KILL(pid, SIGKILL);
>  		SAFE_WAIT(NULL);
>  	}
> +
> +	SAFE_FILE_PRINTF(PROCFILE, "%d", old_proc_limit);
>  }
>  
>  static struct tst_test test = {
>  	.timeout = 600,
> +	.needs_root = 1,
>  	.needs_tmpdir = 1,
>  	.forks_child = 1,
>  	.setup = setup,
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[LTP] [PATCH 1/2] syscalls/inotify06: Terminate child process on test error

[Jan Kara]

On Wed 05-05-21 17:38:46, Martin Doucha wrote:
> If the main test process exits early, the child would keep running and
> interfere with tmpdir cleanup.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  testcases/kernel/syscalls/inotify/inotify06.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

Thanks for the patch. It looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> diff --git a/testcases/kernel/syscalls/inotify/inotify06.c b/testcases/kernel/syscalls/inotify/inotify06.c
> index 96189b082..f39ab46a1 100644
> --- a/testcases/kernel/syscalls/inotify/inotify06.c
> +++ b/testcases/kernel/syscalls/inotify/inotify06.c
> @@ -38,7 +38,8 @@
>  /* Number of files to test (must be > 1) */
>  #define FILES 5
>  
> -char names[FILES][PATH_MAX];
> +static char names[FILES][PATH_MAX];
> +static pid_t pid;
>  
>  static void setup(void)
>  {
> @@ -51,7 +52,6 @@ static void setup(void)
>  static void verify_inotify(void)
>  {
>  	int inotify_fd, fd;
> -	pid_t pid;
>  	int i, tests;
>  
>  	pid = SAFE_FORK();
> @@ -85,14 +85,24 @@ static void verify_inotify(void)
>  
>  	/* Kill the child creating / deleting files and wait for it */
>  	SAFE_KILL(pid, SIGKILL);
> +	pid = 0;
>  	SAFE_WAIT(NULL);
>  }
>  
> +static void cleanup(void)
> +{
> +	if (pid) {
> +		SAFE_KILL(pid, SIGKILL);
> +		SAFE_WAIT(NULL);
> +	}
> +}
> +
>  static struct tst_test test = {
>  	.timeout = 600,
>  	.needs_tmpdir = 1,
>  	.forks_child = 1,
>  	.setup = setup,
> +	.cleanup = cleanup,
>  	.test_all = verify_inotify,
>  };
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[LTP] [PATCH 1/2] syscalls/inotify06: Terminate child process on test error

[Petr Vorel]

Hi Martin, Jan,

> On Wed 05-05-21 17:38:46, Martin Doucha wrote:
> > If the main test process exits early, the child would keep running and
> > interfere with tmpdir cleanup.

> > Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> > ---
> >  testcases/kernel/syscalls/inotify/inotify06.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)

> Thanks for the patch. It looks good to me. You can add:

> Reviewed-by: Jan Kara <jack@suse.cz>

Merged, thanks!

Kind regards,
Petr

[LTP] [PATCH 2/2] syscalls/inotify06: Raise inotify instance limit in /proc

[Martin Doucha]

On 05. 05. 21 18:47, Jan Kara wrote:
> On Wed 05-05-21 17:38:47, Martin Doucha wrote:
>> inotify_init() sometimes fails with EMFILE because there are too many
>> partially closed instances waiting for garbage collection. Bump the limit
>> in /proc/sys/fs/inotify/max_user_instances for the duration of the test.
>>
>> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
>> ---
>>
>> I thought about only reading the procfile and calling yield() after every
>> proc_limit/2 iterations to wait for garbage collection but I'm afraid that
>> it might reduce the likelihood of triggering the bug. Since I currently have
>> no system where I could reproduce the race, I've decided to play it safe and
>> bump the /proc limit.
> 
> So waiting would be fine as well. One process simply creates & deletes
> files in a loop until the other performs TEARDOWNS teardowns. It doesn't
> really matter how fast teardowns happen for the race to trigger. But I have
> no problem with this solution either.

Let's go with the patch as is then. Like I said, when I don't have a
system where the issue is reproducible, I prefer to play it safe.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH 2/2] syscalls/inotify06: Raise inotify instance limit in /proc

[Petr Vorel]

Hi Martin, Jan,

> On 05. 05. 21 18:47, Jan Kara wrote:
> > On Wed 05-05-21 17:38:47, Martin Doucha wrote:
> >> inotify_init() sometimes fails with EMFILE because there are too many
> >> partially closed instances waiting for garbage collection. Bump the limit
> >> in /proc/sys/fs/inotify/max_user_instances for the duration of the test.

> >> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> >> ---

> >> I thought about only reading the procfile and calling yield() after every
> >> proc_limit/2 iterations to wait for garbage collection but I'm afraid that
> >> it might reduce the likelihood of triggering the bug. Since I currently have
> >> no system where I could reproduce the race, I've decided to play it safe and
> >> bump the /proc limit.

> > So waiting would be fine as well. One process simply creates & deletes
> > files in a loop until the other performs TEARDOWNS teardowns. It doesn't
> > really matter how fast teardowns happen for the race to trigger. But I have
> > no problem with this solution either.

> Let's go with the patch as is then. Like I said, when I don't have a
> system where the issue is reproducible, I prefer to play it safe.

Make sense, merged. Thank you both for fixing and review!

Kind regards,
Petr