[PATCHv2,3/5] locks: Fix breaking lease type after dump

Submitted by Pavel Begunkov (Silence) on Aug. 13, 2017, 11:49 p.m.

Details

Message ID 20170813234941.26329-3-asml.silence@gmail.com
State Accepted
Series "Series without cover letter"
Headers show

Commit Message

Pavel Begunkov (Silence) Aug. 13, 2017, 11:49 p.m.
To restore breaking lease we should know 'target lease type' (see fcntl
man), but it is always 'READ' in /proc. The patch sets correct lease
type using fcntl(F_GETLEASE).

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 criu/file-lock.c         | 26 +++++++++++++++++++++++++-
 criu/files.c             |  3 +++
 criu/include/file-lock.h |  1 +
 3 files changed, 29 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/file-lock.c b/criu/file-lock.c
index 0c5da70ca..5b0580782 100644
--- a/criu/file-lock.c
+++ b/criu/file-lock.c
@@ -334,6 +334,30 @@  int note_file_lock(struct pid *pid, int fd, int lfd, struct fd_parms *p)
 	return 0;
 }
 
+int correct_file_leases_type(struct pid *pid, int fd, int lfd)
+{
+	struct file_lock *fl;
+	int target_type;
+
+	list_for_each_entry(fl, &file_lock_list, list) {
+		if (fl->fl_owner != pid->real || fl->owners_fd != fd)
+			continue;
+
+		if (fl->fl_kind == FL_LEASE && fl->fl_ltype & LEASE_BREAKING) {
+			target_type = fcntl(lfd, F_GETLEASE);
+			if (target_type < 0) {
+				perror("Can't get lease type\n");
+				return -1;
+			}
+
+			fl->fl_ltype &= ~O_ACCMODE;
+			fl->fl_ltype |= target_type;
+			break;
+		}
+	}
+	return 0;
+}
+
 static int open_break_cb(int ns_root_fd, struct reg_file_info *rfi, void *arg)
 {
 	int fd, flags = *(int *)arg | O_NONBLOCK;
@@ -365,7 +389,7 @@  static int set_lease_for_breaking(int fd, int fd_type)
 	}
 
 	if (fcntl(fd, F_SETLEASE, lease_type) < 0) {
-		pr_perror("Can't set lease to fd %#x\n", fd);
+		pr_perror("Can't set lease (fd %i)\n", fd);
 		return -1;
 	}
 	return 0;
