...forwarding this to the list, to avoid duplication of work in case somebody else but Alasdair working on that matter. The warnings still apply, it's only marginally tested (I actually loaded the modules and did a pvmove, but did nothing else with them) This patch would not have been possible without Alasdair finding the cause of the problem. Jan
--- Begin Message ---
- To: Alasdair G Kergon <agk@uk.sistina.com>
- Subject: Re: pvmove hangs
- From: Jan Niehusmann <jan@gondor.com>
- Date: Fri, 22 Aug 2003 02:14:27 +0200
- In-reply-to: <20030821230147.U29420@uk.sistina.com>
- References: <20030816102005.GA12916@gondor.com> <20030816195606.B25758@uk.sistina.com> <20030817114638.GA1839@gondor.com> <20030818185719.C29420@uk.sistina.com> <20030818225358.GA1712@gondor.com> <20030819225130.GA1758@gondor.com> <20030821230147.U29420@uk.sistina.com>
- User-agent: Mutt/1.5.4i
On Thu, Aug 21, 2003 at 11:01:47PM +0100, Alasdair G Kergon wrote: > [off list] ok I thought about the realloc problem. It's basically only a problem because a) dm_table is a table and not a linked list (so it needs to move when it grows) and b) the elements contained in dm_table, namely dm_target object, are directly referenced by the mirror_set structures, which do not get updated on dm_table reallocs. So, to solve the problem, one could change a) or b), or use the standard solution in computer science: add another layer of indirection. I think the last option would be quite easy to implement: In struct dm_table, change dm_target *targets to dm_target **targets, and make it an array (table) of pointers instead of a table of dm_targets. So if you resize the table, the actual dm_target structs don't move in memory. So b) is not a problem any more. And the array of pointers can be moved without problems, because it's only accessed from the dm_table structure, which gets updated properly. As the dm code is well structured, that change would be localised to dm-table.c. See the attached patch for an example. But be warned. I just wrote it and checked that it compiles, it may not even boot, or may cause severe corruption... Jan--- linux-2.4.21/drivers/md/dm-table.c.orig 2003-08-22 01:42:35.000000000 +0200 +++ linux-2.4.21/drivers/md/dm-table.c 2003-08-22 02:10:29.000000000 +0200 @@ -30,7 +30,7 @@ unsigned int num_targets; unsigned int num_allocated; sector_t *highs; - struct dm_target *targets; + struct dm_target **targets; /* * Indicates the rw permissions for the new logical @@ -120,18 +120,19 @@ static int alloc_targets(struct dm_table *t, unsigned int num) { sector_t *n_highs; - struct dm_target *n_targets; + struct dm_target **n_targets; int n = t->num_targets; + unsigned int i; /* * Allocate both the target array and offset array at once. */ - n_highs = (sector_t *) vcalloc(sizeof(struct dm_target) + + n_highs = (sector_t *) vcalloc(sizeof(struct dm_target *) + sizeof(sector_t), num); if (!n_highs) return -ENOMEM; - n_targets = (struct dm_target *) (n_highs + num); + n_targets = (struct dm_target **) (n_highs + num); if (n) { memcpy(n_highs, t->highs, sizeof(*n_highs) * n); @@ -141,6 +142,18 @@ memset(n_highs + n, -1, sizeof(*n_highs) * (num - n)); vfree(t->highs); + for(i=n; i<num; i++) { + *(n_targets+i)=vcalloc(sizeof(struct dm_target),1); + if(!n_highs) break; + } + if(i!=num) { // Could not alloc all dm_targets + for(i--;i>n; i--) { + vfree(*(n_targets+i)); + } + vfree(n_highs); + return -ENOMEM; + } + t->num_allocated = num; t->highs = n_highs; t->targets = n_targets; @@ -192,12 +205,13 @@ /* free the targets */ for (i = 0; i < t->num_targets; i++) { - struct dm_target *tgt = t->targets + i; + struct dm_target *tgt = *(t->targets + i); if (tgt->type->dtr) tgt->type->dtr(tgt); dm_put_target_type(tgt->type); + vfree(tgt); } vfree(t->highs); @@ -437,7 +451,7 @@ if (!table->num_targets) return !ti->begin; - prev = &table->targets[table->num_targets - 1]; + prev = table->targets[table->num_targets - 1]; return (ti->begin == (prev->begin + prev->len)); } @@ -521,7 +535,7 @@ if ((r = check_space(t))) return r; - tgt = t->targets + t->num_targets; + tgt = *(t->targets + t->num_targets); memset(tgt, 0, sizeof(*tgt)); tgt->type = dm_get_target_type(type); @@ -640,7 +654,7 @@ if (index > t->num_targets) return NULL; - return t->targets + index; + return *(t->targets + index); } /* @@ -660,7 +674,7 @@ break; } - return &t->targets[(KEYS_PER_NODE * n) + k]; + return t->targets[(KEYS_PER_NODE * n) + k]; } unsigned int dm_table_get_num_targets(struct dm_table *t) @@ -683,7 +697,7 @@ int i; for (i = 0; i < t->num_targets; i++) { - struct dm_target *ti = t->targets + i; + struct dm_target *ti = *(t->targets + i); if (ti->type->suspend) ti->type->suspend(ti); @@ -695,7 +709,7 @@ int i; for (i = 0; i < t->num_targets; i++) { - struct dm_target *ti = t->targets + i; + struct dm_target *ti = *(t->targets + i); if (ti->type->resume) ti->type->resume(ti);Attachment: pgp00507.pgp
Description: PGP signature
--- End Message ---
