Re: [Xen-devel] Blktap fixes and kernel patch.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-08-02 at 17:06 +0100, Dr. Greg Wettstein wrote:
> On Aug 1, 10:17am, Ian Campbell wrote:
> } Subject: Re: Blktap fixes and kernel patch.
> 
> Hi Ian et. al, hope your week is proceeding well.
> 
> > On Tue, 2012-07-31 at 22:41 +0100, Dr. Greg Wettstein wrote:
> 
> > > In the process I updated the blktap2 kernel driver to patch cleanly
> > > into the Linux 3.4 kernel.  These fixes have been validated against
> > > the 3.4 kernel as well as the 3.2 kernel.
> 
> > Just to be clear this is just a straight forward port, there's no
> > part of the deadlock fix in here?
> 
> The kernel patch is just a forward port to 3.4, there were no issues
> with the driver itself that needed to be addressed.
> 
> It seemed the community lacks ready access to the kernel driver so
> hopefully others will find a solid reference site for the patches
> useful.

I think they will, thanks.

The closest thing we have for an "upstream" for these now is the
blktap-dkms packages which are in Debian and Ubuntu, but I don't think
those really have a maintainer as such (I think Mike, CCd, wrangles with
them a bit). Might that be something you would like to maintain?

[...]
> > If you send a mail with a subject "Xen 4.1.x backport request
> > <commit-id>" explaining which commit it is and CC keir@xxxxxxx &
> > ian.jackson@xxxxxxxxxxxxx then we can see about getting this into a
> > future 4.1.x (perhaps even 4.1.3, not sure which stage of rcs we are at
> > there).
> > 
> > If the backport is reasonably trivial then there is often no need to
> > include it but since you have done so you might as well include the
> > patch for reference.
> 
> I will forward along the reference and the patch.

Great thanks.

> > > The second patch corrects the deadlock which occurs between the
> > > blktap2 kernel driver and the blktap2 userspace control plane.  The
> > > deadlock causes a delay in the shutdown of a XEN guest and results in
> > > the 'orphaning' of tapdisk minor number allocations.  As seems to be
> > > typical with these types of things the fix was trivially straight
> > > forward once I finally figured out what was going on.
> 
> > Thanks for this.
> > 
> > Am I right that the important functional change here is that the xs_rm
> > needs to come after we read the params node but before tap_ctl_destroy?
> > Obviously removing the node before calling libxl__device_destroy_tapdisk
> > is wrong since libxl__device_destroy_tapdisk reads from be_path!
> 
> Correct.
> 
> I debated a bit about how to do this in the cleanest fashion
> possible.  Since the be_path is passed to libxl__device_destroy_tapdisk()
> the simplest strategy seemed to be to abstract the libxl_ctx context
> and pull the entry from xenstore after the tapdisk-params key was read
> from the xenstore.

[...]
> While my fix vaguely felt like a layering violation it seemed to be
> the most correct approach.  Since libxl__device_destroy_tapdisk() is
> stubbed out in the non-blktap2 case having the teardown of the backend
> there would seem to generically fix the problem.  Provided of course
> the function can be provided with the correct context, I'm not looking
> at the current code.

It was a little more complicated in unstable, because the rm is now in a
transaction. What I ended up doing was making
libxl__device_destroy_tapdisk take the actual params string instead of
the be_path, so I could read it as part of the transaction and then call
destroy_tapdisk.

I've just sent out V2 of that patch, I CCd you. 
<2c21d5c75dcbdf52987f.1343985208@xxxxxxxxxxxxxxxxxxxxxxxxx>

> > I'd really appreciate it if you could validate whether 4.2.0-rc1
> > works for you or not, I suspect not. We would usually want fix the
> > development version before considering fixes for the stable branches
> > (even if the actual patch ends up looking totally different)
> > otherwise we run the risk of regressions in the next version.
> 
> I'd be happy to give it a test.  Is there a tarball cut or should I
> hone up my Mercurial skills?

We don't do tarballs for rcs. In any case I suggest you use the
Mercurial tip anyway since it has the prerequisites for my patch in it
now.

Those prerequisistes are still being tested so you will need the staging
tree:
        hg clone http://xenbits.xen.org/hg/staging/xen-unstable.hg

once they pass testing (this afternoon BST, assuming all is well) then
you can drop the "/staging" bit. We'll probably to rc2 early next week.

> > Is there a simple command which will list the leaked tap devices? If
> > so we can consider adding it to the leak-check phase of the
> > automated tests (although I'm not sure how much use these make of
> > blktap)
> 
> The tap-ctl tools make managingn all this pretty straight forward.
> 
> If you issue the following command:
> 
>       tap-ctl list
> 
> On a faulty implementation after the startup and shutdown of a blktap2
> using guest you will see the orphaned minor.  They steadily increase
> as guests startup and shutdown.

Thanks, this rings a bell from last time I looked at this.

> [..]

> It appears that 4.1.2 is not properly cleaning up xenstore.  I'm
> chasing that down now and if there are trivial correctness fixes I
> will pass them onward.

Thanks.

> 
> > > Ian for your reference the following change which you introduced to
> > > address this issue:
> > > 
> > > 79e3dbe4b659e78408a9eea76c51a601bd4a383a
> > > tapdisk: respond to destroy request before tearing down the commuication 
> > > channel
> > > 
> > > Is not needed and does not provide formally correct behavior in the
> > > presence of the two patches noted above.
> 
> > Is it incorrect (i.e. should be reverted) or is it just incomplete/not
> > helpful?
> 
> Its incorrect in that it changes a logically correct implementation
> only for the purposes of masking the delay, which in this case, lead
> to the discovery of the root cause of the problem.  Arguably libxl
> needs a bit better error reporting around all this and the patch
> arguably works against that.
> 
> I'm currently running without the patch in 4.1.2 and the tapdisk2
> devices are performing very nicely.

I remember this patch now but I can't seem to find it in any tree, I
suspect it was never applied, which sounds like a good thing.

> Best wishes for a pleasant weekend.

Thanks, and you.

Ian.


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