Skip to content

Commit

Permalink
FatPkg: Improvements to Fat to Fix File Corruption
Browse files Browse the repository at this point in the history 
This patch improves dirty bit checking in FAT and coalesces multiple
writes to adjacent blocks into a single write.

For good read performance, the Fat code caches 64KB lines from the
file system. Any update to the data will force 64KB writes. Writing
the full 64KB causes collateral blocks (i.e. blocks belonging to
other files) to be written. This is inefficient to write blocks that
are not dirty, the FAT driver should only be writing blocks that have
actually been updated through its cache.

This also resolves an issue with existing bootloaders which have ways
to update files outside of the FAT code and the collateral write can
corrupt those files. One such file is the Windows BCD that gets updated
outside of the FAT code and has been seen to be corrupted by the
collateral writes, causing the OS to fail to boot.

This fix keeps track of physical disk block (LBA) size and will only
mark LBA size zones of the 64KB cache line that are written to. When the
file is closed and the cached data is written, only the dirty LBAs are
written, which avoids the collateral write and therefore the corruption.
In addition, adjacent LBAs will be coalesced into one write for
performance considerations.

Signed-off-by: Oliver Smith-Denny <[email protected]>
  • Loading branch information
@mikeytdisco @os-d
mikeytdisco authored and os-d committed Sep 24, 2024
1 parent 07f65af commit f8de886
Show file tree
Hide file tree
Showing 4 changed files with 237 additions and 15 deletions.
216 changes: 210 additions & 6 deletions FatPkg/EnhancedFatDxe/DiskCache.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,206 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#include "Fat.h"

/**
Helper function to clear the dirty state of the cache line.
@param[in] CacheTag - CacheTag to clear
**/
STATIC
VOID
ClearCacheTagDirtyState (
IN CACHE_TAG *CacheTag
)
{
if (CacheTag == NULL) {
ASSERT (CacheTag != NULL);
return;
}

ZeroMem (CacheTag->DirtyBlocks, sizeof (CacheTag->DirtyBlocks));
CacheTag->Dirty = FALSE;
}

/**
Helper function to set a bit in a dirty block. This is used to
track which blocks to later write to disk.
@param[in] BitNumber - Which bit to set in DirtyBlocks
@param[in] DirtyBlocks - Array of bits
**/
STATIC
VOID
SetBitInDirtyBlock (
IN UINTN BitNumber,
IN DIRTY_BLOCKS *DirtyBlocks
)
{
UINTN BlockIndex;
UINTN BitIndex;

//
// ASSERTs checking BitNumber are DEBUG build only to verify the assumptions in the
// Fat.h defines (See Fat.h lines to describe DIRTY_BITS)
//
ASSERT (BitNumber < DIRTY_BITS);

BlockIndex = BitNumber / DIRTY_BITS_PER_BLOCK;
BitIndex = BitNumber % DIRTY_BITS_PER_BLOCK;
DirtyBlocks[BlockIndex] |= LShiftU64 (1ull, BitIndex);
}

/**
Helper function to check if a particular bit in a dirty block is marked dirty or not,
so that it can be written to the disk if it is dirty.
@param[in] BitNumber - Which bit to check in DirtyBlocks
@param[in] DirtyBlocks - Array of bits
**/
STATIC
BOOLEAN
IsBitInBlockDirty (
IN UINTN BitNumber,
IN DIRTY_BLOCKS *DirtyBlocks
)
{
UINTN BlockIndex;
UINTN BitIndex;

ASSERT (BitNumber < DIRTY_BITS);

BlockIndex = BitNumber / DIRTY_BITS_PER_BLOCK;
BitIndex = BitNumber % DIRTY_BITS_PER_BLOCK;
return (DirtyBlocks[BlockIndex] & LShiftU64 (1ull, BitIndex)) != 0;
}

/**
Helper function to set a cache tag dirty for a given offset and length. Dirty blocks marked
here will be flushed to disk when the file is closed.
@param[in] DiskCache - DiskCache
@param[in] CacheTag - CacheTag to update
@param[in] Offset - Offset in the cache line to be marked modified
@param[in] Length - Length of the data to be marked modified
**/
STATIC
VOID
SetCacheTagDirty (
IN DISK_CACHE *DiskCache,
IN CACHE_TAG *CacheTag,
IN UINTN Offset,
IN UINTN Length
)
{
UINTN Bit;
UINTN LastBit;

Bit = Offset / DiskCache->BlockSize;
LastBit = (Offset + Length - 1) / DiskCache->BlockSize;

ASSERT (Bit <= LastBit);
ASSERT (LastBit <= DIRTY_BITS);

do {
SetBitInDirtyBlock (Bit, CacheTag->DirtyBlocks);
} while (++Bit <= LastBit);

CacheTag->Dirty = TRUE;
}

