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

["Dr. Greg Wettstein" <greg@xxxxxxxxxxxxxxxxx>]

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.

> > The first patch is one which was done by Ian for the development tree
> > with minor corrections for 4.1.2.  I'm including it for completeness
> > for those who want a trouble free patch set for a 4.1.2 distribution.
> > This patch fixes the orphaning of the tapdisk2 driver process when xl
> > shuts down.

> This is a fairly straight backport of a patch in unstable?

Correct, the only change besides some version skew noise is a change
in the calling convention for the libxl__gc context passed to
libxl__device_destroy_tapdisk().

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

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

> Looking at 4.2.0-rc1 I see that libxl__device_destroy removes the
> backend before calling libxl__device_destroy_tapdisk, so I think that a
> fix is needed there too.
> 
> I'm less sure about the usage in libxl__initiate_device_remove. I wonder
> if the call to libxl__device_destroy_tapdisk there needs to move to
> device_hotplug_done right after the transaction which cleans up the
> backend back?
> 
> The code which used to be in libxl__devices_destroy is now in
> libxl__initiate_device_remove so I expect that fixing that would be
> sufficient.

The root cause of the problem is a deadlock between the xen-blkback
and blktap2 drivers when the tapdisk2 user space process requests
unmapping of the ring buffer.  The only thing which saves the kernel
is the select timeout on the IPC channel between libxl and the
userspace process.  That timeout allows xl to proceed forward and
teardown the backend which releases the deadlock but the error results
in orphaning of the tapdisk2 minor.

So the overal fix is pretty straight forward, libxl needs to shutdown
the backend before initiating the teardown of the blktap2 userspace
component of the device.

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.

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

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

> For future reference if you intend for a patch to be applied it is best
> to submit it in the form described in
> http://wiki.xen.org/wiki/Submitting_Xen_Patches, that is one patch per
> email, with a changelog specific to that change and a Signed-off-by. In
> this sort of scenario (a patch going to 4.1 which isn't a backport) the
> changelog should also mention why the patch isn't a backport.

Will do, should have been more methodical in my practice.

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.

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

> Ian.

Let me know if you have any additional questions/issues.

Best wishes for a pleasant weekend.

Greg

}-- End of excerpt from Ian Campbell

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: greg@xxxxxxxxxxxx
------------------------------------------------------------------------------
"There is no heavier burden than a great potential."
                                -- Linus' Law

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