> > On 5/12/21 9:09 AM, Hyunchul Lee wrote: > > Hello, > > > > 2021년 5월 12일 (수) 오전 8:57, Eric Sandeen <sandeen@xxxxxxxxxxx>님이 작성: > >> > >> On 5/11/21 6:53 PM, Namjae Jeon wrote: > >> > >>>> One other thing that I ran across is that fsck seems to validate an > >>>> image against the sector size of the device hosting the image > >>>> rather than the sector size found in the boot sector, which seems like another issue that will > come up: > >>>> > >>>> # fsck/fsck.exfat /dev/sdb > >>>> exfatprogs version : 1.1.1 > >>>> /dev/sdb: clean. directories 1, files 0 > >>>> > >>>> # dd if=/dev/sdb of=test.img > >>>> 524288+0 records in > >>>> 524288+0 records out > >>>> 268435456 bytes (268 MB) copied, 1.27619 s, 210 MB/s > >>>> > >>>> # fsck.exfat test.img > >>>> exfatprogs version : 1.1.1 > >>>> checksum of boot region is not correct. 0, but expected 0x3ee721 > >>>> boot region is corrupted. try to restore the region from backup. > >>>> Fix (y/N)? n > >>>> > >>>> Right now the utilities seem to assume that the device they're > >>>> pointed at is always a block device, and image files are problematic. > >>> Okay, Will fix it. > >> > >> Right now I have a hack like this. > >> > >> 1) don't validate the in-image sector size against the host device > >> size (maybe should only skip this check if it's not a bdev? Or is it > >> OK to have a 4k sector size fs on a 512 device? Probably?) > >> > >> 2) populate the "bd" sector size information from the values read from the image. > >> > >> It feels a bit messy, but it works so far. I guess the messiness > >> stems from assuming that we always have a "bd" block device. > >> > > > > I think we need to keep the "bd" sector size to avoid confusion > > between the device's sector size and the exfat's sector size. > > Sure, it's just that for a filesystem in an image file, there is no meaning to the "device sector > size" because there is no device. > > ... > > > Is it better to add a sector size parameter to read_boot_region > > function? This function is also called to read the backup boot region > > to restore the corrupted main boot region. > > During this restoration, we need to read the backup boot region with > > various sector sizes, Because we don't have a certain exfat sector > > size. > > > >> ret = boot_region_checksum(bd, bs_offset); > >> if (ret < 0) > >> goto err; > >> > >> > > > > I sent the pull request to fix these problems. Could you check this request? > > https://protect2.fireeye.com/v1/url?k=7932f7a1-26a9cef7-79337cee-0cc47 > > a31ce52-924b76d62e7bfc04&q=1&e=433c5d9e-f62a-4378-9b98-3c965d70e4da&u= > > https%3A%2F%2Fgithub.com%2Fexfatprogs%2Fexfatprogs%2Fpull%2F167 > > I didn't review that in depth, but it looks like for fsck and dump, it gets the sector size from the > boot sector rather than from the host device or filesystem, which makes sense, at least. > > (As an aside, I'd suggest that your new "c2o" function could have a more descriptive, self-documenting > name.) > > But there are other problems where bd->sector_* is used and assumed to relate to the filesystem, for > example: > > # fsck/fsck.exfat /root/test.img > exfatprogs version : 1.1.1 > /root/test.img: clean. directories 1, files 0 # tune/tune.exfat -I 0x1234 /root/test.img exfatprogs > version : 1.1.1 New volume serial : 0x1234 # fsck/fsck.exfat /root/test.img exfatprogs version : 1.1.1 > checksum of boot region is not correct. 0x3eedc5, but expected 0xe59577e3 boot region is corrupted. > try to restore the region from backup. Fix (y/N)? n > > I think because exfat_write_checksum_sector() relies on bd->sector_size. > > You probably need to audit every use of bd->sector_size and > bd->sector_size_bits outside of mkfs, because anything assuming that it > is related to the filesystem itself, as opposed to the filesytem/device hosting it, could be > problematic. Is there any time outside of mkfs that you actually care about the device sector size? > If not, I might suggest trying to isolate that usage to mkfs. I do not object to updating bd->sector_size to sector size obtained from boot sector. I think that's the best way to do it. > > I also wonder about: > > bd->num_sectors = blk_dev_size / DEFAULT_SECTOR_SIZE; > bd->num_clusters = blk_dev_size / ui->cluster_size; > > is it really correct that this should always be in terms of 512-byte sectors? Yep, Need to fix it. Thanks! > > -Eric
