[03/17] block: Switch BlockRequest to byte-based

Commit Message

Eric Blake June 22, 2016, 3:51 p.m. UTC

BlockRequest is the internal struct used by bdrv_aio_*.  At the
moment, all such calls were sector-based, but we will eventually
convert to byte-based; start by changing the internal variables
to be byte-based.  No change to behavior, although the read and
write code can now go byte-based through more of the stack.

Signed-off-by: Eric Blake <[e-mail]>
---
 block/io.c | 64 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

Comments

Stefan Hajnoczi July 14, 2016, 12:15 p.m. UTC | #1

On Wed, Jun 22, 2016 at 09:51:00AM -0600, Eric Blake wrote:
> @@ -2204,14 +2203,15 @@ BlockAIOCB *bdrv_aio_discard(BlockDriverState *bs,
>  {
>      Coroutine *co;
>      BlockAIOCBCoroutine *acb;
> +    QEMUIOVector qiov = { .size = nb_sectors << BDRV_SECTOR_BITS, };
> 
>      trace_bdrv_aio_discard(bs, sector_num, nb_sectors, opaque);
> 
>      acb = qemu_aio_get(&bdrv_em_co_aiocb_info, bs, cb, opaque);
>      acb->need_bh = true;
>      acb->req.error = -EINPROGRESS;
> -    acb->req.sector = sector_num;
> -    acb->req.nb_sectors = nb_sectors;
> +    acb->req.offset = sector_num << BDRV_SECTOR_BITS;
> +    acb->req.qiov = &qiov;

This looks unsafe: the pointer to a stack-allocated qiov is held after
the function returns.

Eric Blake July 14, 2016, 12:33 p.m. UTC | #2

On 07/14/2016 06:15 AM, Stefan Hajnoczi wrote:
> On Wed, Jun 22, 2016 at 09:51:00AM -0600, Eric Blake wrote:
>> @@ -2204,14 +2203,15 @@ BlockAIOCB *bdrv_aio_discard(BlockDriverState *bs,
>>  {
>>      Coroutine *co;
>>      BlockAIOCBCoroutine *acb;
>> +    QEMUIOVector qiov = { .size = nb_sectors << BDRV_SECTOR_BITS, };
>>
>>      trace_bdrv_aio_discard(bs, sector_num, nb_sectors, opaque);
>>
>>      acb = qemu_aio_get(&bdrv_em_co_aiocb_info, bs, cb, opaque);
>>      acb->need_bh = true;
>>      acb->req.error = -EINPROGRESS;
>> -    acb->req.sector = sector_num;
>> -    acb->req.nb_sectors = nb_sectors;
>> +    acb->req.offset = sector_num << BDRV_SECTOR_BITS;
>> +    acb->req.qiov = &qiov;
> 
> This looks unsafe: the pointer to a stack-allocated qiov is held after
> the function returns.

Hmm, you're right.  I'll definitely have to rework this one.  But since
the ONLY thing being passed through the qiov was the size, it may be
easiest to just add acb->req.size.

Patch

diff --git a/block/io.c b/block/io.c
index 305e5c5..d442ce7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -33,14 +33,13 @@ 

 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */

-static BlockAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
-                                         int64_t sector_num,
-                                         QEMUIOVector *qiov,
-                                         int nb_sectors,
-                                         BdrvRequestFlags flags,
-                                         BlockCompletionFunc *cb,
-                                         void *opaque,
-                                         bool is_write);
+static BlockAIOCB *bdrv_co_aio_prw_vector(BlockDriverState *bs,
+                                          int64_t offset,
+                                          QEMUIOVector *qiov,
+                                          BdrvRequestFlags flags,
+                                          BlockCompletionFunc *cb,
+                                          void *opaque,
+                                          bool is_write);
 static void coroutine_fn bdrv_co_do_rw(void *opaque);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int count, BdrvRequestFlags flags);
@@ -2010,8 +2009,9 @@  BlockAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
 {
     trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);

-    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
-                                 cb, opaque, false);
+    assert(nb_sectors << BDRV_SECTOR_BITS == qiov->size);
+    return bdrv_co_aio_prw_vector(bs, sector_num << BDRV_SECTOR_BITS, qiov, 0,
+                                  cb, opaque, false);
 }

 BlockAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
