| Message ID | 20170504030755.1001-3-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04.05.2017 05:07, Eric Blake wrote: > We had some conflicting documentation: a nice 8-way table that > described all possible combinations of DATA, ZERO, and > OFFSET_VALID, contrasted with text that implied that OFFSET_VALID > always meant raw data could be read directly. Furthermore, the > text refers a lot to bs->file, even though the interface was > updated back in 67a0fd2a to let the driver pass back which BDS (not > necessarily bs->file). Not sure about my English skills here, but is this missing a verb? ("to pass back which BDS...") > As the 8-way table is the intended > semantics, simplify the rest of the text to get rid of the > confusion. > > ALLOCATED is always set by the block layer for convenience (drivers > do not have to worry about it). RAW is used only internally, but Just one space after the period? How inconsistent! :-) > by more than the raw driver. Document these additional items on > the driver callback. > > Suggested-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v12: even more wording tweaks > v11: reserved for blkdebug half of v10 > v10: new patch > --- > include/block/block.h | 35 +++++++++++++++++++---------------- > include/block/block_int.h | 7 +++++++ > 2 files changed, 26 insertions(+), 16 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 862eb56..c8bec7d 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -120,29 +120,32 @@ typedef struct HDGeometry { > #define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) > > /* > - * Allocation status flags > - * BDRV_BLOCK_DATA: data is read from a file returned by bdrv_get_block_status. > - * BDRV_BLOCK_ZERO: sectors read as zero > - * BDRV_BLOCK_OFFSET_VALID: sector stored as raw data in a file returned by > - * bdrv_get_block_status. > + * Allocation status flags for bdrv_get_block_status() and friends. > + * > + * Public flags: > + * BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer > + * BDRV_BLOCK_ZERO: offset reads as zero > + * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data > * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this > - * layer (as opposed to the backing file) > - * BDRV_BLOCK_RAW: used internally to indicate that the request > - * was answered by the raw driver and that one > - * should look in bs->file directly. > + * layer (short for DATA || ZERO), set by block layer > * > - * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in > - * bs->file where sector data can be read from as raw data. > + * Internal flag: > + * BDRV_BLOCK_RAW: used internally to indicate that the request was > + * answered by a passthrough driver such as raw and that the s/passthrough/filter/? But I'm not even sure myself. Well, "passthrough" is a safe bet, so let's just go with it. With the commit message fixed or a “no it's fine”: Reviewed-by: Max Reitz <mreitz@redhat.com> > + * block layer should recompute the answer from bs->file. > * > - * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present. > + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) > + * represent the offset in the returned BDS that is allocated for the > + * corresponding raw data; however, whether that offset actually contains > + * data also depends on BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO, as follows: > * > * DATA ZERO OFFSET_VALID > - * t t t sectors read as zero, bs->file is zero at offset > - * t f t sectors read as valid from bs->file at offset > - * f t t sectors preallocated, read as zero, bs->file not > + * t t t sectors read as zero, returned file is zero at offset > + * t f t sectors read as valid from file at offset > + * 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, > - * bs->file contains garbage at offset > + * returned file contains garbage at offset > * t t f sectors preallocated, read as zero, unknown offset > * t f f sectors read from unknown file or offset > * f t f not allocated or unknown offset, read as zero > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 8773940..1fdfff7 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -165,6 +165,13 @@ struct BlockDriver { > int64_t offset, int count, BdrvRequestFlags flags); > int coroutine_fn (*bdrv_co_pdiscard)(BlockDriverState *bs, > int64_t offset, int count); > + > + /* > + * Building block for bdrv_block_status[_above]. The driver should > + * answer only according to the current layer, and should not > + * set BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h > + * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. > + */ > int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs, > int64_t sector_num, int nb_sectors, int *pnum, > BlockDriverState **file); >
On 05/05/2017 03:06 PM, Max Reitz wrote: > On 04.05.2017 05:07, Eric Blake wrote: >> We had some conflicting documentation: a nice 8-way table that >> described all possible combinations of DATA, ZERO, and >> OFFSET_VALID, contrasted with text that implied that OFFSET_VALID >> always meant raw data could be read directly. Furthermore, the >> text refers a lot to bs->file, even though the interface was >> updated back in 67a0fd2a to let the driver pass back which BDS (not >> necessarily bs->file). > > Not sure about my English skills here, but is this missing a verb? ("to > pass back which BDS...") maybe s/which/a specific/ > >> As the 8-way table is the intended >> semantics, simplify the rest of the text to get rid of the >> confusion. >> >> ALLOCATED is always set by the block layer for convenience (drivers >> do not have to worry about it). RAW is used only internally, but > > Just one space after the period? How inconsistent! :-) But do commit messages really count? :) >> + * Internal flag: >> + * BDRV_BLOCK_RAW: used internally to indicate that the request was >> + * answered by a passthrough driver such as raw and that the > > s/passthrough/filter/? But I'm not even sure myself. Well, "passthrough" > is a safe bet, so let's just go with it. Calling 'raw' a filter driver is a bit weird - but in one sense, it is a no-op filter (filter the protocol layer into the format layer by doing nothing). Meanwhile 'commit' certainly sounds like more of a filter than a passthrough. I could go either way, and filter is slightly shorter. If there's a real reason to respin the series, 'filter' seems reasonable if we're worried about line length, otherwise I'm just as inclined to leave it alone. > > With the commit message fixed or a “no it's fine”: > > Reviewed-by: Max Reitz <mreitz@redhat.com> >
On 05.05.2017 22:13, Eric Blake wrote: > On 05/05/2017 03:06 PM, Max Reitz wrote: >> On 04.05.2017 05:07, Eric Blake wrote: >>> We had some conflicting documentation: a nice 8-way table that >>> described all possible combinations of DATA, ZERO, and >>> OFFSET_VALID, contrasted with text that implied that OFFSET_VALID >>> always meant raw data could be read directly. Furthermore, the >>> text refers a lot to bs->file, even though the interface was >>> updated back in 67a0fd2a to let the driver pass back which BDS (not >>> necessarily bs->file). >> >> Not sure about my English skills here, but is this missing a verb? ("to >> pass back which BDS...") > > maybe s/which/a specific/ Or that, yes. :-) > >> >>> As the 8-way table is the intended >>> semantics, simplify the rest of the text to get rid of the >>> confusion. >>> >>> ALLOCATED is always set by the block layer for convenience (drivers >>> do not have to worry about it). RAW is used only internally, but >> >> Just one space after the period? How inconsistent! :-) > > But do commit messages really count? :) It's a critical bug, I'm telling you!!! >>> + * Internal flag: >>> + * BDRV_BLOCK_RAW: used internally to indicate that the request was >>> + * answered by a passthrough driver such as raw and that the >> >> s/passthrough/filter/? But I'm not even sure myself. Well, "passthrough" >> is a safe bet, so let's just go with it. > > Calling 'raw' a filter driver is a bit weird - but in one sense, it is a > no-op filter (filter the protocol layer into the format layer by doing > nothing). Meanwhile 'commit' certainly sounds like more of a filter > than a passthrough. I could go either way, and filter is slightly > shorter. If there's a real reason to respin the series, 'filter' seems > reasonable if we're worried about line length, otherwise I'm just as > inclined to leave it alone. raw certainly is a filter driver; the thing I wasn't sure about is that I'm not sure whether filter drivers are required to set this flag. But neither the comment nor the code require it necessarily, so using "filter" instead of "passthrough" should be OK. The main reason for using "filter" over "passthrough" is that "filter" is a "real" class of block drivers (just "real" in the sense that we actually only have child-less protocol drivers and non-protocol drivers that do have children; further dividing into "format" and "filter" is just a convention). But it should be clear anyway, so I don't mind either way. Leaving it as it is certainly is simpler. Max > >> >> With the commit message fixed or a “no it's fine”: >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >
diff --git a/include/block/block.h b/include/block/block.h index 862eb56..c8bec7d 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -120,29 +120,32 @@ typedef struct HDGeometry { #define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) /* - * Allocation status flags - * BDRV_BLOCK_DATA: data is read from a file returned by bdrv_get_block_status. - * BDRV_BLOCK_ZERO: sectors read as zero - * BDRV_BLOCK_OFFSET_VALID: sector stored as raw data in a file returned by - * bdrv_get_block_status. + * Allocation status flags for bdrv_get_block_status() and friends. + * + * Public flags: + * BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer + * BDRV_BLOCK_ZERO: offset reads as zero + * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this - * layer (as opposed to the backing file) - * BDRV_BLOCK_RAW: used internally to indicate that the request - * was answered by the raw driver and that one - * should look in bs->file directly. + * layer (short for DATA || ZERO), set by block layer * - * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in - * bs->file where sector data can be read from as raw data. + * Internal flag: + * BDRV_BLOCK_RAW: used internally to indicate that the request was + * answered by a passthrough driver such as raw and that the + * block layer should recompute the answer from bs->file. * - * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present. + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) + * represent the offset in the returned BDS that is allocated for the + * corresponding raw data; however, whether that offset actually contains + * data also depends on BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO, as follows: * * DATA ZERO OFFSET_VALID - * t t t sectors read as zero, bs->file is zero at offset - * t f t sectors read as valid from bs->file at offset - * f t t sectors preallocated, read as zero, bs->file not + * t t t sectors read as zero, returned file is zero at offset + * t f t sectors read as valid from file at offset + * 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, - * bs->file contains garbage at offset + * returned file contains garbage at offset * t t f sectors preallocated, read as zero, unknown offset * t f f sectors read from unknown file or offset * f t f not allocated or unknown offset, read as zero diff --git a/include/block/block_int.h b/include/block/block_int.h index 8773940..1fdfff7 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -165,6 +165,13 @@ struct BlockDriver { int64_t offset, int count, BdrvRequestFlags flags); int coroutine_fn (*bdrv_co_pdiscard)(BlockDriverState *bs, int64_t offset, int count); + + /* + * Building block for bdrv_block_status[_above]. The driver should + * answer only according to the current layer, and should not + * set BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h + * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. + */ int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file);
We had some conflicting documentation: a nice 8-way table that described all possible combinations of DATA, ZERO, and OFFSET_VALID, contrasted with text that implied that OFFSET_VALID always meant raw data could be read directly. Furthermore, the text refers a lot to bs->file, even though the interface was updated back in 67a0fd2a to let the driver pass back which BDS (not necessarily bs->file). As the 8-way table is the intended semantics, simplify the rest of the text to get rid of the confusion. ALLOCATED is always set by the block layer for convenience (drivers do not have to worry about it). RAW is used only internally, but by more than the raw driver. Document these additional items on the driver callback. Suggested-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- v12: even more wording tweaks v11: reserved for blkdebug half of v10 v10: new patch --- include/block/block.h | 35 +++++++++++++++++++---------------- include/block/block_int.h | 7 +++++++ 2 files changed, 26 insertions(+), 16 deletions(-)