link_remap: do not create excessive links for a single file

Submitted by Stanislav Kinsburskiy on June 30, 2016, 5:11 p.m.

Details

Message ID 20160630171050.537491.88088.stgit@skinsbursky-vz7-gold.qa.sw.ru
State Rejected
Series "link_remap: do not create excessive links for a single file"
Headers show

Commit Message

Stanislav Kinsburskiy June 30, 2016, 5:11 p.m.
Currently criu always creates link for each file, considered as "silly-renamed".
Even in those cases, when file is opened multiple times, it creates one new
link for each occurrence.
On restore only one link file is used for all the file descriptors, sharing
this path. And only this link is removed.
The rest of the links are forgotten and abandoned on file system.
This patch fixed this mess by creating the link only iff it's unique. Or
skipping this job otherwise.

Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
---
 criu/files-reg.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/files-reg.c b/criu/files-reg.c
index fd36ae9..b1ad271 100644
--- a/criu/files-reg.c
+++ b/criu/files-reg.c
@@ -70,9 +70,11 @@  struct link_remap_rlb {
 	struct list_head	list;
 	struct ns_id		*mnt_ns;
 	char			*path;
+	char			*orig;
+	u32			id;
 };
 
-static int note_link_remap(char *path, struct ns_id *nsid)
+static int note_link_remap(char *path, char *orig, struct ns_id *nsid, u32 id)
 {
 	struct link_remap_rlb *rlb;
 
@@ -84,11 +86,18 @@  static int note_link_remap(char *path, struct ns_id *nsid)
 	if (!rlb->path)
 		goto err2;
 
+	rlb->orig = strdup(orig);
+	if (!rlb->orig)
+		goto err3;
+
 	rlb->mnt_ns = nsid;
+	rlb->id = id;
 	list_add(&rlb->list, &remaps);
 
 	return 0;
 
+err3:
+	xfree(rlb->path);
 err2:
 	xfree(rlb);
 err:
@@ -96,6 +105,22 @@  err:
 	return -1;
 }
 
+static int find_link_remap(char *path, struct ns_id *nsid, u32 *id)
+{
+	struct link_remap_rlb *rlb;
+
+	list_for_each_entry(rlb, &remaps, list) {
+		if (rlb->mnt_ns != nsid)
+			continue;
+		if (strcmp(rlb->orig, path))
+			continue;
+
+		*id = rlb->id;
+		return 0;
+	}
+	return -ENOENT;
+}
+
 /* Trim "a/b/c/d" to "a/b/d" */
 static int trim_last_parent(char *path)
 {
@@ -725,6 +750,7 @@  static void __rollback_link_remaps(bool do_unlink)
 		}
 
 		list_del(&rlb->list);
+		xfree(rlb->orig);
 		xfree(rlb->path);
 		xfree(rlb);
 	}
@@ -781,28 +807,40 @@  static int create_link_remap(char *path, int len, int lfd,
 		return -1;
 	}
 
-	if (note_link_remap(link_name, nsid))
+	if (note_link_remap(link_name, path, nsid, *idp))
 		return -1;
 
 	return pb_write_one(img_from_set(glob_imgset, CR_FD_REG_FILES), &rfe, PB_REG_FILE);
 }
 
-static int dump_linked_remap(char *path, int len, const struct stat *ost,
-				int lfd, u32 id, struct ns_id *nsid)
+static int dump_linked_remap_type(char *path, int len, int lfd, u32 id, struct ns_id *nsid, RemapType remap_type, bool unique)
 {
 	u32 lid;
 	RemapFilePathEntry rpe = REMAP_FILE_PATH_ENTRY__INIT;
 
-	if (create_link_remap(path, len, lfd, &lid, nsid))
-		return -1;
+	if (!find_link_remap(path, nsid, &lid)) {
+		pr_debug("Link remap for %s already exists with id %x\n",
+				path, lid);
+		if (unique)
+			return 0;
+	} else if (create_link_remap(path, len, lfd, &lid, nsid))
+			return -1;
 
 	rpe.orig_id = id;
 	rpe.remap_id = lid;
+	rpe.has_remap_type = true;
+	rpe.remap_type = remap_type;
 
 	return pb_write_one(img_from_set(glob_imgset, CR_FD_REMAP_FPATH),
 			&rpe, PB_REMAP_FPATH);
 }
 
+static int dump_linked_remap(char *path, int len, const struct stat *ost,
+				int lfd, u32 id, struct ns_id *nsid)
+{
+	return dump_linked_remap_type(path, len, lfd, id, nsid, REMAP_TYPE__LINKED, false);
+}
+
 static pid_t *dead_pids;
 static int n_dead_pids;
 

Comments

Pavel Emelianov July 13, 2016, 12:48 p.m.
On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
> Currently criu always creates link for each file, considered as "silly-renamed".
> Even in those cases, when file is opened multiple times, it creates one new
> link for each occurrence.
> On restore only one link file is used for all the file descriptors, sharing
> this path. And only this link is removed.

Would you cook a zdtm test showing it?

> The rest of the links are forgotten and abandoned on file system.
> This patch fixed this mess by creating the link only iff it's unique. Or
> skipping this job otherwise.
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>  criu/files-reg.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/criu/files-reg.c b/criu/files-reg.c
> index fd36ae9..b1ad271 100644
> --- a/criu/files-reg.c
> +++ b/criu/files-reg.c
> @@ -70,9 +70,11 @@ struct link_remap_rlb {
>  	struct list_head	list;
>  	struct ns_id		*mnt_ns;
>  	char			*path;
> +	char			*orig;
> +	u32			id;
>  };
>  
> -static int note_link_remap(char *path, struct ns_id *nsid)
> +static int note_link_remap(char *path, char *orig, struct ns_id *nsid, u32 id)
>  {
>  	struct link_remap_rlb *rlb;
>  
> @@ -84,11 +86,18 @@ static int note_link_remap(char *path, struct ns_id *nsid)
>  	if (!rlb->path)
>  		goto err2;
>  
> +	rlb->orig = strdup(orig);
> +	if (!rlb->orig)
> +		goto err3;
> +
>  	rlb->mnt_ns = nsid;
> +	rlb->id = id;
>  	list_add(&rlb->list, &remaps);
>  
>  	return 0;
>  
> +err3:
> +	xfree(rlb->path);
>  err2:
>  	xfree(rlb);
>  err:
> @@ -96,6 +105,22 @@ err:
>  	return -1;
>  }
>  
> +static int find_link_remap(char *path, struct ns_id *nsid, u32 *id)
> +{
> +	struct link_remap_rlb *rlb;
> +
> +	list_for_each_entry(rlb, &remaps, list) {
> +		if (rlb->mnt_ns != nsid)
> +			continue;
> +		if (strcmp(rlb->orig, path))
> +			continue;
> +
> +		*id = rlb->id;
> +		return 0;
> +	}
> +	return -ENOENT;
> +}
> +
>  /* Trim "a/b/c/d" to "a/b/d" */
>  static int trim_last_parent(char *path)
>  {
> @@ -725,6 +750,7 @@ static void __rollback_link_remaps(bool do_unlink)
>  		}
>  
>  		list_del(&rlb->list);
> +		xfree(rlb->orig);
>  		xfree(rlb->path);
>  		xfree(rlb);
>  	}
> @@ -781,28 +807,40 @@ static int create_link_remap(char *path, int len, int lfd,
>  		return -1;
>  	}
>  
> -	if (note_link_remap(link_name, nsid))
> +	if (note_link_remap(link_name, path, nsid, *idp))
>  		return -1;
>  
>  	return pb_write_one(img_from_set(glob_imgset, CR_FD_REG_FILES), &rfe, PB_REG_FILE);
>  }
>  
> -static int dump_linked_remap(char *path, int len, const struct stat *ost,
> -				int lfd, u32 id, struct ns_id *nsid)
> +static int dump_linked_remap_type(char *path, int len, int lfd, u32 id, struct ns_id *nsid, RemapType remap_type, bool unique)
>  {
>  	u32 lid;
>  	RemapFilePathEntry rpe = REMAP_FILE_PATH_ENTRY__INIT;
>  
> -	if (create_link_remap(path, len, lfd, &lid, nsid))
> -		return -1;
> +	if (!find_link_remap(path, nsid, &lid)) {
> +		pr_debug("Link remap for %s already exists with id %x\n",
> +				path, lid);
> +		if (unique)
> +			return 0;
> +	} else if (create_link_remap(path, len, lfd, &lid, nsid))
> +			return -1;
>  
>  	rpe.orig_id = id;
>  	rpe.remap_id = lid;
> +	rpe.has_remap_type = true;
> +	rpe.remap_type = remap_type;
>  
>  	return pb_write_one(img_from_set(glob_imgset, CR_FD_REMAP_FPATH),
>  			&rpe, PB_REMAP_FPATH);
>  }
>  
> +static int dump_linked_remap(char *path, int len, const struct stat *ost,
> +				int lfd, u32 id, struct ns_id *nsid)
> +{
> +	return dump_linked_remap_type(path, len, lfd, id, nsid, REMAP_TYPE__LINKED, false);
> +}
> +
>  static pid_t *dead_pids;
>  static int n_dead_pids;
>  
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>
Stanislav Kinsburskiy July 13, 2016, 1:10 p.m.
13.07.2016 14:48, Pavel Emelyanov пишет:
> On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
>> Currently criu always creates link for each file, considered as "silly-renamed".
>> Even in those cases, when file is opened multiple times, it creates one new
>> link for each occurrence.
>> On restore only one link file is used for all the file descriptors, sharing
>> this path. And only this link is removed.
> Would you cook a zdtm test showing it?

Sorry, I don't understand. Showing what exactly?
I caught by using zdtm tests. Test "static/unlink_mmap00" should expose it.
Two links created, only one used and unlinked. Another one stays on the 
disk.

>> The rest of the links are forgotten and abandoned on file system.
>> This patch fixed this mess by creating the link only iff it's unique. Or
>> skipping this job otherwise.
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>> ---
>>   criu/files-reg.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/criu/files-reg.c b/criu/files-reg.c
>> index fd36ae9..b1ad271 100644
>> --- a/criu/files-reg.c
>> +++ b/criu/files-reg.c
>> @@ -70,9 +70,11 @@ struct link_remap_rlb {
>>   	struct list_head	list;
>>   	struct ns_id		*mnt_ns;
>>   	char			*path;
>> +	char			*orig;
>> +	u32			id;
>>   };
>>   
>> -static int note_link_remap(char *path, struct ns_id *nsid)
>> +static int note_link_remap(char *path, char *orig, struct ns_id *nsid, u32 id)
>>   {
>>   	struct link_remap_rlb *rlb;
>>   
>> @@ -84,11 +86,18 @@ static int note_link_remap(char *path, struct ns_id *nsid)
>>   	if (!rlb->path)
>>   		goto err2;
>>   
>> +	rlb->orig = strdup(orig);
>> +	if (!rlb->orig)
>> +		goto err3;
>> +
>>   	rlb->mnt_ns = nsid;
>> +	rlb->id = id;
>>   	list_add(&rlb->list, &remaps);
>>   
>>   	return 0;
>>   
>> +err3:
>> +	xfree(rlb->path);
>>   err2:
>>   	xfree(rlb);
>>   err:
>> @@ -96,6 +105,22 @@ err:
>>   	return -1;
>>   }
>>   
>> +static int find_link_remap(char *path, struct ns_id *nsid, u32 *id)
>> +{
>> +	struct link_remap_rlb *rlb;
>> +
>> +	list_for_each_entry(rlb, &remaps, list) {
>> +		if (rlb->mnt_ns != nsid)
>> +			continue;
>> +		if (strcmp(rlb->orig, path))
>> +			continue;
>> +
>> +		*id = rlb->id;
>> +		return 0;
>> +	}
>> +	return -ENOENT;
>> +}
>> +
>>   /* Trim "a/b/c/d" to "a/b/d" */
>>   static int trim_last_parent(char *path)
>>   {
>> @@ -725,6 +750,7 @@ static void __rollback_link_remaps(bool do_unlink)
>>   		}
>>   
>>   		list_del(&rlb->list);
>> +		xfree(rlb->orig);
>>   		xfree(rlb->path);
>>   		xfree(rlb);
>>   	}
>> @@ -781,28 +807,40 @@ static int create_link_remap(char *path, int len, int lfd,
>>   		return -1;
>>   	}
>>   
>> -	if (note_link_remap(link_name, nsid))
>> +	if (note_link_remap(link_name, path, nsid, *idp))
>>   		return -1;
>>   
>>   	return pb_write_one(img_from_set(glob_imgset, CR_FD_REG_FILES), &rfe, PB_REG_FILE);
>>   }
>>   
>> -static int dump_linked_remap(char *path, int len, const struct stat *ost,
>> -				int lfd, u32 id, struct ns_id *nsid)
>> +static int dump_linked_remap_type(char *path, int len, int lfd, u32 id, struct ns_id *nsid, RemapType remap_type, bool unique)
>>   {
>>   	u32 lid;
>>   	RemapFilePathEntry rpe = REMAP_FILE_PATH_ENTRY__INIT;
>>   
>> -	if (create_link_remap(path, len, lfd, &lid, nsid))
>> -		return -1;
>> +	if (!find_link_remap(path, nsid, &lid)) {
>> +		pr_debug("Link remap for %s already exists with id %x\n",
>> +				path, lid);
>> +		if (unique)
>> +			return 0;
>> +	} else if (create_link_remap(path, len, lfd, &lid, nsid))
>> +			return -1;
>>   
>>   	rpe.orig_id = id;
>>   	rpe.remap_id = lid;
>> +	rpe.has_remap_type = true;
>> +	rpe.remap_type = remap_type;
>>   
>>   	return pb_write_one(img_from_set(glob_imgset, CR_FD_REMAP_FPATH),
>>   			&rpe, PB_REMAP_FPATH);
>>   }
>>   
>> +static int dump_linked_remap(char *path, int len, const struct stat *ost,
>> +				int lfd, u32 id, struct ns_id *nsid)
>> +{
>> +	return dump_linked_remap_type(path, len, lfd, id, nsid, REMAP_TYPE__LINKED, false);
>> +}
>> +
>>   static pid_t *dead_pids;
>>   static int n_dead_pids;
>>   
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>> .
>>
Stanislav Kinsburskiy July 13, 2016, 2:29 p.m.
13.07.2016 16:30, Pavel Emelyanov пишет:
> On 07/13/2016 04:10 PM, Stanislav Kinsburskiy wrote:
>>
>> 13.07.2016 14:48, Pavel Emelyanov пишет:
>>> On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
>>>> Currently criu always creates link for each file, considered as "silly-renamed".
>>>> Even in those cases, when file is opened multiple times, it creates one new
>>>> link for each occurrence.
>>>> On restore only one link file is used for all the file descriptors, sharing
>>>> this path. And only this link is removed.
>>> Would you cook a zdtm test showing it?
>> Sorry, I don't understand. Showing what exactly?
>> I caught by using zdtm tests. Test "static/unlink_mmap00" should expose it.
>> Two links created, only one used and unlinked. Another one stays on the
>> disk.
> And test passes :) Then modify it (or it's cleanup hook) to catch this error.

Checking for no "link_remap*" files left suits your purpose?

>>>> The rest of the links are forgotten and abandoned on file system.
>>>> This patch fixed this mess by creating the link only iff it's unique. Or
>>>> skipping this job otherwise.
>>>>
>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>>>> ---
>>>>    criu/files-reg.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
>>>>    1 file changed, 44 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/criu/files-reg.c b/criu/files-reg.c
>>>> index fd36ae9..b1ad271 100644
>>>> --- a/criu/files-reg.c
>>>> +++ b/criu/files-reg.c
>>>> @@ -70,9 +70,11 @@ struct link_remap_rlb {
>>>>    	struct list_head	list;
>>>>    	struct ns_id		*mnt_ns;
>>>>    	char			*path;
>>>> +	char			*orig;
>>>> +	u32			id;
>>>>    };
>>>>    
>>>> -static int note_link_remap(char *path, struct ns_id *nsid)
>>>> +static int note_link_remap(char *path, char *orig, struct ns_id *nsid, u32 id)
>>>>    {
>>>>    	struct link_remap_rlb *rlb;
>>>>    
>>>> @@ -84,11 +86,18 @@ static int note_link_remap(char *path, struct ns_id *nsid)
>>>>    	if (!rlb->path)
>>>>    		goto err2;
>>>>    
>>>> +	rlb->orig = strdup(orig);
>>>> +	if (!rlb->orig)
>>>> +		goto err3;
>>>> +
>>>>    	rlb->mnt_ns = nsid;
>>>> +	rlb->id = id;
>>>>    	list_add(&rlb->list, &remaps);
>>>>    
>>>>    	return 0;
>>>>    
>>>> +err3:
>>>> +	xfree(rlb->path);
>>>>    err2:
>>>>    	xfree(rlb);
>>>>    err:
>>>> @@ -96,6 +105,22 @@ err:
>>>>    	return -1;
>>>>    }
>>>>    
>>>> +static int find_link_remap(char *path, struct ns_id *nsid, u32 *id)
>>>> +{
>>>> +	struct link_remap_rlb *rlb;
>>>> +
>>>> +	list_for_each_entry(rlb, &remaps, list) {
>>>> +		if (rlb->mnt_ns != nsid)
>>>> +			continue;
>>>> +		if (strcmp(rlb->orig, path))
>>>> +			continue;
>>>> +
>>>> +		*id = rlb->id;
>>>> +		return 0;
>>>> +	}
>>>> +	return -ENOENT;
>>>> +}
>>>> +
>>>>    /* Trim "a/b/c/d" to "a/b/d" */
>>>>    static int trim_last_parent(char *path)
>>>>    {
>>>> @@ -725,6 +750,7 @@ static void __rollback_link_remaps(bool do_unlink)
>>>>    		}
>>>>    
>>>>    		list_del(&rlb->list);
>>>> +		xfree(rlb->orig);
>>>>    		xfree(rlb->path);
>>>>    		xfree(rlb);
>>>>    	}
>>>> @@ -781,28 +807,40 @@ static int create_link_remap(char *path, int len, int lfd,
>>>>    		return -1;
>>>>    	}
>>>>    
>>>> -	if (note_link_remap(link_name, nsid))
>>>> +	if (note_link_remap(link_name, path, nsid, *idp))
>>>>    		return -1;
>>>>    
>>>>    	return pb_write_one(img_from_set(glob_imgset, CR_FD_REG_FILES), &rfe, PB_REG_FILE);
>>>>    }
>>>>    
>>>> -static int dump_linked_remap(char *path, int len, const struct stat *ost,
>>>> -				int lfd, u32 id, struct ns_id *nsid)
>>>> +static int dump_linked_remap_type(char *path, int len, int lfd, u32 id, struct ns_id *nsid, RemapType remap_type, bool unique)
>>>>    {
>>>>    	u32 lid;
>>>>    	RemapFilePathEntry rpe = REMAP_FILE_PATH_ENTRY__INIT;
>>>>    
>>>> -	if (create_link_remap(path, len, lfd, &lid, nsid))
>>>> -		return -1;
>>>> +	if (!find_link_remap(path, nsid, &lid)) {
>>>> +		pr_debug("Link remap for %s already exists with id %x\n",
>>>> +				path, lid);
>>>> +		if (unique)
>>>> +			return 0;
>>>> +	} else if (create_link_remap(path, len, lfd, &lid, nsid))
>>>> +			return -1;
>>>>    
>>>>    	rpe.orig_id = id;
>>>>    	rpe.remap_id = lid;
>>>> +	rpe.has_remap_type = true;
>>>> +	rpe.remap_type = remap_type;
>>>>    
>>>>    	return pb_write_one(img_from_set(glob_imgset, CR_FD_REMAP_FPATH),
>>>>    			&rpe, PB_REMAP_FPATH);
>>>>    }
>>>>    
>>>> +static int dump_linked_remap(char *path, int len, const struct stat *ost,
>>>> +				int lfd, u32 id, struct ns_id *nsid)
>>>> +{
>>>> +	return dump_linked_remap_type(path, len, lfd, id, nsid, REMAP_TYPE__LINKED, false);
>>>> +}
>>>> +
>>>>    static pid_t *dead_pids;
>>>>    static int n_dead_pids;
>>>>    
>>>>
>>>> _______________________________________________
>>>> CRIU mailing list
>>>> CRIU@openvz.org
>>>> https://lists.openvz.org/mailman/listinfo/criu
>>>> .
>>>>
>> .
>>
Pavel Emelianov July 13, 2016, 2:30 p.m.
On 07/13/2016 04:10 PM, Stanislav Kinsburskiy wrote:
> 
> 
> 13.07.2016 14:48, Pavel Emelyanov пишет:
>> On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
>>> Currently criu always creates link for each file, considered as "silly-renamed".
>>> Even in those cases, when file is opened multiple times, it creates one new
>>> link for each occurrence.
>>> On restore only one link file is used for all the file descriptors, sharing
>>> this path. And only this link is removed.
>> Would you cook a zdtm test showing it?
> 
> Sorry, I don't understand. Showing what exactly?
> I caught by using zdtm tests. Test "static/unlink_mmap00" should expose it.
> Two links created, only one used and unlinked. Another one stays on the 
> disk.

