Re: [PATCH] a deadlock bug in the kernel-side device mapper code

[malahal@us.ibm.com]

Alasdair G Kergon [agk@redhat.com] wrote:
> On Mon, Nov 09, 2009 at 09:57:30AM -0800, malahal@us.ibm.com wrote:
> > Why do we need dm_get() and dm_put() in dm_copy_name_and_uuid()?
> > dm_copy_name_and_uuid() already has access to md and there shouldn't be
> > any need to hold a reference. 
> 
> As Mike points out, struct dm_uevent holds a reference without
> incrementing the ref count.
> 
> dm_path_uevent() already takes a reference - can everything get
> simplified if we move this code there (and replace the new mutex with
> a spinlock of course)?  Then dm_send_uevents won't need to use 'md'.

I think the last dm_put() is calling the multipath destructor which
eventually calls (through a work struct) dm_send_uevents. At that point,
dm is guaranteed to exist but you *must* not place a hold on it. As the
current code in dm_copy_name_and_uuid() places a hold on a FREEING md,
it will result in a BUG_ON while calling the dm_put in the same
function. So, removing the dm_get/dm_put from that function should also
solve the BUG_ON issue.

I agree, removing the "md" field from the dm_event struct probably
simplifies other things.

Thanks, Malahal.