[Devel] Re: [PATCH] Fix restoring pipes with full buffers

[Matt Helsley]

On Fri, Jan 28, 2011 at 10:45:11AM -0800, Dan Smith wrote:
> While this fixes restoring pipes that were completely full, it actually
> corrects a potential issue with restoring any pipe buffers.  By using
> splice() to do this work when we are reading the image from another pipe,
> we depend on userspace setting up the buffers in the pipe perfectly
> such that the data to be restored is oriented in the pipe in the same
> way as it is expected (or required) to be in the restored pipe.  The
> "full" case is the hardest to get right, but userspace could break things
> if it loaded up the inbound pipe with lots of small buffers which would
> cause splice() to hit the PIPE_BUFFERS limit before having read the
> requested amount of data.
> 
> Instead, drop the optimization and just read() and write() data into
> the pipe.

Looks good:

Reviewed-by: Matt Helsley <matthltc at us.ibm.com>


While it seems like it should be possible to modify splice internals to
handle this Oren's checkpoint/restart tree doesn't seem like the right
place to attempt that work.

> 
> Signed-off-by: Dan Smith <danms at us.ibm.com>
> ---
>  fs/pipe.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 9664e4f..0da1e3a 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -926,17 +926,60 @@ static int pipe_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
>  	return ret;
>  }
> 
> +static int restore_pipe_buffer(struct ckpt_ctx *ctx,
> +			       struct file *dest,
> +			       int len)
> +{
> +	char *buf;
> +	int ret;
> +	int nread;
> +	int nwrote;
> +	int nleft = len;
> +
> +	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	for (nleft = len; nleft > 0; nleft -= nread) {
> +		int size = nleft < PAGE_SIZE ? nleft : PAGE_SIZE;
> +		loff_t pos;
> +
> +		pos = file_pos_read(ctx->file);
> +		nread = kernel_read(ctx->file, pos, buf, size);
> +		if (nread < 0) {
> +			ret = nread;
> +			goto out;
> +		}
> +		file_pos_write(ctx->file, pos + nread);
> +
> +		pos = file_pos_read(dest);
> +		nwrote = kernel_write(dest, pos, buf, nread);
> +		if (nwrote < 0) {
> +			ret = nwrote;
> +			goto out;
> +		}
> +		file_pos_write(dest, pos + nwrote);
> +
> +		if (nwrote != nread) {
> +			ret = -EPIPE;
> +			goto out;
> +		}
> +	}
> +	ret = len;
> + out:
> +	kfree(buf);
> +	return ret;
> +}

Could you make this more generic and just take a source struct file *
as a parameter too (instead of the struct ckpt_ctx *)? Perhaps rename it
copy_pipe_to_pipe(src_file, dest_file, len) or some such? Seems like a
helper like this should already exist somewhere...

Cheers,
	-Matt Helsley
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers