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

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

Details

Message ID 20170813234941.26329-2-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.
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.

Signed-off-by: Pavel Begunkov <asml.silence@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(-)

Patch hide | download patch | download mbox

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)
+{
+	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;
+}
+
+static int restore_file_lease(FileLockEntry *fle)
+{
+	int ret;
+
+	if (fle->type & LEASE_BREAKING) {
+		return restore_breaking_file_lease(fle);
+	} 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;

Comments

Pavel Emelyanov Aug. 15, 2017, 2:46 p.m.
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@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;
>
Pavel Begunkov (Silence) Aug. 16, 2017, 12:08 p.m.
On 15/08/17 17:46, Pavel Emelyanov wrote:
> 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?
Ok. CRIU restores files at first and file locks after, therefore it will
not break lease automatically. Also, failed non-block open may break lease.

> 
> More comments inline.
> 
>> Signed-off-by: Pavel Begunkov <asml.silence@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
They are almost identical. Function in files.c is using the list
ordering to optimize search, another one is not. We need this, because
ordering will be broken right after restoring of files. Yes, it's a bodge.
Could you say, what to do about it? Maintenance of the ordering after
file restoring is too messy.


> 
>> +{
>> +	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?

If I got it right, it didn't leave lease in "generated" state, but
changes it's state with a open call after.

TL;DR. It is done in 2 steps:
1. Create lease in this "generated" state, i.e. the state before the
break. We can deduce type by file flags.
2. Break lease (open call). It changes behaviour of fcntl(F_GETLEASE),
which will be returning 'target lease type' after. This one should match
with fle->type, that is done by manipulating flags of mentioned open call.


> 
>> +	} 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;
>>
>