Re: [RFC][PATCH 5/8] add f_op for checkpointability

[Dave Hansen <dave@linux.vnet.ibm.com>]

On Fri, 2009-02-27 at 18:14 -0800, Sukadev Bhattiprolu wrote:
> | -int cr_file_supported(struct file *file)
> | +typedef int (do_checkpoint_t)(struct file *, struct cr_ctx *,
> | +			      struct cr_hdr_fd *);
> | +
> | +int generic_file_checkpoint(struct file *file, struct cr_ctx *ctx,
> | +			 struct cr_hdr_fd *hh)
> | +{
> | +	/*
> | +	 * A NULL hh means to make a trial run not
> | +	 * actually writing data.  Just determine
> | +	 * if the file is checkpointable.
> | +	 */
> | +	if (!hh)
> | +		return 0;
> | +
> | +	hh->f_flags = file->f_flags;
> | +	hh->f_mode = file->f_mode;
> | +	hh->f_pos = file->f_pos;
> | +	hh->f_version = file->f_version;
> | +	/* FIX: need also file->uid, file->gid, file->f_owner, etc */
> | +
> | +	return 0;
> | +}
> | +
> | +do_checkpoint_t *cr_file_get_func(struct file *file)
> |  {
> 
> Do we really need this helper ? IOW do callers need this function pointer
> itself ? Or can we have a more generic helper that callers can use both
> check if checkpoint is possible (pass NULL in ctx and hh) and to actually
> checkpoint. Something like:

That helper is there because I overload that f_op->checkpoint for both
the "is this file checkpointable" function and the "checkpoint this
file" operation.

> 	int cr_file_checkpoint(file, ctx, hh)
> 	{
> 		int rc = -1;
> 
> 		if (!cr_fs_checkpointable(fstype))
> 			return rc;
> 		
> 		if (!cr_file_checkpointable(file))
> 			return rc;
> 
> 		if (special_file(file))
> 			return rc;
> 
> 		op = file->f_op->checkpoint;
> 		if (!op)
> 			op = generic_file_checkpoint;
> 
> 		return (*op)(file, ctx, hh);
> 	}

First thing you have to be careful about is that the f_op should be able
to override *everything*.  So it has to be first, always.

The other part is that I'd prefer not to call check (a la !
cr_file_checkpointable()) then try to checkpoint a second later, since
we share the implementation between the two here.  

You are probably right that we should probably be able to do this:

int cr_file_checkpointable(file)
{
	return !cr_file_checkpoint(file, NULL, NULL);
}

I'll look into that and see how natural it is to implement.

> |  	struct inode *inode = file->f_dentry->d_inode;
> |  	struct file_system_type *fs_type = inode->i_sb->s_type;
> | 
> | -	if (fs_is_cr_able(fs_type))
> | -		return 0;
> | +	if (file->f_op->checkpoint)
> | +		return file->f_op->checkpoint;
> | +
> | +	if (!fs_is_cr_able(fs_type))
> | +		return NULL;
> | 
> |  	if (special_file(inode->i_mode))
> | -		return 0;
> | +		return NULL;
> | 
> | -	return 1;
> | +	return generic_file_checkpoint;
> | +}
> | +
> | +int cr_file_supported(struct file *file)
> | +{
> | +	do_checkpoint_t *func = cr_file_get_func(file);
> | +
> | +	if (func)
> | +		return !func(file, NULL, NULL);
> | +
> | +	return 0;
> |  }
> | 
> |  /* cr_write_fd_data - dump the state of a given file pointer */
> |  static int cr_write_fd_data(struct cr_ctx *ctx, struct file *file, int parent)
> |  {
> | +	do_checkpoint_t *ckpt_func;
> |  	struct cr_hdr h;
> |  	struct cr_hdr_fd *hh = cr_hbuf_get(ctx, sizeof(*hh));
> | -	struct dentry *dent = file->f_dentry;
> | -	struct inode *inode = dent->d_inode;
> | -	enum fd_type fd_type;
> |  	int ret;
> | 
> |  	h.type = CR_HDR_FD_DATA;
> |  	h.len = sizeof(*hh);
> |  	h.parent = parent;
> | 
> | -	hh->f_flags = file->f_flags;
> | -	hh->f_mode = file->f_mode;
> | -	hh->f_pos = file->f_pos;
> | -	hh->f_version = file->f_version;
> | -	/* FIX: need also file->uid, file->gid, file->f_owner, etc */
> | -
> | -	switch (inode->i_mode & S_IFMT) {
> | -	case S_IFREG:
> | -		fd_type = CR_FD_FILE;
> | -		break;
> | -	case S_IFDIR:
> | -		fd_type = CR_FD_DIR;
> | -		break;
> | -	default:
> | -		cr_hbuf_put(ctx, sizeof(*hh));
> | -		return -EBADF;
> | -	}
> | +	ckpt_func = cr_file_get_func(file);
> | +	ret = -EBADF;
> | +	if (!ckpt_func)
> | +		goto out;
> | 
> | -	/* FIX: check if the file/dir/link is unlinked */
> | -	hh->fd_type = fd_type;
> | +	ret = ckpt_func(file, ctx, hh);
> | +	if (ret)
> | +		goto out;
> 
> So we can combine these two steps into just one ?
> 
> 	ret = -EBADF;
> 	hh->fd_type = fd_type;
> 	if (cr_file_checkpoint(file, ctx, hh))
> 		goto out;

We could but it would be harder to read. :)

I really don't like operational things inside if()s like that.  I think
it kinda hides important code.

Thanks for taking the time to look at this, Suka.

-- Dave