Re: [Qemu-block] RFC iscsi: set FUA and DPO if !bs->enable_write_cache

[Peter Lieven]

Am 16.04.2015 um 11:37 schrieb Paolo Bonzini:
> On 16/04/2015 09:16, Peter Lieven wrote:
> > Thanks for the clarification. At first glance its not that obvious.
> >
> > There is one thing I want to mention none is not exactly writeback
> > and directsync not exactly writethrough. We do not enable the
> > iscsi->allocationmap if BRDV_O_NOCACHE is in the flags. And this
> > flag seems to be equivalent to cache.direct.
> Correct.  So if you do shared access you _still_ cannot use
> writeback/writethrough/unsafe, even for libiscsi.  You can use it for
> migration if you implement bdrv_invalidate_cache and add a patch like
> the following:

That allocation cache in the iSCSI driver is only a hint. It always confirms 
blocks
are really unallocated before taking the fast path returning zeroes.
So I don't think it is necessary to add invalidate cache, or is it?
Or would you vote for removing that additional check, implementing invalidate
cache and making the allocationmap tri-state (unknown, allocated, unallocated)?

> diff --git a/block.c b/block.c
> index f2f8ae7..345e617 100644
> --- a/block.c
> +++ b/block.c
> @@ -5119,7 +5119,12 @@ void bdrv_invalidate_cache(
>
>       if (bs->drv->bdrv_invalidate_cache) {
>           bs->drv->bdrv_invalidate_cache(bs, &local_err);
> -    } else if (bs->file) {
> +    }
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (bs->file) {
>           bdrv_invalidate_cache(bs->file, &local_err);
>       }
>       if (local_err) {
>
>
> > > Actually, in the case of battery-backed cache file.cache.no-flush=on
> > > (aka file.cache=unsafe) _is_ a good idea, because arrays with
> > > battery-backed cache ignore flushes (just like FUAs).
> > >
> > > Of course cache=unsafe and cache.no-flush=on are not a good idea because
> > > you want to flush the qcow2 caches for example.
> > >
> > > > cachemode writeback is the only one that enables bs->enable_write_cache.
> > > None, too.  I would use cache=none just to be safe: cache=writeback
> > > works with migration only because you are using the iSCSI driver.
> > Or if you use libnfs with the NFS driver for accessing the QCOWs on
> > your NFS share.
> >
> > My current strategy is to use always cache=none except if I have
> > an iSCSI target where I either know that the target has battery backup
> > or if I use virtio-blk (which falls back to writethrough if there is no
> > support for flushes).
> Actually, if you use cache=none, it falls back to cache=directsync.
>
> If I were you:
>
> - I would switch to the cache.foo options, they're clearer and permit
> finer-grained configuration.
>
> - I would never use cache.direct=no if you know you're doing migration,
> until bdrv_invalidate_cache is implemented.  After that, you can always
> use cache.direct=no with libiscsi except if multiple servers are sharing
> the disk.
>
> - I would always use cache.writeback=yes if you know you're using
> virtio-blk (which falls back to cache.writeback=no) or if the guest is
> reasonably recent (2.6.32+ kernel; or any Windows as I'm not aware of
> any change regarding flushes there).
>
> - I would use file.cache.no-flush=yes if you know you're on
> battery-backed storage.

That sounds reasonable. Thanks for the clarification.

Peter