[dm-devel] staged dm_internal_{suspend, resume} related changes for wider review

[Mikulas Patocka]

On Wed, 5 Nov 2014, Mike Snitzer wrote:

> On Wed, Nov 05 2014 at  8:05am -0500,
> Mikulas Patocka <mpatocka at redhat.com> wrote:
> 
> > On Wed, 5 Nov 2014, Mikulas Patocka wrote:
> > 
> > > You can for example set the flag in the prison meaning that the prison is 
> > > suspended and then call dm_internal_suspend immediatelly followed by 
> > > dm_internal_resume - that will clear in-progress bios and prevent new bios 
> > > from coming in (and we don't need to change dm_internal_suspend and 
> > > dm_internal_resume to become so big).
> 
> It may _seem_ like they have gotten big given the code was refactored to
> share code with dm_suspend and dm_resume.  BUT I know you see that the
> actual code complexity isn't big.  I especially wanted you (and/or Bryn)
> to evaluate the performance implications that my changes had on
> dm-stats.  I'm pretty confident there won't be much if any performance
> difference (given the code is identical to what you had, except some
> extra checks are made but ultimately not used, e.g. lockfs/unlockfs).

This is not about performance, it is about unclear behavior.

If someone does internal_suspend followed by remove, what should be the 
correct behavior? The current code deadlocks in this case.

The patch series introduces two suspend mechanisms and it is unclear how 
should they interact with each other.

> The end result of the dm_internal_{suspend,resume} changes is an
> interface that is useful for DM targets in addition to dm-stats.  That
> is the kind of advancement DM needs.
> 
> Please focus on the performance impact of my changes, if any, and we'll
> go from there.
> 
> > No, the correct sequence is this (do this in presuspend handler):
> > 
> > 1. call dm_internal_suspend on all thin devices
> > 2. set the flag in the prison meaning that the prison is blocked
> > 3. call dm_internal_resume on all thin devices
> 
> I really didn't like the idea of reusing the bio-prison to achieve the
> suspend of all thins to begin with.  This proposal is even more suspect
> given the desire to call dm_internal_suspend and dm_internal_resume from
> pool_presuspend.
> 
> It just isn't code that I want to see making its way into the tree.
> Sets bad precedent of hacking around problems within a target for
> questionable gain (questionable in that there really isn't a
> pattern/infrastructure for other more complex targets to follow so
> they'd need to invent their own hack should they have a comparable
> problem).

At least the hack stays within dm-thin and generic dm code isn't 
contaminated by it.

Mikulas