Re: [PATCH] [RFC] Checkpoint/restart eventfd

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Matt Helsley wrote:
> On Sun, Oct 25, 2009 at 02:07:00PM -0400, Oren Laadan wrote:
>>
>> Matt Helsley wrote:
>>> Save/restore eventfd files. These are anon_inodes just like epoll
>>> but instead of a set of files to poll they are a 64-bit counter
>>> and a flag value. Used for AIO.
>>>
>>> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>> Looks fine to me, except a nit below. Unless there are negative
>> comments I'll pull it in a couple of days (and fix the nits).
>>
>> Oren.
>>
>>> NOTE: Marked [RFC] because it strangely does not pass my adapted LTP
>>> 	test cases unless it's running from a checkpointed image.
>>> 	Seems to be a mistake in the test case adaptation.
>>> ---
>>>  checkpoint/files.c             |    7 +++++
>>>  fs/eventfd.c                   |   51 ++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/checkpoint_hdr.h |    8 ++++++
>>>  include/linux/eventfd.h        |   10 ++++++++
>>>  4 files changed, 76 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/checkpoint/files.c b/checkpoint/files.c
>>> index f6de07e..43b95cc 100644
>>> --- a/checkpoint/files.c
>>> +++ b/checkpoint/files.c
>>> @@ -23,6 +23,7 @@
>>>  #include <linux/checkpoint.h>
>>>  #include <linux/checkpoint_hdr.h>
>>>  #include <net/sock.h>
>>> +#include <linux/eventfd.h>
>>>  
>>>  
>>>  /**************************************************************************
>>> @@ -607,6 +608,12 @@ static struct restore_file_ops restore_file_ops[] = {
>>>  		.file_type = CKPT_FILE_TTY,
>>>  		.restore = tty_file_restore,
>>>  	},
>>> +	/* eventfd */
>>> +	{
>>> +		.file_name = "EVENTFD",
>>> +		.file_type = CKPT_FILE_EVENTFD,
>>> +		.restore = eventfd_restore,
>>> +	},
>>>  };
>>>  
>>>  static struct file *do_restore_file(struct ckpt_ctx *ctx)
>>> diff --git a/fs/eventfd.c b/fs/eventfd.c
>>> index 31d12de..5d30cd5 100644
>>> --- a/fs/eventfd.c
>>> +++ b/fs/eventfd.c
>>> @@ -18,6 +18,8 @@
>>>  #include <linux/module.h>
>>>  #include <linux/kref.h>
>>>  #include <linux/eventfd.h>
>>> +#include <linux/checkpoint.h>
>>> +#include <linux/checkpoint_hdr.h>
>>>  
>>>  struct eventfd_ctx {
>>>  	struct kref kref;
>>> @@ -223,11 +225,34 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c
>>>  	return res;
>>>  }
>>>  
>>> +static int eventfd_checkpoint(struct ckpt_ctx *ckpt_ctx, struct file *file)
>>> +{
>>> +	struct eventfd_ctx *ctx;
>> Nit: everywhere else we use @ctx for ckpt_ctx, so to avoid
>> confusion, I suggest:
>> 	struct eventfd_ctx *efd_ctx;
> 
> No. The code in that file usually refers to "ctx" as an eventfd context. It
> seems wrong to adopt a contradictory naming convention just for the
> checkpoint portions of that code. I'd be happy to rename ckpt_ctx but
> not to ctx.

Ok, pulled as is.

Oren.