Re: [Qemu-devel] [PATCH 4/9] mirror: efficiently zero out target

[Denis V. Lunev]

On 06/15/2016 03:34 PM, Eric Blake wrote:
> On 06/15/2016 02:46 AM, Denis V. Lunev wrote:
> > On 06/15/2016 06:00 AM, Eric Blake wrote:
> > > On 06/14/2016 09:25 AM, Denis V. Lunev wrote:
> > > > With a bdrv_co_write_zeroes method on a target BDS zeroes will not be
> > > > placed
> > > > into the wire. Thus the target could be very efficiently zeroed out.
> > > > This
> > > > is should be done with the largest chunk possible.
> > > Probably nicer to track this in bytes.  And do you really want a
> > > hard-coded arbitrary limit, or is it better to live with
> > > MIN_NON_ZERO(target_bs->bl.max_pwrite_zeroes, INT_MAX)?
> > unfortunately we should. INT_MAX is not aligned as required.
> > May be we should align INT_MAX properly to fullfill
> > write_zeroes alignment.
> >
> > Hmm, may be we can align INT_MAX properly down. OK,
> > I'll try to do that gracefully.
> It's fairly easy to round a max_transfer or max_pwrite_zeroes down to an
> aligned value; we already have code in io.c that does that in
> bdrv_co_do_pwrite_zeroes().

ok



> > > > @@ -512,7 +513,8 @@ static int mirror_dirty_init(MirrorBlockJob *s)
> > > >          end = s->bdev_length / BDRV_SECTOR_SIZE;
> > > >    -    if (base == NULL && !bdrv_has_zero_init(target_bs)) {
> > > > +    if (base == NULL && !bdrv_has_zero_init(target_bs) &&
> > > > +            target_bs->drv->bdrv_co_write_zeroes == NULL) {
> > > Indentation is off, although if checkpatch.pl doesn't complain I guess
> > > it doesn't matter that much.
> > >
> > > Why should you care whether the target_bs->drv implements a callback?
> > > Can't you just rely on the normal bdrv_*() functions to do the dirty
> > > work of picking the most efficient implementation without you having to
> > > bypass the block layer?  In fact, isn't that the whole goal of
> > > bdrv_make_zero() - why not call that instead of reimplementing it?
> > this is the idea of the patch actually. If the callback is not
> > implemented, we
> > will have zeroes actually written or send to the wire. In this case
> > there is
> > not much sense to do that, the amount of data actually written will be
> > significantly increased (some areas will be written twice - with zeroes and
> > with the actual data).
> But that's where bdrv_can_write_zeroes_with_unmap() comes in handy - you
> can use the public interface to learn whether bdrv_make_zero() will be
> efficient or not, without having to probe what the backend supports.

bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
{
    BlockDriverInfo bdi;

    if (bs->backing || !(bs->open_flags & BDRV_O_UNMAP)) {
        return false;
    }

    if (bdrv_get_info(bs, &bdi) == 0) {
        return bdi.can_write_zeroes_with_unmap;
    }

    return false;
}


This function looks rotten. We CAN efficiently zero out
QCOW2 images even with backing store available. Though
the availability of the bdrv_co_write_zeroes does not
guarantee that it is working (NFS, CIFS etc for raw_posix.c).




> > On the other hand, if callback is implemented, we will have very small
> > amount
> > of data in the wire and written actually and thus will have a benefit. I am
> > trying to avoid very small chunks of data. Here (during the migration
> > process)
> > the data is sent with 10 Mb chunks and with takes a LOT of time with NBD.
> > We can send chunks 1.5 Gb (currently). They occupies the same 26 bytes
> > of data
> > on the transport layer.
> I agree that we don't want to pre-initialize the device to zero unless
> write zeroes is an efficient operation, but I don't think that the
> existence of bs->drv->bdrv_co_[p]write_zeroes is the right way to find
> that out.
>
> I also think that we need to push harder on the NBD list that under the
> new block limits proposal, we WANT to be able to advertise when the new
> NBD_CMD_WRITE_ZEROES command will accept a larger size than
> NBD_CMD_WRITE (as currently written, the BLOCK_INFO extension proposal
> states that if a server advertises a max transaction size to the client,
> then the client must honor that size for all commands including
> NBD_CMD_WRITE_ZEROES, which would mean your 1.5G request [or my proposed
> 2G - 4k request] is invalid and would have to be a bunch of 32M requests).
> https://sourceforge.net/p/nbd/mailman/message/35081223/


I see...