/**
Cache version of FatDiskIo for writing only those LBA's with dirty data.
Keep track of LBA blocks within a cache line. Allow reads from the disk to read the
full cache line, and all writes to the cache line will update which Lba is dirty in DIRTY_BITS.
At flush time, when the cache line is written out, only write the blocks that are dirty, coalescing
adjacent writes to a single FatDiskIo write.
@param[in] CacheTag - Cache line to check for dirty bits from
@param[in] DataType - Type of Cache.
@param[in] Volume - FAT file system volume.
@param[in] IoMode - The access mode (disk read/write or cache access).
@param[in] Offset - The starting byte offset to read from.
@param[in] BufferSize - Size of Buffer.
@param[in, out] Buffer - Buffer containing read data.
@param[in] Task point to task instance.
@retval EFI_SUCCESS - The operation is performed successfully.
@retval EFI_VOLUME_CORRUPTED - The access is
@return Others - The status of read/write the disk
**/
STATIC
EFI_STATUS
CacheFatDiskIo (
IN CACHE_TAG *CacheTag,
IN CACHE_DATA_TYPE DataType,
IN FAT_VOLUME *Volume,
IN IO_MODE IoMode,
IN UINT64 Offset,
IN UINTN BufferSize,
IN OUT VOID *Buffer,
IN FAT_TASK *Task
)
{
DISK_CACHE *DiskCache;
UINTN BlockIndexInTag;
VOID *WriteBuffer;
UINTN LastBit;
UINT64 StartPos;
EFI_STATUS Status;
UINTN WriteSize;

Status = EFI_SUCCESS;
if ((IoMode == WriteDisk) && (CacheTag->RealSize != 0)) {
DiskCache = &Volume->DiskCache[DataType];
WriteBuffer = Buffer;
LastBit = (CacheTag->RealSize - 1) / DiskCache->BlockSize;
StartPos = Offset;
BlockIndexInTag = 0;
WriteSize = 0;

do {
if (IsBitInBlockDirty (BlockIndexInTag, CacheTag->DirtyBlocks)) {
do {
WriteSize += DiskCache->BlockSize;
BlockIndexInTag++;
if (BlockIndexInTag > LastBit) {
break;
}
} while (IsBitInBlockDirty (BlockIndexInTag, CacheTag->DirtyBlocks));

Status = FatDiskIo (Volume, IoMode, StartPos, WriteSize, WriteBuffer, Task);
if (EFI_ERROR (Status)) {
return Status;
}

StartPos += WriteSize + DiskCache->BlockSize;
WriteBuffer = (VOID *)((UINTN)WriteBuffer + WriteSize + DiskCache->BlockSize);
WriteSize = 0;
BlockIndexInTag++;
} else {
StartPos += DiskCache->BlockSize;
WriteBuffer = (VOID *)((UINTN)WriteBuffer + DiskCache->BlockSize);
BlockIndexInTag++;
}
} while (BlockIndexInTag <= LastBit);

ASSERT (WriteSize == 0);
} else {
Status = FatDiskIo (Volume, IoMode, Offset, BufferSize, Buffer, Task);
if (EFI_ERROR (Status)) {
return Status;
}
}

return Status;
}

