Dear xen-devel. I think the blkif_recover (blkfront''s transparent VM resume) stuff looks quite overcomplicated. We copy the ring message to a shadow request allocated during submit, a process involving some none-obvious-looking get_id_from_freelist() subroutine to obtain a vector slot, and a memcpy. When receiving a resume callback from xenstore, we memcpy the entire shadow vector, reset the original one to zero, then reallocate the thereby freed shadow entries and not only copy the message on the ring, but the shadow back into the shadow vector just freed to keep stuff consistent. Hmmm. I wonder, should we just take the pending request and push it back onto the request_queue (with a blk_requeue_request)? Different from the present code, this should also help preserve original submit order if done right. (Don''t panic, not like it matters a lot anymore since the block barrier flags are gone.) If we want to keep the shadow copy, let''s do so with a prep_rq_fn. It gets called before the request gets pulled off the queue. Looks nicer, and one can arrange things so it only gets called once. Counter opinions? Thanks, Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, Mar 24, 2011 at 2:31 AM, Daniel Stodden <daniel.stodden@citrix.com>wrote:> > Dear xen-devel. > > I think the blkif_recover (blkfront''s transparent VM resume) stuff looks > quite overcomplicated. > > We copy the ring message to a shadow request allocated during submit, a > process involving some none-obvious-looking get_id_from_freelist() > subroutine to obtain a vector slot, and a memcpy. > > When receiving a resume callback from xenstore, we memcpy the entire > shadow vector, reset the original one to zero, then reallocate the > thereby freed shadow entries and not only copy the message on the ring, > but the shadow back into the shadow vector just freed to keep stuff > consistent. Hmmm. > > I wonder, should we just take the pending request and push it back onto > the request_queue (with a blk_requeue_request)? > > Different from the present code, this should also help preserve original > submit order if done right. (Don''t panic, not like it matters a lot > anymore since the block barrier flags are gone.) > > If we want to keep the shadow copy, let''s do so with a prep_rq_fn. It > gets called before the request gets pulled off the queue. Looks nicer, > and one can arrange things so it only gets called once. > > Counter opinions? > > A bit confused. If things were as simple as stuffing the pending reqs backinto the req_queue, why resort to shadowing the requests in the first place? (esp, with the blk barrier flags gone) shriram> Thanks, > Daniel > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2011-03-24 at 16:08 -0400, Shriram Rajagopalan wrote:> I wonder, should we just take the pending request and push it > back onto > the request_queue (with a blk_requeue_request)? > > Different from the present code, this should also help > preserve original > submit order if done right. (Don''t panic, not like it matters > a lot > anymore since the block barrier flags are gone.) > > If we want to keep the shadow copy, let''s do so with a > prep_rq_fn. It > gets called before the request gets pulled off the queue. > Looks nicer, > and one can arrange things so it only gets called once. > > Counter opinions? > > A bit confused. If things were as simple as stuffing the pending reqs > back > into the req_queue, why resort to shadowing the requests in the first > place?You''re not confused at all. I wasn''t sure yet if pushing back requests and just redoing them might be flawed somehow. Then just checking out what the options would be if the shadow is still wanted. One tiny little detail I forgot to consider was that the grant entries need to flip MFNs. So R/W requests still need fixup before requeuing. Also, we still have to tear them down after completion :>. So, keeping the entire request struct for reference, instead of growing a custom vector type, isn''t so bad. One pretty way to fix up segments is to blk_unprep_request (tearing them down), and then push the it back. They''ll be reprepped (ouh, I love that word) before they return. Or keep the old way around, i.e. just a row update). Unprep might drop some extra lines, and performance is a no-brainer. I''ll guess I''ll just try it out.> (esp, with the blk barrier flags gone)Well yeah, on newer kernels. But deliberately breaking it in subtle little ways doesn''t look so smart either. And despite the fact that FSes seem to have been draining quite eagerly for quite some time, I think I''ve seen post-FLUSH+FUA patches for jbd2 adding missing drains where it still used to rely on queue order. No idea sure if those missed in stable kernels. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 24/03/2011 09:31, "Daniel Stodden" <daniel.stodden@citrix.com> wrote:> Dear xen-devel. > > I think the blkif_recover (blkfront''s transparent VM resume) stuff looks > quite overcomplicated. > > We copy the ring message to a shadow request allocated during submit, a > process involving some none-obvious-looking get_id_from_freelist() > subroutine to obtain a vector slot, and a memcpy. > > When receiving a resume callback from xenstore, we memcpy the entire > shadow vector, reset the original one to zero, then reallocate the > thereby freed shadow entries and not only copy the message on the ring, > but the shadow back into the shadow vector just freed to keep stuff > consistent. Hmmm. > > I wonder, should we just take the pending request and push it back onto > the request_queue (with a blk_requeue_request)?Are you suggesting to get rid of the shadow state? It is needed, because in-flight requests can be overwritten by out-of-order responses written into the shared ring by the backend driver. -- Keir> Different from the present code, this should also help preserve original > submit order if done right. (Don''t panic, not like it matters a lot > anymore since the block barrier flags are gone.) > > If we want to keep the shadow copy, let''s do so with a prep_rq_fn. It > gets called before the request gets pulled off the queue. Looks nicer, > and one can arrange things so it only gets called once. > > Counter opinions? > > Thanks, > Daniel > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2011-03-24 at 17:47 -0400, Keir Fraser wrote:> On 24/03/2011 09:31, "Daniel Stodden" <daniel.stodden@citrix.com> wrote: > > > Dear xen-devel. > > > > I think the blkif_recover (blkfront''s transparent VM resume) stuff looks > > quite overcomplicated. > > > > We copy the ring message to a shadow request allocated during submit, a > > process involving some none-obvious-looking get_id_from_freelist() > > subroutine to obtain a vector slot, and a memcpy. > > > > When receiving a resume callback from xenstore, we memcpy the entire > > shadow vector, reset the original one to zero, then reallocate the > > thereby freed shadow entries and not only copy the message on the ring, > > but the shadow back into the shadow vector just freed to keep stuff > > consistent. Hmmm. > > > > I wonder, should we just take the pending request and push it back onto > > the request_queue (with a blk_requeue_request)? > > Are you suggesting to get rid of the shadow state? It is needed, because > in-flight requests can be overwritten by out-of-order responses written into > the shared ring by the backend driver.I was suggesting just that while missing the somewhat essential fact that we''re currently using the segment vectors in shadow state as the single backing store for our gref lists. :) I''m aware that this is a duplex channel sharing message slots, and also wouldn''t suggest some daredevil mode which reads critical state back from the sring even if that were not the case. Now, blkif segments are by far the are the most significant payload, not much point in isolating them. Nor does scattering the memcpies look like a particularly good idea. Also, one might want to add least a few more paranoia BUG_ON/fail-if in case of request/response mismatch (id, op, etc) than we currently do. So keeping the full message makes perfect sense. In summary, yesterdays idea was ''Yeah, maybe''. Right now it''s rather ''hell, no'' :) Still, pushing requests back on the queue seems more straightforward than what''s happening now. Once I get it to run and it still looks good. Also, I might have found a pretty optimization for the shadow copies. Cheers + Thanks, Daniel> -- Keir > > > Different from the present code, this should also help preserve original > > submit order if done right. (Don''t panic, not like it matters a lot > > anymore since the block barrier flags are gone.) > > > > If we want to keep the shadow copy, let''s do so with a prep_rq_fn. It > > gets called before the request gets pulled off the queue. Looks nicer, > > and one can arrange things so it only gets called once. > > > > Counter opinions? > > > > Thanks, > > Daniel > > > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xensource.com > > http://lists.xensource.com/xen-devel > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel