[stlankes]

The device driver is quite simple and supports just one virtqueue. The file system is extended to support FAT. The extension based mainly on Hadris FAT.

I tested the current approach by using rusty_demo, which was build as followed:

HERMIT_LOG_LEVEL_FILTER="hermit::drivers::blk=debug,hermit::fs=debug,smoltcp,info" cargo build --release -Zbuild-std=std,panic_abort --target=aarch64-unknown-hermit -p rusty_demo --features hermit/pci,hermit/virtio-blk,hermit/loader,hermit/pci-ids,fs 

Afterwards, I tested the current approach by following command:

qemu-system-aarch64 -display none -serial stdio \
  -machine virt,gic-version=3 \
  -cpu max,lpa2=off  \
  -smp 1 -m 1024M \
  -global virtio-mmio.force-legacy=off \
  -kernel hermit-loader-aarch64 \
  -device guest-loader,addr=0x48000000,initrd=target/aarch64-unknown-hermit/release/rusty_demo  -drive file=disk.img,format=raw,if=none,id=disk0 \
-device virtio-blk-pci,drive=disk0,disable-legacy=on

[mkroening]

A student of ours has already done this. We need to compare before we merge either of the two.

[stlankes]

I know, I was interested. It isn't necessary to merge the PR. I was more interested in Hadris FAT.

[github-actions]

Benchmark Results

Details

Benchmark	Current: e5d8917	Previous: 2e23902	Performance Ratio
startup_benchmark Build Time	86.56 s	80.34 s	1.08 ❗
startup_benchmark File Size	0.80 MB	0.80 MB	1.00 ❗
Startup Time - 1 core	0.71 s (±0.02 s)	0.75 s (±0.02 s)	0.95 ❗
Startup Time - 2 cores	0.72 s (±0.03 s)	0.74 s (±0.02 s)	0.97
Startup Time - 4 cores	0.73 s (±0.03 s)	0.74 s (±0.02 s)	0.99
multithreaded_benchmark Build Time	90.21 s	82.11 s	1.10 ❗
multithreaded_benchmark File Size	0.94 MB	0.86 MB	1.10 ❗
Multithreaded Pi Efficiency - 2 Threads	90.28 % (±10.45 %)	85.89 % (±6.61 %)	1.05
Multithreaded Pi Efficiency - 4 Threads	44.24 % (±3.64 %)	43.43 % (±2.56 %)	1.02
Multithreaded Pi Efficiency - 8 Threads	25.87 % (±2.04 %)	25.76 % (±1.53 %)	1.00
micro_benchmarks Build Time	85.50 s	80.40 s	1.06 ❗
micro_benchmarks File Size	0.94 MB	0.86 MB	1.10 ❗
Scheduling time - 1 thread	65.34 ticks (±4.38 ticks)	62.65 ticks (±4.06 ticks)	1.04
Scheduling time - 2 threads	36.35 ticks (±3.99 ticks)	34.08 ticks (±4.10 ticks)	1.07
Micro - Time for syscall (getpid)	4.27 ticks (±0.90 ticks)	3.45 ticks (±0.58 ticks)	1.24
Memcpy speed - (built_in) block size 4096	80641.24 MByte/s (±55932.84 MByte/s)	82448.38 MByte/s (±56997.13 MByte/s)	0.98
Memcpy speed - (built_in) block size 1048576	30351.05 MByte/s (±24532.05 MByte/s)	30585.98 MByte/s (±24707.84 MByte/s)	0.99
Memcpy speed - (built_in) block size 16777216	26611.61 MByte/s (±22067.23 MByte/s)	26340.06 MByte/s (±21720.96 MByte/s)	1.01
Memset speed - (built_in) block size 4096	80193.71 MByte/s (±55728.80 MByte/s)	82292.76 MByte/s (±56891.50 MByte/s)	0.97
Memset speed - (built_in) block size 1048576	31098.18 MByte/s (±24960.35 MByte/s)	31323.85 MByte/s (±25145.86 MByte/s)	0.99
Memset speed - (built_in) block size 16777216	27349.72 MByte/s (±22517.44 MByte/s)	27104.68 MByte/s (±22209.94 MByte/s)	1.01
Memcpy speed - (rust) block size 4096	75717.19 MByte/s (±52998.41 MByte/s)	74097.96 MByte/s (±51811.44 MByte/s)	1.02
Memcpy speed - (rust) block size 1048576	30186.68 MByte/s (±24462.88 MByte/s)	30361.60 MByte/s (±24602.37 MByte/s)	0.99
Memcpy speed - (rust) block size 16777216	26394.87 MByte/s (±21997.77 MByte/s)	27625.34 MByte/s (±22806.88 MByte/s)	0.96
Memset speed - (rust) block size 4096	76392.19 MByte/s (±53447.31 MByte/s)	74373.47 MByte/s (±51976.48 MByte/s)	1.03
Memset speed - (rust) block size 1048576	30960.69 MByte/s (±24903.04 MByte/s)	31110.89 MByte/s (±25033.24 MByte/s)	1.00
Memset speed - (rust) block size 16777216	27166.69 MByte/s (±22486.03 MByte/s)	28386.93 MByte/s (±23265.03 MByte/s)	0.96
alloc_benchmarks Build Time	82.27 s	74.76 s	1.10 ❗
alloc_benchmarks File Size	0.87 MB	0.87 MB	1.00 ❗
Allocations - Allocation success	91.35 %	91.31 %	1.00 ❗
Allocations - Deallocation success	100.00 %	100.00 %	1
Allocations - Pre-fail Allocations	61.61 %	61.44 %	1.00 ❗
Allocations - Average Allocation time	9283.23 Ticks (±198.31 Ticks)	5860.58 Ticks (±98.43 Ticks)	1.58 ❗
Allocations - Average Allocation time (no fail)	9678.18 Ticks (±195.87 Ticks)	6554.81 Ticks (±92.86 Ticks)	1.48 ❗
Allocations - Average Deallocation time	1404.43 Ticks (±418.24 Ticks)	1805.01 Ticks (±250.35 Ticks)	0.78
mutex_benchmark Build Time	84.56 s	79.82 s	1.06 ❗
mutex_benchmark File Size	0.95 MB	0.86 MB	1.10 ❗
Mutex Stress Test Average Time per Iteration - 1 Threads	12.62 ns (±0.80 ns)	12.10 ns (±0.41 ns)	1.04
Mutex Stress Test Average Time per Iteration - 2 Threads	89.02 ns (±6.26 ns)	40.26 ns (±1.68 ns)	2.21 ❗

