c/r support on FUSE filesystems.

Matt Helsley matthltc at us.ibm.com
Fri Apr 1 15:00:21 PDT 2011


On Fri, Apr 01, 2011 at 11:30:24AM +0530, Vishnumurthy Prabhu wrote:
> hi,
> I just worked with implementation of of CR support for FUSE filesystems. I
> have attached two files here that support FUSE-cr. fuse_kcr.h/c belongs to
> fs/fuse dir. What I found is that similar approach as that of generic file
> checkpoint can be used in fuse also and it works fine. In fact I did
> separate implementation just because if there is any need for extra
> functionalities, we can add that in these, so I kept it separate.  I want
> know any problems or issues in this implementation.

This is definitely something we're interested in seeing.

My guess is since there are a lot of "Only in " lines that you didn't do the
manual diff correctly. That's making this posting hard to review.

We use the LKML patch review/submission process here too. Please have a look
at what LKML patch submissions are supposed to look like. At least read
Documentation/SubmittingPatches and Documentation/CodingStyle.

It seems like you generated the patch by hand and then concatenated some
source files by hand without mentioning their paths/names. I don't think
any tools will apply this patch correctly (most people use tools
which won't recognize this). Please consider using patch-generating tools
like git, mercurial, or quilt for example. git and mercurial have other big
advantages too -- you won't have to keep a mental list of modified files
or you won't have to wait a long time for diff to compare two huge kernel
source trees.

Can you tell us about the testing you've done with this? What kind of
additions/changes does this make to the FUSE userspace API? Can you
please describe how it's supposed to work with FUSE userspace code?

Thanks!

> 
> 
> Thank you
> regards,
> Vishnumurthy P

> Only in linux-cr: .config
> Only in linux-cr/drivers/gpu/drm/radeon: r100_reg_safe.h
> Only in linux-cr/drivers/gpu/drm/radeon: r200_reg_safe.h
> Only in linux-cr/drivers/gpu/drm/radeon: r300_reg_safe.h
> Only in linux-cr/drivers/gpu/drm/radeon: r420_reg_safe.h
> Only in linux-cr/drivers/gpu/drm/radeon: r600_reg_safe.h
> Only in linux-cr/drivers/gpu/drm/radeon: rn50_reg_safe.h
> Only in linux-cr/drivers/gpu/drm/radeon: rs600_reg_safe.h
> Only in linux-cr/drivers/gpu/drm/radeon: rv515_reg_safe.h

For example, I can't imagine that you modified these files when you
wrote FUSE c/r support. I suspect you're diff'ing two different kernel
versions.

> diff -crB linux-cr1/fs/checkpoint.c linux-cr/fs/checkpoint.c

If you continue diff'ing by hand could you please make it a unified context
diff (-u) with functions (-p) instead of a plain context diff (-c)?

> *** linux-cr1/fs/checkpoint.c	2010-09-14 06:59:05.026511080 +0530
> --- linux-cr/fs/checkpoint.c	2011-03-30 21:14:15.260306880 +0530
> ***************
> *** 711,716 ****
> --- 711,718 ----
>   				  struct ckpt_hdr_file *ptr);
>   };
>   
> + extern struct file *fuse_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr);
> + 
>   static struct restore_file_ops restore_file_ops[] = {
>   	/* ignored file */
>   	{
> ***************
> *** 760,765 ****
> --- 762,773 ----
>   		.file_type = CKPT_FILE_EVENTFD,
>   		.restore = eventfd_restore,
>   	},
> + 	/*FUSE filesystem .It needs special handling*/
> + 	{
> + 		.file_name = "FUSE",
> + 		.file_type = CKPT_FILE_FUSE,
> + 		.restore = fuse_file_restore,
> + 	},	
>   };
>   
>   static void *restore_file(struct ckpt_ctx *ctx)
> diff -crB linux-cr1/fs/fuse/dev.c linux-cr/fs/fuse/dev.c
> *** linux-cr1/fs/fuse/dev.c	2010-09-14 06:59:05.114511787 +0530
> --- linux-cr/fs/fuse/dev.c	2011-03-28 21:59:53.786456000 +0530
> ***************
> *** 35,41 ****
>   	memset(req, 0, sizeof(*req));
>   	INIT_LIST_HEAD(&req->list);
>   	INIT_LIST_HEAD(&req->intr_entry);
> ! 	init_waitqueue_head(&req->waitq);
>   	atomic_set(&req->count, 1);
>   }
>   
> --- 34,41 ----
>   	memset(req, 0, sizeof(*req));
>   	INIT_LIST_HEAD(&req->list);
>   	INIT_LIST_HEAD(&req->intr_entry);
> ! 	init_waitqueue_head(&req->waitq); //used to initialize a wait queue head variable that was allocated dynamically

Kernel folks tend to prefer seeing old-style C comments and not C++.
You might run a corrected patch through scripts/checkpatch.pl to see if there
are any other style points to address -- I'm not going to call them out
right now.

> ! 
>   	atomic_set(&req->count, 1);
>   }
>   
> ***************
> *** 102,108 ****
>   
>   	atomic_inc(&fc->num_waiting);
>   	block_sigs(&oldset);
> ! 	intr = wait_event_interruptible(fc->blocked_waitq, !fc->blocked);
>   	restore_sigs(&oldset);
>   	err = -EINTR;
>   	if (intr)
> --- 102,108 ----
>   
>   	atomic_inc(&fc->num_waiting);
>   	block_sigs(&oldset);
> ! 	intr = wait_event_interruptible(fc->blocked_waitq, !fc->blocked);  //waits on fc->blocked_waitq until fc->blocked becomes 0.

I don't think these comments are relevant to the patch...

>   	restore_sigs(&oldset);
>   	err = -EINTR;
>   	if (intr)
> diff -crB linux-cr1/fs/fuse/dir.c linux-cr/fs/fuse/dir.c
> *** linux-cr1/fs/fuse/dir.c	2010-09-14 06:59:05.114511787 +0530
> --- linux-cr/fs/fuse/dir.c	2011-03-28 21:59:53.802473000 +0530
> ***************
> *** 1576,1581 ****
> --- 1576,1584 ----
>   	.open		= fuse_dir_open,
>   	.release	= fuse_dir_release,
>   	.fsync		= fuse_dir_fsync,
> +  //#ifdef CONFIG_CHECKPOINT
> + 	//.checkpoint	= fuse_dir_checkpoint,
> +  //#endif
>   };
>   
>   static const struct inode_operations fuse_common_inode_operations = {
> diff -crB linux-cr1/fs/fuse/file.c linux-cr/fs/fuse/file.c
> *** linux-cr1/fs/fuse/file.c	2010-09-14 06:59:05.114511787 +0530
> --- linux-cr/fs/fuse/file.c	2011-03-28 21:59:53.737442000 +0530
> ***************
> *** 7,12 ****
> --- 7,13 ----
>   */
>   
>   #include "fuse_i.h"
> + #include "fuse_kcr.h"
>   
>   #include <linux/pagemap.h>
>   #include <linux/slab.h>
> ***************
> *** 1991,1996 ****
> --- 1992,2000 ----
>   	.unlocked_ioctl	= fuse_file_ioctl,
>   	.compat_ioctl	= fuse_file_compat_ioctl,
>   	.poll		= fuse_file_poll,
> + //#ifdef CONFIG_CHECKPOINT
> + 	.checkpoint	= fuse_file_checkpoint,
> + //#endif

Why are these cpp lines commented out?

>   };
>   
>   static const struct file_operations fuse_direct_io_file_operations = {
> ***************
> *** 2007,2012 ****
> --- 2011,2019 ----
>   	.unlocked_ioctl	= fuse_file_ioctl,
>   	.compat_ioctl	= fuse_file_compat_ioctl,
>   	.poll		= fuse_file_poll,
> + //#ifdef CONFIG_CHECKPOINT
> + 	.checkpoint	= fuse_file_checkpoint,
> + //#endif
>   	/* no splice_read */
>   };
>   
> Only in linux-cr/fs/fuse: fuse_kcr.c
> Only in linux-cr/fs/fuse: fuse_kcr.h

This is the core change introduced by your patchset and it looks like
it's missing...

