Re: [Qemu-devel] [PATCH 06/17] bochs: Implement .bdrv_co_preadv() interface

From: Kevin Wolf <kwolf@redhat.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: qemu-block@nongnu.org, mreitz@redhat.com, famz@redhat.com,
	sw@weilnetz.de, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 06/17] bochs: Implement .bdrv_co_preadv() interface
Date: Wed, 27 Apr 2016 16:33:41 +0200	[thread overview]
Message-ID: <20160427143341.GC4756@noname.str.redhat.com> (raw)
In-Reply-To: <20160427140647.GF1985@stefanha-x1.localdomain>

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

Am 27.04.2016 um 16:06 hat Stefan Hajnoczi geschrieben:
> On Wed, Apr 27, 2016 at 11:52:36AM +0200, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/bochs.c | 46 +++++++++++++++++++++++++++++-----------------
> >  1 file changed, 29 insertions(+), 17 deletions(-)
> > 
> > diff --git a/block/bochs.c b/block/bochs.c
> > index af8b7ab..d148454 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -104,6 +104,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
> >      int ret;
> >  
> >      bs->read_only = 1; // no write support yet
> > +    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
> 
> Can this be the default in block.c?  Drivers that have other alignment
> characteristics can set it explicitly but most drivers will want
> BDRV_SECTOR_SIZE so it's nice to make it common code.

I absolutely don't expect BDRV_SECTOR_SIZE to be what most drivers will
want. It's basically a sign that a driver is either a protocol that
imposes restrictions (like O_DIRECT on raw-posix) or it is broken and we
don't care enough about the emulation overhead to fix it (like this one).

The expected value for most block drivers is 1.

> >      while (nb_sectors > 0) {
> >          int64_t block_offset = seek_to_sector(bs, sector_num);
> >          if (block_offset < 0) {
> >              return block_offset;
> 
> s->lock must be unlocked.

I can't believe I messed this up in so many places. Thanks for catching
this, I'll have to go through all patches and check the locking.

Kevin

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