Closes #290

kernel-rs/src/arena/mod.rs

@@ -48,17 +48,23 @@ pub trait Arena: Sized + Sync {
     ///
     /// This method is automatically used by the `Rc`.
     /// Usually, you don't need to manually call this method.
-    fn dealloc<'id>(self: ArenaRef<'id, &Self>, handle: Handle<'id, Self::Data>) {
+    fn dealloc<'id, 'a, 'b>(
+        self: ArenaRef<'id, &Self>,
+        handle: Handle<'id, Self::Data>,
+        ctx: <Self::Data as ArenaObject>::Ctx<'a, 'b>,
+    ) {
         if let Ok(mut rm) = handle.0.into_inner().into_mut() {
-            rm.finalize::<Self>();
+            rm.finalize::<Self>(ctx);
         }
     }
 }
 
 pub trait ArenaObject {
+    type Ctx<'a, 'b: 'a>;
+
     /// Finalizes the `ArenaObject`.
     /// This function is automatically called when the last `Rc` referring to this `ArenaObject` gets dropped.
-    fn finalize<A: Arena>(&mut self);
+    fn finalize<'a, 'b: 'a, A: Arena>(&mut self, ctx: Self::Ctx<'a, 'b>);
 }
 
 /// A branded reference to an arena.
@@ -152,12 +158,19 @@ impl<A: Arena> Clone for ArenaRc<A> {
     }
 }
 
-impl<A: Arena> Drop for ArenaRc<A> {
-    fn drop(&mut self) {
+impl<A: Arena> ArenaRc<A> {
+    pub fn free(mut self, ctx: <A::Data as ArenaObject>::Ctx<'_, '_>) {
         let inner = unsafe { ManuallyDrop::take(&mut self.inner) };
         self.map_arena(|arena| {
             let inner = Handle(arena.0.brand(inner));
-            arena.dealloc(inner);
+            arena.dealloc(inner, ctx);
         });
+        core::mem::forget(self);
+    }
+}
+
+impl<A: Arena> Drop for ArenaRc<A> {
+    fn drop(&mut self) {
+        panic!();
     }
 }

kernel-rs/src/arena/mru_arena.rs

@@ -170,10 +170,14 @@ impl<T: 'static + ArenaObject + Unpin + Send, const CAPACITY: usize> Arena
         )
     }
 
-    fn dealloc<'id>(self: ArenaRef<'id, &Self>, handle: Handle<'id, Self::Data>) {
+    fn dealloc<'id, 'a, 'b>(
+        self: ArenaRef<'id, &Self>,
+        handle: Handle<'id, Self::Data>,
+        ctx: <Self::Data as ArenaObject>::Ctx<'a, 'b>,
+    ) {
         if let Ok(mut rm) = handle.0.into_inner().into_mut() {
             // Finalize the arena object.
-            rm.finalize::<Self>();
+            rm.finalize::<Self>(ctx);
 
             // Move this entry to the back of the list.
             let this = self.pinned_lock_unchecked();

kernel-rs/src/bio.rs

@@ -14,8 +14,9 @@
 use core::mem::{self, ManuallyDrop};
 use core::ops::{Deref, DerefMut};
 
+use crate::arena::ArenaRc;
 use crate::{
-    arena::{Arena, ArenaObject, ArenaRc, MruArena},
+    arena::{Arena, ArenaObject, MruArena},
     lock::{Sleeplock, Spinlock},
     param::{BSIZE, NBUF},
     proc::{KernelCtx, WaitChannel},
@@ -49,7 +50,10 @@ impl const Default for BufEntry {
 }
 
 impl ArenaObject for BufEntry {
-    fn finalize<A: Arena>(&mut self) {
+    type Ctx<'a, 'id: 'a> = ();
+
+    #[allow(clippy::needless_lifetimes)]
+    fn finalize<'a, 'id: 'a, A: Arena>(&mut self, _: ()) {
         // The buffer contents should have been written. Does nothing.
     }
 }
@@ -98,7 +102,7 @@ impl BufInner {
 pub type Bcache = Spinlock<MruArena<BufEntry, NBUF>>;
 
 /// A reference counted smart pointer to a `BufEntry`.
-pub type BufUnlocked = ArenaRc<Bcache>;
+pub struct BufUnlocked(ManuallyDrop<ArenaRc<Bcache>>);
 
 /// A locked `BufEntry`.
 ///
@@ -109,6 +113,30 @@ pub struct Buf {
     inner: ManuallyDrop<BufUnlocked>,
 }
 
+impl BufUnlocked {
+    pub fn lock(self, ctx: &KernelCtx<'_, '_>) -> Buf {
+        mem::forget(self.inner.lock(ctx));
+        Buf {
+            inner: ManuallyDrop::new(self),
+        }
+    }
+}
+
+impl Deref for BufUnlocked {
+    type Target = BufEntry;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl Drop for BufUnlocked {
+    fn drop(&mut self) {
+        // SAFETY: `self` is being dropped.
+        unsafe { ManuallyDrop::take(&mut self.0) }.free(());
+    }
+}
+
 impl Buf {
     pub fn deref_inner(&self) -> &BufInner {
         let entry: &BufEntry = &self.inner;
@@ -162,23 +190,16 @@ impl Bcache {
 
     /// Return a unlocked buf with the contents of the indicated block.
     pub fn get_buf(&self, dev: u32, blockno: u32) -> BufUnlocked {
-        self.find_or_alloc(
-            |buf| buf.dev == dev && buf.blockno == blockno,
-            |buf| {
-                buf.dev = dev;
-                buf.blockno = blockno;
-                buf.inner.get_mut().valid = false;
-            },
-        )
-        .expect("[BufGuard::new] no buffers")
-    }
-}
-
-impl BufUnlocked {
-    pub fn lock(self, ctx: &KernelCtx<'_, '_>) -> Buf {
-        mem::forget(self.inner.lock(ctx));
-        Buf {
-            inner: ManuallyDrop::new(self),
-        }
+        BufUnlocked(ManuallyDrop::new(
+            self.find_or_alloc(
+                |buf| buf.dev == dev && buf.blockno == blockno,
+                |buf| {
+                    buf.dev = dev;
+                    buf.blockno = blockno;
+                    buf.inner.get_mut().valid = false;
+                },
+            )
+            .expect("[BufGuard::new] no buffers"),
+        ))
     }
 }

kernel-rs/src/exec.rs

@@ -103,11 +103,10 @@ impl KernelCtx<'_, '_> {
 
         let allocator = allocator();
 
-        // TODO(https://github.com/kaist-cp/rv6/issues/290):
-        // Dropping an RcInode requires a transaction.
         let tx = self.kernel().fs().begin_tx(self);
         let tx = scopeguard::guard(tx, |t| t.end(self));
         let ptr = self.kernel().fs().namei(path, &tx, self)?;
+        let ptr = scopeguard::guard(ptr, |ptr| ptr.free((&tx, self)));
         let ip = ptr.lock(self);
         let mut ip = scopeguard::guard(ip, |ip| ip.free(self));
 

kernel-rs/src/file.rs

@@ -16,7 +16,7 @@ use crate::{
     lock::Spinlock,
     param::{BSIZE, MAXOPBLOCKS, NFILE},
     pipe::AllocatedPipe,
-    proc::{kernel_ctx, KernelCtx},
+    proc::KernelCtx,
 };
 
 pub enum FileType {
@@ -244,32 +244,26 @@ impl const Default for File {
 }
 
 impl ArenaObject for File {
-    fn finalize<A: Arena>(&mut self) {
-        let finalize_inner = |ctx: KernelCtx<'_, '_>| {
-            let typ = mem::replace(&mut self.typ, FileType::None);
-            match typ {
-                FileType::Pipe { pipe } => {
-                    if let Some(page) = pipe.close(self.writable, ctx.kernel()) {
-                        allocator().free(page);
-                    }
-                }
-                FileType::Inode {
-                    inner: InodeFileType { ip, .. },
-                }
-                | FileType::Device { ip, .. } => {
-                    // TODO(https://github.com/kaist-cp/rv6/issues/290):
-                    // Dropping an RcInode requires a transaction.
-                    let tx = ctx.kernel().fs().begin_tx(&ctx);
-                    drop(ip);
-                    tx.end(&ctx);
+    type Ctx<'a, 'id: 'a> = &'a KernelCtx<'id, 'a>;
+
+    fn finalize<'a, 'id: 'a, A: Arena>(&mut self, ctx: Self::Ctx<'a, 'id>) {
+        let typ = mem::replace(&mut self.typ, FileType::None);
+        match typ {
+            FileType::Pipe { pipe } => {
+                if let Some(page) = pipe.close(self.writable, ctx) {
+                    allocator().free(page);
                 }
-                _ => (),
             }
-        };
-
-        // SAFETY: `FileTable` does not use `Arena::find_or_alloc`.
-        // TODO(https://github.com/kaist-cp/rv6/issues/290): remove kernel_ctx()
-        unsafe { kernel_ctx(finalize_inner) };
+            FileType::Inode {
+                inner: InodeFileType { ip, .. },
+            }
+            | FileType::Device { ip, .. } => {
+                let tx = ctx.kernel().fs().begin_tx(ctx);
+                ip.free((&tx, ctx));
+                tx.end(ctx);
+            }
+            _ => (),
+        }
     }
 }
 
@@ -287,14 +281,15 @@ impl FileTable {
 impl RcFile {
     /// Allocate a file descriptor for the given file.
     /// Takes over file reference from caller on success.
-    pub fn fdalloc(self, ctx: &mut KernelCtx<'_, '_>) -> Result<i32, Self> {
+    pub fn fdalloc(self, ctx: &mut KernelCtx<'_, '_>) -> Result<i32, ()> {
         let proc_data = ctx.proc_mut().deref_mut_data();
         for (fd, f) in proc_data.open_files.iter_mut().enumerate() {
             if f.is_none() {
                 *f = Some(self);
                 return Ok(fd as i32);
             }
         }
-        Err(self)
+        self.free(ctx);
+        Err(())
     }
 }

kernel-rs/src/fs/lfs.rs

@@ -10,7 +10,10 @@ use crate::{
 pub struct InodeInner {}
 
 impl ArenaObject for Inode<InodeInner> {
-    fn finalize<A: Arena>(&mut self) {}
+    type Ctx<'a, 'id: 'a> = ();
+
+    #[allow(clippy::needless_lifetimes)]
+    fn finalize<'a, 'id: 'a, A: Arena>(&mut self, _: ()) {}
 }
 
 pub struct Lfs {}

kernel-rs/src/fs/ufs/inode.rs

@@ -87,7 +87,7 @@ use crate::{
     lock::{Sleeplock, Spinlock},
     param::ROOTDEV,
     param::{BSIZE, NINODE},
-    proc::{kernel_ctx, KernelCtx},
+    proc::KernelCtx,
 };
 
 /// Directory is a file containing a sequence of Dirent structures.
@@ -225,7 +225,8 @@ impl InodeGuard<'_, InodeInner> {
         ctx: &KernelCtx<'_, '_>,
     ) -> Result<(), ()> {
         // Check that name is not present.
-        if let Ok((_ip, _)) = self.dirlookup(name, ctx) {
+        if let Ok((ip, _)) = self.dirlookup(name, ctx) {
+            ip.free((tx, ctx));
             return Err(());
         };
 
@@ -655,51 +656,30 @@ impl const Default for Inode<InodeInner> {
 }
 
 impl ArenaObject for Inode<InodeInner> {
+    type Ctx<'a, 'id: 'a> = (&'a UfsTx<'a>, &'a KernelCtx<'id, 'a>);
+
     /// Drop a reference to an in-memory inode.
     /// If that was the last reference, the inode table entry can
     /// be recycled.
     /// If that was the last reference and the inode has no links
     /// to it, free the inode (and its content) on disk.
     /// All calls to Inode::put() must be inside a transaction in
     /// case it has to free the inode.
-    #[allow(clippy::cast_ref_to_mut)]
-    fn finalize<A: Arena>(&mut self) {
+    fn finalize<'a, 'id: 'a, A: Arena>(&mut self, ctx: Self::Ctx<'a, 'id>) {
+        let (tx, ctx) = ctx;
         if self.inner.get_mut().valid && self.inner.get_mut().nlink == 0 {
             // inode has no links and no other references: truncate and free.
 
-            let finalize_inner = |ctx: KernelCtx<'_, '_>| {
-                let kernel = ctx.kernel();
-
-                // TODO(https://github.com/kaist-cp/rv6/issues/290)
-                // Disk write operations must happen inside a transaction. However,
-                // we cannot begin a new transaction here because beginning of a
-                // transaction acquires a sleep lock while the spin lock of this
-                // arena has been acquired before the invocation of this method.
-                // To mitigate this problem, we make a fake transaction and pass
-                // it as an argument for each disk write operation below. As a
-                // transaction does not start here, any operation that can drop an
-                // inode must begin a transaction even in the case that the
-                // resulting Tx value is never used. Such transactions
-                // can be found in finalize in file.rs, sys_chdir in sysfile.rs,
-                // close_files in proc.rs, and exec in exec.rs.
-                let tx = mem::ManuallyDrop::new(UfsTx { fs: &kernel.fs() });
-
-                // self->ref == 1 means no other process can have self locked,
-                // so this acquiresleep() won't block (or deadlock).
-                let mut ip = self.lock(&ctx);
-
-                ip.itrunc(&tx, &ctx);
-                ip.deref_inner_mut().typ = InodeType::None;
-                ip.update(&tx, &ctx);
-                ip.deref_inner_mut().valid = false;
-
-                ip.free(&ctx);
-            };
+            // self->ref == 1 means no other process can have self locked,
+            // so this acquiresleep() won't block (or deadlock).
+            let mut ip = self.lock(ctx);
 
-            // TODO(https://github.com/kaist-cp/rv6/issues/290): remove kernel_ctx()
-            unsafe {
-                kernel_ctx(finalize_inner);
-            }
+            ip.itrunc(tx, ctx);
+            ip.deref_inner_mut().typ = InodeType::None;
+            ip.update(tx, ctx);
+            ip.deref_inner_mut().valid = false;
+
+            ip.free(ctx);
         }
     }
 }
@@ -888,9 +868,7 @@ impl Itable<InodeInner> {
         &self,
         mut path: &'s Path,
         parent: bool,
-        // TODO(https://github.com/kaist-cp/rv6/issues/290):
-        // Dropping an RcInode requires a transaction.
-        _tx: &UfsTx<'_>,
+        tx: &UfsTx<'_>,
         ctx: &KernelCtx<'_, '_>,
     ) -> Result<(RcInode<InodeInner>, Option<&'s FileName<{ DIRSIZ }>>), ()> {
         let mut ptr = if path.is_absolute() {
@@ -902,21 +880,24 @@ impl Itable<InodeInner> {
         while let Some((new_path, name)) = path.skipelem() {
             path = new_path;
 
-            let ip = ptr.lock(ctx);
-            let mut ip = scopeguard::guard(ip, |ip| ip.free(ctx));
+            let mut ip = ptr.lock(ctx);
             if ip.deref_inner().typ != InodeType::Dir {
+                ip.free(ctx);
+                ptr.free((tx, ctx));
                 return Err(());
             }
             if parent && path.is_empty_string() {
                 // Stop one level early.
-                drop(ip);
+                ip.free(ctx);
                 return Ok((ptr, Some(name)));
             }
             let next = ip.dirlookup(name, ctx);
-            drop(ip);
+            ip.free(ctx);
+            ptr.free((tx, ctx));
             ptr = next?.0
         }
         if parent {
+            ptr.free((tx, ctx));
             return Err(());
         }
         Ok((ptr, None))