S5 Fixes (#6)

* S5 Fixes

* Remove #ifdef OLD

* Fix comments for s5_get_disk_block

* Add info for newp return parameter

* Add stencil comment to s5_file_block_to_disk_block

* Remove ifdefno

---------

Co-authored-by: Ayma-n <aymanbt14@gmail.com>

7 files changed, 16 insertions, 95 deletions

diff --git a/kernel/drivers/blockdev.c b/kernel/drivers/blockdev.c
index 5c8eb82..79f25c2 100644
--- a/kernel/drivers/blockdev.c
+++ b/kernel/drivers/blockdev.c
@@ -79,7 +79,6 @@ long blockdev_fill_pframe(mobj_t *mobj, pframe_t *pf)
     KASSERT(mobj && pf);
     KASSERT(pf->pf_pagenum <= (1UL << (8 * sizeof(blocknum_t))));
     blockdev_t *bd = CONTAINER_OF(mobj, s5fs_t, s5f_mobj)->s5f_bdev;
-    KASSERT(pf->pf_loc);
     return bd->bd_ops->read_block(bd, pf->pf_addr, (blocknum_t)pf->pf_loc,
                                   1);
 }
@@ -90,7 +89,6 @@ long blockdev_flush_pframe(mobj_t *mobj, pframe_t *pf)
     KASSERT(pf->pf_pagenum <= (1UL << (8 * sizeof(blocknum_t))));
     dbg(DBG_S5FS, "writing disk block %lu\n", pf->pf_pagenum);
     blockdev_t *bd = CONTAINER_OF(mobj, s5fs_t, s5f_mobj)->s5f_bdev;
-    KASSERT(pf->pf_loc);
     return bd->bd_ops->write_block(bd, pf->pf_addr, (blocknum_t)pf->pf_loc,
                                    1);
 }
\ No newline at end of file
diff --git a/kernel/fs/s5fs/s5fs.c b/kernel/fs/s5fs/s5fs.c
index fe2a16c..d1df3a5 100644
--- a/kernel/fs/s5fs/s5fs.c
+++ b/kernel/fs/s5fs/s5fs.c
@@ -195,11 +195,11 @@ long s5fs_mount(fs_t *fs)
  *     - FS_TO_S5FS to obtain the s5fs object
  *     - S5_INODE_BLOCK(vn->v_vno) to determine the block number of the block that
  *       contains the inode info
- *     - s5_get_disk_block and s5_release_disk_block to handle the disk block
+ *     - s5_get_meta_disk_block and s5_release_disk_block to handle the disk block
  *     - S5_INODE_OFFSET to find the desired inode within the disk block
  *       containing it (returns the offset that the inode is stored within the block)
  *  - You should initialize the s5_node_t's inode field by reading directly from
- *    the inode on disk by using the page frame returned from s5_get_disk_block. Also 
+ *    the inode on disk by using the page frame returned from s5_get_meta_disk_block. Also 
  *    make sure to initialize the dirtied_inode field.
  *  - Using the inode info, you need to initialize the following vnode fields:
  *    vn_len, vn_mode, and vn_ops using the fields found in the s5_inode struct.
@@ -268,23 +268,17 @@ static long s5fs_umount(fs_t *fs)
 
 static void s5fs_sync(fs_t *fs)
 {
-#ifdef FIXME
     s5fs_t *s5fs = FS_TO_S5FS(fs);
-    #ifdef OLD
-    mobj_t *mobj = S5FS_TO_VMOBJ(s5fs);
-    #endif
-    mobj_t *mobj = 0;  // XXX FIX ME
-
-    mobj_lock(mobj);
+    mobj_t *mobj = &s5fs->s5f_mobj;
 
     pframe_t *pf;
-    mobj_get_pframe(mobj, S5_SUPER_BLOCK, 1, &pf);
+    s5_get_meta_disk_block(s5fs, S5_SUPER_BLOCK, 1, &pf);
     memcpy(pf->pf_addr, &s5fs->s5f_super, sizeof(s5_super_t));
-    pframe_release(&pf);
+    s5_release_disk_block(&pf);
 
-    mobj_flush(S5FS_TO_VMOBJ(s5fs));
-    mobj_unlock(S5FS_TO_VMOBJ(s5fs));
-#endif
+    mobj_lock(&s5fs->s5f_mobj);
+    mobj_flush(mobj);
+    mobj_unlock(&s5fs->s5f_mobj);
 }
 
 /* Wrapper around s5_read_file. */
@@ -577,21 +571,6 @@ static void s5fs_truncate_file(vnode_t *file)
     vunlock(file); 
 }
 