And test passes :) Then modify it (or it's cleanup hook) to catch this error.

>>> The rest of the links are forgotten and abandoned on file system.
>>> This patch fixed this mess by creating the link only iff it's unique. Or
>>> skipping this job otherwise.
>>>
>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>>> ---
>>>   criu/files-reg.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
>>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/criu/files-reg.c b/criu/files-reg.c
>>> index fd36ae9..b1ad271 100644
>>> --- a/criu/files-reg.c
>>> +++ b/criu/files-reg.c
>>> @@ -70,9 +70,11 @@ struct link_remap_rlb {
>>>   	struct list_head	list;
>>>   	struct ns_id		*mnt_ns;
>>>   	char			*path;
>>> +	char			*orig;
>>> +	u32			id;
>>>   };
>>>   
>>> -static int note_link_remap(char *path, struct ns_id *nsid)
>>> +static int note_link_remap(char *path, char *orig, struct ns_id *nsid, u32 id)
>>>   {
>>>   	struct link_remap_rlb *rlb;
>>>   
>>> @@ -84,11 +86,18 @@ static int note_link_remap(char *path, struct ns_id *nsid)
>>>   	if (!rlb->path)
>>>   		goto err2;
>>>   
>>> +	rlb->orig = strdup(orig);
>>> +	if (!rlb->orig)
>>> +		goto err3;
>>> +
>>>   	rlb->mnt_ns = nsid;
>>> +	rlb->id = id;
>>>   	list_add(&rlb->list, &remaps);
>>>   
>>>   	return 0;
>>>   
>>> +err3:
>>> +	xfree(rlb->path);
>>>   err2:
>>>   	xfree(rlb);
>>>   err:
>>> @@ -96,6 +105,22 @@ err:
>>>   	return -1;
>>>   }
>>>   
>>> +static int find_link_remap(char *path, struct ns_id *nsid, u32 *id)
>>> +{
>>> +	struct link_remap_rlb *rlb;
>>> +
>>> +	list_for_each_entry(rlb, &remaps, list) {
>>> +		if (rlb->mnt_ns != nsid)
>>> +			continue;
>>> +		if (strcmp(rlb->orig, path))
>>> +			continue;
>>> +
>>> +		*id = rlb->id;
>>> +		return 0;
>>> +	}
>>> +	return -ENOENT;
>>> +}
>>> +
>>>   /* Trim "a/b/c/d" to "a/b/d" */
>>>   static int trim_last_parent(char *path)
>>>   {
>>> @@ -725,6 +750,7 @@ static void __rollback_link_remaps(bool do_unlink)
>>>   		}
>>>   
>>>   		list_del(&rlb->list);
>>> +		xfree(rlb->orig);
>>>   		xfree(rlb->path);
>>>   		xfree(rlb);
>>>   	}
>>> @@ -781,28 +807,40 @@ static int create_link_remap(char *path, int len, int lfd,
>>>   		return -1;
>>>   	}
>>>   
>>> -	if (note_link_remap(link_name, nsid))
>>> +	if (note_link_remap(link_name, path, nsid, *idp))
>>>   		return -1;
>>>   
>>>   	return pb_write_one(img_from_set(glob_imgset, CR_FD_REG_FILES), &rfe, PB_REG_FILE);
>>>   }
>>>   
>>> -static int dump_linked_remap(char *path, int len, const struct stat *ost,
>>> -				int lfd, u32 id, struct ns_id *nsid)
>>> +static int dump_linked_remap_type(char *path, int len, int lfd, u32 id, struct ns_id *nsid, RemapType remap_type, bool unique)
>>>   {
>>>   	u32 lid;
>>>   	RemapFilePathEntry rpe = REMAP_FILE_PATH_ENTRY__INIT;
>>>   
>>> -	if (create_link_remap(path, len, lfd, &lid, nsid))
>>> -		return -1;
>>> +	if (!find_link_remap(path, nsid, &lid)) {
>>> +		pr_debug("Link remap for %s already exists with id %x\n",
>>> +				path, lid);
>>> +		if (unique)
>>> +			return 0;
>>> +	} else if (create_link_remap(path, len, lfd, &lid, nsid))
>>> +			return -1;
>>>   
>>>   	rpe.orig_id = id;
>>>   	rpe.remap_id = lid;
>>> +	rpe.has_remap_type = true;
>>> +	rpe.remap_type = remap_type;
>>>   
>>>   	return pb_write_one(img_from_set(glob_imgset, CR_FD_REMAP_FPATH),
>>>   			&rpe, PB_REMAP_FPATH);
>>>   }
>>>   
>>> +static int dump_linked_remap(char *path, int len, const struct stat *ost,
>>> +				int lfd, u32 id, struct ns_id *nsid)
>>> +{
>>> +	return dump_linked_remap_type(path, len, lfd, id, nsid, REMAP_TYPE__LINKED, false);
>>> +}
>>> +
>>>   static pid_t *dead_pids;
>>>   static int n_dead_pids;
>>>   
>>>
>>> _______________________________________________
>>> CRIU mailing list
>>> CRIU@openvz.org
>>> https://lists.openvz.org/mailman/listinfo/criu
>>> .
>>>
> 
> .
>
Pavel Emelianov July 14, 2016, 10:56 a.m.
On 07/13/2016 05:29 PM, Stanislav Kinsburskiy wrote:
> 
> 
> 13.07.2016 16:30, Pavel Emelyanov пишет:
>> On 07/13/2016 04:10 PM, Stanislav Kinsburskiy wrote:
>>>
>>> 13.07.2016 14:48, Pavel Emelyanov пишет:
>>>> On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
>>>>> Currently criu always creates link for each file, considered as "silly-renamed".
>>>>> Even in those cases, when file is opened multiple times, it creates one new
>>>>> link for each occurrence.
>>>>> On restore only one link file is used for all the file descriptors, sharing
>>>>> this path. And only this link is removed.
>>>> Would you cook a zdtm test showing it?
>>> Sorry, I don't understand. Showing what exactly?
>>> I caught by using zdtm tests. Test "static/unlink_mmap00" should expose it.
>>> Two links created, only one used and unlinked. Another one stays on the
>>> disk.
>> And test passes :) Then modify it (or it's cleanup hook) to catch this error.
> 
> Checking for no "link_remap*" files left suits your purpose?

Yup :) IIRC we already have some test hook doing checks for ghost/remaps files
hanging around.