This comment was automatically generated by workflow using github-action-benchmark.

[stlankes]

I revised the PR. The critical part is in vfat.rs starting from line 432.

// `FatVolume` is not `Sync`, because it keeps counters in `Cell`s.
struct SharedVolume(FatVolume<FatStream>);

// SAFETY: the volume is only accessed while `VFAT_LOCK` is held, see `volume`.
unsafe impl Sync for SharedVolume {}

static VFAT: OnceCell<SharedVolume> = OnceCell::new();

/// Serializes file system operations, see [`volume`].
static VFAT_LOCK: async_lock::Mutex<()> = async_lock::Mutex::new(());

...

/// Locks the volume for one complete file system operation.
///
/// `hadris-fat` locks only single accesses, so a lookup and the following
/// write could otherwise race on another core.
async fn volume() -> io::Result<Volume> {
	let fs = &VFAT.get().ok_or(Errno::Nodev)?.0;
	let _guard = VFAT_LOCK.lock().await;

	Ok(Volume { fs, _guard })
}

In principle, I need a sync FatVolume, which Hadis doesn't provide. In my workaround, I define explicitly SharedVolume as sysnc and use volume as interface to access the shared volume. Do you have a better idea?

Otherwise, the PR is ready to be merged.

[jounathaen]

read_run: Vec::with_capacity(RUN_SECTORS * SECTOR_SIZE),

?

[stlankes]

I agree

[jounathaen]

Wouldn't it be more performant to first fill_run the buffer and then resize/pad with zeroes?

[stlankes]

I also agree.

[jounathaen]

Maybe I'm just overlooking something. But isn't read_run a write-only variable in the current code?

[stlankes]

You are right. I fixed it with my latest commit

[jounathaen]

Why doesn't the Mutex hold the OnceCell directly? Then we also don't need the unsafe impl for SharedVolume

[stlankes]

I tried that, but it doesn't work with the reader cache. VfatFileHandle keeps the FileReader of the last read, so that sequential reads continue at the current cluster. FileReader borrows the volume, and the reader has to outlive the lock. A Mutex<Option<FatVolume>> only hands out references that live as long as the guard. Without unsafe, I would have to drop the reader cache, which slows down sequential reads considerably.

The root cause is in hadris-fat: FatVolume is not Sync only because Fat32FsExt stores free_count and next_free in Cells.

If I see it correctly, hadris-fat solve this in the next release.

[jounathaen]

I think this looks quite good in principle. I've just made a few small remarks. They are not blocking, so feel free to ignore them if merging is urgent.

[jounathaen]

Maybe create a follow-up issues for the Mutex+Hadris-FAT as well as the HashMap in the cache.

[jounathaen]

Could you maybe also squash the commits?