Re: Basic blktap2 functionality issues.

[Ian Campbell <Ian.Campbell@citrix.com>]

On Sat, 2012-04-07 at 16:59 +0100, Dr. Greg Wettstein wrote:
> On Mar 30, 12:23pm, Ian Campbell wrote:
> } Subject: Re: [Xen-devel] Basic blktap2 functionality issues.
> 
> Hi, hope the weekend is going well for everyone.
> 
> Sorry for the delay in getting back to everyone on this, had a
> deadline on another project.
> 
> > On Fri, 2012-03-30 at 09:17 +0100, Ian Campbell wrote:
> > > I think an approach worth trying would be to have
> > > tapdisk_control_detach_vbd respond to TAPDISK_MESSAGE_DETACH before
> > > doing the actual detach. i.e. it would respond with "Yes, I will do
> > > that" rather than "Yes, I have done that". My speculation is that this
> > > will allow libxl to continue and hopefully avoid the deadlock.
> 
> > This seems to be the case as the following fixes things for
> > me. Thanks very much for your analysis which lead me to this
> > solution...
> 
> I ported your fix into 4.1.2 but I think we still have a problem, at
> least in this codebase.
> 
> I no longer see the select timeout delay when xl shuts down but upon
> shutdown the minor number is not freed.  A 'tap-ctl list' shows a
> steadily increasing set of orphaned minor numbers as VM's are started
> up and shutdown.
> 
> Are you seeing this in your development codebase?

It turns out that I am, yes. e.g. after starting/stopping a guest 3
times:
# tap-ctl list
       -  0    -          - -
       -  1    -          - -
       -  2    -          - -

> The culprit is a failed ioctl call for BLKTAP2_IOCTL_FREE_TAP in
> tap_ctl_free().  The underlying reason for the ioctl failure is the
> check in [linuxsrc]:drivers/block/blktap/ring.c:blktap_ring_destroy()

drivers/*xen*/... right? Or do you have a different blktap to me?

> for whether or not the task_struct pointer in the blktap_ring
> structure has been NULLed.
> 
> Which certainly makes sense since there is a race between xl's call to
> tap_ctl_free() and tapdisk2 getting to the point where it can close
> its descriptor to the blktap instance and thus invoke the .release
> method which translates into a call to blktap_ring_release() which
> NULL's the task_struct pointer.

Sounds right, but then you would expect both the ioctl and release path
to cleanup, depending on who loses the race? 

There also seems to be a BLKTAP_SHUTDOWN_REQUESTED bit which looks like
it is involved somehow...

We've gotten way beyond my understanding of blktap internals though.

> If you are not seeing the orphan minor numbers there must be ordering
> changes in the unstable version of xl which eliminate or alters the
> race timing.
> 
> For the sake of completeness of information for this thread I captured
> the following stack trace of a tapdisk2 when it is deadlocked against
> xl:
[... thanks ...]
> Let me know if you are seeing the issues I'm seeing, in the meantime I
> will keep hunting to see if I can rundown the ultimate cause of the
> deadlock.  Given the above trace it has to be an issue with xl
> orchestrating the release of resources which reference the tapdev
> block device.

I'm not convinced that the shutdown stuff on the kernel side isn't
either horribly broken or at best fragile (perhaps xl just tickles it
differently to xend).

Please do keep hunting and let us know what you find...

Thanks,
Ian