>>>>> The rest of the links are forgotten and abandoned on file system.
>>>>> This patch fixed this mess by creating the link only iff it's unique. Or
>>>>> skipping this job otherwise.
>>>>>
>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>>>>> ---
>>>>>    criu/files-reg.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
>>>>>    1 file changed, 44 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/criu/files-reg.c b/criu/files-reg.c
>>>>> index fd36ae9..b1ad271 100644
>>>>> --- a/criu/files-reg.c
>>>>> +++ b/criu/files-reg.c
>>>>> @@ -70,9 +70,11 @@ struct link_remap_rlb {
>>>>>    	struct list_head	list;
>>>>>    	struct ns_id		*mnt_ns;
>>>>>    	char			*path;
>>>>> +	char			*orig;
>>>>> +	u32			id;
>>>>>    };
>>>>>    
>>>>> -static int note_link_remap(char *path, struct ns_id *nsid)
>>>>> +static int note_link_remap(char *path, char *orig, struct ns_id *nsid, u32 id)
>>>>>    {
>>>>>    	struct link_remap_rlb *rlb;
>>>>>    
>>>>> @@ -84,11 +86,18 @@ static int note_link_remap(char *path, struct ns_id *nsid)
>>>>>    	if (!rlb->path)
>>>>>    		goto err2;
>>>>>    
>>>>> +	rlb->orig = strdup(orig);
>>>>> +	if (!rlb->orig)
>>>>> +		goto err3;
>>>>> +
>>>>>    	rlb->mnt_ns = nsid;
>>>>> +	rlb->id = id;
>>>>>    	list_add(&rlb->list, &remaps);
>>>>>    
>>>>>    	return 0;
>>>>>    
>>>>> +err3:
>>>>> +	xfree(rlb->path);
>>>>>    err2:
>>>>>    	xfree(rlb);
>>>>>    err:
>>>>> @@ -96,6 +105,22 @@ err:
>>>>>    	return -1;
>>>>>    }
>>>>>    
>>>>> +static int find_link_remap(char *path, struct ns_id *nsid, u32 *id)
>>>>> +{
>>>>> +	struct link_remap_rlb *rlb;
>>>>> +
>>>>> +	list_for_each_entry(rlb, &remaps, list) {
>>>>> +		if (rlb->mnt_ns != nsid)
>>>>> +			continue;
>>>>> +		if (strcmp(rlb->orig, path))
>>>>> +			continue;
>>>>> +
>>>>> +		*id = rlb->id;
>>>>> +		return 0;
>>>>> +	}
>>>>> +	return -ENOENT;
>>>>> +}
>>>>> +
>>>>>    /* Trim "a/b/c/d" to "a/b/d" */
>>>>>    static int trim_last_parent(char *path)
>>>>>    {
>>>>> @@ -725,6 +750,7 @@ static void __rollback_link_remaps(bool do_unlink)
>>>>>    		}
>>>>>    
>>>>>    		list_del(&rlb->list);
>>>>> +		xfree(rlb->orig);
>>>>>    		xfree(rlb->path);
>>>>>    		xfree(rlb);
>>>>>    	}
>>>>> @@ -781,28 +807,40 @@ static int create_link_remap(char *path, int len, int lfd,
>>>>>    		return -1;
>>>>>    	}
>>>>>    
>>>>> -	if (note_link_remap(link_name, nsid))
>>>>> +	if (note_link_remap(link_name, path, nsid, *idp))
>>>>>    		return -1;
>>>>>    
>>>>>    	return pb_write_one(img_from_set(glob_imgset, CR_FD_REG_FILES), &rfe, PB_REG_FILE);
>>>>>    }
>>>>>    
>>>>> -static int dump_linked_remap(char *path, int len, const struct stat *ost,
>>>>> -				int lfd, u32 id, struct ns_id *nsid)
>>>>> +static int dump_linked_remap_type(char *path, int len, int lfd, u32 id, struct ns_id *nsid, RemapType remap_type, bool unique)
>>>>>    {
>>>>>    	u32 lid;
>>>>>    	RemapFilePathEntry rpe = REMAP_FILE_PATH_ENTRY__INIT;
>>>>>    
>>>>> -	if (create_link_remap(path, len, lfd, &lid, nsid))
>>>>> -		return -1;
>>>>> +	if (!find_link_remap(path, nsid, &lid)) {
>>>>> +		pr_debug("Link remap for %s already exists with id %x\n",
>>>>> +				path, lid);
>>>>> +		if (unique)
>>>>> +			return 0;
>>>>> +	} else if (create_link_remap(path, len, lfd, &lid, nsid))
>>>>> +			return -1;
>>>>>    
>>>>>    	rpe.orig_id = id;
>>>>>    	rpe.remap_id = lid;
>>>>> +	rpe.has_remap_type = true;
>>>>> +	rpe.remap_type = remap_type;
>>>>>    
>>>>>    	return pb_write_one(img_from_set(glob_imgset, CR_FD_REMAP_FPATH),
>>>>>    			&rpe, PB_REMAP_FPATH);
>>>>>    }
>>>>>    
>>>>> +static int dump_linked_remap(char *path, int len, const struct stat *ost,
>>>>> +				int lfd, u32 id, struct ns_id *nsid)
>>>>> +{
>>>>> +	return dump_linked_remap_type(path, len, lfd, id, nsid, REMAP_TYPE__LINKED, false);
>>>>> +}
>>>>> +
>>>>>    static pid_t *dead_pids;
>>>>>    static int n_dead_pids;
>>>>>    
>>>>>
>>>>> _______________________________________________
>>>>> CRIU mailing list
>>>>> CRIU@openvz.org
>>>>> https://lists.openvz.org/mailman/listinfo/criu
>>>>> .
>>>>>
>>> .
>>>
> 
> .
>
Stanislav Kinsburskiy July 27, 2016, 4:33 p.m.
14.07.2016 12:56, Pavel Emelyanov пишет:
> On 07/13/2016 05:29 PM, Stanislav Kinsburskiy wrote:
>>
>> 13.07.2016 16:30, Pavel Emelyanov пишет:
>>> On 07/13/2016 04:10 PM, Stanislav Kinsburskiy wrote:
>>>> 13.07.2016 14:48, Pavel Emelyanov пишет:
>>>>> On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
>>>>>> Currently criu always creates link for each file, considered as "silly-renamed".
>>>>>> Even in those cases, when file is opened multiple times, it creates one new
>>>>>> link for each occurrence.
>>>>>> On restore only one link file is used for all the file descriptors, sharing
>>>>>> this path. And only this link is removed.
>>>>> Would you cook a zdtm test showing it?
>>>> Sorry, I don't understand. Showing what exactly?
>>>> I caught by using zdtm tests. Test "static/unlink_mmap00" should expose it.
>>>> Two links created, only one used and unlinked. Another one stays on the
>>>> disk.
>>> And test passes :) Then modify it (or it's cleanup hook) to catch this error.
>> Checking for no "link_remap*" files left suits your purpose?
> Yup :) IIRC we already have some test hook doing checks for ghost/remaps files
> hanging around.
>

