| Message ID | 20210617155247.442150-5-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: block-status cache for data regions | expand |
On Thu, Jun 17, 2021 at 05:52:45PM +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. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/gluster.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) 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> > --- > block/gluster.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index e8ee14c8e9..8ef7bb18d5 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -1461,7 +1461,8 @@ exit: > * 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. > * > * (Based on raw_co_block_status() from file-posix.c.) > */ > @@ -1500,12 +1501,12 @@ static int coroutine_fn qemu_gluster_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; > ret = BDRV_BLOCK_DATA; Interesting, isn't it a bug that we don't ROUND_UP *pnum to request_alignment here like it is done in file-posix ? > } 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; > } > >
On 19.06.21 12:36, Vladimir Sementsov-Ogievskiy wrote: > 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> > > >> --- >> block/gluster.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/block/gluster.c b/block/gluster.c >> index e8ee14c8e9..8ef7bb18d5 100644 >> --- a/block/gluster.c >> +++ b/block/gluster.c >> @@ -1461,7 +1461,8 @@ exit: >> * 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. >> * >> * (Based on raw_co_block_status() from file-posix.c.) >> */ >> @@ -1500,12 +1501,12 @@ static int coroutine_fn >> qemu_gluster_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; >> ret = BDRV_BLOCK_DATA; > > Interesting, isn't it a bug that we don't ROUND_UP *pnum to > request_alignment here like it is done in file-posix ? Guess I forgot gluster in 9c3db310ff0 O:) I don’t think I’ll be able to reproduce it for gluster, but I suppose just doing the same thing for gluster should be fine... Max
diff --git a/block/gluster.c b/block/gluster.c index e8ee14c8e9..8ef7bb18d5 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -1461,7 +1461,8 @@ exit: * 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. * * (Based on raw_co_block_status() from file-posix.c.) */ @@ -1500,12 +1501,12 @@ static int coroutine_fn qemu_gluster_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; ret = BDRV_BLOCK_DATA; } 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; }
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/gluster.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)