Re: [Qemu-devel] Re: [PATCH 3/3] block/nbd: Make the NBD block device use the AIO interface

[Nicholas Thomas]

> > + * Send I/O requests to the server.
> > + *
> > + * This function sends requests to the server, links the requests to
> > + * the outstanding_list in BDRVNBDState, and exits without waiting for
> > + * the response.  The responses are received in the `aio_read_response'
> > + * function which is called from the main loop as a fd handler.
> > + * If this is a write request and it's >1MB, split it into multiple AIOReqs
> > + */
> > +static void nbd_readv_writev_bh_cb(void *p)
> >  {
> > -    BDRVNBDState *s = bs->opaque;
> > -    struct nbd_request request;
> > +    NBDAIOCB *acb = p;
> > +    int ret = 0;
> >
> > -    request.type = NBD_CMD_DISC;
> > -    request.handle = (uint64_t)(intptr_t)bs;
> > -    request.from = 0;
> > -    request.len = 0;
> > -    nbd_send_request(s->sock, &request);
> > +    size_t len, done = 0;
> > +    size_t total = acb->nb_sectors * SECTOR_SIZE;
> > +
> > +    /* Where the read/write starts from */
> > +    size_t offset = acb->sector_num * SECTOR_SIZE;
> 
> On 32-bit hosts size_t is only 32 bits wide.  It's not suitable for
> storing the byte offset into the device.  Please use off_t.
> 
> > +    BDRVNBDState *s = acb->common.bs->opaque;
> 
> I don't follow this.  You have no guarantee that acb->common.bs->opaque
> is the BDRVNBDState *.

Another idiom from sheepdog. Seems qed, qcow2, qcow & vdi do the same.

It seems the magic is being done in nbd_aio_setup & qemu_aio_setup 
(gdb) bt
#0  nbd_aio_setup (bs=0xa69600, qiov=0x7fffffffdc00, sector_num=0,
nb_sectors=4, cb=0x4041c6 <aio_rw_done>, 
    opaque=0x7fffffffdbac) at block/nbd.c:443
#1  0x00000000004395c3 in nbd_aio_readv (bs=0xa69600, sector_num=0,
qiov=0x7fffffffdc00, nb_sectors=4, 
    cb=0x4041c6 <aio_rw_done>, opaque=0x7fffffffdbac) at block/nbd.c:538
#2  0x000000000041223c in bdrv_aio_readv (bs=0xa69600, sector_num=0,
qiov=0x7fffffffdc00, nb_sectors=4, 
    cb=0x4041c6 <aio_rw_done>, opaque=0x7fffffffdbac) at block.c:2158
[...]

(gdb) inspect acb
$17 = (NBDAIOCB *) 0x40f708

(gdb) inspect &acb->common
$21 = (BlockDriverAIOCB *) 0x40f708

(Sorry, the line numbers are unlikely to match yours).

It's some kind of simple polymorphism? Everything south of block/nbd.c
accesses the memory region as a BlockDriverAIOCB, while everything north
accesses it as an NBDAIOCB - with the additional fields that implies?
I've never come across something like this before - but it does seem to
work. 

acb->common.bs is set to the BlockDriverState we use everywhere - whose
opaque field is known. In what situations will this fail to work?

/Nick