> -----Original Message----- > From: NeilBrown [mailto:neilb@xxxxxxx] > Sent: Tuesday, February 15, 2011 2:27 AM > To: Kwolek, Adam > Cc: linux-raid@xxxxxxxxxxxxxxx; Williams, Dan J; Ciechanowski, Ed; > Neubauer, Wojciech > Subject: Re: [PATCH 2/5] imsm: prepare update for level migrations > reshape > > On Mon, 14 Feb 2011 14:12:57 +0100 Adam Kwolek <adam.kwolek@xxxxxxxxx> > wrote: > > > For level migrations: > > 1. raid0->raid5 > > 2. raid5->raid0 > > metadata update is prepared > > > > For migration raid0->raid5 spare device is required. > > It is checked in mdadm for spares, but it is not included in to > update. > > Mdmon will decide what spare device > > > > Signed-off-by: Adam Kwolek <adam.kwolek@xxxxxxxxx> > > --- > > > > super-intel.c | 116 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 114 insertions(+), 2 deletions(-) > > > > diff --git a/super-intel.c b/super-intel.c > > index f3e248e..489340f 100644 > > --- a/super-intel.c > > +++ b/super-intel.c > > @@ -305,6 +305,7 @@ enum imsm_update_type { > > update_rename_array, > > update_add_remove_disk, > > update_reshape_container_disks, > > + update_reshape_migration, > > update_takeover > > }; > > > > @@ -340,6 +341,12 @@ struct imsm_update_reshape { > > enum imsm_update_type type; > > int old_raid_disks; > > int new_raid_disks; > > + /* fields for array migration changes > > + */ > > + int subdev; > > + int new_level; > > + int new_layout; > > + > > I don't like how you are overloading this structure. You are adding > fields > to a structure that doesn't need them in its original context, and > using a > structure containing old_raid_disks and new_raid_disks in a context > where > they are needed. > > It would be much better to have a separate structure for each different > imsm_update_type. > > > > int new_disks[1]; /* new_raid_disks - old_raid_disks makedev > number */ > > Hmmm... new_disks is already there, isn't it. > I'm not at all sure that I'm happy about that. > It is used when increasing the number of devices in an array, but I > would > like that to just change the numbers and leave mdmon to add the > devices. > > 'reshape_super' should just verify that the change is legal, update > the > metadata to show that the change has happened or is happening, and > possibly > create an update to send to mdmon. > reshape_super should not choose or allocate spares. > > 'prepare_update' should *only* perform any allocations that > process_update > will need. It doesn't allocate spares either. It *only* allocates > memory. > > 'process_update' takes the update description and modifies the > metadata, and > updates internal data structures so that they accurately reflect the > information in the metadata. It doesn't allocate spares either. > > 'activate_spare' is the only place that should activate spares. It > analyses > the array, determines if any devices need to be added, finds them, and > creates a metadata update to describe the new devices. > manage_member() then adds them to the array and process_update > eventually > records these new allocations in the metadata. > > It is really important to keep these function separate and make sure > each > function just does the one thing that it is supposed to do. Otherwise > all > the interactions become too complex and so are easily broken. I'll think how to connect all together. > > > > }; > > > > @@ -6133,6 +6140,9 @@ static void imsm_process_update(struct > supertype *st, > > super->updates_pending++; > > break; > > } > > + case update_reshape_migration: { > > + break; > > + } > > case update_activate_spare: { > > struct imsm_update_activate_spare *u = (void *) update- > >buf; > > struct imsm_dev *dev = get_imsm_dev(super, u->array); > > @@ -6526,6 +6536,9 @@ static void imsm_prepare_update(struct > supertype *st, > > dprintf("New anchor length is %llu\n", (unsigned long > long)len); > > break; > > } > > + case update_reshape_migration: { > > + break; > > + } > > case update_create_array: { > > struct imsm_update_create_array *u = (void *) update->buf; > > struct intel_dev *dv; > > @@ -6892,6 +6905,89 @@ abort: > > return 0; > > } > > > > > +/********************************************************************* > ********* > > + * function: imsm_create_metadata_update_for_migration() > > + * Creates update for IMSM array. > > + * > > + > *********************************************************************** > *******/ > > +static int imsm_create_metadata_update_for_migration( > > + struct supertype *st, > > + struct geo_params *geo, > > + struct imsm_update_reshape **updatep) > > +{ > > + struct intel_super *super = st->sb; > > + int update_memory_size = 0; > > + struct imsm_update_reshape *u = NULL; > > + int delta_disks = 0; > > + struct imsm_dev *dev; > > + int previous_level = -1; > > + > > + dprintf("imsm_create_metadata_update_for_migration(enter)" > > + " New Level = %i\n", geo->level); > > + delta_disks = 0; > > + > > + /* size of all update data without anchor */ > > + update_memory_size = sizeof(struct imsm_update_reshape); > > + > > + /* now add space for spare disks that we need to add. */ > > + if (delta_disks > 0) > > + update_memory_size += sizeof(u->new_disks[0]) > > + * (delta_disks - 1); > > + > > + u = calloc(1, update_memory_size); > > + if (u == NULL) { > > + dprintf("error: cannot get memory for " > > + "imsm_create_metadata_update_for_migration\n"); > > + return 0; > > + } > > + u->type = update_reshape_migration; > > + u->subdev = super->current_vol; > > + u->new_level = geo->level; > > + u->new_layout = geo->layout; > > + u->new_raid_disks = u->old_raid_disks = geo->raid_disks; > > + u->new_disks[0] = -1; > > + > > + dev = get_imsm_dev(super, u->subdev); > > + if (dev) { > > + struct imsm_map *map; > > + > > + map = get_imsm_map(dev, 0); > > + if (map) > > + previous_level = map->raid_level; > > + } > > + if ((geo->level == 5) && (previous_level == 0)) { > > + struct mdinfo *spares = NULL; > > + > > + u->new_raid_disks++; > > + spares = get_spares_for_grow(st); > > And here you are allocating spares in mdadm even though we agreed that > you > weren't going to do that. > > > NeilBrown Yes you are right, using spares below is my mistake during code rebasing. Allocating spares I think is ok as spares check, but not using them. Summarizing I have make some changes to all patches. This have to wait few day, I'm working now on issue in expansion. Details, I'll sent you shortly. BR Adam > > > > > > + if (spares) { > > + struct dl *dl; > > + struct mdinfo *dev; > > + > > + dev = spares->devs; > > + if (dev) { > > + u->new_disks[0] = makedev(dev->disk.major, > > + dev->disk.minor); > > + dl = get_disk_super(super, > > + dev->disk.major, > > + dev->disk.minor); > > + dl->index = u->old_raid_disks; > > + dev = dev->next; > > + } > > + sysfs_free(spares); > > + } else { > > + free(u); > > + dprintf("error: cannot get spare device " > > + "for requested migration"); > > + return 0; > > + } > > + } > > + dprintf("imsm: reshape update preparation : OK\n"); > > + *updatep = u; > > + > > + return update_memory_size; > > +} > > + > > static void imsm_update_metadata_locally(struct supertype *st, > > void *buf, int len) > > { > > @@ -7146,9 +7242,25 @@ static int imsm_reshape_super(struct supertype > *st, long long size, int level, > > case CH_TAKEOVER: > > ret_val = imsm_takeover(st, &geo); > > break; > > - case CH_MIGRATION: > > + case CH_MIGRATION: { > > + struct imsm_update_reshape *u = NULL; > > + int len = > > + imsm_create_metadata_update_for_migration( > > + st, &geo, &u); > > + if (len < 1) { > > + dprintf("imsm: " > > + "Cannot prepare update\n"); > > + } > > ret_val = 0; > > - break; > > + /* update metadata locally */ > > + imsm_update_metadata_locally(st, u, len); > > + /* and possibly remotely */ > > + if (st->update_tail) > > + append_metadata_update(st, u, len); > > + else > > + free(u); > > + } > > + break; > > default: > > ret_val = 1; > > } -- 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
