dm-table: delayed cleanup for dm_table_destroy()
 

On Tue, Mar 20 2012 at 11:35am -0400,
Hannes Reinecke <hare@suse.de> wrote:

> Hi Mikulas,
>
> On 03/19/2012 10:45 PM, Mikulas Patocka wrote:
> > Hi Hannes
> >
> > I wouldn't do this. This behavior actually existed in kernels <= 2.6.28.
> > And it caused trouble.
> >
> > The problems are these:
> >
> > Some code may hold a spinlock and call dm_table_put. Currently,
> > dm_table_put just decrements the counter and the cleanup is done
> > synchronously in dm_table_destroy. With your patch, cleanup would be done
> > in dm_table_put --- you call a target destructor (which may sleep) in a
> > non-sleeping context.
> >
> > Some code may hold a mutex and call dm_table_put. If you call a target
> > destructor from dm_table_put and it takes the same mutex, it deadlocks.
> >
> > Currently, when dm_table_destroy exits, it is guaranteed that the table is
> > destroyed and all target destructors have been called. With your patch,
> > dm_table_destroy may exit and the table could be still alive because of
> > some other reference. This may cause leaked references to some other files
> > or devices. For example, suppose that the user has a device mapper device
> > "dm" that redirects requests to "/dev/sda". The user assumes that if he
> > runs "dmsetup remove dm" and this command returns, "/dev/sda" will not be
> > open. Your patch breaks this assumption.
> >
> Hmm. Yes, true.
>
> > Look at the commit "d58168763f74d1edbc296d7038c60efe6493fdd4" in 2.6.29
> > --- I was actually removing the old-style reference counting (that is
> > functionally equivalent to what your patch does) because of these three
> > problems. The old code really caused a crash although it happened very
> > rarely.
> >
> Ah. That'll explain it.
> The actual problem I'm trying to track down is that I'm seeing an
> excessive duration for the 'resume' dm ioctl after a table update.
> I've got reports where the ioctl can take up to several seconds.
> Which (as this is multipath) causes an extremely erratic behaviour.
>
> And the 'msleep' here is one of the obvious culprits.
>
> But I'll continue debugging, maybe I'll find something else.

I once wanted to replace all msleep(1); with cpu_relax();:
http://www.redhat.com/archives/dm-devel/2010-September/msg00100.html

But Alasdair wasn't sure if cpu_relax() would provide the required
delay effect:
https://www.redhat.com/archives/dm-devel/2011-June/msg00080.html

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

dm-table: delayed cleanup for dm_table_destroy()
 

On Tue, 20 Mar 2012, Mike Snitzer wrote:

> I once wanted to replace all msleep(1); with cpu_relax();:
> http://www.redhat.com/archives/dm-devel/2010-September/msg00100.html
>
> But Alasdair wasn't sure if cpu_relax() would provide the required
> delay effect:
> https://www.redhat.com/archives/dm-devel/2011-June/msg00080.html

cpu_relax() would basically kill the kernel if compiled without preempt.

cpu_relax() makes the current processor sleep for a little moment, but it
doesn't schedule a different process.

cpu_relax() is useful in spinlocks - so that process waiting on a spinlock
is not slowing down the other process on a CPU with hyperthreading.


There was another possibility --- replace msleep(1) with yield(). The
problem with yield() is that if the process has readltime priority and
calls yield(), it doesn't give CPU to a different process with a lower
priority. So it would cause deadlock if executed with realtime priority.
msleep(1) doesn't have this problem, so it's the best.

Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel