On Tue, Nov 06 2018, Gi-Oh Kim wrote:
> On Tue, Nov 6, 2018 at 12:23 AM NeilBrown <neilb@xxxxxxxx> wrote:
>>
>> On Mon, Nov 05 2018, Gioh Kim wrote:
>>
>> > getinfo_super() is called before updating metadata with update_super().
>> > Some information in content variable is not updated.
>> >
>> > Currently there is no problem with it. But I think it could be problematic
>> > at any moment.
>> >
>> > Signed-off-by: Gioh Kim <gi-oh.kim@xxxxxxxxxxxxxxxx>
>>
>> This doesn't make any sense at all to me.
>> This is all in the c->update branch. What the code does is:
>> - load the metadata
>> - update the metadata
>> - save the metadata
>>
>> You want to move part of "load the metadata" to after "save the
>> metadata". That makes no sense.
>
> Hi Neil,
>
> I think loading the metadata is done by load_super().
>
> Current code is:
> load_super(): load the metadata to st->sb
> getinfo_super: copy some metadata from st->sb to "struct mdinfo *content"
> update_super: update the metadata of st->sb
This step can use information from content (which is called 'info'
inside update_super1()). So we need to have called getinfo_super() at
this point.
> store_super: store the metadata of st->sb into the disk
>
> If update_super() changes some metadata, the data of content variable
> is not updated.
It might make sense to call getinfo_super() *again* after calling
update_super(), but it doesn't make sense to remove the call to
getinfo_super() from *before* update_super().
Or maybe the update_super code should be required to update the info
struct as well - not sure.
NeilBrown
>
> For example, update_super() changes st->sb->devflags (of course,
> current code does not change devflags),
> the content variable has only the old flag.
>
> Again, current update_super() does not change data which is copied to
> struct mdinfo *content.
> So there is no problem yet.
> But I think it is not logical.
>
>>
>> NeilBrown
>>
>>
>> > ---
>> > Assemble.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Assemble.c b/Assemble.c
>> > index fc0d464..9fdd4da 100644
>> > --- a/Assemble.c
>> > +++ b/Assemble.c
>> > @@ -618,7 +618,6 @@ static int load_devices(struct devs *devices, char *devmap,
>> > *stp = st;
>> > return -1;
>> > }
>> > - tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
>> >
>> > memcpy(content->uuid, ident->uuid, 16);
>> > strcpy(content->name, ident->name);
>> > @@ -675,6 +674,8 @@ static int load_devices(struct devs *devices, char *devmap,
>> > else
>> > bitmap_done = 1;
>> > }
>> > +
>> > + tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
>> > } else {
>> > dfd = dev_open(devname,
>> > tmpdev->disposition == 'I'
>> > --
>> > 2.14.1
>
>
>
> --
> GIOH KIM
> Linux Kernel Entwickler
>
> 1&1 IONOS Cloud GmbH
> Greifswalder Str. 207
> D - 10405 Berlin
>
> Tel: +49 176 2697 8962
> Fax: +49 30 577 008 299
> Email: gi-oh.kim@xxxxxxxxxxxxxxx
> URL: https://www.ionos.com
>
> Sitz der Gesellschaft: Berlin
> Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
> Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens
Attachment:
signature.asc
Description: PGP signature