Looks like this patch is obsolete with current criu dev in terms of 
abandoned link files left after restore.
Now criu uses all of them as expected (the dumped one is used for 
restore and then unlinked).
Thus this patch is nothing more that optimization (create only one link 
instead of many).
Are you still willing to take it in this circumstances?
Pavel Emelianov July 29, 2016, 9:05 a.m.
On 07/27/2016 07:33 PM, Stanislav Kinsburskiy wrote:
> 
> 
> 14.07.2016 12:56, Pavel Emelyanov пишет:
>> On 07/13/2016 05:29 PM, Stanislav Kinsburskiy wrote:
>>>
>>> 13.07.2016 16:30, Pavel Emelyanov пишет:
>>>> On 07/13/2016 04:10 PM, Stanislav Kinsburskiy wrote:
>>>>> 13.07.2016 14:48, Pavel Emelyanov пишет:
>>>>>> On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
>>>>>>> Currently criu always creates link for each file, considered as "silly-renamed".
>>>>>>> Even in those cases, when file is opened multiple times, it creates one new
>>>>>>> link for each occurrence.
>>>>>>> On restore only one link file is used for all the file descriptors, sharing
>>>>>>> this path. And only this link is removed.
>>>>>> Would you cook a zdtm test showing it?
>>>>> Sorry, I don't understand. Showing what exactly?
>>>>> I caught by using zdtm tests. Test "static/unlink_mmap00" should expose it.
>>>>> Two links created, only one used and unlinked. Another one stays on the
>>>>> disk.
>>>> And test passes :) Then modify it (or it's cleanup hook) to catch this error.
>>> Checking for no "link_remap*" files left suits your purpose?
>> Yup :) IIRC we already have some test hook doing checks for ghost/remaps files
>> hanging around.
>>
> 
> Looks like this patch is obsolete with current criu dev in terms of 
> abandoned link files left after restore.
> Now criu uses all of them as expected (the dumped one is used for 
> restore and then unlinked).
> Thus this patch is nothing more that optimization (create only one link 
> instead of many).
> Are you still willing to take it in this circumstances?

