Merge #554

554: Closes #539 r=Medowhill a=Medowhill

Closes #539

`RawSleeplock`이 `acquire`와 `release`할 때 `KernelCtx`를 인자로 받습니다. 이제 `RawSleeplock`이 `RawLock`을 구현하지 않습니다. 이렇게 해도 코드 중복이 많아 보이지는 않습니다.

Co-authored-by: Jaemin Hong <[email protected]>

kernel-rs/src/bio.rs

@@ -18,7 +18,7 @@ use crate::{
     arena::{Arena, ArenaObject, ArenaRc, MruArena},
     lock::{Sleeplock, Spinlock},
     param::{BSIZE, NBUF},
-    proc::WaitChannel,
+    proc::{KernelCtx, WaitChannel},
 };
 
 pub struct BufEntry {
@@ -122,14 +122,18 @@ impl Buf {
         unsafe { &mut *entry.inner.get_mut_raw() }
     }
 
-    pub fn unlock(mut self) -> BufUnlocked {
+    pub fn unlock(mut self, ctx: &KernelCtx<'_, '_>) -> BufUnlocked {
         // SAFETY: this method consumes self and self.inner will not be used again.
         let inner = unsafe { ManuallyDrop::take(&mut self.inner) };
         // SAFETY: this method consumes self.
-        unsafe { inner.inner.unlock() };
+        unsafe { inner.inner.unlock(ctx) };
         mem::forget(self);
         inner
     }
+
+    pub fn free(self, ctx: &KernelCtx<'_, '_>) {
+        let _ = self.unlock(ctx);
+    }
 }
 
 impl Deref for Buf {
@@ -142,9 +146,9 @@ impl Deref for Buf {
 
 impl Drop for Buf {
     fn drop(&mut self) {
-        // SAFETY: self will be dropped and self.inner will not be
-        // used again.
-        unsafe { ManuallyDrop::take(&mut self.inner).inner.unlock() };
+        // HACK(@efenniht): we really need linear type here:
+        // https://github.com/rust-lang/rfcs/issues/814
+        panic!("Buf must never drop.");
     }
 }
 
@@ -171,8 +175,8 @@ impl Bcache {
 }
 
 impl BufUnlocked {
-    pub fn lock(self) -> Buf {
-        mem::forget(self.inner.lock());
+    pub fn lock(self, ctx: &KernelCtx<'_, '_>) -> Buf {
+        mem::forget(self.inner.lock(ctx));
         Buf {
             inner: ManuallyDrop::new(self),
         }

kernel-rs/src/exec.rs

@@ -106,9 +106,10 @@ impl KernelCtx<'_, '_> {
         // 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 tx = scopeguard::guard(tx, |t| t.end(self));
         let ptr = self.kernel().fs().namei(path, &tx, self)?;
-        let mut ip = ptr.lock(self);
+        let ip = ptr.lock(self);
+        let mut ip = scopeguard::guard(ip, |ip| ip.free(self));
 
         // Check ELF header
         let mut elf: ElfHdr = Default::default();
@@ -137,7 +138,7 @@ impl KernelCtx<'_, '_> {
         }
         drop(ip);
         drop(ptr);
-        scopeguard::ScopeGuard::into_inner(tx).end(self);
+        drop(tx);
 
         // Allocate two pages at the next page boundary.
         // Use the second as the user stack.

kernel-rs/src/file.rs

@@ -1,6 +1,12 @@
 //! Support functions for system calls that involve file descriptors.
 
-use core::{cell::UnsafeCell, cmp, mem, ops::Deref, ops::DerefMut};
+use core::{
+    cell::UnsafeCell,
+    cmp,
+    mem::{self, ManuallyDrop},
+    ops::Deref,
+    ops::DerefMut,
+};
 
 use crate::{
     arch::addr::UVAddr,
@@ -42,7 +48,7 @@ pub struct InodeFileType {
 /// inode. `off` is a mutable reference to the offset. Accessing `off` is guaranteed to be safe
 /// since the inode is locked.
 struct InodeFileTypeGuard<'a, I> {
-    ip: InodeGuard<'a, I>,
+    ip: ManuallyDrop<InodeGuard<'a, I>>,
     off: &'a mut u32,
 }
 
@@ -78,7 +84,18 @@ impl InodeFileType {
         let ip = self.ip.lock(ctx);
         // SAFETY: `ip` is locked and `off` can be exclusively accessed.
         let off = unsafe { &mut *self.off.get() };
-        InodeFileTypeGuard { ip, off }
+        InodeFileTypeGuard {
+            ip: ManuallyDrop::new(ip),
+            off,
+        }
+    }
+}
+
+impl<I> InodeFileTypeGuard<'_, I> {
+    fn free(mut self, ctx: &KernelCtx<'_, '_>) {
+        let ip = unsafe { ManuallyDrop::take(&mut self.ip) };
+        ip.free(ctx);
+        core::mem::forget(self);
     }
 }
 
@@ -96,6 +113,14 @@ impl<'a, I> DerefMut for InodeFileTypeGuard<'a, I> {
     }
 }
 
+impl<I> Drop for InodeFileTypeGuard<'_, I> {
+    fn drop(&mut self) {
+        // HACK(@efenniht): we really need linear type here:
+        // https://github.com/rust-lang/rfcs/issues/814
+        panic!("InodeFileTypeGuard must never drop.");
+    }
+}
+
 impl File {
     pub const fn new(typ: FileType, readable: bool, writable: bool) -> Self {
         Self {
@@ -117,7 +142,7 @@ impl File {
                 inner: InodeFileType { ip, .. },
             }
             | FileType::Device { ip, .. } => {
-                let st = ip.stat();
+                let st = ip.stat(ctx);
                 ctx.proc_mut().memory_mut().copy_out(addr, &st)
             }
             _ => Err(()),
@@ -140,6 +165,7 @@ impl File {
                 if let Ok(v) = ret {
                     *ip.off += v as u32;
                 }
+                ip.free(ctx);
                 ret
             }
             FileType::Device { major, .. } => {
@@ -184,9 +210,12 @@ impl File {
                         ctx,
                         &tx,
                     );
+                    if let Ok(r) = r {
+                        *ip.off += r as u32;
+                    }
                     tx.end(ctx);
+                    ip.free(ctx);
                     let r = r?;
-                    *ip.off += r as u32;
                     if r != bytes_to_write {
                         // error from write_user
                         break;

kernel-rs/src/fs/mod.rs

@@ -72,13 +72,20 @@ impl<I> InodeGuard<'_, I> {
         // SAFETY: self.inner is locked and &mut self is exclusive.
         unsafe { &mut *self.inner.get_mut_raw() }
     }
+
+    pub fn free(self, ctx: &KernelCtx<'_, '_>) {
+        // SAFETY: self will be dropped.
+        unsafe { self.inner.unlock(ctx) };
+        core::mem::forget(self);
+    }
 }
 
 /// Unlock and put the given inode.
 impl<I> Drop for InodeGuard<'_, I> {
     fn drop(&mut self) {
-        // SAFETY: self will be dropped.
-        unsafe { self.inner.unlock() };
+        // HACK(@efenniht): we really need linear type here:
+        // https://github.com/rust-lang/rfcs/issues/814
+        panic!("InodeGuard must never drop.");
     }
 }
 

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

@@ -311,7 +311,7 @@ impl InodeGuard<'_, InodeInner> {
         (*dip).size = inner.size;
         (*dip).addr_direct.copy_from_slice(&inner.addr_direct);
         (*dip).addr_indirect = inner.addr_indirect;
-        tx.write(bp);
+        tx.write(bp, ctx);
     }
 
     /// Truncate inode (discard contents).
@@ -335,7 +335,7 @@ impl InodeGuard<'_, InodeInner> {
                     tx.bfree(dev, *a, ctx);
                 }
             }
-            drop(bp);
+            bp.free(ctx);
             tx.bfree(dev, self.deref_inner().addr_indirect, ctx);
             self.deref_inner_mut().addr_indirect = 0
         }
@@ -371,9 +371,9 @@ impl InodeGuard<'_, InodeInner> {
         self.read_internal(
             off,
             dst.len() as u32,
-            |off, src, ctx| {
+            |off, src, _| {
                 dst[off as usize..off as usize + src.len()].clone_from_slice(src);
-                Ok(ctx)
+                Ok(())
             },
             ctx,
         )
@@ -397,8 +397,7 @@ impl InodeGuard<'_, InodeInner> {
             |off, src, ctx| {
                 ctx.proc_mut()
                     .memory_mut()
-                    .copy_out_bytes(dst + off as usize, src)?;
-                Ok(ctx)
+                    .copy_out_bytes(dst + off as usize, src)
             },
             ctx,
         )
@@ -415,11 +414,12 @@ impl InodeGuard<'_, InodeInner> {
     // However, writing to user memory needs page table accesses since a single
     // consecutive region in user memory may split into several pages in
     // physical memory.
+    #[inline]
     fn read_internal<
         'id,
         's,
         K: Deref<Target = KernelCtx<'id, 's>>,
-        F: FnMut(u32, &[u8], K) -> Result<K, ()>,
+        F: FnMut(u32, &[u8], &mut K) -> Result<(), ()>,
     >(
         &mut self,
         mut off: u32,
@@ -442,7 +442,9 @@ impl InodeGuard<'_, InodeInner> {
             let m = core::cmp::min(n - tot, BSIZE as u32 - off % BSIZE as u32);
             let begin = (off % BSIZE as u32) as usize;
             let end = begin + m as usize;
-            k = f(tot, &bp.deref_inner().data[begin..end], k)?;
+            let res = f(tot, &bp.deref_inner().data[begin..end], &mut k);
+            bp.free(&k);
+            res?;
             tot += m;
             off += m;
         }
@@ -478,9 +480,9 @@ impl InodeGuard<'_, InodeInner> {
         self.write_internal(
             off,
             src.len() as u32,
-            |off, dst, ctx| {
+            |off, dst, _| {
                 dst.clone_from_slice(&src[off as usize..off as usize + src.len()]);
-                Ok(ctx)
+                Ok(())
             },
             tx,
             ctx,
@@ -502,14 +504,9 @@ impl InodeGuard<'_, InodeInner> {
             off,
             n,
             |off, dst, ctx| {
-                match ctx
-                    .proc_mut()
+                ctx.proc_mut()
                     .memory_mut()
                     .copy_in_bytes(dst, src + off as usize)
-                {
-                    Ok(_) => Ok(ctx),
-                    Err(_) => Err(ctx),
-                }
             },
             tx,
             ctx,
@@ -529,11 +526,12 @@ impl InodeGuard<'_, InodeInner> {
     // However, reading user memory needs page table accesses since a single
     // consecutive region in user memory may split into several pages in
     // physical memory.
+    #[inline]
     fn write_internal<
         'id,
         's,
         K: Deref<Target = KernelCtx<'id, 's>>,
-        F: FnMut(u32, &mut [u8], K) -> Result<K, K>,
+        F: FnMut(u32, &mut [u8], &mut K) -> Result<(), ()>,
     >(
         &mut self,
         mut off: u32,
@@ -558,14 +556,12 @@ impl InodeGuard<'_, InodeInner> {
             let m = core::cmp::min(n - tot, BSIZE as u32 - off % BSIZE as u32);
             let begin = (off % BSIZE as u32) as usize;
             let end = begin + m as usize;
-            match f(tot, &mut bp.deref_inner_mut().data[begin..end], k) {
-                Ok(rk) => k = rk,
-                Err(rk) => {
-                    k = rk;
-                    break;
-                }
+            if f(tot, &mut bp.deref_inner_mut().data[begin..end], &mut k).is_ok() {
+                tx.write(bp, &k);
+            } else {
+                bp.free(&k);
+                break;
             }
-            tx.write(bp);
             tot += m;
             off += m;
         }
@@ -630,7 +626,9 @@ impl InodeGuard<'_, InodeInner> {
                 let tx = tx_opt.expect("bmap: out of range");
                 addr = tx.balloc(self.dev, ctx);
                 data[bn] = addr;
-                tx.write(bp);
+                tx.write(bp, ctx);
+            } else {
+                bp.free(ctx);
             }
             addr
         }
@@ -694,6 +692,8 @@ impl ArenaObject for Inode<InodeInner> {
                 ip.deref_inner_mut().typ = InodeType::None;
                 ip.update(&tx, &ctx);
                 ip.deref_inner_mut().valid = false;
+
+                ip.free(&ctx);
             };
 
             // TODO(https://github.com/kaist-cp/rv6/issues/290): remove kernel_ctx()
@@ -708,7 +708,7 @@ impl Inode<InodeInner> {
     /// Lock the given inode.
     /// Reads the inode from disk if necessary.
     pub fn lock(&self, ctx: &KernelCtx<'_, '_>) -> InodeGuard<'_, InodeInner> {
-        let mut guard = self.inner.lock();
+        let mut guard = self.inner.lock(ctx);
         if !guard.valid {
             let mut bp = hal().disk.read(
                 self.dev,
@@ -743,7 +743,7 @@ impl Inode<InodeInner> {
             guard.size = dip.size;
             guard.addr_direct.copy_from_slice(&dip.addr_direct);
             guard.addr_indirect = dip.addr_indirect;
-            drop(bp);
+            bp.free(ctx);
             guard.valid = true;
             assert_ne!(guard.typ, InodeType::None, "Inode::lock: no type");
         };
@@ -770,8 +770,8 @@ impl Inode<InodeInner> {
     }
 
     /// Copy stat information from inode.
-    pub fn stat(&self) -> Stat {
-        let inner = self.inner.lock();
+    pub fn stat(&self, ctx: &KernelCtx<'_, '_>) -> Stat {
+        let inner = self.inner.lock(ctx);
         Stat {
             dev: self.dev as i32,
             ino: self.inum,
@@ -851,8 +851,10 @@ impl Itable<InodeInner> {
                 }
 
                 // mark it allocated on the disk
-                tx.write(bp);
+                tx.write(bp, ctx);
                 return self.get_inode(dev, inum);
+            } else {
+                bp.free(ctx);
             }
         }
         panic!("[Itable::alloc_inode] no inodes");
@@ -900,7 +902,8 @@ impl Itable<InodeInner> {
         while let Some((new_path, name)) = path.skipelem() {
             path = new_path;
 
-            let mut ip = ptr.lock(ctx);
+            let ip = ptr.lock(ctx);
+            let mut ip = scopeguard::guard(ip, |ip| ip.free(ctx));
             if ip.deref_inner().typ != InodeType::Dir {
                 return Err(());
             }