> diff -crB linux-cr1/fs/fuse/inode.c linux-cr/fs/fuse/inode.c
> *** linux-cr1/fs/fuse/inode.c	2010-09-14 06:59:05.118512015 +0530
> --- linux-cr/fs/fuse/inode.c	2011-03-28 21:59:53.805496000 +0530
> ***************
> *** 1200,1206 ****
>   
>   	printk(KERN_INFO "fuse init (API version %i.%i)\n",
>   	       FUSE_KERNEL_VERSION, FUSE_KERNEL_MINOR_VERSION);
> ! 
>   	INIT_LIST_HEAD(&fuse_conn_list);
>   	res = fuse_fs_init();
>   	if (res)
> --- 1200,1208 ----
>   
>   	printk(KERN_INFO "fuse init (API version %i.%i)\n",
>   	       FUSE_KERNEL_VERSION, FUSE_KERNEL_MINOR_VERSION);
> !     printk("#######################################\n");
> !     printk("#FUSE kernel module is initializing...#\n");
> !     printk("#######################################\n");

Remove

>   	INIT_LIST_HEAD(&fuse_conn_list);
>   	res = fuse_fs_init();
>   	if (res)
> ***************
> *** 1235,1240 ****
> --- 1237,1245 ----
>   
>   static void __exit fuse_exit(void)
>   {
> +     printk("##################################\n");
> +     printk("#FUSE kernel module is exiting...#\n");
> +     printk("##################################\n");

Remove

>   	printk(KERN_DEBUG "fuse exit\n");
>   
>   	fuse_ctl_cleanup();
> diff -crB linux-cr1/fs/fuse/Makefile linux-cr/fs/fuse/Makefile
> *** linux-cr1/fs/fuse/Makefile	2010-09-14 06:59:05.114511787 +0530
> --- linux-cr/fs/fuse/Makefile	2011-03-22 20:23:18.427370000 +0530
> ***************
> *** 5,8 ****
>   obj-$(CONFIG_FUSE_FS) += fuse.o
>   obj-$(CONFIG_CUSE) += cuse.o
>   
> ! fuse-objs := dev.o dir.o file.o inode.o control.o
> --- 5,8 ----
>   obj-$(CONFIG_FUSE_FS) += fuse.o
>   obj-$(CONFIG_CUSE) += cuse.o
>   
> ! fuse-objs := dev.o dir.o file.o inode.o control.o fuse_kcr.o
> Only in linux-cr/include: config
> Only in linux-cr/include: generated
> diff -crB linux-cr1/include/linux/checkpoint_hdr.h linux-cr/include/linux/checkpoint_hdr.h
> *** linux-cr1/include/linux/checkpoint_hdr.h	2010-09-14 06:59:05.515511078 +0530
> --- linux-cr/include/linux/checkpoint_hdr.h	2011-03-28 21:59:53.913441000 +0530
> ***************
> *** 561,567 ****
>   #define CKPT_FILE_EPOLL CKPT_FILE_EPOLL
>   	CKPT_FILE_EVENTFD,
>   #define CKPT_FILE_EVENTFD CKPT_FILE_EVENTFD
> ! 	CKPT_FILE_MAX
>   #define CKPT_FILE_MAX CKPT_FILE_MAX
>   };
>   
> --- 561,569 ----
>   #define CKPT_FILE_EPOLL CKPT_FILE_EPOLL
>   	CKPT_FILE_EVENTFD,
>   #define CKPT_FILE_EVENTFD CKPT_FILE_EVENTFD
> ! 	CKPT_FILE_FUSE,
> ! #define CKPT_FILE_FUSE CKPT_FILE_FUSE
> ! 	CKPT_FILE_MAX,
>   #define CKPT_FILE_MAX CKPT_FILE_MAX
>   };
>   
> diff -crB linux-cr1/include/linux/fuse.h linux-cr/include/linux/fuse.h
> *** linux-cr1/include/linux/fuse.h	2010-09-14 06:59:05.543512193 +0530
> --- linux-cr/include/linux/fuse.h	2011-03-28 21:59:53.846445000 +0530
> ***************
> *** 249,254 ****
> --- 249,258 ----
>   	FUSE_IOCTL         = 39,
>   	FUSE_POLL          = 40,
>   
> + #ifdef CONFIG_CHECKPOINT
> + 	FUSE_CHECKPOINT    = 1024,
> + #endif                                  /*CONFIG_CHECKPOINT*/