@@ -2020,8 +2020,9 @@  BlockAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
 {
     trace_bdrv_aio_writev(bs, sector_num, nb_sectors, opaque);

-    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
-                                 cb, opaque, true);
+    assert(nb_sectors << BDRV_SECTOR_BITS == qiov->size);
+    return bdrv_co_aio_prw_vector(bs, sector_num << BDRV_SECTOR_BITS, qiov, 0,
+                                  cb, opaque, true);
 }

 void bdrv_aio_cancel(BlockAIOCB *acb)
@@ -2057,8 +2058,8 @@  typedef struct BlockRequest {
     union {
         /* Used during read, write, trim */
         struct {
-            int64_t sector;
-            int nb_sectors;
+            int64_t offset;
+            int bytes;
             int flags;
             QEMUIOVector *qiov;
         };
@@ -2122,24 +2123,23 @@  static void coroutine_fn bdrv_co_do_rw(void *opaque)
     BlockDriverState *bs = acb->common.bs;

     if (!acb->is_write) {
-        acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
-            acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
+        acb->req.error = bdrv_co_preadv(bs, acb->req.offset,
+            acb->req.qiov->size, acb->req.qiov, acb->req.flags);
     } else {
-        acb->req.error = bdrv_co_do_writev(bs, acb->req.sector,
-            acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
+        acb->req.error = bdrv_co_pwritev(bs, acb->req.offset,
+            acb->req.qiov->size, acb->req.qiov, acb->req.flags);
     }

     bdrv_co_complete(acb);
 }

-static BlockAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
-                                         int64_t sector_num,
-                                         QEMUIOVector *qiov,
-                                         int nb_sectors,
-                                         BdrvRequestFlags flags,
-                                         BlockCompletionFunc *cb,
-                                         void *opaque,
-                                         bool is_write)
+static BlockAIOCB *bdrv_co_aio_prw_vector(BlockDriverState *bs,
+                                          int64_t offset,
+                                          QEMUIOVector *qiov,
+                                          BdrvRequestFlags flags,
+                                          BlockCompletionFunc *cb,
+                                          void *opaque,
+                                          bool is_write)
 {
     Coroutine *co;
     BlockAIOCBCoroutine *acb;
@@ -2147,8 +2147,7 @@  static BlockAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
     acb = qemu_aio_get(&bdrv_em_co_aiocb_info, bs, cb, opaque);
     acb->need_bh = true;
     acb->req.error = -EINPROGRESS;
-    acb->req.sector = sector_num;
-    acb->req.nb_sectors = nb_sectors;
+    acb->req.offset = offset;
     acb->req.qiov = qiov;
     acb->req.flags = flags;
     acb->is_write = is_write;
@@ -2193,8 +2192,8 @@  static void coroutine_fn bdrv_aio_discard_co_entry(void *opaque)
     BlockAIOCBCoroutine *acb = opaque;
     BlockDriverState *bs = acb->common.bs;

-    acb->req.error = bdrv_co_pdiscard(bs, acb->req.sector << BDRV_SECTOR_BITS,
-                                      acb->req.nb_sectors << BDRV_SECTOR_BITS);
+    acb->req.error = bdrv_co_pdiscard(bs, acb->req.offset,
+                                      acb->req.qiov->size);
     bdrv_co_complete(acb);
 }

@@ -2204,14 +2203,15 @@  BlockAIOCB *bdrv_aio_discard(BlockDriverState *bs,
 {
     Coroutine *co;
     BlockAIOCBCoroutine *acb;
+    QEMUIOVector qiov = { .size = nb_sectors << BDRV_SECTOR_BITS, };

     trace_bdrv_aio_discard(bs, sector_num, nb_sectors, opaque);

     acb = qemu_aio_get(&bdrv_em_co_aiocb_info, bs, cb, opaque);
     acb->need_bh = true;
     acb->req.error = -EINPROGRESS;
-    acb->req.sector = sector_num;
-    acb->req.nb_sectors = nb_sectors;
+    acb->req.offset = sector_num << BDRV_SECTOR_BITS;
+    acb->req.qiov = &qiov;
     co = qemu_coroutine_create(bdrv_aio_discard_co_entry);
     qemu_coroutine_enter(co, acb);