[Libguestfs] [nbdkit PATCH] cache: Reduce use of bounce-buffer

[Eric Blake]

On 5/13/19 4:21 AM, Richard W.M. Jones wrote:
> On Sat, May 11, 2019 at 03:30:04PM -0500, Eric Blake wrote:
>> Although the time spent in memcpy/memset probably pales in comparison
>> to time spent in socket I/O, it's still worth worth reducing the
>> number of times we have to utilize a bounce buffer when we already
>> have aligned data.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---

>> @@ -233,11 +234,13 @@ cache_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
>>    CLEANUP_FREE uint8_t *block = NULL;
>>
>>    assert (!flags);
>> -  block = malloc (blksize);
>> -  if (block == NULL) {
>> -    *err = errno;
>> -    nbdkit_error ("malloc: %m");
>> -    return -1;
>> +  if (!IS_ALIGNED (count | offset, blksize)) {
>> +    block = malloc (blksize);
>> +    if (block == NULL) {
>> +      *err = errno;
>> +      nbdkit_error ("malloc: %m");
>> +      return -1;
>> +    }

If we have an unaligned count or offset, then we need the bounce buffer,
but we only have to use it...

>>    }
>>
>>    /* XXX This breaks up large read requests into smaller ones, which
>> @@ -258,12 +261,14 @@ cache_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
>>
>>      {
>>        ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
>> -      r = blk_read (next_ops, nxdata, blknum, block, err);
>> +      r = blk_read (next_ops, nxdata, blknum,
>> +                    blkoffs || n < blksize ? block : buf, err);

...on the unaligned head and unaligned tail of the overall request...

>>      }
>>      if (r == -1)
>>        return -1;
>>
>> -    memcpy (buf, &block[blkoffs], n);
>> +    if (blkoffs || n < blksize)
>> +      memcpy (buf, &block[blkoffs], n);

...and the memcpy is only necessary to copy into the caller's buf when
we read into our bounce buffer.  But yes, I agree with your complaint...

> 
> I've stared at this patch for a while and I don't really understand
> it.
> 
> One suggestion though.  The patch might be better (or at least might
> be more obviously correct) if the function defined a boolean at the
> top which was used to tell if the bounce buffer was in use, something
> like:
> 
>   const bool use_bounce_buffer = /* condition */;

...and will rewrite it to be more obvious about the three-part handling
of any request.  Writing this email made me also realize that the
blocksize filter doesn't play nicely with the streaming plugin (although
I don't know if that would ever work nicely, since the streaming plugin
does not handle read-modify-write), because the blocksize filter does
the same head/body/tail splitting of an unaligned request, but reorders
things to visit the tail before the body, rather than visiting
everything in order.

It would be interesting if we had way to share the blocksize, cache, and
cow filters' common code for adjusting requests onto appropriate block
size boundaries.  For that matter, someday I really want to get nbdkit
to support NBD_INFO_BLOCK_SIZE, where plugins/filters can advertise
alignment to the client, and where nbdkit itself takes care of the
alignment issues rather than every plugin having to deal with bounce
buffers, size rounding and read-modify-write.  Ah well, a future project
(as the caching project is already a big set to get working).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190513/31393a75/attachment.sig>