Will it let us use less system calls to dump anything? If yes, then I will, but
it looks like Evgeny Batalov's patch with criu gc will interfere with this one.

-- Pavel
Stanislav Kinsburskiy July 29, 2016, 9:34 a.m.
29.07.2016 11:05, Pavel Emelyanov пишет:
> On 07/27/2016 07:33 PM, Stanislav Kinsburskiy wrote:
>>
>> 14.07.2016 12:56, Pavel Emelyanov пишет:
>>> On 07/13/2016 05:29 PM, Stanislav Kinsburskiy wrote:
>>>> 13.07.2016 16:30, Pavel Emelyanov пишет:
>>>>> On 07/13/2016 04:10 PM, Stanislav Kinsburskiy wrote:
>>>>>> 13.07.2016 14:48, Pavel Emelyanov пишет:
>>>>>>> On 06/30/2016 08:11 PM, Stanislav Kinsburskiy wrote:
>>>>>>>> Currently criu always creates link for each file, considered as "silly-renamed".
>>>>>>>> Even in those cases, when file is opened multiple times, it creates one new
>>>>>>>> link for each occurrence.
>>>>>>>> On restore only one link file is used for all the file descriptors, sharing
>>>>>>>> this path. And only this link is removed.
>>>>>>> Would you cook a zdtm test showing it?
>>>>>> Sorry, I don't understand. Showing what exactly?
>>>>>> I caught by using zdtm tests. Test "static/unlink_mmap00" should expose it.
>>>>>> Two links created, only one used and unlinked. Another one stays on the
>>>>>> disk.
>>>>> And test passes :) Then modify it (or it's cleanup hook) to catch this error.
>>>> Checking for no "link_remap*" files left suits your purpose?
>>> Yup :) IIRC we already have some test hook doing checks for ghost/remaps files
>>> hanging around.
>>>
>> Looks like this patch is obsolete with current criu dev in terms of
>> abandoned link files left after restore.
>> Now criu uses all of them as expected (the dumped one is used for
>> restore and then unlinked).
>> Thus this patch is nothing more that optimization (create only one link
>> instead of many).
>> Are you still willing to take it in this circumstances?
> Will it let us use less system calls to dump anything? If yes, then I will, but
> it looks like Evgeny Batalov's patch with criu gc will interfere with this one.

It will.
I can rebase this patch, if you satisfied with the one checking for link 
remap files in zdtm.py.

> -- Pavel
>