This looks like it will be part of an ABI with userspace. That means it
should always be defined/reserved for checkpointing.

> + 
>   	/* CUSE specific operations */
>   	CUSE_INIT          = 4096,
>   };
> ***************
> *** 565,568 ****
> --- 568,583 ----
>   	__u32	padding;
>   };
>   
> + #ifdef CONFIG_CHECKPOINT
> + 	struct fuse_checkpoint_in{
> + 	__u64	fh;
> + 	__u32	flags;
> + 	__u32	padding;
> +         };
> + 
> +         struct fuse_checkpoint_out{
> +         __u32 status;
> +         };
> + #endif                                                  /*CONFIG_CHECKPOINT*/
> + 
>   #endif /* _LINUX_FUSE_H */
> Only in linux-cr/include/linux: version.h
> Only in linux-cr/scripts/basic: docproc
> Only in linux-cr/scripts/basic: fixdep
> Only in linux-cr/scripts/basic: hash
> Only in linux-cr/scripts: conmakehash
> Only in linux-cr/scripts/genksyms: genksyms
> Only in linux-cr/scripts/genksyms: keywords.c
> Only in linux-cr/scripts/genksyms: lex.c
> Only in linux-cr/scripts/genksyms: parse.c
> Only in linux-cr/scripts/genksyms: parse.h
> Only in linux-cr/scripts: kallsyms
> Only in linux-cr/scripts/kconfig: conf
> Only in linux-cr/scripts/kconfig: lex.zconf.c
> Only in linux-cr/scripts/kconfig: mconf
> Only in linux-cr/scripts/kconfig: zconf.hash.c
> Only in linux-cr/scripts/kconfig: zconf.tab.c
> Only in linux-cr/scripts/mod: elfconfig.h
> Only in linux-cr/scripts/mod: mk_elfconfig
> Only in linux-cr/scripts/mod: modpost
> Only in linux-cr/scripts/selinux/genheaders: genheaders
> Only in linux-cr/scripts/selinux/mdp: mdp
> Only in linux-cr/security/selinux: av_permissions.h
> Only in linux-cr: .version

The above also looks irrelevant to the kernel patch.


Is the content below the missing pair of files?

> /*
> fuse_cr: Checkpoint-Restart implementation for FUSE filesystem kernel module.
> 
> Authors: Manoj Kumar and Vishnumurthy Prabhu 
>          NITK Surathkal
> 
> */
> 
> #include "fuse_kcr.h"
> #include <linux/file.h>

swap header inclusion order -- "local" headers and less-generic kernel
headers go after.

> /*
> 	FUSE filesystem .It needs special handling
> 	{
> 		.file_name = "FUSE",
> 		.file_type = CKPT_FILE_FUSE,
> 		.restore = fuse_file_restore,
> 	},	
> */

Huh? What's this comment supposed to tell us?

> 
> struct ckpt_hdr_file_fuse {
> 	struct ckpt_hdr_file common;
> 	
> } __attribute__((aligned(8)));

This should be in include/linux/checkpoint_hdr.h *except* since it
doesn't need anything besides the ckpt_hdr_file I don't see why you
don't just use that struct instead.

> 
> //extern int generic_file_checkpoint(struct ckpt_ctx *ctx, struct file *file);
> 
> int fuse_file_ckpt_send(struct file *file)
> {
>     struct inode *inode  = file->f_path.dentry->d_inode;
> 	struct fuse_conn *fc = get_fuse_conn(inode);
> //	struct fuse_file *ff = file->private_data;

Leftover garbage?

> 	struct fuse_req *req;
> 	struct fuse_checkpoint_in inarg;
> 	struct fuse_checkpoint_out outarg;
> 	__u64 nodeid;
> 	int err;
> 	struct fuse_file *ff;
> 
>     ff = file->private_data;
> 	if (is_bad_inode(inode))

CodingStyle whitespace issues. Please try checkpatch.pl

> 		return -EIO;
> 
>     nodeid = get_node_id(inode);
> 	req = fuse_get_req(fc);

I'm not familiar enough with FUSE internals. Shouldn't you check
if req is NULL or an ERR_PTR() before using it below?

> 	memset(&inarg, 0, sizeof(inarg));
> // inargs must be supplied.	
>     inarg.fh = ff->fh;
>     inarg.flags &= ~(0);
> //inargs to be supplied    
>     
>     req->in.h.opcode = FUSE_CHECKPOINT;
> 	req->in.h.nodeid = nodeid;
> 	req->in.numargs = 1;
> 	req->in.args[0].size = sizeof(inarg);
> 	req->in.args[0].value = &inarg;
> 	req->out.numargs = 1;
> 	req->out.args[0].size = sizeof(outarg);
> 	req->out.args[0].value = &outarg;
> 
> //aboue is sample inargs need to be changed...
> 	fuse_request_send(fc, req);
> 	err = req->out.h.error;
> 	fuse_put_request(fc, req);

IMPORTANT:

We need to make sure the task(s) responsible for handling the request in
userspace aren't frozen. As best I can tell that's a very real
possibility.

> 	if (err == -ENOSYS) {
> 		fc->no_flush = 1;
> 		err = 0;
> 	}
> 	return outarg.status;
> }
> 
> int fuse_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
> {
>    int ret;
>    struct ckpt_hdr_file_fuse *h;
>    
> 	if (d_unlinked(file->f_dentry)) {
> 		ckpt_err(ctx, -EBADF, "%(T)%(P)Unlinked files unsupported\n",
> 			 file);
> 		return -EBADF;

Unlinked files are important too. Have you had a chance to review my
recent patches for unlinked files?

> 	}
> 
> 	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_FILE);
> 	if (!h)
> 		return -ENOMEM;
> 
> 	h->common.f_type = CKPT_FILE_FUSE;   
>    
> 	ret = checkpoint_file_common(ctx, file, &h->common);
> 	if (ret < 0)
> 		goto out;
> 	ret = ckpt_write_obj(ctx, &h->common.h);
> 	if (ret < 0)
> 		goto out;
> 	ret = checkpoint_fname(ctx, &file->f_path, &ctx->root_fs_path);
> 	   
> //   ret = fuse_file_ckpt_send(file);

This looks like the only real difference from generic_file_checkpoint() yet
it's commented out. What's going on? Have you compiled this code?
Tested it?

>     out:
> 	  ckpt_hdr_put(ctx, h);
>     printk("**************************fuse_file_checkpoint is working fine\n");

Remove printk

> 	  return ret;	
> 
> }
> EXPORT_SYMBOL(fuse_file_checkpoint);
> 
> /******************************************************************************************************************************************************
> Restart
> */
> 
> struct file *fuse_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
> {
>     struct file *file;
> 	int ret;
> 
> 	if (ptr->h.type != CKPT_HDR_FILE  ||
> 	    ptr->h.len != sizeof(*ptr) || ptr->f_type != CKPT_FILE_FUSE)
> 		return ERR_PTR(-EINVAL);
> 
> 	file = restore_open_fname(ctx, ptr->f_flags);
> 	if (IS_ERR(file))
> 		return file;
> 
> 	ret = restore_file_common(ctx, file, ptr);
> 	if (ret < 0) {
> 		fput(file);
> 		file = ERR_PTR(ret);
> 	}
>     printk("**************************fuse_file_restore is working fine\n");

Remove this printk too. At most you'd want to use ckpt_debug() here and
ckpt_err() above when you encounter errors.

> 	return file;
> }
> 

> #ifdef CONFIG_CHECKPOINT
Is this a separate file from the previous code?

I don't think the #ifdef above should wrap the whole file. Actually,
my guess is these function declarations should go elsewhere within
the existing fuse headers.

> #ifndef _FUSE_KCR_H_
> #define _FUSE_KCR_H_
> 
> #include <linux/fuse.h>
> #include <linux/checkpoint.h>
> #include "fuse_i.h"
> 
> int fuse_file_checkpoint(struct ckpt_ctx *ctx, struct file *file);
> struct file *fuse_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr);
> #endif
> #endif

Needs alot more work but it seems like a start. I hope you'll continue
to add to and refine it and repost. Thanks!

Cheers,
	-Matt Helsley


More information about the Containers mailing list