| Message ID | 20210617155247.442150-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: block-status cache for data regions | expand |
On Thu, Jun 17, 2021 at 05:52:44PM +0200, Max Reitz wrote: > bdrv_co_block_status() does it for us, we do not need to do it here. > > The advantage of not capping *pnum is that bdrv_co_block_status() can > cache larger data regions than requested by its caller. We should update the documentation in include/block/block_int.h to mention that the driver's block_status callback may treat *pnum as a soft cap, and that returning a larger value is fine. But I agree with this change in the individual drivers, as long as we remember to make our global contract explicit that we can now rely on it ;) Reviewed-by: Eric Blake <eblake@redhat.com>
17.06.2021 18:52, Max Reitz wrote: > bdrv_co_block_status() does it for us, we do not need to do it here. > > The advantage of not capping *pnum is that bdrv_co_block_status() can > cache larger data regions than requested by its caller. > > Signed-off-by: Max Reitz<mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 18.06.21 22:16, Eric Blake wrote: > On Thu, Jun 17, 2021 at 05:52:44PM +0200, Max Reitz wrote: >> bdrv_co_block_status() does it for us, we do not need to do it here. >> >> The advantage of not capping *pnum is that bdrv_co_block_status() can >> cache larger data regions than requested by its caller. > We should update the documentation in include/block/block_int.h to > mention that the driver's block_status callback may treat *pnum as a > soft cap, and that returning a larger value is fine. Oh, sure. Max > But I agree with this change in the individual drivers, as long as we > remember to make our global contract explicit that we can now rely on > it ;) > > Reviewed-by: Eric Blake <eblake@redhat.com> >
diff --git a/block/file-posix.c b/block/file-posix.c index b3fbb9bd63..aeb370d5bb 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2689,7 +2689,8 @@ static int find_allocation(BlockDriverState *bs, off_t start, * the specified offset) that are known to be in the same * allocated/unallocated state. * - * 'bytes' is the max value 'pnum' should be set to. + * 'bytes' is a soft cap for 'pnum'. If the information is free, 'pnum' may + * well exceed it. */ static int coroutine_fn raw_co_block_status(BlockDriverState *bs, bool want_zero, @@ -2727,7 +2728,7 @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs, } else if (data == offset) { /* On a data extent, compute bytes to the end of the extent, * possibly including a partial sector at EOF. */ - *pnum = MIN(bytes, hole - offset); + *pnum = hole - offset; /* * We are not allowed to return partial sectors, though, so @@ -2746,7 +2747,7 @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs, } else { /* On a hole, compute bytes to the beginning of the next extent. */ assert(hole == offset); - *pnum = MIN(bytes, data - offset); + *pnum = data - offset; ret = BDRV_BLOCK_ZERO; } *map = offset;
bdrv_co_block_status() does it for us, we do not need to do it here. The advantage of not capping *pnum is that bdrv_co_block_status() can cache larger data regions than requested by its caller. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/file-posix.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)