On Sat, 23 Jul 2022 14:37:11 +1000
"NeilBrown" <neilb@xxxxxxx> wrote:
> On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote:
> > Hi Neil,
> >
> > On Wed, 13 Jul 2022 13:48:11 +1000
> > "NeilBrown" <neilb@xxxxxxx> wrote:
> >
> > > When the metadata is at the start of the device, it is possible that it
> > > describes a device large than the one it is actually stored on. When
> > > this happens, report it loudly in --examine.
> > >
> > > ....
> > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO
> > > SMALL State : clean TRUNCATED DEVICE
> > > ....
> >
> > State : clean TRUNCATED DEVICE is enough. "DEVICE TOO SMALL" seems to be
> > redundant.
>
> I needed to change the "Unused Space" line because before the patch the
> "after=" value is close to 2^64. I needed to make it negative. But having
> a negative value there is strange so I thought it would be good to
> highlight it and explain why.
Got it, thanks.
>
> > >
> > > Also report in --assemble so that the failure which the kernel will
> > > report will be explained.
> >
> > Understand but you've added it in load_super1() so it affects all
> > load_super() calls, is it indented? I assume yes but please confirm.
>
> Yes, it is intended for all calls to ->load_super() on v1 metadata.
> The test is gated on ->ignore_hw_compat so that it does still look like
> v1.x metadata (so --examine can report on it), but an error results for
> any attempt to use the metadata in an active array.
>
> ->ignore_hw_compat isn't a perfect fit for the concept, but it is a
> perfect fit for the desired behaviour. Maybe we should rethink the name
> for that field.
>
> > >
> > > mdadm: Device /dev/sdb is not large enough for data described in
> > > superblock mdadm: no RAID superblock on /dev/sdb
> > > mdadm: /dev/sdb has no superblock - assembly aborted
> > >
> > > Scenario can be demonstrated as follows:
> > >
> > > mdadm: Note: this array has metadata at the start and
> > > may not be suitable as a boot device. If you plan to
> > > store '/boot' on this device please ensure that
> > > your boot-loader understands md/v1.x metadata, or use
> > > --metadata=0.90
> > > mdadm: Defaulting to version 1.2 metadata
> > > mdadm: array /dev/md/test started.
> > > mdadm: stopped /dev/md/test
> > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO
> > > SMALL State : clean TRUNCATED DEVICE
> > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO
> > > SMALL State : clean TRUNCATED DEVICE
> > >
> > > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > > ---
> > > super1.c | 34 +++++++++++++++++++++++++++-------
> > > 1 file changed, 27 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/super1.c b/super1.c
> > > index 71af860c0e3e..4d8dba8a5a44 100644
> > > --- a/super1.c
> > > +++ b/super1.c
> > > @@ -406,12 +406,18 @@ static void examine_super1(struct supertype *st,
> > > char *homehost)
> > > st->ss->getinfo_super(st, &info, NULL);
> > > if (info.space_after != 1 &&
> > > - !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET))
> > > - printf(" Unused Space : before=%llu sectors, after=%llu
> > > sectors\n",
> > > - info.space_before, info.space_after);
> > > -
> > > - printf(" State : %s\n",
> > > - (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean");
> > > + !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) {
> > > + printf(" Unused Space : before=%llu sectors, ",
> > > + info.space_before);
> > > + if (info.space_after < INT64_MAX)
> > > + printf("after=%llu sectors\n", info.space_after);
> > > + else
> > > + printf("after=-%llu sectors DEVICE TOO SMALL\n",
> > > + UINT64_MAX - info.space_after);
> > As above, for me this else here is not necessary.
>
> The change to report a negative is necessary.
>
> >
> > > + }
> > > + printf(" State : %s%s\n",
> > > + (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean",
Please add space before '?' and between and after ':' (same as below).
> > > + info.space_after > INT64_MAX ? " TRUNCATED DEVICE" : "");
> > >
> >
> > Could you use standard if instruction to make the code more readable? We are
> > avoiding ternary operators if possible now.
>
> I could. I don't want to.
> I think the code is quite readable. Putting a space before the first
> '?' would help, as might lining up the two '?'.
Please fix formatting and I'm fine with that. In this case ternary if is
reasonable.
>
> >
> > > printf(" Device UUID : ");
> > > for (i=0; i<16; i++) {
> > > if ((i&3)==0 && i != 0)
> > > @@ -2206,6 +2212,7 @@ static int load_super1(struct supertype *st, int fd,
> > > char *devname) tst.ss = &super1;
> > > for (tst.minor_version = 0; tst.minor_version <= 2;
> > > tst.minor_version++) {
> > > + tst.ignore_hw_compat = st->ignore_hw_compat;
> > > switch(load_super1(&tst, fd, devname)) {
> > > case 0: super = tst.sb;
> > > if (bestvers == -1 ||
> > > @@ -2312,7 +2319,6 @@ static int load_super1(struct supertype *st, int fd,
> > > char *devname) free(super);
> > > return 2;
> > > }
> > > - st->sb = super;
> > >
> > > bsb = (struct bitmap_super_s *)(((char*)super)+MAX_SB_SIZE);
> > >
> > > @@ -2322,6 +2328,20 @@ static int load_super1(struct supertype *st, int
> > > fd, char *devname) if (st->data_offset == INVALID_SECTORS)
> > > st->data_offset = __le64_to_cpu(super->data_offset);
> > >
> > > + if (st->minor_version >= 1 &&
> > > + st->ignore_hw_compat == 0 &&
> > > + (__le64_to_cpu(super->data_offset) +
> > > + __le64_to_cpu(super->size) > dsize ||
> > > + __le64_to_cpu(super->data_offset) +
> > > + __le64_to_cpu(super->data_size) > dsize)) {
> > > + if (devname)
> > > + pr_err("Device %s is not large enough for data
> > > described in superblock\n",
> > > + devname);
> >
> > why not just:
> > if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) >
> > dsize) from my understanding, only this check matters.
>
> It seemed safest to test both. I don't remember the difference between
> ->size and ->data_size. In getinfo_super1() we have
>
> if (info->array.level <= 0)
> data_size = __le64_to_cpu(sb->data_size);
> else
> data_size = __le64_to_cpu(sb->size);
>
> which suggests that either could be relevant.
> I guess ->size should always be less than ->data_size. But
> load_super1() doesn't check that, so it isn't safe to assume it.
Honestly, I don't understand why but I didn't realize that you are checking two
different fields (size and data_size). I focused on understanding what is going
on here, and didn't catch difference in variables (because data_offset and
data_size have similar prefix).
For me, something like:
unsigned long long _size;
if (st->minor_version >= 1 && st->ignore_hw_compat == 0)
_size= __le64_to_cpu(super->size);
else
_size= __le64_to_cpu(super->data_size);
if (__le64_to_cpu(super->data_offset) + _size > dsize)
{....}
is more readable because I don't need to analyze complex if to get the
difference. Also, I removed doubled __le64_to_cpu(super->data_offset).
Could you refactor this part?
Thanks,
Mariusz
