Re: [Qemu-devel] [PATCH v5 2/3] block/io: bdrv_pdiscard: support int64_t bytes parameter

[Stefano Garzarella <sgarzare@redhat.com>]

On Tue, Apr 30, 2019 at 10:03:08AM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 30.04.2019 12:24, Stefano Garzarella wrote:
> > On Tue, Apr 23, 2019 at 03:57:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> This fixes at least one overflow in qcow2_process_discards, which
> >> passes 64bit region length to bdrv_pdiscard where bytes (or sectors in
> >> the past) parameter is int since its introduction in 0b919fae.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >>   include/block/block.h |  4 ++--
> >>   block/io.c            | 16 ++++++++--------
> >>   2 files changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/include/block/block.h b/include/block/block.h
> >> index c7a26199aa..69fa18867e 100644
> >> --- a/include/block/block.h
> >> +++ b/include/block/block.h
> >> @@ -432,8 +432,8 @@ void bdrv_drain_all(void);
> >>       AIO_WAIT_WHILE(bdrv_get_aio_context(bs_),              \
> >>                      cond); })
> >>   
> >> -int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes);
> >> -int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes);
> >> +int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
> >> +int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
> >>   int bdrv_has_zero_init_1(BlockDriverState *bs);
> >>   int bdrv_has_zero_init(BlockDriverState *bs);
> >>   bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
> >> diff --git a/block/io.c b/block/io.c
> >> index dfc153b8d8..16b6c5d855 100644
> >> --- a/block/io.c
> >> +++ b/block/io.c
> >> @@ -2653,7 +2653,7 @@ int bdrv_flush(BlockDriverState *bs)
> >>   typedef struct DiscardCo {
> >>       BdrvChild *child;
> >>       int64_t offset;
> >> -    int bytes;
> >> +    int64_t bytes;
> >>       int ret;
> >>   } DiscardCo;
> >>   static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque)
> >> @@ -2664,14 +2664,15 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque)
> >>       aio_wait_kick();
> >>   }
> >>   
> >> -int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
> >> +int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
> >> +                                  int64_t bytes)
> >>   {
> >>       BdrvTrackedRequest req;
> >>       int max_pdiscard, ret;
> >>       int head, tail, align;
> >>       BlockDriverState *bs = child->bs;
> >>   
> >> -    if (!bs || !bs->drv) {
> >> +    if (!bs || !bs->drv || !bdrv_is_inserted(bs)) {
> > 
> > Should we describe this change in the commit message?
> 
> Honestly, don't want to resend the series for this.
> 
> > IIUC you added this check because you removed bdrv_check_byte_request()
> > below,
> > 
> > Maybe we can also remove '!bs->drv', since it is checked in
> > bdrv_is_inserted().
> 
> Hmm, on v4 Kevin commented, that bdrv_is_inserted not needed, and, as I understand, not only
> in bdrv_co_pdiscard it should be removed, but it may be done later. So, I'd prefer to keep it
> as is for now.
> 

Make sense if it will be removed.

> > 
> >>           return -ENOMEDIUM;
> >>       }
> >>   
> >> @@ -2679,9 +2680,8 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
> >>           return -EPERM;
> >>       }
> >>   
> >> -    ret = bdrv_check_byte_request(bs, offset, bytes);
> >> -    if (ret < 0) {
> >> -        return ret;
> >> +    if (offset < 0 || bytes < 0 || bytes > INT64_MAX - offset) {
> >> +        return -EIO;
> >>       }
> > 
> > Should we check if 'bytes' is greater than
> > 'BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS'?
> > 
> 
> No, as we are contrariwise trying to support large bytes parameter in bdrv_co_pdiscard, which will
> exceed max request. If @bytes is large, it will be divided into several smaller requests in the
> following loop.
> 

I understood.
I saw that we limit the request to the driver to 'max_pdiscard' or 'INT_MAX'.

As future work, could we use int64_t also for the driver callbacks?

Anyway, the patch LGTM.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano