Re: blkback making assumptions about the id of the requests
On Sep 3, 2013, at 3:39 AM, Roger Pau Monné wrote: > On 02/09/13 22:03, Justin T. Gibbs wrote: >> On Sep 2, 2013, at 11:58 AM, Roger Pau Monné wrote: >> >>> Hello, >>> >>> While playing with driver domains using FreeBSD I've found out that >>> blkback in FreeBSD makes assumptions about the id of a request instead >>> of actually using the id of the request on the shared ring. This seems >>> wrong to me, since a frontend might choose whatever ids it like for the >>> requests (like using 100-131 instead of 0-31). The patch attached fixes >>> it by copying the id from the request on the ring to blkback internal >>> request structure. >>> >>> Roger. >> >> It looks to me like the id is set in xbb_dispatch_io(). Why it is done there >> and not earlier, I don't recall. > > Sorry, I've missed to spot that the id is set there, the problem is that > requests of type BLKIF_OP_FLUSH_DISKCACHE never reach that point, so > they end up having incorrect requests ids. I've reworded the commit > message and removed the late setting of the request id, now it is set > earlier so requests of type flush also have a valid id when writing the > response on the ring. Makes sense. Committed. -- Justin ___ freebsd-xen@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-xen To unsubscribe, send any mail to "freebsd-xen-unsubscr...@freebsd.org"
Re: blkback making assumptions about the id of the requests
On 02/09/13 22:03, Justin T. Gibbs wrote: > On Sep 2, 2013, at 11:58 AM, Roger Pau Monné wrote: > >> Hello, >> >> While playing with driver domains using FreeBSD I've found out that >> blkback in FreeBSD makes assumptions about the id of a request instead >> of actually using the id of the request on the shared ring. This seems >> wrong to me, since a frontend might choose whatever ids it like for the >> requests (like using 100-131 instead of 0-31). The patch attached fixes >> it by copying the id from the request on the ring to blkback internal >> request structure. >> >> Roger. > > It looks to me like the id is set in xbb_dispatch_io(). Why it is done there > and not earlier, I don't recall. Sorry, I've missed to spot that the id is set there, the problem is that requests of type BLKIF_OP_FLUSH_DISKCACHE never reach that point, so they end up having incorrect requests ids. I've reworded the commit message and removed the late setting of the request id, now it is set earlier so requests of type flush also have a valid id when writing the response on the ring. From 80dc3cbf5ed88be890d28ae198ab69968caf9d63 Mon Sep 17 00:00:00 2001 From: Roger Pau Monne Date: Mon, 2 Sep 2013 15:51:47 +0200 Subject: [PATCH] xen-blkback: set request id correctly for flush OP Originally the internal request id is set on xbb_dispatch_io, but requests of type BLKIF_OP_FLUSH_DISKCACHE never reach this point, so they end up having wrong request ids when writting the response on the ring, breaking the frontend. This patch fixes it by setting the request id for all request types earlier, so that BLKIF_OP_FLUSH_DISKCACHE operations have the right id. --- sys/dev/xen/blkback/blkback.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sys/dev/xen/blkback/blkback.c b/sys/dev/xen/blkback/blkback.c index 2a220c4..fa9a744 100644 --- a/sys/dev/xen/blkback/blkback.c +++ b/sys/dev/xen/blkback/blkback.c @@ -1239,6 +1239,7 @@ xbb_get_resources(struct xbb_softc *xbb, struct xbb_xen_reqlist **reqlist, nreq->reqlist = *reqlist; nreq->req_ring_idx = ring_idx; + nreq->id = ring_req->id; if (xbb->abi != BLKIF_PROTOCOL_NATIVE) { bcopy(ring_req, &nreq->ring_req_storage, sizeof(*ring_req)); @@ -1608,7 +1609,6 @@ xbb_dispatch_io(struct xbb_softc *xbb, struct xbb_xen_reqlist *reqlist) req_ring_idx = nreq->req_ring_idx; nr_sects = 0; nseg = ring_req->nr_segments; - nreq->id = ring_req->id; nreq->nr_pages= nseg; nreq->nr_512b_sectors = 0; req_seg_idx = 0; -- 1.7.7.5 (Apple Git-26) ___ freebsd-xen@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-xen To unsubscribe, send any mail to "freebsd-xen-unsubscr...@freebsd.org"
Re: blkback making assumptions about the id of the requests
On Sep 2, 2013, at 11:58 AM, Roger Pau Monné wrote: > Hello, > > While playing with driver domains using FreeBSD I've found out that > blkback in FreeBSD makes assumptions about the id of a request instead > of actually using the id of the request on the shared ring. This seems > wrong to me, since a frontend might choose whatever ids it like for the > requests (like using 100-131 instead of 0-31). The patch attached fixes > it by copying the id from the request on the ring to blkback internal > request structure. > > Roger. It looks to me like the id is set in xbb_dispatch_io(). Why it is done there and not earlier, I don't recall. -- Justin ___ freebsd-xen@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-xen To unsubscribe, send any mail to "freebsd-xen-unsubscr...@freebsd.org"
blkback making assumptions about the id of the requests
Hello, While playing with driver domains using FreeBSD I've found out that blkback in FreeBSD makes assumptions about the id of a request instead of actually using the id of the request on the shared ring. This seems wrong to me, since a frontend might choose whatever ids it like for the requests (like using 100-131 instead of 0-31). The patch attached fixes it by copying the id from the request on the ring to blkback internal request structure. Roger. >From 01c3edc4446b113ec85537bb75c56c6072c4ee49 Mon Sep 17 00:00:00 2001 From: Roger Pau Monne Date: Mon, 2 Sep 2013 15:51:47 +0200 Subject: [PATCH] xen-blkback: don't make assumptions about request ids Blkback makes assumptions about the id of the request it received from the frontend, this patch fixes it by always honoring the id passed from the frontend instead of assuming there's an implicit order in the id of the requests. --- sys/dev/xen/blkback/blkback.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/sys/dev/xen/blkback/blkback.c b/sys/dev/xen/blkback/blkback.c index 2a220c4..500b347 100644 --- a/sys/dev/xen/blkback/blkback.c +++ b/sys/dev/xen/blkback/blkback.c @@ -1239,6 +1239,7 @@ xbb_get_resources(struct xbb_softc *xbb, struct xbb_xen_reqlist **reqlist, nreq->reqlist = *reqlist; nreq->req_ring_idx = ring_idx; + nreq->id = ring_req->id; if (xbb->abi != BLKIF_PROTOCOL_NATIVE) { bcopy(ring_req, &nreq->ring_req_storage, sizeof(*ring_req)); -- 1.7.7.5 (Apple Git-26) ___ freebsd-xen@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-xen To unsubscribe, send any mail to "freebsd-xen-unsubscr...@freebsd.org"