"Brian C. Lane" writes: >> >> crc = __efi_crc32(sb, sumoff, PED_LE32_TO_CPU(sb->s_crc_seed)); >> >> @@ -113,11 +113,13 @@ nilfs2_probe (PedGeometry* geom) >> >> const int sectors = (4096 + geom->dev->sector_size - 1) / >> >> geom->dev->sector_size; >> >> char *buf = alloca (sectors * geom->dev->sector_size); >> >> - void *buff2 = alloca (geom->dev->sector_size); >> >> + const int sectors2 = sizeof(struct nilfs2_super_block) / geom->dev->sector_size + >> >> + (sizeof(struct nilfs2_super_block) % geom->dev->sector_size == 0) ? 0 : 1; >> > >> > This calculation is correct, but I find it hard to read. If you use the >> > same technique as it does for sectors it would be easier to understand >> > in the future, and I don't think the superblock size is going to change. >> >> Probably I should have spent more time trying to understand the way >> sectors was calculated or asked about it before submitting the patch. It >> confused me, since in my case, where geom->dev->sector_size was 512, >> that calculation gave a size that meant eight 512 byte sectors were read >> instead of two (sizeof nilfs2_super_block = 1024): >> >> (4096 + 512 - 1) / 512 = 8. >> >> And that's what it did, except all at once, based on the strace... >> >> lseek(3, 11813257216, SEEK_SET) = 11813257216 >> read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096 >> >> And then there was the 1024 offset introduced when assigning to the >> primary superblock, sb, which I didn't understand the purpose of... >> >> if (ped_geometry_read(geom, buf, 0, sectors)) >> sb = (struct nilfs2_super_block *)(buf+1024); >> >> >> I wasn't sure if reading the extra six sectors for the 2nd superblock >> would be okay, e.g. if the superblock was really close to the end of a >> disk. And in general there are these things about reading the first >> superblock which I don't understand, so I'm unclear if the two lengths >> should be computed the same way. If so should we look for the 2nd >> superblock 1024 bytes into the 4096 bytes read like we do for the 1st >> superblock? > > I can't seem to find a decent reference for NILFS other than this code > and the linux kernel code so I'm not sure why it reads so much for the > first one. I think you've got the logic right, I just think it would be > easier to read as: > > sectors2 = (1024 + geom->dev->sector_size - 1) / geom->dev->sector_size; > > When reading the 2nd superblock it looks like it starts on a sector > boundary so that's why it doesn't need the 4096 offset. I've attached a corrected fix with that calculation written more clearly as you suggest. -- Mike Small smallm@sdf.org