Re: [PATCH v4] md/r5cache: handle alloc_page failure

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2517 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]