xen-blkfront: simplify resume?

xen-blkfront: simplify resume?

[Daniel Stodden]

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

Re: xen-blkfront: simplify resume?

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 1637 bytes --]

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 back
into 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
>

[-- Attachment #1.2: Type: text/html, Size: 2323 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: xen-blkfront: simplify resume?

[Daniel Stodden]

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

Re: xen-blkfront: simplify resume?

[Keir Fraser]

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

Re: xen-blkfront: simplify resume?

[Daniel Stodden]

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