Re: [Qemu-devel] [PATCH 13/20] iscsi: Support BDRV_REQ_FUA

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]

Am 26.03.2016 um 21:44 hat Max Reitz geschrieben:
> On 18.03.2016 19:21, Kevin Wolf wrote:
> > This replaces the existing hack in the iscsi driver that sent the FUA
> > bit in writethrough mode and ignored the following flush in order to
> > optimise the number of roundtrips (see commit 73b5394e).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/iscsi.c | 24 +++++++-----------------
> >  1 file changed, 7 insertions(+), 17 deletions(-)
> > 
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 3b54536..4f75204 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> 
> [...]
> 
> > @@ -1851,7 +1840,8 @@ static BlockDriver bdrv_iscsi = {
> >      .bdrv_co_discard      = iscsi_co_discard,
> >      .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
> >      .bdrv_co_readv         = iscsi_co_readv,
> > -    .bdrv_co_writev        = iscsi_co_writev,
> > +    .bdrv_co_writev_flags  = iscsi_co_writev_flags,
> > +    .supported_write_flags = BDRV_REQ_FUA,
> >      .bdrv_co_flush_to_disk = iscsi_co_flush,
> >  
> >  #ifdef __linux__
> > 
> 
> Hm, wait, maybe not R-b. I can see three places in block/io.c which call
> bdrv_co_writev(), and only one of them diverts to bdrv_co_writev_flags()
> if that is available. Maybe we don't need to care about the
> bounce-buffer case for write_zeroes, but I do think we need to care
> about the COR case.
> 
> Of course bdrv_co_writev() can trivially be forwarded to
> bdrv_co_writev_flags(), but I'm not sure who is supposed to do this
> forwarding. I can imagine three ways:
> 
> (1) Keep a wrapper per block driver. Simple, but not so elegant.
> (2) Make all bdrv_co_writev() callers call bdrv_co_writev_flags() if
>     the former is not available but the latter is.
> (3) Introduce a generic function replacing every drv->bdrv_co_writev()
>     call which then decides which driver function to invoke.

Good catch, thanks!

Going for (1) for now, because I think (2) is even less elegant and
while I have a slight preference for (3) from the code perspective, it
could be argued that it impacts the hot write path of raw images and
I don't want to deal with potential performance changes that late in the
cycle.

And now that I'm writing this, I realise that the hot path already calls
.bdrv_co_writev_flags, so that's not a real argument. But I've already
implemented (1), so I'll leave it at that... The long term plan is
anyway to convert everything to .bdrv_co_writev_flags.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]