Re: [Qemu-devel] [PATCH 2/3] block: explicit I/O accounting

[Christoph Hellwig]

On Mon, Aug 22, 2011 at 05:48:37PM +0200, Kevin Wolf wrote:
> > +static inline void
> > +bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t 
> > bytes,
> > +        enum BlockIOType type)
> > +{
> > +    cookie->bytes = bytes;
> > +    cookie->type = type;
> > +}
> > +
> > +static inline void
> > +bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
> > +{
> > +    bs->nr_bytes[cookie->type] += cookie->bytes;
> > +    bs->nr_ops[cookie->type]++;
> > +}
> 
> Device models actually shouldn't include block_int.h, so this isn't very
> nice. Moving it to block.h would lose the inline, does it matter?

I have moved it out of line, we can change it if anyone cares.

> > +
> > +        bdrv_acct_start(s->bs, &s->acct, n * BDRV_SECTOR_SIZE, 
> > BDRV_ACCT_READ);
> >          ret = bdrv_read(s->bs, sector_num, s->io_buffer, n);
> > +        bdrv_acct_done(s->bs, &s->acct);
> 
> The commit message says that you're only converting bdrv_aio_readv. I
> think it makes sense to add the accounting to the sychronous calls, too,
> so that devices have complete accounting or none at all.

That was the plan, I just worded it incorrectly.  The idea is to fully
convert device modles that were using bdrv_aio_* in some places, but
leave those that never had any accounting out for now.

> If your plan is to convert all bdrv_read calls, I think you've missed
> some in atapi.c.

Added.


> > +    switch (dma_cmd) {
> > +    case IDE_DMA_READ:
> > +        bdrv_acct_start(s->bs, &s->acct, s->nsector * BDRV_SECTOR_SIZE,
> > +                        BDRV_ACCT_READ);
> > +        break;
> > +    case IDE_DMA_WRITE:
> > +        bdrv_acct_start(s->bs, &s->acct, s->nsector * BDRV_SECTOR_SIZE,
> > +                        BDRV_ACCT_WRITE);
> > +   break;
> > +    default:
> > +        break;
> > +    }
> 
> So should the semantics of bdrv_acct_done be that it does nothing if no
> bdrv_acct_start has been called before? If so, its implementation is
> broken. Otherwise, the default case of this switch statement looks
> broken to me.

I have fixed ide_dma_cb to only do the accounting for read and write requests.

> > +    switch (s->dma_cmd) {
> > +    case IDE_DMA_READ:
> > +        bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
> > +        break;
> > +    case IDE_DMA_WRITE:
> > +        bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_WRITE);
> > +        break;
> > +    default:
> > +        break;
> 
> Can it happen? If yes, see above. If no, abort() is better.

We can get a trim request, so it can happen.

> >  static void scsi_read_complete(void * opaque, int ret)
> >  {
> >      SCSIDiskReq *r = (SCSIDiskReq *)opaque;
> > +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> >      int n;
> >  
> >      r->req.aiocb = NULL;
> > @@ -117,6 +119,8 @@ static void scsi_read_complete(void * op
> >          }
> >      }
> >  
> > +    bdrv_acct_done(s->bs, &r->acct);
> > +
> >      DPRINTF("Data ready tag=0x%x len=%zd\n", r->req.tag, r->iov.iov_len);
> 
> scsi-disk doesn't account for failed requests. IDE does. I don't have a
> strong preference on which way we handle it, but I think it should be
> consistent.

The idea is to call bdrv_acct_done even for failed request to make sure
the calls are balanced.  In the future we might want to pass the error
code to it to e.g. count failed requests separately.

> > Index: qemu/hw/xen_disk.c
> > ===================================================================
> > --- qemu.orig/hw/xen_disk.c 2011-08-21 11:49:29.000000000 -0700
> > +++ qemu/hw/xen_disk.c      2011-08-21 14:57:09.517558463 -0700
> > @@ -79,6 +79,7 @@ struct ioreq {
> >  
> >      struct XenBlkDev    *blkdev;
> >      QLIST_ENTRY(ioreq)   list;
> > +    BlockAcctCookie     acct;
> 
> Indentation is off.

Actually only the indentation of list if off compared to the rest of
the structure and everything else in the file.