[v2,13/24] block: Introduce bdrv_co_do_pwritev()

This is going to become the bdrv_co_do_preadv() equivalent for writes.
In this patch, however, just a function taking byte offsets is created,
it doesn't align anything yet.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

On 13.12.2013 14:22, Kevin Wolf wrote:
> This is going to become the bdrv_co_do_preadv() equivalent for writes.
> In this patch, however, just a function taking byte offsets is created,
> it doesn't align anything yet.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index 385fb8a..a80db2e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3010,8 +3010,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>   /*
>    * Handle a write request in coroutine context
>    */
> -static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> -    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
> +static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>       BdrvRequestFlags flags)
>   {
>       int ret;
> @@ -3022,21 +3022,32 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>       if (bs->read_only) {
>           return -EACCES;
>       }
> -    if (bdrv_check_request(bs, sector_num, nb_sectors)) {
> +    if (bdrv_check_byte_request(bs, offset, bytes)) {
>           return -EIO;
>       }
>   
>       /* throttling disk I/O */
>       if (bs->io_limits_enabled) {
> -        bdrv_io_limits_intercept(bs, nb_sectors, true);
> +        bdrv_io_limits_intercept(bs, bytes << BDRV_SECTOR_BITS, true);
>       }
>   
> -    ret = bdrv_aligned_pwritev(bs, sector_num << BDRV_SECTOR_BITS,
> -                               nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
> +    ret = bdrv_aligned_pwritev(bs, offset, bytes, qiov, flags);
>   
>       return ret;
>   }
>   
> +static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
> +    BdrvRequestFlags flags)
> +{
> +    if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) {

This should probably be INT_MAX, since nb_sectors is an integer. If 
nb_sectors is between INT_MAX >> BDRV_SECTOR_BITS and UINT_MAX >> 
BDRV_SECTOR_BITS, the result of nb_sectors << BDRV_SECTOR_BITS (which 
will be a signed integer) is undefined. It is obviously then implicitly 
casted to an unsigned integer (which is the type of the parameter 
"bytes") which will probably solve the problem as intended, but it is 
still technically undefined.

Thus, I'd either change this to INT_MAX or cast nb_sectors to an 
unsigned int before shifting it below.

Max

> +        return -EINVAL;
> +    }
> +
> +    return bdrv_co_do_pwritev(bs, sector_num << BDRV_SECTOR_BITS,
> +                              nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
> +}
> +
>   int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
>       int nb_sectors, QEMUIOVector *qiov)
>   {

Am 13.12.2013 um 14:22 hat Kevin Wolf geschrieben:
> This is going to become the bdrv_co_do_preadv() equivalent for writes.
> In this patch, however, just a function taking byte offsets is created,
> it doesn't align anything yet.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 385fb8a..a80db2e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3010,8 +3010,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>  /*
>   * Handle a write request in coroutine context
>   */
> -static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> -    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
> +static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>      BdrvRequestFlags flags)
>  {
>      int ret;
> @@ -3022,21 +3022,32 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>      if (bs->read_only) {
>          return -EACCES;
>      }
> -    if (bdrv_check_request(bs, sector_num, nb_sectors)) {
> +    if (bdrv_check_byte_request(bs, offset, bytes)) {
>          return -EIO;
>      }
>  
>      /* throttling disk I/O */
>      if (bs->io_limits_enabled) {
> -        bdrv_io_limits_intercept(bs, nb_sectors, true);
> +        bdrv_io_limits_intercept(bs, bytes << BDRV_SECTOR_BITS, true);
>      }

Oh nice, this shifts in the wrong direction.

If somebody feels like writing a test case, something testing that I/O
throttling actually throttles would be useful...

Kevin

On Thu, 01/16 13:25, Kevin Wolf wrote:
> Am 13.12.2013 um 14:22 hat Kevin Wolf geschrieben:
> > This is going to become the bdrv_co_do_preadv() equivalent for writes.
> > In this patch, however, just a function taking byte offsets is created,
> > it doesn't align anything yet.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 385fb8a..a80db2e 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -3010,8 +3010,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
> >  /*
> >   * Handle a write request in coroutine context
> >   */
> > -static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> > -    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
> > +static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
> > +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
> >      BdrvRequestFlags flags)
> >  {
> >      int ret;
> > @@ -3022,21 +3022,32 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> >      if (bs->read_only) {
> >          return -EACCES;
> >      }
> > -    if (bdrv_check_request(bs, sector_num, nb_sectors)) {
> > +    if (bdrv_check_byte_request(bs, offset, bytes)) {
> >          return -EIO;
> >      }
> >  
> >      /* throttling disk I/O */
> >      if (bs->io_limits_enabled) {
> > -        bdrv_io_limits_intercept(bs, nb_sectors, true);
> > +        bdrv_io_limits_intercept(bs, bytes << BDRV_SECTOR_BITS, true);
> >      }
> 
> Oh nice, this shifts in the wrong direction.
> 
> If somebody feels like writing a test case, something testing that I/O
> throttling actually throttles would be useful...
> 

Good idea. Should be possible for a basic test in qemu-iotests. I'll give it a
try.

Thanks,
Fam