Re: raid5d hangs when stopping an array during reshape

[Shaohua Li <shli@kernel.org>]

On Thu, Feb 25, 2016 at 10:48:45AM -0800, Dan Williams wrote:
> On Thu, Feb 25, 2016 at 10:42 AM, Shaohua Li <shli@kernel.org> wrote:
> > On Thu, Feb 25, 2016 at 05:05:17PM +0100, Artur Paszkiewicz wrote:
> >> On 02/25/2016 02:17 AM, Shaohua Li wrote:
> >> > On Thu, Feb 25, 2016 at 11:31:04AM +1100, Neil Brown wrote:
> >> >> On Thu, Feb 25 2016, Shaohua Li wrote:
> >> >>
> >> >>>
> >> >>> As for the bug, write requests run in raid5d, mddev_suspend() waits for all IO,
> >> >>> which waits for the write requests. So this is a clear deadlock. I think we
> >> >>> should delete the check_reshape() in md_check_recovery(). If we change
> >> >>> layout/disks/chunk_size, check_reshape() is already called. If we start an
> >> >>> array, the .run() already handles new layout. There is no point
> >> >>> md_check_recovery() check_reshape() again.
> >> >>
> >> >> Are you sure?
> >> >> Did you look at the commit which added that code?
> >> >> commit b4c4c7b8095298ff4ce20b40bf180ada070812d0
> >> >>
> >> >> When there is an IO error, reshape (or resync or recovery) will abort
> >> >> and then possibly be automatically restarted.
> >> >
> >> > thanks pointing out this.
> >> >> Without the check here a reshape might be attempted on an array which
> >> >> has failed.  Not sure if that would be harmful, but it would certainly
> >> >> be pointless.
> >> >>
> >> >> But you are right that this is causing the problem.
> >> >> Maybe we should keep track of the size of the 'scribble' arrays and only
> >> >> call resize_chunks if the size needs to change?  Similar to what
> >> >> resize_stripes does.
> >> >
> >> > yep, this is my first solution, but think check_reshape() is useless here
> >> > later, apparently miss the restart case. I'll go this way.
> >>
> >> My idea was to replace mddev_suspend()/mddev_resume() in resize_chunks()
> >> with a rw lock that would prevent collisions with raid_run_ops(), since
> >> scribble is used only there. But if the parity operations are executed
> >> asynchronously this would also need to wait until all the submitted
> >> operations have completed. Seems a bit overkill, but I came up with
> >> this:
> >
> > Looks it should work, but it's overkill indead, especially the extra lock, we
> > can replace it with srcu though. The 'track scribble array size' is much
> > simpler, so I'd prefer that way. In the future, we probably should move
> > resize_stripes()/resize_chunks() to .start_reshape().
> > resize_stripes()/resize_chunks() sounds not qualified as .check_reshape().
> >
> 
> Any time any linux-raid mail mentions the raid5_run_ops infrastructure
> I am prompted to remind that async_tx needs to die and be up leveled
> to md directly.  The "help wanted" request is still pending.

A quick search shows async_tx has another user: exofs