diff --git a/criu/files.c b/criu/files.c
index 2c3389e3f..6c27e2e26 100644
--- a/criu/files.c
+++ b/criu/files.c
@@ -459,6 +459,9 @@  static int dump_one_file(struct pid *pid, int fd, int lfd, struct fd_opts *opts,
 	if (note_file_lock(pid, fd, lfd, &p))
 		return -1;
 
+	if (correct_file_leases_type(pid, fd, lfd))
+		return -1;
+
 	p.fd_ctl = ctl; /* Some dump_opts require this to talk to parasite */
 
 	if (S_ISSOCK(p.stat.st_mode))
diff --git a/criu/include/file-lock.h b/criu/include/file-lock.h
index 7280e0b55..7b19524d7 100644
--- a/criu/include/file-lock.h
+++ b/criu/include/file-lock.h
@@ -68,6 +68,7 @@  extern struct collect_image_info file_locks_cinfo;
 
 struct pid;
 struct fd_parms;
+extern int correct_file_leases_type(struct pid *, int fd, int lfd);
 extern int note_file_lock(struct pid *, int fd, int lfd, struct fd_parms *);
 extern int dump_file_locks(void);
 

Comments

Pavel Emelyanov Aug. 15, 2017, 2:49 p.m.
On 08/14/2017 02:49 AM, Pavel Begunkov wrote:
> To restore breaking lease we should know 'target lease type' (see fcntl
> man), but it is always 'READ' in /proc. The patch sets correct lease
> type using fcntl(F_GETLEASE).
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  criu/file-lock.c         | 26 +++++++++++++++++++++++++-
>  criu/files.c             |  3 +++
>  criu/include/file-lock.h |  1 +
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/file-lock.c b/criu/file-lock.c
> index 0c5da70ca..5b0580782 100644
> --- a/criu/file-lock.c
> +++ b/criu/file-lock.c
> @@ -334,6 +334,30 @@ int note_file_lock(struct pid *pid, int fd, int lfd, struct fd_parms *p)
>  	return 0;
>  }
>  
> +int correct_file_leases_type(struct pid *pid, int fd, int lfd)
> +{
> +	struct file_lock *fl;
> +	int target_type;
> +
> +	list_for_each_entry(fl, &file_lock_list, list) {

I don't get why the list_for_each is here. The correct_file_leases_type() is done
right after note_file_lock which already walks the list of locks. Why not just re-use it?

> +		if (fl->fl_owner != pid->real || fl->owners_fd != fd)
> +			continue;
> +
> +		if (fl->fl_kind == FL_LEASE && fl->fl_ltype & LEASE_BREAKING) {

Please, add braces to the & operation.

> +			target_type = fcntl(lfd, F_GETLEASE);
> +			if (target_type < 0) {
> +				perror("Can't get lease type\n");
> +				return -1;
> +			}
> +
> +			fl->fl_ltype &= ~O_ACCMODE;
> +			fl->fl_ltype |= target_type;
> +			break;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int open_break_cb(int ns_root_fd, struct reg_file_info *rfi, void *arg)
>  {
>  	int fd, flags = *(int *)arg | O_NONBLOCK;
> @@ -365,7 +389,7 @@ static int set_lease_for_breaking(int fd, int fd_type)
>  	}
>  
>  	if (fcntl(fd, F_SETLEASE, lease_type) < 0) {
> -		pr_perror("Can't set lease to fd %#x\n", fd);
> +		pr_perror("Can't set lease (fd %i)\n", fd);

Merge this hunk into prev patch.

>  		return -1;
>  	}
>  	return 0;
> diff --git a/criu/files.c b/criu/files.c
> index 2c3389e3f..6c27e2e26 100644
> --- a/criu/files.c
> +++ b/criu/files.c
> @@ -459,6 +459,9 @@ static int dump_one_file(struct pid *pid, int fd, int lfd, struct fd_opts *opts,
>  	if (note_file_lock(pid, fd, lfd, &p))
>  		return -1;
>  
> +	if (correct_file_leases_type(pid, fd, lfd))
> +		return -1;
> +
>  	p.fd_ctl = ctl; /* Some dump_opts require this to talk to parasite */
>  
>  	if (S_ISSOCK(p.stat.st_mode))
> diff --git a/criu/include/file-lock.h b/criu/include/file-lock.h
> index 7280e0b55..7b19524d7 100644
> --- a/criu/include/file-lock.h
> +++ b/criu/include/file-lock.h
> @@ -68,6 +68,7 @@ extern struct collect_image_info file_locks_cinfo;
>  
>  struct pid;
>  struct fd_parms;
> +extern int correct_file_leases_type(struct pid *, int fd, int lfd);
>  extern int note_file_lock(struct pid *, int fd, int lfd, struct fd_parms *);
>  extern int dump_file_locks(void);
>  
>
Pavel Begunkov (Silence) Aug. 16, 2017, 12:14 p.m.
On 15/08/17 17:49, Pavel Emelyanov wrote:
> On 08/14/2017 02:49 AM, Pavel Begunkov wrote:
>> To restore breaking lease we should know 'target lease type' (see fcntl
>> man), but it is always 'READ' in /proc. The patch sets correct lease
>> type using fcntl(F_GETLEASE).
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  criu/file-lock.c         | 26 +++++++++++++++++++++++++-
>>  criu/files.c             |  3 +++
>>  criu/include/file-lock.h |  1 +
>>  3 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/criu/file-lock.c b/criu/file-lock.c
>> index 0c5da70ca..5b0580782 100644
>> --- a/criu/file-lock.c
>> +++ b/criu/file-lock.c
>> @@ -334,6 +334,30 @@ int note_file_lock(struct pid *pid, int fd, int lfd, struct fd_parms *p)
>>  	return 0;
>>  }
>>  
>> +int correct_file_leases_type(struct pid *pid, int fd, int lfd)
>> +{
>> +	struct file_lock *fl;
>> +	int target_type;
>> +
>> +	list_for_each_entry(fl, &file_lock_list, list) {
> 
> I don't get why the list_for_each is here. The correct_file_leases_type() is done
> right after note_file_lock which already walks the list of locks. Why not just re-use it?
I made extra function, because it have different responsibilities with
note_file_lock. The first one is fixing leases types. The second is
restoring association file lock -> file descriptor (see more in answer
to the next patchset part). note_file_lock is used only if
procfs/<pid>/fdinfo doesn't contain info about locks, new function is
not. Merging will lead to unclear code, also it'll make it less
maintainable.

Also, if locks in procfs/<pid>/fdinfo is presented it will be even
slighty faster without excess condition statements it the loop.


Should I really merge them?


> 
>> +		if (fl->fl_owner != pid->real || fl->owners_fd != fd)
>> +			continue;
>> +
>> +		if (fl->fl_kind == FL_LEASE && fl->fl_ltype & LEASE_BREAKING) {
> 
> Please, add braces to the & operation.
> 
>> +			target_type = fcntl(lfd, F_GETLEASE);
>> +			if (target_type < 0) {
>> +				perror("Can't get lease type\n");
>> +				return -1;
>> +			}
>> +
>> +			fl->fl_ltype &= ~O_ACCMODE;
>> +			fl->fl_ltype |= target_type;
>> +			break;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>>  static int open_break_cb(int ns_root_fd, struct reg_file_info *rfi, void *arg)
>>  {
>>  	int fd, flags = *(int *)arg | O_NONBLOCK;
>> @@ -365,7 +389,7 @@ static int set_lease_for_breaking(int fd, int fd_type)
>>  	}
>>  
>>  	if (fcntl(fd, F_SETLEASE, lease_type) < 0) {
>> -		pr_perror("Can't set lease to fd %#x\n", fd);
>> +		pr_perror("Can't set lease (fd %i)\n", fd);
> 
> Merge this hunk into prev patch.
> 
>>  		return -1;
>>  	}
>>  	return 0;
>> diff --git a/criu/files.c b/criu/files.c
>> index 2c3389e3f..6c27e2e26 100644
>> --- a/criu/files.c
>> +++ b/criu/files.c
>> @@ -459,6 +459,9 @@ static int dump_one_file(struct pid *pid, int fd, int lfd, struct fd_opts *opts,
>>  	if (note_file_lock(pid, fd, lfd, &p))
>>  		return -1;
>>  
>> +	if (correct_file_leases_type(pid, fd, lfd))
>> +		return -1;
>> +
>>  	p.fd_ctl = ctl; /* Some dump_opts require this to talk to parasite */
>>  
>>  	if (S_ISSOCK(p.stat.st_mode))
>> diff --git a/criu/include/file-lock.h b/criu/include/file-lock.h
>> index 7280e0b55..7b19524d7 100644
>> --- a/criu/include/file-lock.h
>> +++ b/criu/include/file-lock.h
>> @@ -68,6 +68,7 @@ extern struct collect_image_info file_locks_cinfo;
>>  
>>  struct pid;
>>  struct fd_parms;
>> +extern int correct_file_leases_type(struct pid *, int fd, int lfd);
>>  extern int note_file_lock(struct pid *, int fd, int lfd, struct fd_parms *);
>>  extern int dump_file_locks(void);
>>  
>>
>