diff mbox series

[6/8] virtio-blk: mark IO_CODE functions

Message ID 20220609143727.1151816-7-eesposit@redhat.com
State New
Headers show
Series virtio-blk: removal of AioContext lock | expand

Commit Message

Emanuele Giuseppe Esposito June 9, 2022, 2:37 p.m. UTC 
Just as done in the block API, mark functions in virtio-blk
that are called also from iothread(s).

We know such functions are IO because many are blk_* callbacks,
running always in the device iothread, and remaining are propagated
from the leaf IO functions (if a function calls a IO_CODE function,
itself is categorized as IO_CODE too).

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  4 ++++
 hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

Comments

Stefan Hajnoczi July 5, 2022, 2:39 p.m. UTC | #1 
On Thu, Jun 09, 2022 at 10:37:25AM -0400, Emanuele Giuseppe Esposito wrote:
> Just as done in the block API, mark functions in virtio-blk
> that are called also from iothread(s).
> 
> We know such functions are IO because many are blk_* callbacks,
> running always in the device iothread, and remaining are propagated
> from the leaf IO functions (if a function calls a IO_CODE function,
> itself is categorized as IO_CODE too).
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c |  4 ++++
>  hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)

The definition of IO_CODE() is:

  I/O API functions. These functions are thread-safe, and therefore
  can run in any thread as long as the thread has called
  aio_context_acquire/release().

I'm not sure it matches with the exact semantics you have in mind. Are
they really allowed to be called from any thread and even from multiple
threads? Or maybe just from the BlockBackend's AioContext thread?

We need to be very careful to define these terms precisely and avoid
applying them in cases that are similar but different as that will cause
problems in the future.

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Emanuele Giuseppe Esposito July 8, 2022, 9:19 a.m. UTC | #2 
Am 05/07/2022 um 16:39 schrieb Stefan Hajnoczi:
> On Thu, Jun 09, 2022 at 10:37:25AM -0400, Emanuele Giuseppe Esposito wrote:
>> Just as done in the block API, mark functions in virtio-blk
>> that are called also from iothread(s).
>>
>> We know such functions are IO because many are blk_* callbacks,
>> running always in the device iothread, and remaining are propagated
>> from the leaf IO functions (if a function calls a IO_CODE function,
>> itself is categorized as IO_CODE too).
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>  hw/block/dataplane/virtio-blk.c |  4 ++++
>>  hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
>>  2 files changed, 39 insertions(+)
> 
> The definition of IO_CODE() is:
> 
>   I/O API functions. These functions are thread-safe, and therefore
>   can run in any thread as long as the thread has called
>   aio_context_acquire/release().
> 
> I'm not sure it matches with the exact semantics you have in mind. Are
> they really allowed to be called from any thread and even from multiple
> threads? Or maybe just from the BlockBackend's AioContext thread?

I think it is just from the BlockBackend's AioContext thread. But I
classified blk_* functions as IO_CODE.

What is your opinion on that?

> 
> We need to be very careful to define these terms precisely and avoid
> applying them in cases that are similar but different as that will cause
> problems in the future.
> 
> Otherwise:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
Stefan Hajnoczi July 12, 2022, 12:26 p.m. UTC | #3 
On Fri, Jul 08, 2022 at 11:19:43AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/07/2022 um 16:39 schrieb Stefan Hajnoczi:
> > On Thu, Jun 09, 2022 at 10:37:25AM -0400, Emanuele Giuseppe Esposito wrote:
> >> Just as done in the block API, mark functions in virtio-blk
> >> that are called also from iothread(s).
> >>
> >> We know such functions are IO because many are blk_* callbacks,
> >> running always in the device iothread, and remaining are propagated
> >> from the leaf IO functions (if a function calls a IO_CODE function,
> >> itself is categorized as IO_CODE too).
> >>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >> ---
> >>  hw/block/dataplane/virtio-blk.c |  4 ++++
> >>  hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
> >>  2 files changed, 39 insertions(+)
> > 
> > The definition of IO_CODE() is:
> > 
> >   I/O API functions. These functions are thread-safe, and therefore
> >   can run in any thread as long as the thread has called
> >   aio_context_acquire/release().
> > 
> > I'm not sure it matches with the exact semantics you have in mind. Are
> > they really allowed to be called from any thread and even from multiple
> > threads? Or maybe just from the BlockBackend's AioContext thread?
> 
> I think it is just from the BlockBackend's AioContext thread. But I
> classified blk_* functions as IO_CODE.
> 
> What is your opinion on that?

There is a difference between blk_*() APIs and device emulation code.
Device emulation code controls exactly where it runs (vCPU thread, main
loop, IOThread). blk_*() APIs may be called from more of contexts and
they have no control over it.

I'd like to make sure that the annotations match the actual usage that
the code was designed for.

Stefan
diff mbox series

Patch

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index bda6b3e8de..9dc6347350 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -63,6 +63,8 @@  static void notify_guest_bh(void *opaque)
     unsigned long bitmap[BITS_TO_LONGS(nvqs)];
     unsigned j;
 
+    IO_CODE();
+
     memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
     memset(s->batch_notify_vqs, 0, sizeof(bitmap));
 
