Re: [Xen-devel] [PATCH 1/2] xen-unstable/blkif: Move read/write/barrier specific fields into a union

[Keir Fraser <keir@xxxxxxx>]

Redefining the blkif_req struct naming seems an unnecessary pain. Why not
define a separate 'struct blkif_trim_request' that starts with the uint8_t
BLKIF_OP_TRIM and thereafter is defined exactly how you like? Then drivers
can cast a request pointer to your new type when reading/writing a trim
request, and no existing code needs to be changed. If all this does is
introduce new definitions into xen-unstable, rather than changing existing
ones, we could still potentially slip it in for 4.1.0 as an obviously safe
patch.

 -- Keir

On 14/01/2011 16:44, "owen.smith@xxxxxxxxxx" <owen.smith@xxxxxxxxxx> wrote:

> From: Owen Smith <owen.smith@xxxxxxxxxx>
> 
> # HG 
> changeset patch
> # User root@xxxxxxxxxxxxxxxxxxxxxxx
> # Date 1294919321 0
> # Node ID 428420495fd3cfda164e7b355b0ffcc0845af2e4
> # Parent  7b4c82f07281ad9c48b652e2a305a7be607c5283
> blkif: Move read/write/barrier specific fields into a union
> 
> Patches xen-unstable.hg
> 
> Modifies the blkif ring interface by placing the request specific
> fields into a union in order to support additional operation types.
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> 
> diff -r 7b4c82f07281 -r 428420495fd3 extras/mini-os/blkfront.c
> --- a/extras/mini-os/blkfront.c Wed Jan 05 23:54:15 2011 +0000
> +++ b/extras/mini-os/blkfront.c Thu Jan 13 11:48:41 2011 +0000
> @@ -361,22 +361,22 @@
>      req->nr_segments = n;
>      req->handle = dev->handle;
>      req->id = (uintptr_t) aiocbp;
> -    req->sector_number = aiocbp->aio_offset / 512;
> +    req->u.rw.sector_number = aiocbp->aio_offset / 512;
>  
>      for (j = 0; j < n; j++) {
> -        req->seg[j].first_sect = 0;
> -        req->seg[j].last_sect = PAGE_SIZE / 512 - 1;
> +        req->u.rw.seg[j].first_sect = 0;
> +        req->u.rw.seg[j].last_sect = PAGE_SIZE / 512 - 1;
>      }
> -    req->seg[0].first_sect = ((uintptr_t)aiocbp->aio_buf & ~PAGE_MASK) / 512;
> -    req->seg[n-1].last_sect = (((uintptr_t)aiocbp->aio_buf +
> aiocbp->aio_nbytes - 1) & ~PAGE_MASK) / 512;
> +    req->u.rw.seg[0].first_sect = ((uintptr_t)aiocbp->aio_buf & ~PAGE_MASK) /
> 512;
> +    req->u.rw.seg[n-1].last_sect = (((uintptr_t)aiocbp->aio_buf +
> aiocbp->aio_nbytes - 1) & ~PAGE_MASK) / 512;
>      for (j = 0; j < n; j++) {
> uintptr_t data = start + j * PAGE_SIZE;
>          if (!write) {
>              /* Trigger CoW if needed */
> -            *(char*)(data + (req->seg[j].first_sect << 9)) = 0;
> +            *(char*)(data + (req->u.rw.seg[j].first_sect << 9)) = 0;
>              barrier();
>          }
> - aiocbp->gref[j] = req->seg[j].gref =
> + aiocbp->gref[j] = req->u.rw.seg[j].gref =
>              gnttab_grant_access(dev->dom, virtual_to_mfn(data), write);
>      }
>  
> @@ -432,7 +432,7 @@
>      req->handle = dev->handle;
>      req->id = id;
>      /* Not needed anyway, but the backend will check it */
> -    req->sector_number = 0;
> +    req->u.rw.sector_number = 0;
>      dev->ring.req_prod_pvt = i + 1;
>      wmb();
>      RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&dev->ring, notify);
> diff -r 7b4c82f07281 -r 428420495fd3 tools/blktap/drivers/tapdisk.c
> --- a/tools/blktap/drivers/tapdisk.c Wed Jan 05 23:54:15 2011 +0000
> +++ b/tools/blktap/drivers/tapdisk.c Thu Jan 13 11:48:41 2011 +0000
> @@ -528,10 +528,10 @@
>  segment_start(blkif_request_t *req, int sidx)
>  {
> int i;
> - uint64_t start = req->sector_number;
> + uint64_t start = req->u.rw.sector_number;
>  
> for (i = 0; i < sidx; i++)
> -  start += (req->seg[i].last_sect - req->seg[i].first_sect + 1);
> +  start += (req->u.rw.seg[i].last_sect - req->u.rw.seg[i].first_sect + 1);
>  
> return start;
>  }
> @@ -600,13 +600,13 @@
> struct disk_driver *parent = dd->next;
> 
> seg_start = segment_start(req, sidx);
> - seg_end   = seg_start + req->seg[sidx].last_sect + 1;
> + seg_end   = seg_start + req->u.rw.seg[sidx].last_sect + 1;
> 
> ASSERT(sector >= seg_start && sector + nr_secs <= seg_end);
>  
> page  = (char *)MMAP_VADDR(info->vstart,
>   (unsigned long)req->id, sidx);
> - page += (req->seg[sidx].first_sect << SECTOR_SHIFT);
> + page += (req->u.rw.seg[sidx].first_sect << SECTOR_SHIFT);
> page += ((sector - seg_start) << SECTOR_SHIFT);
>  
> if (!parent) {
> @@ -667,7 +667,7 @@
>       req, sizeof(*req));
> blkif->pending_list[idx].status = BLKIF_RSP_OKAY;
> blkif->pending_list[idx].submitting = 1;
> -   sector_nr = req->sector_number;
> +   sector_nr = req->u.rw.sector_number;
> }
>  
> if ((dd->flags & TD_RDONLY) &&
> @@ -677,16 +677,16 @@
> }
>  
> for (i = start_seg; i < req->nr_segments; i++) {
> -   nsects = req->seg[i].last_sect -
> -     req->seg[i].first_sect + 1;
> +   nsects = req->u.rw.seg[i].last_sect -
> +     req->u.rw.seg[i].first_sect + 1;
> 
> -   if ((req->seg[i].last_sect >= page_size >> 9) ||
> +   if ((req->u.rw.seg[i].last_sect >= page_size >> 9) ||
>    (nsects <= 0))
> continue;
>  
> page  = (char *)MMAP_VADDR(info->vstart,
>   (unsigned long)req->id, i);
> -   page += (req->seg[i].first_sect << SECTOR_SHIFT);
> +   page += (req->u.rw.seg[i].first_sect << SECTOR_SHIFT);
>  
> if (sector_nr >= s->size) {
> DPRINTF("Sector request failed:\n");
> diff -r 7b4c82f07281 -r 428420495fd3 tools/blktap2/drivers/tapdisk-diff.c
> --- a/tools/blktap2/drivers/tapdisk-diff.c Wed Jan 05 23:54:15 2011 +0000
> +++ b/tools/blktap2/drivers/tapdisk-diff.c Thu Jan 13 11:48:41 2011 +0000
> @@ -363,13 +363,13 @@
> breq                = &sreq->blkif_req;
> breq->id            = idx;
> breq->nr_segments   = r->blkif_req.nr_segments;
> - breq->sector_number = r->blkif_req.sector_number;
> + breq->u.rw.sector_number = r->blkif_req.u.rw.sector_number;
> breq->operation     = BLKIF_OP_READ;
>  
> for (int i = 0; i < r->blkif_req.nr_segments; i++) {
> -  struct blkif_request_segment *seg = breq->seg + i;
> -  seg->first_sect = r->blkif_req.seg[i].first_sect;
> -  seg->last_sect  = r->blkif_req.seg[i].last_sect;
> +  struct blkif_request_segment *seg = breq->u.rw.seg + i;
> +  seg->first_sect = r->blkif_req.u.rw.seg[i].first_sect;
> +  seg->last_sect  = r->blkif_req.u.rw.seg[i].last_sect;
> }
> s->cur += sreq->secs;
>  
> @@ -426,12 +426,12 @@
> breq                = &sreq->blkif_req;
> breq->id            = idx;
> breq->nr_segments   = 0;
> -  breq->sector_number = sreq->sec;
> +  breq->u.rw.sector_number = sreq->sec;
> breq->operation     = BLKIF_OP_READ;
>  
> for (i = 0; i < BLKIF_MAX_SEGMENTS_PER_REQUEST; i++) {
> uint32_t secs;
> -   struct blkif_request_segment *seg = breq->seg + i;
> +   struct blkif_request_segment *seg = breq->u.rw.seg + i;
>  
> secs = MIN(s->end - s->cur, psize >> SECTOR_SHIFT);
> secs = MIN(((blk + 1) << SPB_SHIFT) - s->cur, secs);
> diff -r 7b4c82f07281 -r 428420495fd3 tools/blktap2/drivers/tapdisk-image.c
> --- a/tools/blktap2/drivers/tapdisk-image.c Wed Jan 05 23:54:15 2011 +0000
> +++ b/tools/blktap2/drivers/tapdisk-image.c Thu Jan 13 11:48:41 2011 +0000
> @@ -148,15 +148,15 @@
> psize = getpagesize();
>  
> for (i = 0; i < req->nr_segments; i++) {
> -  nsects = req->seg[i].last_sect - req->seg[i].first_sect + 1;
> +  nsects = req->u.rw.seg[i].last_sect - req->u.rw.seg[i].first_sect + 1;
> 
> -  if (req->seg[i].last_sect >= psize >> 9 || nsects <= 0)
> +  if (req->u.rw.seg[i].last_sect >= psize >> 9 || nsects <= 0)
> goto fail;
>  
> total += nsects;
> }
>  
> - if (req->sector_number + nsects > info->size)
> + if (req->u.rw.sector_number + nsects > info->size)
> goto fail;
>  
> return 0;
> @@ -164,6 +164,6 @@
>  fail:
> ERR(-EINVAL, "bad request on %s (%s, %"PRIu64"): id: %"PRIu64": %d at
> %"PRIu64,
>    image->name, (rdonly ? "ro" : "rw"), info->size, req->id,
> -     req->operation, req->sector_number + total);
> +     req->operation, req->u.rw.sector_number + total);
> return -EINVAL;
>  }
> diff -r 7b4c82f07281 -r 428420495fd3 tools/blktap2/drivers/tapdisk-stream.c
> --- a/tools/blktap2/drivers/tapdisk-stream.c Wed Jan 05 23:54:15 2011 +0000
> +++ b/tools/blktap2/drivers/tapdisk-stream.c Thu Jan 13 11:48:41 2011 +0000
> @@ -293,12 +293,12 @@
> breq                = &sreq->blkif_req;
> breq->id            = idx;
> breq->nr_segments   = 0;
> -  breq->sector_number = sreq->sec;
> +  breq->u.rw.sector_number = sreq->sec;
> breq->operation     = BLKIF_OP_READ;
>  
> for (i = 0; i < BLKIF_MAX_SEGMENTS_PER_REQUEST; i++) {
> uint32_t secs = MIN(s->end - s->cur, psize >> SECTOR_SHIFT);
> -   struct blkif_request_segment *seg = breq->seg + i;
> +   struct blkif_request_segment *seg = breq->u.rw.seg + i;
>  
> if (!secs)
> break;
> diff -r 7b4c82f07281 -r 428420495fd3 tools/blktap2/drivers/tapdisk-vbd.c
> --- a/tools/blktap2/drivers/tapdisk-vbd.c Wed Jan 05 23:54:15 2011 +0000
> +++ b/tools/blktap2/drivers/tapdisk-vbd.c Thu Jan 13 11:48:41 2011 +0000
> @@ -1066,7 +1066,7 @@
> rsp->status = vreq->status;
>  
> DBG(TLOG_DBG, "writing req %d, sec 0x%08"PRIx64", res %d to ring\n",
> -     (int)tmp.id, tmp.sector_number, vreq->status);
> +     (int)tmp.id, tmp.u.rw.sector_number, vreq->status);
>  
> if (rsp->status != BLKIF_RSP_OKAY)
> ERR(EIO, "returning BLKIF_RSP %d", rsp->status);
> @@ -1181,10 +1181,10 @@
>  tapdisk_vbd_breq_get_sector(blkif_request_t *breq, td_request_t treq)
>  {
>      int seg, nsects;
> -    uint64_t sector_nr = breq->sector_number;
> +    uint64_t sector_nr = breq->u.rw.sector_number;
>      
>      for(seg=0; seg < treq.sidx; seg++) {
> -        nsects = breq->seg[seg].last_sect - breq->seg[seg].first_sect + 1;
> +        nsects = breq->u.rw.seg[seg].last_sect -
> breq->u.rw.seg[seg].first_sect + 1;
>          sector_nr += nsects;
>      }
>  
> @@ -1366,7 +1366,7 @@
> req       = &vreq->req;
> id        = req->id;
> ring      = &vbd->ring;
> - sector_nr = req->sector_number;
> + sector_nr = req->u.rw.sector_number;
> image     = tapdisk_vbd_first_image(vbd);
>  
> vreq->submitting = 1;
> @@ -1385,10 +1385,10 @@
> goto fail;
>  
> for (i = 0; i < req->nr_segments; i++) {
> -  nsects = req->seg[i].last_sect - req->seg[i].first_sect + 1;
> +  nsects = req->u.rw.seg[i].last_sect - req->u.rw.seg[i].first_sect + 1;
> page   = (char *)MMAP_VADDR(ring->vstart,
>   (unsigned long)req->id, i);
> -  page  += (req->seg[i].first_sect << SECTOR_SHIFT);
> +  page  += (req->u.rw.seg[i].first_sect << SECTOR_SHIFT);
>  
> treq.id             = id;
> treq.sidx           = i;
> @@ -1482,7 +1482,7 @@
> vreq->status = BLKIF_RSP_OKAY;
> DBG(TLOG_DBG, "retry #%d of req %"PRIu64", "
>    "sec 0x%08"PRIx64", nr_segs: %d\n", vreq->num_retries,
> -      vreq->req.id, vreq->req.sector_number,
> +      vreq->req.id, vreq->req.u.rw.sector_number,
>    vreq->req.nr_segments);
>  
> err = tapdisk_vbd_issue_request(vbd, vreq);
> diff -r 7b4c82f07281 -r 428420495fd3 xen/include/public/io/blkif.h
> --- a/xen/include/public/io/blkif.h Wed Jan 05 23:54:15 2011 +0000
> +++ b/xen/include/public/io/blkif.h Thu Jan 13 11:48:41 2011 +0000
> @@ -103,7 +103,22 @@
>      uint8_t     first_sect, last_sect;
>  };
>  
> +struct blkif_request_rw {
> +    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
> +    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> +};
> +
>  struct blkif_request {
> +    uint8_t        operation;    /* BLKIF_OP_???                         */
> +    uint8_t        nr_segments;  /* number of segments                   */
> +    blkif_vdev_t   handle;       /* only for read/write requests         */
> +    uint64_t       id;           /* private guest value, echoed in resp  */
> +    union {
> +        struct blkif_request_rw     rw;
> +    } u;
> +};
> +
> +struct blkif_request_old {
>      uint8_t        operation;    /* BLKIF_OP_???                         */
>      uint8_t        nr_segments;  /* number of segments                   */
>      blkif_vdev_t   handle;       /* only for read/write requests         */
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel