[CRIU] [PATCHv2 2/5] locks: Add c/r of breaking leases (kernel>=v4.1)

[Pavel Emelyanov]

On 08/14/2017 02:49 AM, Pavel Begunkov wrote:
> Leases can be taken out only on regular files. In order to restore
> breaking lease it break restored lease by opening a file with which
> lease is associated.

But if the lease was broken, then there should be a file in
the images, that is opened. Why opening it one more time?

More comments inline.

> Signed-off-by: Pavel Begunkov <asml.silence at gmail.com>
> ---
>  criu/file-lock.c         | 99 ++++++++++++++++++++++++++++++++++++++++++++++--
>  criu/files.c             |  1 +
>  criu/include/file-lock.h |  3 ++
>  criu/proc_parse.c        |  7 ++--
>  4 files changed, 103 insertions(+), 7 deletions(-)
> 
> diff --git a/criu/file-lock.c b/criu/file-lock.c
> index 92d8bd394..0c5da70ca 100644
> --- a/criu/file-lock.c
> +++ b/criu/file-lock.c
> @@ -18,6 +18,8 @@
>  #include "proc_parse.h"
>  #include "servicefd.h"
>  #include "file-lock.h"
> +#include "pstree.h"
> +#include "files-reg.h"
>  
>  struct file_lock_rst {
>  	FileLockEntry *fle;
> @@ -332,6 +334,97 @@ int note_file_lock(struct pid *pid, int fd, int lfd, struct fd_parms *p)
>  	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;
> +
> +	fd = openat(ns_root_fd, rfi->path, flags);
> +	if (fd >= 0) {
> +		pr_err("Conflicting lease wasn't found\n");
> +		close(fd);
> +		return -1;
> +	}
> +	if (errno != EWOULDBLOCK) {
> +		pr_perror("Can't open file to break lease\n");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static int set_lease_for_breaking(int fd, int fd_type)
> +{
> +	int lease_type;
> +
> +	if (fd_type == O_RDONLY) {
> +		lease_type = F_RDLCK;
> +	} else if (fd_type == O_WRONLY) {
> +		lease_type = F_WRLCK;
> +	} else {
> +		pr_err("Incompatible with lease file type (%i)\n", fd_type);
> +		return -1;
> +	}
> +
> +	if (fcntl(fd, F_SETLEASE, lease_type) < 0) {
> +		pr_perror("Can't set lease to fd %#x\n", fd);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static struct fdinfo_list_entry *find_fd(struct pstree_item *task, int fd)

There's already such a helper in files.c

> +{
> +	struct list_head *head = &rsti(task)->fds;
> +	struct fdinfo_list_entry *fle;
> +
> +	list_for_each_entry_reverse(fle, head, ps_list) {
> +		if (fle->fe->fd == fd)
> +			return fle;
> +	}
> +	return NULL;
> +}
> +
> +static int restore_breaking_file_lease(FileLockEntry *fle)
> +{
> +	struct fdinfo_list_entry *fdle;
> +	int break_flags;
> +	int lease_break_type = fle->type & (~LEASE_BREAKING);
> +
> +	fdle = find_fd(current, fle->fd);
> +	if (fdle == NULL) {
> +		pr_err("Can't get file description\n");
> +		return -1;
> +	}
> +
> +	if (set_lease_for_breaking(fle->fd, fdle->desc->ops->type))
> +		return -1;
> +
> +	if (lease_break_type == F_UNLCK) {
> +		break_flags = O_WRONLY;
> +	} else if (lease_break_type == F_RDLCK) {
> +		break_flags = O_RDONLY;
> +	} else {
> +		pr_err("Incorrect breaking type of lease\n");
> +		return -1;
> +	}
> +	if (open_path(fdle->desc, open_break_cb, (void *)&break_flags) < 0)
> +		return -1;
> +	return 0;

Just return open_path();?

> +}
> +
> +static int restore_file_lease(FileLockEntry *fle)
> +{
> +	int ret;
> +
> +	if (fle->type & LEASE_BREAKING) {
> +		return restore_breaking_file_lease(fle);

The restore_breaking_file_lease() leaves the lease in the "generated" state,
not in the fle->type one. Is that intentional?

> +	} else {
> +		ret = fcntl(fle->fd, F_SETLEASE, fle->type);
> +		if (ret < 0)
> +			pr_perror("Can't restore non broken lease");
> +		return ret;
> +	}
> +}
> +
>  static int restore_file_lock(FileLockEntry *fle)
>  {
>  	int ret = -1;
> @@ -399,11 +492,9 @@ static int restore_file_lock(FileLockEntry *fle)
>  			goto err;
>  		}
>  	} else if (fle->flag & FL_LEASE) {
> -		ret = fcntl(fle->fd, F_SETLEASE, fle->type);
> -		if (ret < 0) {
> -			pr_perror("Can't set lease!\n");
> +		ret = restore_file_lease(fle);
> +		if (ret < 0)
>  			goto err;
> -		}
>  	} else {
>  		pr_err("Unknown file lock style!\n");
>  		goto err;
> diff --git a/criu/files.c b/criu/files.c
> index 01cd4c0e9..2c3389e3f 100644
> --- a/criu/files.c
> +++ b/criu/files.c
> @@ -14,6 +14,7 @@
>  #include <stdlib.h>
>  
>  #include "types.h"
> +#include "kerndat.h"
>  #include "files.h"
>  #include "file-ids.h"
>  #include "files-reg.h"
> diff --git a/criu/include/file-lock.h b/criu/include/file-lock.h
> index f70739adb..7280e0b55 100644
> --- a/criu/include/file-lock.h
> +++ b/criu/include/file-lock.h
> @@ -38,6 +38,9 @@
>  #define LOCK_WRITE	128	/* which allows concurrent write operations */
>  #define LOCK_RW		192	/* which allows concurrent read & write ops */
>  
> +/* for leases */
> +#define LEASE_BREAKING	4
> +
>  struct file_lock {
>  	long long	fl_id;
>  	int		fl_kind;
> diff --git a/criu/proc_parse.c b/criu/proc_parse.c
> index d3893272c..89be4ceb5 100644
> --- a/criu/proc_parse.c
> +++ b/criu/proc_parse.c
> @@ -1996,6 +1996,10 @@ static int parse_file_lock_buf(char *buf, struct file_lock *fl,
>  	else
>  		fl->fl_kind = FL_UNKNOWN;
>  
> +	if (fl->fl_kind == FL_LEASE && !strcmp(fl_type, "BREAKING")) {
> +		fl->fl_ltype |= LEASE_BREAKING;
> +	}
> +
>  	if (!strcmp(fl_type, "MSNFS")) {
>  		fl->fl_ltype |= LOCK_MAND;
>  
> @@ -2009,9 +2013,6 @@ static int parse_file_lock_buf(char *buf, struct file_lock *fl,
>  			pr_err("Unknown lock option!\n");
>  			return -1;
>  		}
> -	} else if (fl->fl_kind == FL_LEASE && !strcmp(fl_type, "BREAKING")) {
> -		pr_err("Breaking leases are not supported (%d): %s\n",
> -			num, buf);
>  	} else {
>  		if (!strcmp(fl_option, "UNLCK")) {
>  			fl->fl_ltype |= F_UNLCK;
>