[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status(
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status() |
Date: |
Wed, 14 Feb 2018 15:55:00 +0100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
Am 14.02.2018 um 15:44 hat Eric Blake geschrieben:
> On 02/14/2018 06:05 AM, Kevin Wolf wrote:
> > Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> > > We are gradually moving away from sector-based interfaces, towards
> > > byte-based. Update the null driver accordingly.
> > >
> > > Signed-off-by: Eric Blake <address@hidden>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > Reviewed-by: Fam Zheng <address@hidden>
> > >
>
> > > if (s->read_zeroes) {
> > > - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
> > > - } else {
> > > - return BDRV_BLOCK_OFFSET_VALID | start;
> > > + ret |= BDRV_BLOCK_ZERO;
> > > }
> > > + return ret;
> > > }
> >
> > Preexisting, but I think this return value is wrong. OFFSET_VALID
> > without DATA is to documented to have the following semantics:
> >
> > * DATA ZERO OFFSET_VALID
> > * f t t sectors preallocated, read as zero, returned
> > file not
> > * necessarily zero at offset
> > * f f t sectors preallocated but read from backing_hd,
> > * returned file contains garbage at offset
> >
> > I'm not sure what OFFSET_VALID is even supposed to mean for null.
>
> Yeah, and I was even thinking about that a bit yesterday when figuring out
> what to do with nvme. It does highlight the fact that you get garbage when
> reading from the null driver (unless the zero option was enabled, then ZERO
> is set and you know you read zeros instead) - but there no pointer that is
> preallocated (whether it contains garbage or otherwise) that you can
> actually dereference to read what the guest would see.
>
> >
> > Or in fact, what it is supposed to mean for any protocol driver, because
> > normally it just means I can use this offset for accessing bs->file. But
> > protocol drivers don't have a bs->file, so it's interesting to see that
> > they still all set this flag.
> >
> > OFFSET_VALID | DATA might be excusable because I can see that it's
> > convenient that a protocol driver refers to itself as *file instead of
> > returning NULL there and then the offset is valid (though it would be
> > pointless to actually follow the file pointer), but OFFSET_VALID without
> > DATA probably isn't.
>
> Hmm, you're probably right. Maybe that means I should tweak the
> documentation to be more explicit: for a format driver, OFFSET_VALID can
> always be used (and *file will be set to the underlying protocol driver);
> but for a protocol driver, OFFSET_VALID only makes sense if *file is the BDS
> itself and there is an actual buffer to read (that is, the protocol driver
> must also be returning DATA and/or ZERO). Or maybe we can indeed state that
> protocol drivers always set *file to NULL (there is no further backing file
> to reference), and thus never need to return OFFSET_VALID (but I'm not sure
> whether that will accidentally propagate back up the call stack and
> negatively affect status queries of format drivers).
>
> Since it is pre-existing, should I respin to address the issue in a separate
> patch, or should that be a followup after this series?
It's a more fundamental question that shouldn't hold up this series. I
just wanted to raise it while I was looking at it. So yes, a followup is
fine.
Kevin
- [Qemu-devel] [PATCH v8 02/21] nvme: Drop pointless .bdrv_co_get_block_status(), (continued)
- [Qemu-devel] [PATCH v8 02/21] nvme: Drop pointless .bdrv_co_get_block_status(), Eric Blake, 2018/02/13
- [Qemu-devel] [PATCH v8 06/21] iscsi: Switch cluster_sectors to byte-based, Eric Blake, 2018/02/13
- [Qemu-devel] [PATCH v8 04/21] file-posix: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/13
- [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/13
- [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/13
- Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status(), Kevin Wolf, 2018/02/14
- Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/23
- Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status(), Kevin Wolf, 2018/02/23
- Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/23
- Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status(), Kevin Wolf, 2018/02/26
[Qemu-devel] [PATCH v8 14/21] raw: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/13
[Qemu-devel] [PATCH v8 07/21] iscsi: Switch iscsi_allocmap_update() to byte-based, Eric Blake, 2018/02/13
[Qemu-devel] [PATCH v8 05/21] gluster: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/13
[Qemu-devel] [PATCH v8 11/21] qcow: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/13
[Qemu-devel] [PATCH v8 01/21] block: Add .bdrv_co_block_status() callback, Eric Blake, 2018/02/13
[Qemu-devel] [PATCH v8 10/21] parallels: Switch to .bdrv_co_block_status(), Eric Blake, 2018/02/13