[CRIU] [PATCH 14/15] bfd: Implement buffered writes

[Andrew Vagin]

On Mon, Sep 29, 2014 at 12:50:29PM +0400, Pavel Emelyanov wrote:
> Dump times (top-5) look like
> 
> Before patch:
> 	writev:     1595  0.048337 (15.1%)
> 	openat:     1326  0.041976 (13.1%)
> 	 close:     1434  0.034661 (10.8%)
> 	  read:      988  0.028760 (9.0%)
> 	 wait4:      170  0.028271 (8.8%)
> 
> After patch:
> 	openat:     1326  0.040010 (16.4%)
> 	 close:     1434  0.030039 (12.3%)
> 	  read:      988  0.025827 (10.6%)
> 	 wait4:      170  0.025549 (10.5%)
> 	ptrace:      834  0.021624 (8.9%)
> 
> So write-s go away from top list (turn into 8th position).
> 
> Funny thing is that all object writes get merged with the
> magic writes, so the total amount of write()-s (not writev-s)
> in the strace remain intact :)
> 
> Signed-off-by: Pavel Emelyanov <xemul at parallels.com>
> ---
>  bfd.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/bfd.c b/bfd.c
> index 2572679..0bc9a64 100644
> --- a/bfd.c
> +++ b/bfd.c
> @@ -84,10 +84,22 @@ int bfdopen(struct bfd *f)
>  	return 0;
>  }
>  
> +static int bflush(struct bfd *bfd);
> +
>  void bclose(struct bfd *f)
>  {
> -	if (bfd_buffered(f))
> +	if (bfd_buffered(f)) {
> +		if (bflush(f) < 0)
> +			/*
> +			 * FIXME -- propagate error up. It's
> +			 * hardly possible by returning and
> +			 * checking it, but setting a static
> +			 * flag, failing further bfdopen-s and
> +			 * checking one at the end would work.
> +			 */
> +			pr_err("Error flushing image\n");
>  		buf_put(&f->b);
> +	}

Nobody reads logs if the exit code is zero. I would prefer to return
error from here and check it.

>  	close(f->fd);
>  }
>  
> @@ -178,14 +190,69 @@ again:
>  	goto again;
>  }
>  
> +static int bflush(struct bfd *bfd)
> +{
> +	struct xbuf *b = &bfd->b;
> +	int ret;
> +
> +	if (!b->sz)
> +		return 0;
> +
> +	ret = write(bfd->fd, b->data, b->sz);
> +	if (ret != b->sz)

Can we print error here?

> +		return -1;
> +
> +	b->sz = 0;
> +	return 0;
> +}
> +
> +static int __bwrite(struct bfd *bfd, const void *buf, int size)
> +{
> +	struct xbuf *b = &bfd->b;
> +
> +	if (b->sz + size > BUFSIZE) {
> +		int ret;
> +		ret = bflush(bfd);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (size > BUFSIZE)
> +		return write(bfd->fd, buf, size);

Looks like you want to say that a caller whould get a proper errno and
print errors.

> +
> +	memcpy(b->data + b->sz, buf, size);
> +	b->sz += size;
> +	return size;
> +}
> +
>  int bwrite(struct bfd *bfd, const void *buf, int size)
>  {
> -	return write(bfd->fd, buf, size);
> +	if (!bfd_buffered(bfd))
> +		return write(bfd->fd, buf, size);
> +
> +	return __bwrite(bfd, buf, size);
>  }
>  
>  int bwritev(struct bfd *bfd, const struct iovec *iov, int cnt)
>  {
> -	return writev(bfd->fd, iov, cnt);
> +	int i, written = 0;
> +
> +	if (!bfd_buffered(bfd))
> +		return writev(bfd->fd, iov, cnt);
> +
> +	for (i = 0; i < cnt; i++) {
> +		int ret;
> +
> +		ret = __bwrite(bfd, (const void *)iov[i].iov_base, iov[i].iov_len);
> +		if (ret < 0)
> +			return ret;

		written += ret;

> +		if (ret < iov[i].iov_len)
> +			break;
> +
> +		written += ret;

It should be moved upper.

> +	}
> +
> +	return written;
>  }
>  
>  int bread(struct bfd *bfd, void *buf, int size)
> -- 
> 1.8.4.2
> 
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu