cubelinux: hold the store lock across the whole op, not inside append
Initialising STORE_FILE stopped the oops and exposed what it had been hiding: twelve concurrent puts all reported `ok` and one write survived. The lock guarded the file handle, not the store — it is taken, the handle is fetched or cached, and released before the caller does anything with it. Sharing a struct file * is safe, so that is all the handle needs, and it is not enough for the store. Widening it inside append is also not enough, and this commit exists because the first attempt did exactly that and changed nothing. append receives the layout as an argument, and the caller read it from device_and_layout() *before* calling: twelve writers each read log_used = 0, then queued on a lock inside append, then each appended at the same offset against the layout they had already read. Measured with the lock in append: one 68-byte entry, twelve `ok`s. The lock has to be held from before the read. So STORE_OP is taken by the four write ops — put, del, sync and the device write — and everything under them runs with it held: ensure_boot_record, device_and_layout, append, fold_now. None of those may take it again; a kernel mutex is not reentrant, and append folds and then calls itself, while fold_now is reachable both from inside append and on its own. That is why the lock sits at the ops rather than in the writers. verify-file-store.sh MODE=race, twelve concurrent puts: 816 bytes of log = 12 x 68, and control generation 13 = 1 + 12, where the previous kernel produced 68 and 2. Regression: MODE=seq still exact; verify-boot-record passes (it is the path this changes most, since ensure_boot_record now runs under the lock); verify-kernel-append passes, four acknowledged writes surviving a SIGKILL with no shutdown.
This commit is contained in:
@@ -348,6 +348,29 @@ kernel::sync::global_lock! {
|
||||
unsafe(uninit) static STORE_FILE: Mutex<Option<StoreFile>> = None;
|
||||
}
|
||||
|
||||
kernel::sync::global_lock! {
|
||||
/// Serialises the store's read-modify-write operations.
|
||||
///
|
||||
/// [`STORE_FILE`]'s mutex guards the file *handle*, not the store: it is taken, the handle is
|
||||
/// fetched or cached, and it is released before the caller does anything with it. Sharing a
|
||||
/// `struct file *` is safe, so that is all the handle needs — and it is not enough for the
|
||||
/// store. An append is a read-modify-write of the control block: read `log_used`, write the
|
||||
/// entry at that offset, then write a control block counting it. Writers that both read
|
||||
/// `log_used = 0` write their entries to the same offset and commit the same count, so one
|
||||
/// entry survives, the rest are discarded, and every caller is told `ok`.
|
||||
///
|
||||
/// Measured with this lock absent: `verify-file-store.sh MODE=race`, twelve concurrent puts,
|
||||
/// 12 × 68 bytes of log expected — a single 68-byte entry landed and all twelve reported
|
||||
/// success. The same shape is on the box, where the store has several writers.
|
||||
///
|
||||
/// It is taken by the *entry points* only — [`append`] and [`fold_now`] — and their inner
|
||||
/// bodies assume it is already held. `append` folds and then calls itself, and `fold_now` is
|
||||
/// reachable both from inside `append` and on its own, so a lock taken at the top of either
|
||||
/// body would re-enter itself and deadlock: a kernel mutex is not reentrant.
|
||||
/// SAFETY: Initialized (to None) before first use.
|
||||
unsafe(uninit) static STORE_OP: Mutex<()> = ();
|
||||
}
|
||||
|
||||
/// The store device's file, opened on first use and cached for the boot. O_RDWR, because the one
|
||||
/// handle serves both the reads and the log-append / checkpoint writes.
|
||||
fn store_file() -> Result<*mut bindings::file> {
|
||||
@@ -1257,10 +1280,17 @@ fn older_copy(generation: u64) -> usize {
|
||||
/// survives a power cut; one it has not may or may not, which is exactly the boundary an
|
||||
/// acknowledgement is supposed to mark.
|
||||
///
|
||||
/// Append a mutation to the log. **Requires [`STORE_OP`] to be held by the caller.**
|
||||
///
|
||||
/// On a store device the entry goes after the log bytes already in use, and the control
|
||||
/// block's count is raised afterwards — entry first, count second, so a crash between them
|
||||
/// loses an *unacknowledged* mutation rather than counting one that is not there. A count
|
||||
/// that ran ahead would make replay read past valid data; a count that lags only forgets.
|
||||
///
|
||||
/// That read-then-write pair is what must not interleave — and the lock has to be taken by the
|
||||
/// caller, *before* it reads the layout this writes against. Taking it here is too late: writers
|
||||
/// that each read `log_used = 0` before queueing on a lock inside this function still append at
|
||||
/// the same offset. Measured with the lock here: one 68-byte entry and twelve `ok`s.
|
||||
fn append(
|
||||
image: &[u8],
|
||||
layout: &Layout,
|
||||
@@ -1313,6 +1343,7 @@ fn append(
|
||||
// to, so it keeps writing the log it has.
|
||||
Some(WAL_VERSION) if used > 0 => {
|
||||
if layout.control.is_some() {
|
||||
// [`STORE_OP`] is held by the op that reached here, so these are plain calls.
|
||||
fold_now(image, layout)?;
|
||||
let (device, layout) = device_and_layout()?;
|
||||
return append(&device, &layout, m, op);
|
||||
@@ -1529,6 +1560,8 @@ fn checkpoint(ctl: &Control, merged: &mut Merged) -> Result<u64, Error> {
|
||||
/// Fold the current log into the image — the shared body of `sync` and the append-time
|
||||
/// migration. After it, the image is the pinned v4 shape and the log is empty, so the next
|
||||
/// append starts a fresh v2 log.
|
||||
///
|
||||
/// **Requires [`STORE_OP`] to be held by the caller.**
|
||||
fn fold_now(image: &[u8], layout: &Layout) -> Result<(), Error> {
|
||||
let ctl = match layout.control {
|
||||
Some(c) => c,
|
||||
@@ -1726,6 +1759,9 @@ pub unsafe extern "C" fn cubelinux_kernel_put(
|
||||
len: usize,
|
||||
flags: u16,
|
||||
) -> i32 {
|
||||
// The lock spans the whole op — reading the layout and committing the count are one
|
||||
// read-modify-write, and `ensure_boot_record` below writes too.
|
||||
let _op = STORE_OP.lock();
|
||||
ensure_boot_record();
|
||||
let (sp, key) = unsafe { coord_key(space, x, y, z) };
|
||||
let mut bytes = KVVec::<u8>::new();
|
||||
@@ -4122,6 +4158,7 @@ pub unsafe extern "C" fn cubelinux_kernel_spaces(cursor: u64, space_out: *mut u8
|
||||
/// `space` must point to 32 readable bytes.
|
||||
#[unsafe(no_mangle)]
|
||||
pub unsafe extern "C" fn cubelinux_kernel_del(space: *const u8, x: u64, y: u64, z: u64) -> i32 {
|
||||
let _op = STORE_OP.lock();
|
||||
ensure_boot_record();
|
||||
let (sp, key) = unsafe { coord_key(space, x, y, z) };
|
||||
let mutation = Mutation {
|
||||
@@ -4145,6 +4182,7 @@ pub unsafe extern "C" fn cubelinux_kernel_del(space: *const u8, x: u64, y: u64,
|
||||
/// `CUBE_OP_SYNC`: fold the log into the image.
|
||||
#[unsafe(no_mangle)]
|
||||
pub extern "C" fn cubelinux_kernel_sync() -> i32 {
|
||||
let _op = STORE_OP.lock();
|
||||
ensure_boot_record();
|
||||
let (device, layout) = match device_and_layout() {
|
||||
Ok(pair) => pair,
|
||||
@@ -4185,6 +4223,8 @@ impl kernel::InPlaceModule for CubeStoreModule {
|
||||
// a journal that simply stops.
|
||||
// SAFETY: called exactly once, in the module initializer, before anything can take it.
|
||||
unsafe { STORE_FILE.init() };
|
||||
// SAFETY: called exactly once, in the module initializer, before anything can take it.
|
||||
unsafe { STORE_OP.init() };
|
||||
try_pin_init!(Self {
|
||||
_miscdev <- MiscDeviceRegistration::register(MiscDeviceOptions {
|
||||
name: c_str!("cubelinux"),
|
||||
@@ -4234,6 +4274,8 @@ impl MiscDevice for CubeStore {
|
||||
}
|
||||
// A write to the store device is the kernel taking the write path, so this is one of the
|
||||
// places a boot gets recorded — the same hook the syscall's write operations call.
|
||||
// The lock spans the op: everything below reads the layout and commits against it.
|
||||
let _op = STORE_OP.lock();
|
||||
ensure_boot_record();
|
||||
let me = kiocb.file();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user