[PATCHv7,10/17] block: honour BlockLimits in bdrv_co_discard

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

Am 24.10.2013 um 12:06 hat Peter Lieven geschrieben:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c |   37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 0c0b0ac..b28dd42 100644
> --- a/block.c
> +++ b/block.c
> @@ -4234,6 +4234,11 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
>      rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
>  }
>  
> +/* if no limit is specified in the BlockLimits use a default
> + * of 32768 512-byte sectors (16 MiB) per request.
> + */
> +#define MAX_DISCARD_DEFAULT 32768
> +
>  int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>                                   int nb_sectors)
>  {
> @@ -4255,7 +4260,37 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>      }
>  
>      if (bs->drv->bdrv_co_discard) {
> -        return bs->drv->bdrv_co_discard(bs, sector_num, nb_sectors);
> +        int max_discard = bs->bl.max_discard ?
> +                          bs->bl.max_discard : MAX_DISCARD_DEFAULT;
> +
> +        while (nb_sectors > 0) {
> +            int ret;
> +            int num = nb_sectors;
> +
> +            /* align request */
> +            if (bs->bl.discard_alignment &&
> +                num >= bs->bl.discard_alignment &&
> +                sector_num % bs->bl.discard_alignment) {
> +                if (num > bs->bl.discard_alignment) {
> +                    num = bs->bl.discard_alignment;
> +                }
> +                num -= sector_num % bs->bl.discard_alignment;
> +            }
> +
> +            /* limit request size */
> +            if (num > max_discard) {
> +                num = max_discard;
> +            }
> +
> +            ret = bs->drv->bdrv_co_discard(bs, sector_num, num);
> +            if (ret) {
> +                return ret;
> +            }
> +
> +            sector_num += num;
> +            nb_sectors -= num;
> +        }
> +        return 0;
>      } else if (bs->drv->bdrv_aio_discard) {
>          BlockDriverAIOCB *acb;
>          CoroutineIOCompletion co = {

You're only optimising drivers which have a .bdrv_co_discard()
implementation, but not those with .bdrv_aio_discard(). Not very nice,
and it would have been easy to avoid this by putting the loop around the
whole if statement instead of inside the 'then' branch.

Kevin

On 11.11.2013 14:20, Kevin Wolf wrote:
> Am 24.10.2013 um 12:06 hat Peter Lieven geschrieben:
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block.c |   37 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 0c0b0ac..b28dd42 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4234,6 +4234,11 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
>>       rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
>>   }
>>   
>> +/* if no limit is specified in the BlockLimits use a default
>> + * of 32768 512-byte sectors (16 MiB) per request.
>> + */
>> +#define MAX_DISCARD_DEFAULT 32768
>> +
>>   int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>                                    int nb_sectors)
>>   {
>> @@ -4255,7 +4260,37 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>       }
>>   
>>       if (bs->drv->bdrv_co_discard) {
>> -        return bs->drv->bdrv_co_discard(bs, sector_num, nb_sectors);
>> +        int max_discard = bs->bl.max_discard ?
>> +                          bs->bl.max_discard : MAX_DISCARD_DEFAULT;
>> +
>> +        while (nb_sectors > 0) {
>> +            int ret;
>> +            int num = nb_sectors;
>> +
>> +            /* align request */
>> +            if (bs->bl.discard_alignment &&
>> +                num >= bs->bl.discard_alignment &&
>> +                sector_num % bs->bl.discard_alignment) {
>> +                if (num > bs->bl.discard_alignment) {
>> +                    num = bs->bl.discard_alignment;
>> +                }
>> +                num -= sector_num % bs->bl.discard_alignment;
>> +            }
>> +
>> +            /* limit request size */
>> +            if (num > max_discard) {
>> +                num = max_discard;
>> +            }
>> +
>> +            ret = bs->drv->bdrv_co_discard(bs, sector_num, num);
>> +            if (ret) {
>> +                return ret;
>> +            }
>> +
>> +            sector_num += num;
>> +            nb_sectors -= num;
>> +        }
>> +        return 0;
>>       } else if (bs->drv->bdrv_aio_discard) {
>>           BlockDriverAIOCB *acb;
>>           CoroutineIOCompletion co = {
> You're only optimising drivers which have a .bdrv_co_discard()
> implementation, but not those with .bdrv_aio_discard(). Not very nice,
> and it would have been easy to avoid this by putting the loop around the
> whole if statement instead of inside the 'then' branch.
Good point. I wonder noone noticed before ;-)

Do you want me to respin or is ok to send a follow up patch?
Stefan has it already in block-next. This patch doesn't make
the situation worse and we need follow up patches for
all the drivers to supply alignment information anyway.

Peter

Il 12/11/2013 09:53, Peter Lieven ha scritto:
> Good point. I wonder noone noticed before ;-)
> 
> Do you want me to respin or is ok to send a follow up patch?
> Stefan has it already in block-next. This patch doesn't make
> the situation worse and we need follow up patches for
> all the drivers to supply alignment information anyway.

I can do it too, since I'm working on follow-up patches anyway.

Paolo

On 12.11.2013 10:19, Paolo Bonzini wrote:
> Il 12/11/2013 09:53, Peter Lieven ha scritto:
>> Good point. I wonder noone noticed before ;-)
>>
>> Do you want me to respin or is ok to send a follow up patch?
>> Stefan has it already in block-next. This patch doesn't make
>> the situation worse and we need follow up patches for
>> all the drivers to supply alignment information anyway.
> I can do it too, since I'm working on follow-up patches anyway.
That would be great if Kevin does not insist on a respin.

Thank you.

Am 12.11.2013 um 10:21 hat Peter Lieven geschrieben:
> On 12.11.2013 10:19, Paolo Bonzini wrote:
> >Il 12/11/2013 09:53, Peter Lieven ha scritto:
> >>Good point. I wonder noone noticed before ;-)
> >>
> >>Do you want me to respin or is ok to send a follow up patch?
> >>Stefan has it already in block-next. This patch doesn't make
> >>the situation worse and we need follow up patches for
> >>all the drivers to supply alignment information anyway.
> >I can do it too, since I'm working on follow-up patches anyway.
> That would be great if Kevin does not insist on a respin.

A follow-up patch is fine.

Kevin