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

[Paulina Szubarczyk <paulinaszubarczyk@gmail.com>]

On Mon, 2016-06-13 at 11:15 +0100, David Vrabel wrote:
> 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.

The 'feature_grant_copy' is a boolean and could be set to false in such case.
There could be added a node in XenStore, for example
'feature-force-grant-map', which when set by frontend will be read
during a connection and changed the value to false forcing the grant map
operation. 

> > +    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.

I might removed that log or change the level, though when the mapping
fails for grant map it is logged in a similar manner.

> > +        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.

Yes, thank you.
> 
> > +
> > +    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
> 
Paulina



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