-#ifdef OLD
-/*
- * Wrapper around mobj_get_pframe. Remember to lock the memory object around
- * the call to mobj_get_pframe. Assert that the get_pframe does not fail.
- */
-inline void s5_get_disk_block(s5fs_t *s5fs, blocknum_t blocknum, long forwrite,
-                              pframe_t **pfp)
-{
-    mobj_lock(S5FS_TO_VMOBJ(s5fs));
-    long ret = mobj_get_pframe(S5FS_TO_VMOBJ(s5fs), blocknum, forwrite, pfp);
-    mobj_unlock(S5FS_TO_VMOBJ(s5fs));
-    KASSERT(!ret && *pfp);
-}
-#endif
-
 /*
  * Wrapper around device's read_block function; first looks up block in file-system cache.
  * If not there, allocates and fills a page frame. 
@@ -605,6 +584,7 @@ inline void s5_get_meta_disk_block(s5fs_t *s5fs, uint64_t blocknum, long forwrit
     if (*pfp)
     {
         // block is cached
+        (*pfp)->pf_dirty |= forwrite;
         mobj_unlock(&s5fs->s5f_mobj);
         return;
     }
@@ -615,7 +595,7 @@ inline void s5_get_meta_disk_block(s5fs_t *s5fs, uint64_t blocknum, long forwrit
 
     blockdev_t *bd = s5fs->s5f_bdev;
     long ret = bd->bd_ops->read_block(bd, pf->pf_addr, (blocknum_t)pf->pf_loc, 1);
-    pf->pf_dirty |= forwrite;  // needed?
+    pf->pf_dirty |= forwrite;  // yes, needed
     KASSERT (!ret);
     mobj_unlock(&s5fs->s5f_mobj);
     KASSERT(!ret && *pfp);
@@ -637,7 +617,7 @@ static inline void s5_get_file_disk_block(vnode_t *vnode, uint64_t blocknum, uin
     KASSERT(pf->pf_addr);
     blockdev_t *bd = VNODE_TO_S5FS(vnode)->s5f_bdev;
     long ret = bd->bd_ops->read_block(bd, pf->pf_addr, pf->pf_loc, 1);
-    pf->pf_dirty |= forwrite;  // needed?
+    pf->pf_dirty |= forwrite; // yes, needed
     KASSERT (!ret);
 }
 
@@ -680,36 +660,13 @@ inline void s5_release_disk_block(pframe_t **pfp) { pframe_release(pfp); }
 static long s5fs_get_pframe(vnode_t *vnode, uint64_t pagenum, long forwrite,
                             pframe_t **pfp)
 {
-#ifdef OLD
-    if (vnode->vn_len <= pagenum * PAGE_SIZE)
-        return -EINVAL;
-    long loc =
-        s5_file_block_to_disk_block(VNODE_TO_S5NODE(vnode), pagenum, forwrite);
-    if (loc < 0)
-        return loc;
-    if (loc)
-    {
-        mobj_find_pframe(&vnode->vn_mobj, pagenum, pfp);
-        if (*pfp)
-        {
-            mobj_free_pframe(&vnode->vn_mobj, pfp);
-        }
-        s5_get_disk_block(VNODE_TO_S5FS(vnode), (blocknum_t)loc, forwrite, pfp);
-        return 0;
-    }
-    else
-    {
-        KASSERT(!forwrite);
-        return mobj_default_get_pframe(&vnode->vn_mobj, pagenum, forwrite, pfp);
-    }
-#endif
-
     if (vnode->vn_len <= pagenum * PAGE_SIZE)
         return -EINVAL;
     mobj_find_pframe(&vnode->vn_mobj, pagenum, pfp);
     if (*pfp)
     {
         // block is cached
+        (*pfp)->pf_dirty |= forwrite;
         return 0;
     }
     int new;
diff --git a/kernel/fs/s5fs/s5fs_subr.c b/kernel/fs/s5fs/s5fs_subr.c
index 0187e74..33b03c9 100644
--- a/kernel/fs/s5fs/s5fs_subr.c
+++ b/kernel/fs/s5fs/s5fs_subr.c
@@ -91,7 +91,6 @@ static inline void s5_release_file_block(pframe_t **pfp)
     pframe_release(pfp);
 }
 
-#ifdef OLD
 /* Given a file and a file block number, return the disk block number of the
  * desired file block.
  *
@@ -100,6 +99,8 @@ static inline void s5_release_file_block(pframe_t **pfp)
  *                  the file
  *  alloc         - If set, allocate the block / indirect block as necessary
  *                  If clear, don't allocate sparse blocks
+ *  newp          - Return parameter that should be set to 1 if the returned
+ *                  block number is new (block has just been allocated)
  *
  * Return a disk block number on success, or:
  *  - 0: The block is sparse, and alloc is clear, OR
@@ -121,20 +122,13 @@ static inline void s5_release_file_block(pframe_t **pfp)
  *    1) file_blocknum < S_NDIRECT_BLOCKS
  *    2) Indirect block is not allocated but alloc is set. Be careful not to
  *       leak a block in an error case!
+      2a) Make sure you allocate the indirect block on disk and create a 
+         corresponding pframe_t on the mobj (Hint: see s5_cache_and_clear_block).
  *    3) Indirect block is allocated. The desired block may be sparse, and you
  *       may have to allocate it.
  *    4) The indirect block has not been allocated and alloc is clear.
  */
 long s5_file_block_to_disk_block(s5_node_t *sn, size_t file_blocknum,
-                                 int alloc)
-{
-    NOT_YET_IMPLEMENTED("S5FS: s5_file_block_to_disk_block");
-    return -1;
-}
-#endif
-
-
-long s5_file_block_to_disk_block(s5_node_t *sn, size_t file_blocknum,
                                  int alloc, int *newp)
 {
     NOT_YET_IMPLEMENTED("S5FS: s5_file_block_to_disk_block");
@@ -211,7 +205,6 @@ ssize_t s5_write_file(s5_node_t *sn, size_t pos, const char *buf, size_t len)
     return -1;
 }
 
-#ifdef OLD
 /* Allocate one block from the filesystem.
  *
  * Return the block number of the newly allocated block, or:
@@ -243,13 +236,6 @@ static long s5_alloc_block(s5fs_t *s5fs)
     NOT_YET_IMPLEMENTED("S5FS: s5_alloc_block");
     return -1;
 }
-#endif
-
-static long s5_alloc_block(s5fs_t *s5fs)
-{
-    NOT_YET_IMPLEMENTED("S5FS: s5_alloc_block");
-    return -1;
-}
 
 /*
  * The exact opposite of s5_alloc_block: add blockno to the free list of the
diff --git a/kernel/include/fs/s5fs/s5fs_subr.h b/kernel/include/fs/s5fs/s5fs_subr.h
index ff4c570..df2c835 100644
--- a/kernel/include/fs/s5fs/s5fs_subr.h
+++ b/kernel/include/fs/s5fs/s5fs_subr.h
@@ -44,10 +44,4 @@ void s5_remove_blocks(struct s5_node *vnode);
 /* Converts a vnode_t* to the s5fs_t* (s5fs file system) struct */
 #define VNODE_TO_S5FS(vn) ((s5fs_t *)((vn)->vn_fs->fs_i))
 
-#ifdef OLD
-/* Converts an s5fs_t* to its memory object (the memory object of the block device) */
-#define S5FS_TO_VMOBJ(s5fs) (&(s5fs)->s5f_bdev->bd_mobj)
-#endif
-
-
 pframe_t *s5_cache_and_clear_block(mobj_t *mo, long block, long loc);
diff --git a/kernel/include/mm/mobj.h b/kernel/include/mm/mobj.h
index bca1b38..800f689 100644
--- a/kernel/include/mm/mobj.h
+++ b/kernel/include/mm/mobj.h
@@ -14,11 +14,7 @@ typedef enum
     MOBJ_VNODE = 1,
     MOBJ_SHADOW,
     MOBJ_ANON,
-#ifdef OLD
-    MOBJ_BLOCKDEV,
-#else
     MOBJ_FS,
-#endif
 } mobj_type_t;
 
 typedef struct mobj_ops
diff --git a/kernel/mm/mobj.c b/kernel/mm/mobj.c
index 4b9c80f..02aced7 100644
--- a/kernel/mm/mobj.c
+++ b/kernel/mm/mobj.c
@@ -122,9 +122,6 @@ long mobj_get_pframe(mobj_t *o, uint64_t pagenum, long forwrite,
  * Create and initialize a pframe and add it to the mobj's mo_pframes list.
  * Upon successful return, the pframe's pf_mutex is locked.
  */
-#ifdef OLD
-static void mobj_create_pframe(mobj_t *o, uint64_t pagenum, pframe_t **pfp)
-#endif
 void mobj_create_pframe(mobj_t *o, uint64_t pagenum, uint64_t loc, pframe_t **pfp)
 {
     KASSERT(kmutex_owns_mutex(&o->mo_mutex));
diff --git a/kernel/mm/pagecache.c b/kernel/mm/pagecache.c
index b1763ba..e3bf73e 100644
--- a/kernel/mm/pagecache.c
+++ b/kernel/mm/pagecache.c
@@ -14,10 +14,3 @@ long pagecache_get_page(pframe_t *pf) {
    KASSERT(0 && "page not in pagecache");
    return 0;
 }
-
-#ifdef NO
-void pagecache_newsource(pframe_t pf, blockdev_t *dev, long loc) {
-   pf->pf_srcdev.pf_dev = dev; 
-   pf->pf_loc = loc;
-}
-#endif
\ No newline at end of file