Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 2/2] block: Fix blk_aio_write_zeroes()

[Kevin Wolf <kwolf@redhat.com>]

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

Am 15.04.2016 um 01:30 hat Max Reitz geschrieben:
> On 13.04.2016 13:06, Kevin Wolf wrote:
> > Commit 57d6a428 broke blk_aio_write_zeroes() because in some write
> > functions in the call path don't have an explicit length argument but
> > reuse qiov->size instead. Which is great, except that write_zeroes
> > doesn't have a qiov, which this commit interprets as 0 bytes.
> > Consequently, blk_aio_write_zeroes() didn't effectively do anything.
> 
> You mean it has written null data? :-)
> 
> Alternative version: So it has written zero bytes? Works as intended, then.

I consciously avoided the latter wording. ;-)

> > This patch introduces an explicit acb->bytes in BlkAioEmAIOCB and uses
> > that instead of acb->rwco.size.
> > 
> > The synchronous version of the function is okay because it does pass a
> > qiov (with the right size and a NULL pointer as its base).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> > @@ -937,7 +940,8 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
> >          return blk_abort_aio_request(blk, cb, opaque, -EINVAL);
> >      }
> >  
> > -    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, NULL,
> > +    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS,
> > +                        nb_sectors << BDRV_SECTOR_BITS, NULL,
> >                          blk_aio_write_entry, BDRV_REQ_ZERO_WRITE, cb, opaque);
> >  }
> 
> Another thing about blk_aio_write_zeroes() that might need fixing is
> that it completely ignores its flags parameter.
> 
> That can be done in a seperate patch, however (and is probably not
> critical, as this currently just means that a potential REQ_MAY_UNMAP
> from scsi-disk is ignored), so:

Oh dear... Commit 57d6a428 was not my best patch ever, and apparently
Stefan wasn't suspicious enough to catch the bugs. Maybe someone should
re-review the whole thing...

"just" ignoring REQ_MAY_UNMAP is a regression in discard support, so no,
that's not what I would call harmless. Definitely needs fixing for 2.6.
And we probably need more qtests checking such things.

Kevin

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