/**
This function is used by the Data Cache.
Expand Down Expand Up @@ -57,8 +257,8 @@ FatFlushDataCacheRange (
CacheTag = &DiskCache->CacheTag[GroupNo];
if ((CacheTag->RealSize > 0) && (CacheTag->PageNo == PageNo)) {
//
// When reading data form disk directly, if some dirty data
// in cache is in this rang, this data in the Buffer need to
// When reading data from disk directly, if some dirty data
// in cache is in this range, this data in the Buffer needs to
// be updated with the cache's dirty data.
//
if (IoMode == ReadDisk) {
Expand Down Expand Up @@ -119,7 +319,7 @@ FatExchangeCachePage (
GroupNo = PageNo & DiskCache->GroupMask;
PageAlignment = DiskCache->PageAlignment;
PageAddress = DiskCache->CacheBase + (GroupNo << PageAlignment);
EntryPos = DiskCache->BaseAddress + LShiftU64 (PageNo, PageAlignment);
EntryPos = (DiskCache->BaseAddress + LShiftU64 (PageNo, PageAlignment));
RealSize = CacheTag->RealSize;
if (IoMode == ReadDisk) {
RealSize = (UINTN)1 << PageAlignment;
Expand All @@ -139,15 +339,15 @@ FatExchangeCachePage (
//
// Only fat table writing will execute more than once
//
Status = FatDiskIo (Volume, IoMode, EntryPos, RealSize, PageAddress, Task);
Status = CacheFatDiskIo (CacheTag, DataType, Volume, IoMode, EntryPos, RealSize, PageAddress, Task);
if (EFI_ERROR (Status)) {
return Status;
}

EntryPos += Volume->FatSize;
} while (--WriteCount > 0);

CacheTag->Dirty = FALSE;
ClearCacheTagDirtyState (CacheTag);
CacheTag->RealSize = RealSize;
return EFI_SUCCESS;
}
Expand Down Expand Up @@ -248,7 +448,7 @@ FatAccessUnalignedCachePage (
Source = DiskCache->CacheBase + (GroupNo << DiskCache->PageAlignment) + Offset;
Destination = Buffer;
if (IoMode != ReadDisk) {
CacheTag->Dirty = TRUE;
SetCacheTagDirty (DiskCache, CacheTag, Offset, Length);
DiskCache->Dirty = TRUE;
Destination = Source;
Source = Buffer;
Expand Down Expand Up @@ -489,5 +689,9 @@ FatInitializeDiskCache (
Volume->CacheBuffer = CacheBuffer;
DiskCache[CacheFat].CacheBase = CacheBuffer;
DiskCache[CacheData].CacheBase = CacheBuffer + FatCacheSize;

DiskCache[CacheFat].BlockSize = Volume->BlockIo->Media->BlockSize;
DiskCache[CacheData].BlockSize = Volume->BlockIo->Media->BlockSize;

return EFI_SUCCESS;
}
21 changes: 18 additions & 3 deletions FatPkg/EnhancedFatDxe/Fat.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,19 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define FAT_FATCACHE_GROUP_MIN_COUNT 1
#define FAT_FATCACHE_GROUP_MAX_COUNT 16

// For cache block bits, use a UINT64
typedef UINT64 DIRTY_BLOCKS;
#define BITS_PER_BYTE 8
#define DIRTY_BITS_PER_BLOCK ((sizeof (DIRTY_BLOCKS) * BITS_PER_BYTE))

// largest cache line (64KB) / MinLbaSize (512) = 128 bits
#define DIRTY_BITS ((1 << FAT_DATACACHE_PAGE_MAX_ALIGNMENT) / (1 << MIN_BLOCK_ALIGNMENT))

// Number of DIRTY_BLOCKS to hold DIRTY_BITS bits.
#define DIRTY_BLOCKS_SIZE (DIRTY_BITS / sizeof (DIRTY_BLOCKS))

STATIC_ASSERT ((((1 << FAT_DATACACHE_PAGE_MAX_ALIGNMENT) / (1 << MIN_BLOCK_ALIGNMENT)) % sizeof (DIRTY_BLOCKS)) == 0, "DIRTY_BLOCKS not a proper size");

//
// Used in 8.3 generation algorithm
//
Expand Down Expand Up @@ -143,15 +156,17 @@ typedef enum {
// Disk cache tag
//
typedef struct {
UINTN PageNo;
UINTN RealSize;
BOOLEAN Dirty;
UINTN PageNo;
UINTN RealSize;
BOOLEAN Dirty;
DIRTY_BLOCKS DirtyBlocks[DIRTY_BLOCKS_SIZE];
} CACHE_TAG;

typedef struct {
UINT64 BaseAddress;
UINT64 LimitAddress;
UINT8 *CacheBase;
UINT32 BlockSize;
BOOLEAN Dirty;
UINT8 PageAlignment;
UINTN GroupMask;
Expand Down
14 changes: 8 additions & 6 deletions FatPkg/EnhancedFatDxe/Init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,24 @@ FatAllocateVolume (
Volume->RootDirEnt.FileString = Volume->RootFileString;
Volume->RootDirEnt.Entry.Attributes = FAT_ATTRIBUTE_DIRECTORY;

//
// Check to see if the underlying block device's BlockSize meets what the FAT spec requires
//
if ((BlockIo == NULL) || (BlockIo->Media == NULL)) {
DEBUG ((DEBUG_ERROR, "%a BlockIo or BlockIo is NULL!\n", __func__));
Status = EFI_INVALID_PARAMETER;
goto Done;
}

if ((BlockIo->Media->BlockSize > (1 << MAX_BLOCK_ALIGNMENT)) ||
(BlockIo->Media->BlockSize < (1 << MIN_BLOCK_ALIGNMENT)))
//
// Check to see if the underlying block device's BlockSize meets what the FAT spec requires
//
if ((BlockIo->Media->BlockSize != 512) &&
(BlockIo->Media->BlockSize != SIZE_1KB) &&
(BlockIo->Media->BlockSize != SIZE_2KB) &&
(BlockIo->Media->BlockSize != SIZE_4KB))
{
Status = EFI_UNSUPPORTED;
DEBUG ((
DEBUG_ERROR,
"%a invalid BlockIo BlockSize %u for FAT filesystem on MediaId %u. Min 512b, max 4kb\n",
"%a invalid BlockIo BlockSize %u for FAT filesystem on MediaId %u. Must be 512B, 1KB, 2KB, or 4KB\n",
__func__,
BlockIo->Media->BlockSize,
BlockIo->Media->MediaId
Expand Down
1 change: 1 addition & 0 deletions FatPkg/FatPkg.ci.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"FFFFFFFFL",
"CDVOL",
"DMDEPKG",
"lba's",
"loongarch",
# MU_CHANGE - Start
"loongson",
Expand Down

0 comments on commit f8de886

Please sign in to comment.