@@ -299,6 +301,8 @@  static void virtio_blk_data_plane_stop_bh(void *opaque)
     VirtIOBlockDataPlane *s = opaque;
     unsigned i;
 
+    IO_CODE();
+
     for (i = 0; i < s->conf->num_queues; i++) {
         VirtQueue *vq = virtio_get_queue(s->vdev, i);
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 2eb0408f92..e1aaa606ba 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -62,6 +62,8 @@  static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
 {
+    IO_CODE();
+
     req->dev = s;
     req->vq = vq;
     req->qiov.size = 0;
@@ -80,6 +82,8 @@  static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
+    IO_CODE();
+
     trace_virtio_blk_req_complete(vdev, req, status);
 
     stb_p(&req->in->status, status);
@@ -99,6 +103,8 @@  static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     VirtIOBlock *s = req->dev;
     BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
 
+    IO_CODE();
+
     if (action == BLOCK_ERROR_ACTION_STOP) {
         /* Break the link as the next request is going to be parsed from the
          * ring again. Otherwise we may end up doing a double completion! */
@@ -166,7 +172,9 @@  static void virtio_blk_flush_complete(void *opaque, int ret)
     VirtIOBlockReq *req = opaque;
     VirtIOBlock *s = req->dev;
 
+    IO_CODE();
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
+
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, 0, true)) {
             goto out;
@@ -188,7 +196,9 @@  static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
     bool is_write_zeroes = (virtio_ldl_p(VIRTIO_DEVICE(s), &req->out.type) &
                             ~VIRTIO_BLK_T_BARRIER) == VIRTIO_BLK_T_WRITE_ZEROES;
 
+    IO_CODE();
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
+
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, false, is_write_zeroes)) {
             goto out;
@@ -221,6 +231,8 @@  static void virtio_blk_ioctl_complete(void *opaque, int status)
     struct virtio_scsi_inhdr *scsi;
     struct sg_io_hdr *hdr;
 
+    IO_CODE();
+
     scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
 
     if (status) {
@@ -262,6 +274,8 @@  static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s, VirtQueue *vq)
 {
     VirtIOBlockReq *req = virtqueue_pop(vq, sizeof(VirtIOBlockReq));
 
+    IO_CODE();
+
     if (req) {
         virtio_blk_init_request(s, vq, req);
     }
@@ -282,6 +296,8 @@  static int virtio_blk_handle_scsi_req(VirtIOBlockReq *req)
     BlockAIOCB *acb;
 #endif
 
+    IO_CODE();
+
     /*
      * We require at least one output segment each for the virtio_blk_outhdr
      * and the SCSI command block.
@@ -380,6 +396,7 @@  fail:
 static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
     int status;
+    IO_CODE();
 
     status = virtio_blk_handle_scsi_req(req);
     if (status != -EINPROGRESS) {
@@ -395,6 +412,8 @@  static inline void submit_requests(BlockBackend *blk, MultiReqBuffer *mrb,
     int64_t sector_num = mrb->reqs[start]->sector_num;
     bool is_write = mrb->is_write;
 
+    IO_CODE();
+
     if (num_reqs > 1) {
         int i;
         struct iovec *tmp_iov = qiov->iov;
@@ -438,6 +457,8 @@  static int multireq_compare(const void *a, const void *b)
     const VirtIOBlockReq *req1 = *(VirtIOBlockReq **)a,
                          *req2 = *(VirtIOBlockReq **)b;
 
+    IO_CODE();
+
     /*
      * Note that we can't simply subtract sector_num1 from sector_num2
      * here as that could overflow the return value.
@@ -457,6 +478,8 @@  static void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
     uint32_t max_transfer;
     int64_t sector_num = 0;
 
+    IO_CODE();
+
     if (mrb->num_reqs == 1) {
         submit_requests(blk, mrb, 0, 1, -1);
         mrb->num_reqs = 0;
@@ -506,6 +529,8 @@  static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     VirtIOBlock *s = req->dev;
 
+    IO_CODE();
+
     block_acct_start(blk_get_stats(s->blk), &req->acct, 0,
                      BLOCK_ACCT_FLUSH);
 
@@ -524,6 +549,8 @@  static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
     uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
     uint64_t total_sectors;
 
+    IO_CODE();
+
     if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return false;
     }
@@ -550,6 +577,8 @@  static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
     uint8_t err_status;
     int bytes;
 
+    IO_CODE();
+
     sector = virtio_ldq_p(vdev, &dwz_hdr->sector);
     num_sectors = virtio_ldl_p(vdev, &dwz_hdr->num_sectors);
     flags = virtio_ldl_p(vdev, &dwz_hdr->flags);
@@ -628,6 +657,8 @@  static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
+    IO_CODE();
+
     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
         virtio_error(vdev, "virtio-blk missing headers");
         return -1;
@@ -778,6 +809,8 @@  void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
     MultiReqBuffer mrb = {};
     bool suppress_notifications = virtio_queue_get_notification(vq);
 
+    IO_CODE();
+
     aio_context_acquire(blk_get_aio_context(s->blk));
     blk_io_plug(s->blk);
 
@@ -811,6 +844,8 @@  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBlock *s = (VirtIOBlock *)vdev;
 
+    IO_CODE();
+
     if (s->dataplane && !s->dataplane_started) {
         /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
          * dataplane here instead of waiting for .set_status().