On Thu, Feb 25, 2016 at 11:17 AM, Shaohua Li <shli@xxxxxxxxxx> wrote: > 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@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. > > A quick search shows async_tx has another user: exofs Yes, same up leveling of api internals into the user directly needs to be done there as well. More help wanted :-). -- 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
