[v2] zdtm: Make file_locks04 test based on /proc/{pid}/fdinfo/{fd}

Submitted by Kirill Tkhai on June 28, 2016, 7:08 p.m.

Details

Message ID 146714088484.24563.13605865489675045177.stgit@pro
State Accepted
Series "zdtm: Make file_locks04 test based on /proc/{pid}/fdinfo/{fd}"
Commit 46f68e84afeb09e9bfc38651a2043552d4e54979
Headers show

Commit Message

Kirill Tkhai June 28, 2016, 7:08 p.m.
/proc/locks is racy with adding/removing locks,
so we may lose lock on check.

Use fdinfo's list of locks instead.

v2: Feature 'fdinfo_lock'

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 test/zdtm/static/file_locks04.c    |   31 +++++++++++++++++--------------
 test/zdtm/static/file_locks04.desc |    2 +-
 2 files changed, 18 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

diff --git a/test/zdtm/static/file_locks04.c b/test/zdtm/static/file_locks04.c
index 995dd09..28ed497 100644
--- a/test/zdtm/static/file_locks04.c
+++ b/test/zdtm/static/file_locks04.c
@@ -6,6 +6,7 @@ 
 #include <unistd.h>
 #include <sys/file.h>
 #include <string.h>
+#include <limits.h>
 #include <sys/wait.h>
 
 #include "zdtmtst.h"
@@ -16,27 +17,27 @@  const char *test_author	= "Pavel Emelyanov <xemul@parallels.com>";
 char *filename;
 TEST_OPTION(filename, string, "file name", 1);
 
-static int check_file_locks(int alt_pid, int fd)
+static int check_file_locks(pid_t child_pid, int fd, int child_fd)
 {
+	char		path[PATH_MAX];
 	FILE		*fp_locks = NULL;
 	char		buf[100], fl_flag[16], fl_type[16], fl_option[16];
-	pid_t		pid = getpid();
 	int		found = 0, num, fl_owner;
 
-	fp_locks = fopen("/proc/locks", "r");
-	if (!fp_locks)
+	sprintf(path, "/proc/%d/fdinfo/%d", child_pid, child_fd);
+	fp_locks = fopen(path, "r");
+	if (!fp_locks) {
+		pr_err("Can't open %s\n", path);
 		return -1;
-
-	test_msg("C: %d/%d\n", pid, alt_pid);
+	}
 
 	while (fgets(buf, sizeof(buf), fp_locks)) {
-		test_msg("c: %s", buf);
-
-		if (strstr(buf, "->"))
+		if (strncmp(buf, "lock:\t", 6) != 0)
 			continue;
+		test_msg("c: %s", buf);
 
 		num = sscanf(buf,
-			"%*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
+			"%*s %*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
 			fl_flag, fl_type, fl_option, &fl_owner);
 
 		if (num < 4) {
@@ -44,8 +45,10 @@  static int check_file_locks(int alt_pid, int fd)
 			break;
 		}
 
-		if (fl_owner != pid && fl_owner != alt_pid)
+		if (fl_owner != child_pid && fl_owner != getpid()) {
+			pr_err("Wrong owner\n");
 			continue;
+		}
 
 		if (!strcmp(fl_flag, "FLOCK") &&
 				!strcmp(fl_type, "ADVISORY") &&
@@ -67,11 +70,11 @@  static int check_file_locks(int alt_pid, int fd)
 
 int main(int argc, char **argv)
 {
-	int fd, pid;
+	int fd, child_fd, pid;
 
 	test_init(argc, argv);
 
-	fd = open(filename, O_CREAT | O_RDWR, 0600);
+	fd = child_fd = open(filename, O_CREAT | O_RDWR, 0600);
 	if (fd < 0) {
 		pr_perror("No file");
 		return -1;
@@ -105,7 +108,7 @@  int main(int argc, char **argv)
 	test_daemon();
 	test_waitsig();
 
-	if (check_file_locks(pid, fd))
+	if (check_file_locks(pid, fd, child_fd) > 0)
 		pass();
 	else
 		fail("Flock file locks check failed");
diff --git a/test/zdtm/static/file_locks04.desc b/test/zdtm/static/file_locks04.desc
index 80cd04e..41aad3f 100644
--- a/test/zdtm/static/file_locks04.desc
+++ b/test/zdtm/static/file_locks04.desc
@@ -1 +1 @@ 
-{'flags': 'excl', 'opts': '--file-locks'}
+{'flags': 'excl', 'opts': '--file-locks', 'feature': 'fdinfo_lock'}

Comments

Pavel Emelianov July 13, 2016, 12:38 p.m.
Applied.

Why only 04 test? How about the others?
Kirill Tkhai July 13, 2016, 2:14 p.m.
On 13.07.2016 15:38, Pavel Emelyanov wrote:
> Applied.
> 
> Why only 04 test? How about the others?

Are they also affected? I didn't investigate them. There just were a report about problems in 04.
If there are problems, I'll fix them too a little bit later.
Pavel Emelianov July 14, 2016, 10:55 a.m.
Andrey, would you ack/nack this patch?

On 06/28/2016 10:08 PM, Kirill Tkhai wrote:
> /proc/locks is racy with adding/removing locks,
> so we may lose lock on check.
> 
> Use fdinfo's list of locks instead.
> 
> v2: Feature 'fdinfo_lock'
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  test/zdtm/static/file_locks04.c    |   31 +++++++++++++++++--------------
>  test/zdtm/static/file_locks04.desc |    2 +-
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/test/zdtm/static/file_locks04.c b/test/zdtm/static/file_locks04.c
> index 995dd09..28ed497 100644
> --- a/test/zdtm/static/file_locks04.c
> +++ b/test/zdtm/static/file_locks04.c
> @@ -6,6 +6,7 @@
>  #include <unistd.h>
>  #include <sys/file.h>
>  #include <string.h>
> +#include <limits.h>
>  #include <sys/wait.h>
>  
>  #include "zdtmtst.h"
> @@ -16,27 +17,27 @@ const char *test_author	= "Pavel Emelyanov <xemul@parallels.com>";
>  char *filename;
>  TEST_OPTION(filename, string, "file name", 1);
>  
> -static int check_file_locks(int alt_pid, int fd)
> +static int check_file_locks(pid_t child_pid, int fd, int child_fd)
>  {
> +	char		path[PATH_MAX];
>  	FILE		*fp_locks = NULL;
>  	char		buf[100], fl_flag[16], fl_type[16], fl_option[16];
> -	pid_t		pid = getpid();
>  	int		found = 0, num, fl_owner;
>  
> -	fp_locks = fopen("/proc/locks", "r");
> -	if (!fp_locks)
> +	sprintf(path, "/proc/%d/fdinfo/%d", child_pid, child_fd);
> +	fp_locks = fopen(path, "r");
> +	if (!fp_locks) {
> +		pr_err("Can't open %s\n", path);
>  		return -1;
> -
> -	test_msg("C: %d/%d\n", pid, alt_pid);
> +	}
>  
>  	while (fgets(buf, sizeof(buf), fp_locks)) {
> -		test_msg("c: %s", buf);
> -
> -		if (strstr(buf, "->"))
> +		if (strncmp(buf, "lock:\t", 6) != 0)
>  			continue;
> +		test_msg("c: %s", buf);
>  
>  		num = sscanf(buf,
> -			"%*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
> +			"%*s %*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
>  			fl_flag, fl_type, fl_option, &fl_owner);
>  
>  		if (num < 4) {
> @@ -44,8 +45,10 @@ static int check_file_locks(int alt_pid, int fd)
>  			break;
>  		}
>  
> -		if (fl_owner != pid && fl_owner != alt_pid)
> +		if (fl_owner != child_pid && fl_owner != getpid()) {
> +			pr_err("Wrong owner\n");
>  			continue;
> +		}
>  
>  		if (!strcmp(fl_flag, "FLOCK") &&
>  				!strcmp(fl_type, "ADVISORY") &&
> @@ -67,11 +70,11 @@ static int check_file_locks(int alt_pid, int fd)
>  
>  int main(int argc, char **argv)
>  {
> -	int fd, pid;
> +	int fd, child_fd, pid;
>  
>  	test_init(argc, argv);
>  
> -	fd = open(filename, O_CREAT | O_RDWR, 0600);
> +	fd = child_fd = open(filename, O_CREAT | O_RDWR, 0600);
>  	if (fd < 0) {
>  		pr_perror("No file");
>  		return -1;
> @@ -105,7 +108,7 @@ int main(int argc, char **argv)
>  	test_daemon();
>  	test_waitsig();
>  
> -	if (check_file_locks(pid, fd))
> +	if (check_file_locks(pid, fd, child_fd) > 0)
>  		pass();
>  	else
>  		fail("Flock file locks check failed");
> diff --git a/test/zdtm/static/file_locks04.desc b/test/zdtm/static/file_locks04.desc
> index 80cd04e..41aad3f 100644
> --- a/test/zdtm/static/file_locks04.desc
> +++ b/test/zdtm/static/file_locks04.desc
> @@ -1 +1 @@
> -{'flags': 'excl', 'opts': '--file-locks'}
> +{'flags': 'excl', 'opts': '--file-locks', 'feature': 'fdinfo_lock'}
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>
Andrey Vagin July 15, 2016, 4:46 p.m.
On Thu, Jul 14, 2016 at 01:55:43PM +0300, Pavel Emelyanov wrote:
> Andrey, would you ack/nack this patch?

It's ok for me, but here are a few facts:

* I don't see any reason to fix only this test. Other lock tests has the
  same problems. We use the 'excl' flag to not run these tests
  concurrently.

* If we will fix all tests by this way, we will not be able to test locks
  on kernels where fdinfo doesn't contains locks. We know that in this
  case we can meet the same race in criu and may be it isn't a big
  problem.
> 
> On 06/28/2016 10:08 PM, Kirill Tkhai wrote:
> > /proc/locks is racy with adding/removing locks,
> > so we may lose lock on check.
> > 
> > Use fdinfo's list of locks instead.
> > 
> > v2: Feature 'fdinfo_lock'
> > 
> > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> > ---
> >  test/zdtm/static/file_locks04.c    |   31 +++++++++++++++++--------------
> >  test/zdtm/static/file_locks04.desc |    2 +-
> >  2 files changed, 18 insertions(+), 15 deletions(-)
> > 
> > diff --git a/test/zdtm/static/file_locks04.c b/test/zdtm/static/file_locks04.c
> > index 995dd09..28ed497 100644
> > --- a/test/zdtm/static/file_locks04.c
> > +++ b/test/zdtm/static/file_locks04.c
> > @@ -6,6 +6,7 @@
> >  #include <unistd.h>
> >  #include <sys/file.h>
> >  #include <string.h>
> > +#include <limits.h>
> >  #include <sys/wait.h>
> >  
> >  #include "zdtmtst.h"
> > @@ -16,27 +17,27 @@ const char *test_author	= "Pavel Emelyanov <xemul@parallels.com>";
> >  char *filename;
> >  TEST_OPTION(filename, string, "file name", 1);
> >  
> > -static int check_file_locks(int alt_pid, int fd)
> > +static int check_file_locks(pid_t child_pid, int fd, int child_fd)
> >  {
> > +	char		path[PATH_MAX];
> >  	FILE		*fp_locks = NULL;
> >  	char		buf[100], fl_flag[16], fl_type[16], fl_option[16];
> > -	pid_t		pid = getpid();
> >  	int		found = 0, num, fl_owner;
> >  
> > -	fp_locks = fopen("/proc/locks", "r");
> > -	if (!fp_locks)
> > +	sprintf(path, "/proc/%d/fdinfo/%d", child_pid, child_fd);
> > +	fp_locks = fopen(path, "r");
> > +	if (!fp_locks) {
> > +		pr_err("Can't open %s\n", path);
> >  		return -1;
> > -
> > -	test_msg("C: %d/%d\n", pid, alt_pid);
> > +	}
> >  
> >  	while (fgets(buf, sizeof(buf), fp_locks)) {
> > -		test_msg("c: %s", buf);
> > -
> > -		if (strstr(buf, "->"))
> > +		if (strncmp(buf, "lock:\t", 6) != 0)
> >  			continue;
> > +		test_msg("c: %s", buf);
> >  
> >  		num = sscanf(buf,
> > -			"%*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
> > +			"%*s %*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
> >  			fl_flag, fl_type, fl_option, &fl_owner);
> >  
> >  		if (num < 4) {
> > @@ -44,8 +45,10 @@ static int check_file_locks(int alt_pid, int fd)
> >  			break;
> >  		}
> >  
> > -		if (fl_owner != pid && fl_owner != alt_pid)
> > +		if (fl_owner != child_pid && fl_owner != getpid()) {
> > +			pr_err("Wrong owner\n");
> >  			continue;
> > +		}
> >  
> >  		if (!strcmp(fl_flag, "FLOCK") &&
> >  				!strcmp(fl_type, "ADVISORY") &&
> > @@ -67,11 +70,11 @@ static int check_file_locks(int alt_pid, int fd)
> >  
> >  int main(int argc, char **argv)
> >  {
> > -	int fd, pid;
> > +	int fd, child_fd, pid;
> >  
> >  	test_init(argc, argv);
> >  
> > -	fd = open(filename, O_CREAT | O_RDWR, 0600);
> > +	fd = child_fd = open(filename, O_CREAT | O_RDWR, 0600);
> >  	if (fd < 0) {
> >  		pr_perror("No file");
> >  		return -1;
> > @@ -105,7 +108,7 @@ int main(int argc, char **argv)
> >  	test_daemon();
> >  	test_waitsig();
> >  
> > -	if (check_file_locks(pid, fd))
> > +	if (check_file_locks(pid, fd, child_fd) > 0)
> >  		pass();
> >  	else
> >  		fail("Flock file locks check failed");
> > diff --git a/test/zdtm/static/file_locks04.desc b/test/zdtm/static/file_locks04.desc
> > index 80cd04e..41aad3f 100644
> > --- a/test/zdtm/static/file_locks04.desc
> > +++ b/test/zdtm/static/file_locks04.desc
> > @@ -1 +1 @@
> > -{'flags': 'excl', 'opts': '--file-locks'}
> > +{'flags': 'excl', 'opts': '--file-locks', 'feature': 'fdinfo_lock'}
> > 
> > _______________________________________________
> > CRIU mailing list
> > CRIU@openvz.org
> > https://lists.openvz.org/mailman/listinfo/criu
> > .
> > 
>
Pavel Emelianov July 21, 2016, 2:01 p.m.
On 07/15/2016 07:46 PM, Andrew Vagin wrote:
> On Thu, Jul 14, 2016 at 01:55:43PM +0300, Pavel Emelyanov wrote:
>> Andrey, would you ack/nack this patch?
> 
> It's ok for me, but here are a few facts:
> 
> * I don't see any reason to fix only this test. Other lock tests has the
>   same problems. We use the 'excl' flag to not run these tests
>   concurrently.
> 
> * If we will fix all tests by this way, we will not be able to test locks
>   on kernels where fdinfo doesn't contains locks. We know that in this
>   case we can meet the same race in criu and may be it isn't a big
>   problem.

OK, so checking CRIU works on 3.11 is essential. Thus I'd keep tests on
legacy APIs __unless__ there's an issue we cannot overcome without using
the new one.

-- Pavel

>> On 06/28/2016 10:08 PM, Kirill Tkhai wrote:
>>> /proc/locks is racy with adding/removing locks,
>>> so we may lose lock on check.
>>>
>>> Use fdinfo's list of locks instead.
>>>
>>> v2: Feature 'fdinfo_lock'
>>>
>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>> ---
>>>  test/zdtm/static/file_locks04.c    |   31 +++++++++++++++++--------------
>>>  test/zdtm/static/file_locks04.desc |    2 +-
>>>  2 files changed, 18 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/test/zdtm/static/file_locks04.c b/test/zdtm/static/file_locks04.c
>>> index 995dd09..28ed497 100644
>>> --- a/test/zdtm/static/file_locks04.c
>>> +++ b/test/zdtm/static/file_locks04.c
>>> @@ -6,6 +6,7 @@
>>>  #include <unistd.h>
>>>  #include <sys/file.h>
>>>  #include <string.h>
>>> +#include <limits.h>
>>>  #include <sys/wait.h>
>>>  
>>>  #include "zdtmtst.h"
>>> @@ -16,27 +17,27 @@ const char *test_author	= "Pavel Emelyanov <xemul@parallels.com>";
>>>  char *filename;
>>>  TEST_OPTION(filename, string, "file name", 1);
>>>  
>>> -static int check_file_locks(int alt_pid, int fd)
>>> +static int check_file_locks(pid_t child_pid, int fd, int child_fd)
>>>  {
>>> +	char		path[PATH_MAX];
>>>  	FILE		*fp_locks = NULL;
>>>  	char		buf[100], fl_flag[16], fl_type[16], fl_option[16];
>>> -	pid_t		pid = getpid();
>>>  	int		found = 0, num, fl_owner;
>>>  
>>> -	fp_locks = fopen("/proc/locks", "r");
>>> -	if (!fp_locks)
>>> +	sprintf(path, "/proc/%d/fdinfo/%d", child_pid, child_fd);
>>> +	fp_locks = fopen(path, "r");
>>> +	if (!fp_locks) {
>>> +		pr_err("Can't open %s\n", path);
>>>  		return -1;
>>> -
>>> -	test_msg("C: %d/%d\n", pid, alt_pid);
>>> +	}
>>>  
>>>  	while (fgets(buf, sizeof(buf), fp_locks)) {
>>> -		test_msg("c: %s", buf);
>>> -
>>> -		if (strstr(buf, "->"))
>>> +		if (strncmp(buf, "lock:\t", 6) != 0)
>>>  			continue;
>>> +		test_msg("c: %s", buf);
>>>  
>>>  		num = sscanf(buf,
>>> -			"%*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
>>> +			"%*s %*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
>>>  			fl_flag, fl_type, fl_option, &fl_owner);
>>>  
>>>  		if (num < 4) {
>>> @@ -44,8 +45,10 @@ static int check_file_locks(int alt_pid, int fd)
>>>  			break;
>>>  		}
>>>  
>>> -		if (fl_owner != pid && fl_owner != alt_pid)
>>> +		if (fl_owner != child_pid && fl_owner != getpid()) {
>>> +			pr_err("Wrong owner\n");
>>>  			continue;
>>> +		}
>>>  
>>>  		if (!strcmp(fl_flag, "FLOCK") &&
>>>  				!strcmp(fl_type, "ADVISORY") &&
>>> @@ -67,11 +70,11 @@ static int check_file_locks(int alt_pid, int fd)
>>>  
>>>  int main(int argc, char **argv)
>>>  {
>>> -	int fd, pid;
>>> +	int fd, child_fd, pid;
>>>  
>>>  	test_init(argc, argv);
>>>  
>>> -	fd = open(filename, O_CREAT | O_RDWR, 0600);
>>> +	fd = child_fd = open(filename, O_CREAT | O_RDWR, 0600);
>>>  	if (fd < 0) {
>>>  		pr_perror("No file");
>>>  		return -1;
>>> @@ -105,7 +108,7 @@ int main(int argc, char **argv)
>>>  	test_daemon();
>>>  	test_waitsig();
>>>  
>>> -	if (check_file_locks(pid, fd))
>>> +	if (check_file_locks(pid, fd, child_fd) > 0)
>>>  		pass();
>>>  	else
>>>  		fail("Flock file locks check failed");
>>> diff --git a/test/zdtm/static/file_locks04.desc b/test/zdtm/static/file_locks04.desc
>>> index 80cd04e..41aad3f 100644
>>> --- a/test/zdtm/static/file_locks04.desc
>>> +++ b/test/zdtm/static/file_locks04.desc
>>> @@ -1 +1 @@
>>> -{'flags': 'excl', 'opts': '--file-locks'}
>>> +{'flags': 'excl', 'opts': '--file-locks', 'feature': 'fdinfo_lock'}
>>>
>>> _______________________________________________
>>> CRIU mailing list
>>> CRIU@openvz.org
>>> https://lists.openvz.org/mailman/listinfo/criu
>>> .
>>>
>>
> .
>