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:
signature.asc
Description: PGP signature
