On Fri, Oct 06, 2017 at 01:22:12PM +1100, Neil Brown wrote: > On Thu, Oct 05 2017, Matthias Kaehlcke wrote: > > > Hi Neil, > > > > El Fri, Oct 06, 2017 at 10:58:59AM +1100 NeilBrown ha dit: > > > >> On Thu, Oct 05 2017, Matthias Kaehlcke wrote: > >> > >> > The raid10 driver can't be built with clang since it uses a variable > >> > length array in a structure (VLAIS): > >> > > >> > drivers/md/raid10.c:4583:17: error: fields must have a constant size: > >> > 'variable length array in structure' extension will never be supported > >> > > >> > Allocate the r10bio struct with kmalloc instead of using the VLAIS > >> > construct. > >> > > >> > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx> > >> > --- > >> > drivers/md/raid10.c | 13 ++++++++----- > >> > 1 file changed, 8 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > >> > index 374df5796649..9616163eaf8c 100644 > >> > --- a/drivers/md/raid10.c > >> > +++ b/drivers/md/raid10.c > >> > @@ -4578,15 +4578,16 @@ static int handle_reshape_read_error(struct mddev *mddev, > >> > /* Use sync reads to get the blocks from somewhere else */ > >> > int sectors = r10_bio->sectors; > >> > struct r10conf *conf = mddev->private; > >> > - struct { > >> > - struct r10bio r10_bio; > >> > - struct r10dev devs[conf->copies]; > >> > - } on_stack; > >> > - struct r10bio *r10b = &on_stack.r10_bio; > >> > + struct r10bio *r10b; > >> > int slot = 0; > >> > int idx = 0; > >> > struct page **pages; > >> > > >> > + r10b = kmalloc(sizeof(*r10b) + > >> > + sizeof(struct r10dev) * conf->copies, GFP_KERNEL); > >> > >> GFP_KERNEL isn't a good idea here. > >> This could wait for writeback, and if writeback tries to write to the > >> region of the array which is being reshaped, it might deadlock. > >> > >> GFP_NOIO is safer. > > > > Good point, thanks! > > > >> given that conf->copies is almost always 2 it might be nicer to > >> have > >> > >> struct { > >> struct r10bio r10_bio; > >> struct r10dev devs[2]; > >> } on_stack; > >> > >> struct r10bio *r10b; > >> > >> if (conf->copies <= ARRAY_SIZE(on_stack.devs)) > >> r10b = &on_stack.r10_bio; > >> else > >> r10b = kmalloc(sizeof(*r10b) + > >> sizeof(struct r10dev) * conf->copies, GFP_NOIO); > > > > It would add also add an extra condition to determine if r10b needs to > > be freed or not. > > True. > > > > > Given that array reshaping is a rare operation and an error during > > this operation is an exceptional condition I think the simpler code > > with always dynamic allocation is preferable. That said I'm fine with > > reworking the patch according to your suggestion if you or Shaohua > > prefer it. > > I don't feel strongly about it. As long as the GFP_KERNEL->GFP_NOIO > change happens I'm OK with this patch. Let's use GFP_NOIO then, should not be big deal. I updated the patch. Thanks, Shaohua -- 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