Re: blkback making assumptions about the id of the requests

2013-09-03 Thread Roger Pau Monné
On 02/09/13 22:03, Justin T. Gibbs wrote:
 On Sep 2, 2013, at 11:58 AM, Roger Pau Monné roger@citrix.com 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 roger@citrix.com
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

2013-09-03 Thread Justin T. Gibbs
On Sep 3, 2013, at 3:39 AM, Roger Pau Monné roger@citrix.com wrote:

 On 02/09/13 22:03, Justin T. Gibbs wrote:
 On Sep 2, 2013, at 11:58 AM, Roger Pau Monné roger@citrix.com 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: Current problem reports assigned to freebsd-xen@FreeBSD.org

2013-09-03 Thread Colin Percival
On 09/03/13 07:38, Adam McDougall wrote:
 o kern/154428  xen[xen] xn0 network interface and PF - Massive 
 performan
 I had similar issues, some testing I did last week makes me satisfied
 with r252781 in 9-stable (MFC of r291296, r291297, r291393) but I can't
 speak for everyone.
 
 f kern/135178  xen[xen] Xen domU outgoing data transfer stall when 
 TSO i
 Looks like feedback timeout.

I'd say that generally any PRs talking about performance problems or packet loss
with TSO turned on can be closed.

Also any PRs talking about FreeBSD/i386 Xen/PV can probably be closed with yes,
that code is horribly broken and needs a complete rewrite, don't even try to use
it for now.

-- 
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid

___
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