On 09/29/2014 02:36 PM, Andrew Vagin wrote:
> 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.
Nobody checks errors from closing the images :)
I've already written how to handle such errors.
>> 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?
Sure.
>> + 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.
The only existing user -- pb_write|read -- does.
>> +
>> + 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.
Yup.
>> + }
>> +
>> + 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
> .
>