> On Nov 23, 2016, at 10:18 PM, NeilBrown <neilb@xxxxxxxx> wrote:
>
> On Thu, Nov 24 2016, Song Liu wrote:
>
>> +void r5c_use_extra_page(struct stripe_head *sh)
>> +{
>> + struct r5conf *conf = sh->raid_conf;
>> + int i;
>> + struct r5dev *dev;
>> + struct page *p;
>> +
>> + for (i = sh->disks; i--; ) {
>> + dev = &sh->dev[i];
>> + if (dev->orig_page != dev->page) {
>> + p = dev->orig_page;
>> + dev->orig_page = dev->page;
>> put_page(p);
>> }
>> + dev->orig_page = conf->disks[i].extra_page;
>
> It seems a bit pointless to assign to dev->orig_page twice.
> Why not:
>
> if (dev->orig_page != dev->page)
> put_page(dev->orig_page);
> dev->orig_page = conf->......
>
> ??
>
>> @@ -2255,8 +2258,27 @@ static int resize_stripes(struct r5conf *conf, int newsize)
>> if (ndisks) {
>> for (i=0; i<conf->raid_disks; i++)
>> ndisks[i] = conf->disks[i];
>> - kfree(conf->disks);
>> - conf->disks = ndisks;
>> +
>> + /* allocate extra_page for ndisks */
>> + for (i = 0; i < newsize; i++) {
>> + ndisks[i].extra_page = alloc_page(GFP_NOIO);
>> + if (!ndisks[i].extra_page)
>> + err = -ENOMEM;
>> + }
>> +
>> + if (err) {
>> + /* if any error, free extra_page for ndisks */
>> + for (i = 0; i < newsize; i++)
>> + if (ndisks[i].extra_page)
>> + put_page(ndisks[i].extra_page);
>> + kfree(ndisks);
>> + } else {
>> + /* if no error, free extra_page for old disks */
>> + for (i = 0; i < conf->previous_raid_disks; i++)
>> + put_page(ndisks[i].extra_page);
>> + kfree(conf->disks);
>> + conf->disks = ndisks;
>> + }
>
> This looks a bit odd too. We never reduce conf->pool_size, so we never
> need to free anything.
>
> for (i = conf->pool_size; i < newsize; i++)
> if ((ndisks[i].extra_page = alloc_page(GFP_NOIO)) == NULL)
> err = -ENOMEM;
> for (i = conf->pool_size; err == -ENOMEM && i < newsize; i++)
> if (ndisks[i].extra_page)
> put_page(ndisks[i].extra_page);
>
>
> Maybe that it a little terse, but something like that would be better I
> think.
> Certainly you don't need to free ndisks. If the allocation succeeds,
> just use the new array whether other allocations succeed or fail.
>
>> @@ -6466,6 +6515,12 @@ static void free_conf(struct r5conf *conf)
>> free_thread_groups(conf);
>> shrink_stripes(conf);
>> raid5_free_percpu(conf);
>> + for (i = 0; i < conf->raid_disks; i++)
>> + if (conf->disks[i].extra_page) {
>> + put_page(conf->disks[i].extra_page);
>> + conf->disks[i].extra_page = NULL;
>
> There is no point setting extra_page to NULL, as the whole array is
> freed on the next line.
>
>
> Apart from those few little things, it looks good. Thanks.
>
> NeilBrown
Thanks for these feedback! New patch coming.
Song--
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
