On Thu, Feb 25, 2016 at 10:42 AM, Shaohua Li <shli@xxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
