Re: [PATCH v2 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[David Vrabel <david.vrabel@citrix.com>]

On 13/06/16 10:43, Paulina Szubarczyk wrote:
> Copy data operated on during request from/to local buffers to/from 
> the grant references. 
> 
> Before grant copy operation local buffers must be allocated what is 
> done by calling ioreq_init_copy_buffers. For the 'read' operation, 
> first, the qemu device invokes the read operation on local buffers 
> and on the completion grant copy is called and buffers are freed. 
> For the 'write' operation grant copy is performed before invoking 
> write by qemu device. 
> 
> A new value 'feature_grant_copy' is added to recognize when the 
> grant copy operation is supported by a guest. 
> The body of the function 'ioreq_runio_qemu_aio' is moved to 
> 'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
> on the support for grant copy according checks, initialization, grant 
> operation are made, then the 'ioreq_runio_qemu_aio_blk' function is 
> called. 

I think you should add an option to force the use of grant mapping even
if copy support is detected.  If future changes to the grant map
infrastructure makes it faster or if grant map scales better in some
systems, then it would be useful to be able to use it.

> +    rc = xc_gnttab_grant_copy(gnt, count, segs);
> +
> +    if (rc) {
> +        xen_be_printf(&ioreq->blkdev->xendev, 0, 
> +                      "failed to copy data %d \n", rc);

I don't think you want to log anything here.  A guest could spam the
logs by repeatedly submitting requests with (for example) bad grant
references.

> +        ioreq->aio_errors++;
> +        r = -1; goto out;

return -1;

> @@ -1020,10 +1163,18 @@ static int blk_connect(struct XenDevice *xendev)
>  
>      xen_be_bind_evtchn(&blkdev->xendev);
>  
> +    xc_gnttab_grant_copy_segment_t seg;
> +    blkdev->feature_grant_copy = 
> +                (xc_gnttab_grant_copy(blkdev->xendev.gnttabdev, 0, &seg) == 0);

You can pass NULL for the segments here.

> +
> +    xen_be_printf(&blkdev->xendev, 3, "GRANT COPY %s\n", 
> +                  blkdev->feature_grant_copy ? "ENABLED" : "DISABLED");
> +
>      xen_be_printf(&blkdev->xendev, 1, "ok: proto %s, ring-ref %d, "
>                    "remote port %d, local port %d\n",
>                    blkdev->xendev.protocol, blkdev->ring_ref,
>                    blkdev->xendev.remote_port, blkdev->xendev.local_port);
> +
>      return 0;
>  }

David


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel