On Mon, Nov 25, 2024 at 06:47:48PM +0100, Caleb Connolly wrote:
> Hi Varadarajan,
>
> Thanks for re-spinning this.
>
> On 25/11/2024 09:52, Varadarajan Narayanan wrote:
> > Do FAT read and write based on the device sector size
> > instead of the size recorded in the FAT meta data.
> >
> > FAT code issues i/o in terms of the sector size. Convert that to
> > device sector size before doing the actual i/o. Additionally,
> > handle leading/trailing blocks when the meta data based block
> > no and i/o size is not an exact multiple of the device sector
> > size or vice versa.
> >
> > Tested on UFS device with sector size 4096 and meta data recorded
> > sector size 512.
> >
> > Signed-off-by: Varadarajan Narayanan <[email protected]>
>
> I'd appreciate it if you could just CC me in the v2, then you don't have
> to reply to the v1 thread.
Sure, will do it from next version.
> It would also be great if you could include a changelog so we can keep
> track of things across versions. Just for future reference.
Main motivation for this respin to address the comments of V1
* Updated commit description with better explanation
* Changed 'fat_sect_size' to be static
* Removed the incorrect read related code in write function
Does this suffice, or should I update this in the commit log and
post a v3? Please let me know.
Thanks
Varada
> Kind regards,
> > ---
> > fs/fat/fat.c | 218 ++++++++++++++++++++++++++++++++++++++++++---
> > fs/fat/fat_write.c | 19 ----
> > 2 files changed, 205 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index e2570e8167..efbf892c7a 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -44,24 +44,214 @@ static void downcase(char *str, size_t len)
> >
> > static struct blk_desc *cur_dev;
> > static struct disk_partition cur_part_info;
> > +static int fat_sect_size;
> >
> > #define DOS_BOOT_MAGIC_OFFSET 0x1fe
> > #define DOS_FS_TYPE_OFFSET 0x36
> > #define DOS_FS32_TYPE_OFFSET 0x52
> >
> > -static int disk_read(__u32 block, __u32 nr_blocks, void *buf)
> > +inline __u32 sect_to_block(__u32 sect, __u32 *off)
> > {
> > - ulong ret;
> > + *off = 0;
> > + if (fat_sect_size && fat_sect_size < cur_part_info.blksz) {
> > + int div = cur_part_info.blksz / fat_sect_size;
> > +
> > + *off = sect % div;
> > + return sect / div;
> > + } else if (fat_sect_size && (fat_sect_size > cur_part_info.blksz)) {
> > + return sect * (fat_sect_size / cur_part_info.blksz);
> > + }
> >
> > - if (!cur_dev)
> > - return -1;
> > + return sect;
> > +}
> >
> > - ret = blk_dread(cur_dev, cur_part_info.start + block, nr_blocks, buf);
> > +inline __u32 size_to_blocks(__u32 size)
> > +{
> > + return (size + (cur_part_info.blksz - 1)) / cur_part_info.blksz;
> > +}
> >
> > - if (ret != nr_blocks)
> > - return -1;
> > +static int disk_read(__u32 sect, __u32 nr_sect, void *buf)
> > +{
> > + int ret;
> > + __u8 *block = NULL;
> > + __u32 rem, size;
> > + __u32 s, n;
> >
> > - return ret;
> > + rem = nr_sect * fat_sect_size;
> > + /*
> > + * block N block N + 1 block N + 2
> > + * +-+-+--+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > + * | | | | |s|e|c|t|o|r| | |s|e|c|t|o|r| | |s|e|c|t|o|r| | | | |
> > + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > + * . . . | | | | . . .
> > + * ------+---------------+---------------+---------------+------
> > + * |<--- FAT reads in sectors --->|
> > + *
> > + * | part 1 | part 2 | part 3 |
> > + *
> > + */
> > +
> > + /* Do part 1 */
> > + if (fat_sect_size) {
> > + __u32 offset;
> > +
> > + /* Read one block and copy out the leading sectors */
> > + block = malloc_cache_aligned(cur_dev->blksz);
> > + if (!block) {
> > + printf("Error: allocating block: %lu\n",
> > cur_dev->blksz);
> > + return -1;
> > + }
> > +
> > + s = sect_to_block(sect, &offset);
> > + offset = offset * fat_sect_size;
> > +
> > + ret = blk_dread(cur_dev, cur_part_info.start + s, 1, block);
> > + if (ret != 1) {
> > + ret = -1;
> > + goto exit;
> > + }
> > +
> > + if (rem > (cur_part_info.blksz - offset))
> > + size = cur_part_info.blksz - offset;
> > + else
> > + size = rem;
> > +
> > + memcpy(buf, block + offset, size);
> > + rem -= size;
> > + buf += size;
> > + s++;
> > + } else {
> > + /*
> > + * fat_sect_size not being set implies, this is the first read
> > + * to partition. The first sector is being read to get the
> > + * FS meta data. The FAT sector size is got from this meta data.
> > + */
> > + ret = blk_dread(cur_dev, cur_part_info.start + s, 1, buf);
> > + if (ret != 1)
> > + return -1;
> > + }
> > +
> > + /* Do part 2, read directly into the given buffer */
> > + if (rem > cur_part_info.blksz) {
> > + n = rem / cur_part_info.blksz;
> > + ret = blk_dread(cur_dev, cur_part_info.start + s, n, buf);
> > + if (ret != n) {
> > + ret = -1;
> > + goto exit;
> > + }
> > + buf += n * cur_part_info.blksz;
> > + rem = rem % cur_part_info.blksz;
> > + s += n;
> > + }
> > +
> > + /* Do part 3, read a block and copy the trailing sectors */
> > + if (rem) {
> > + ret = blk_dread(cur_dev, cur_part_info.start + s, 1, block);
> > + if (ret != 1) {
> > + ret = -1;
> > + goto exit;
> > + } else {
> > + memcpy(buf, block, rem);
> > + }
> > + }
> > +exit:
> > + if (block)
> > + free(block);
> > +
> > + return (ret == -1) ? -1 : nr_sect;
> > +}
> > +
> > +int disk_write(__u32 sect, __u32 nr_sect, void *buf)
> > +{
> > + int ret;
> > + __u8 *block = NULL;
> > + __u32 rem, size;
> > + __u32 s, n;
> > +
> > + rem = nr_sect * fat_sect_size;
> > + /*
> > + * block N block N + 1 block N + 2
> > + * +-+-+--+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > + * | | | | |s|e|c|t|o|r| | |s|e|c|t|o|r| | |s|e|c|t|o|r| | | | |
> > + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > + * . . . | | | | . . .
> > + * ------+---------------+---------------+---------------+------
> > + * |<--- FAT reads in sectors --->|
> > + *
> > + * | part 1 | part 2 | part 3 |
> > + *
> > + */
> > +
> > + /* Do part 1 */
> > + if (fat_sect_size) {
> > + __u32 offset;
> > +
> > + /* Read one block and overwrite the leading sectors */
> > + block = malloc_cache_aligned(cur_dev->blksz);
> > + if (!block) {
> > + printf("Error: allocating block: %lu\n",
> > cur_dev->blksz);
> > + return -1;
> > + }
> > +
> > + s = sect_to_block(sect, &offset);
> > + offset = offset * fat_sect_size;
> > +
> > + ret = blk_dread(cur_dev, cur_part_info.start + s, 1, block);
> > + if (ret != 1) {
> > + ret = -1;
> > + goto exit;
> > + }
> > +
> > + if (rem > (cur_part_info.blksz - offset))
> > + size = cur_part_info.blksz - offset;
> > + else
> > + size = rem;
> > +
> > + memcpy(block + offset, buf, size);
> > + ret = blk_dwrite(cur_dev, cur_part_info.start + s, 1, block);
> > + if (ret != 1) {
> > + ret = -1;
> > + goto exit;
> > + }
> > +
> > + rem -= size;
> > + buf += size;
> > + s++;
> > + }
> > +
> > + /* Do part 2, write directly from the given buffer */
> > + if (rem > cur_part_info.blksz) {
> > + n = rem / cur_part_info.blksz;
> > + ret = blk_dwrite(cur_dev, cur_part_info.start + s, n, buf);
> > + if (ret != n) {
> > + ret = -1;
> > + goto exit;
> > + }
> > + buf += n * cur_part_info.blksz;
> > + rem = rem % cur_part_info.blksz;
> > + s += n;
> > + }
> > +
> > + /* Do part 3, read a block and copy the trailing sectors */
> > + if (rem) {
> > + ret = blk_dread(cur_dev, cur_part_info.start + s, 1, block);
> > + if (ret != 1) {
> > + ret = -1;
> > + goto exit;
> > + } else {
> > + memcpy(block, buf, rem);
> > + }
> > + ret = blk_dwrite(cur_dev, cur_part_info.start + s, 1, block);
> > + if (ret != 1) {
> > + ret = -1;
> > + goto exit;
> > + }
> > + }
> > +exit:
> > + if (block)
> > + free(block);
> > +
> > + return (ret == -1) ? -1 : nr_sect;
> > }
> >
> > int fat_set_blk_dev(struct blk_desc *dev_desc, struct disk_partition *info)
> > @@ -575,6 +765,8 @@ read_bootsectandvi(boot_sector *bs, volume_info
> > *volinfo, int *fatsize)
> > return -1;
> > }
> >
> > + fat_sect_size = 0;
> > +
> > if (disk_read(0, 1, block) < 0) {
> > debug("Error: reading block\n");
> > ret = -1;
> > @@ -645,12 +837,12 @@ static int get_fs_info(fsdata *mydata)
> > mydata->rootdir_sect = mydata->fat_sect + mydata->fatlength * bs.fats;
> >
> > mydata->sect_size = get_unaligned_le16(bs.sector_size);
> > + fat_sect_size = mydata->sect_size;
> > mydata->clust_size = bs.cluster_size;
> > - if (mydata->sect_size != cur_part_info.blksz) {
> > - log_err("FAT sector size mismatch (fs=%u, dev=%lu)\n",
> > - mydata->sect_size, cur_part_info.blksz);
> > - return -1;
> > - }
> > + if (mydata->sect_size != cur_part_info.blksz)
> > + log_info("FAT sector size mismatch (fs=%u, dev=%lu)\n",
> > + mydata->sect_size, cur_part_info.blksz);
> > +
> > if (mydata->clust_size == 0) {
> > log_err("FAT cluster size not set\n");
> > return -1;
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index ea877ee917..f7a210d790 100644
> > --- a/fs/fat/fat_write.c
> > +++ b/fs/fat/fat_write.c
> > @@ -192,25 +192,6 @@ out:
> > }
> >
> > static int total_sector;
> > -static int disk_write(__u32 block, __u32 nr_blocks, void *buf)
> > -{
> > - ulong ret;
> > -
> > - if (!cur_dev)
> > - return -1;
> > -
> > - if (cur_part_info.start + block + nr_blocks >
> > - cur_part_info.start + total_sector) {
> > - printf("error: overflow occurs\n");
> > - return -1;
> > - }
> > -
> > - ret = blk_dwrite(cur_dev, cur_part_info.start + block, nr_blocks, buf);
> > - if (nr_blocks && ret == 0)
> > - return -1;
> > -
> > - return ret;
> > -}
> >
> > /*
> > * Write fat buffer into block device
>
> --
> // Caleb (they/them)
>
