Individually manages buffers in the buffer cache
authorBarret Rhoden <brho@cs.berkeley.edu>
Sat, 9 Oct 2010 01:42:42 +0000 (18:42 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:55 +0000 (17:35 -0700)
Check the docs, expect this to change (issues with refcnting and how we
can rip an individual block out of the buffer cache).

The main point of this is to make sure that PGSIZE worth of buffers are
*not* pulled in at the same time in the blockdev's buffer cache, since
they all are not necessarily metadata blocks.

Documentation/vfs.txt
kern/include/blockdev.h
kern/include/ext2fs.h
kern/src/blockdev.c
kern/src/ext2fs.c

index 18eb986..1028f72 100644 (file)
@@ -419,24 +419,41 @@ example, the superblock, inodes, inode indirect blocks, and to a certain
 extent the directory's contents are all metadata.  There isn't an FS file (not
 to be confused with an OS file) that corresponds to this data, but it needs to
 be read in and cached.  Furthermore, this cache needs to be managed and
-written back when dirty.
+written back when dirty.  Note that file data can be broken up into two
+different types: read()/write() data and mmap'd data.  When people talk about
+buffer caches versus page caches, they are talking about these two different
+types of file data (at least NetBSD did
+http://www.usenix.org/event/usenix2000/freenix/full_papers/silvers/silvers_html/).
 
 A very simple buffer cache would include anything *ever* read from disk.  It
 would then get copied into the page cache in PGSIZE chunks for the page cache.
-This would suck, since we now have two or three copies.
+This would suck, since we now have two or three copies.  We obviously want to
+use the page cache for both mmap() and read/write().  It's not clear about the
+metadata.
 
-Another buffer cache would be to cache only the disk blocks needed for
-metadata.  I think this is what Linux did before it unified its buffer and
+Another usage of a buffer cache would be to cache only the disk blocks needed
+for metadata.  I think this is what Linux did before it unified its buffer and
 page caches (implied in UTLK).  The main issue with this is that you have two
 different systems for managing essentially similar data - we only want to deal
 with flushing, syncing, and writebacks of one subsystem, not in multiple
-different systems.
+different subsystems.
 
-The solution is to use the page cache to cache the metadata block operations.
-We do this by having the block device be mapped in PGSIZE chunks through its
-own struct page_mapping (a.k.a. struct address_space in Linux).  This way,
-both files and the block device are mapped in PGSIZE chunks via the same
-page_mapping structure, and will be managed by the same code.
+The solution is to use the page cache to cache the metadata blocks' buffers.
+We do this by having the block device be 'mapped' (but not read) in PGSIZE
+chunks through its own struct page_mapping (a.k.a. struct address_space in
+Linux).  This way, both files and the block device are mapped in PGSIZE chunks
+via the same page_mapping structure, and will be managed by the same code.
+
+Sort of.  We don't actually read in PGSIZE chunks for the block buffer cache.
+If blocks will be in the bdev's cache, then they will be adjacent and on the
+same page.  It is possible some adjacent blocks (which would be on the same
+page) are not both metadata.
+
+A more accurate way to describe what we do is that metadata blocks are copied
+into a 'buffer cache' that is mapped and managed similarly to the page cache.
+Pages are made of buffer heads, which hold data, and the reclaiming of pages of
+memory from either the page cache or the buffer cache will use the same code -
+since they are both just made of buffer pages.
 
 X.2: Mapping Blockdev Data vs File Data
 --------------------
@@ -500,19 +517,25 @@ tradeoff is against having multiple BHs, which would mean multiple block IO
 requests for writing a single page (and writing the whole page may be a common
 operation).
 
-X.3: What about opening a blockdev?
+X.3: What about opening a blockdev as a file?
 --------------------
-All of this means we are using the page cache to cache metadata blocks from
-the block device.  But what about the other blocks?  Those are "file" blocks,
-which are in the page mapping of the inode.  But what about when the block
-device is opened as if it was a file?  And then if we were to access the file
-blocks in this manner?  Yeah, that would screw things up - you will get
-inconsistencies, under the current plan.  There is one easy way to avoid this:
-don't open a bdev file if a file system is mounted on top of it.  If you do,
-don't be surprised about inconsistencies.  Ideally, the FS will never leave
-the actual disk in an inconsistent state, but the bdev's page cache could read
-things at different times and get some weird results.  Just don't do this (for
-now - not like I plan to make this possible any time soon).
+Basically, don't do it for now.  The way we actually do things is a buffer cache
+page is "up to date", but has no BHs or data until a specific block is
+requested.  This is because we don't always know if all the blocks mapped by a
+page are actually metadata blocks.  If they aren't we'll have issues where we
+read in extra blocks that exist in both a file's page cache and the block
+device's buffer cache.
+
+A safe invariant is that a given block will only ever be in one cache: either a
+file's page mapping or the buffer cache's page mapping.  When these blocks are
+freed, we'll need to rip them out of their mapping (and possibly flush them).
+
+There is one easy way to avoid this: don't open a bdev file if a file system is
+mounted on top of it.  If you do, don't be surprised about inconsistencies.
+Ideally, the FS will never leave the actual disk in an inconsistent state, but
+the bdev's page cache could read things at different times and get some weird
+results.  Just don't do this (for now - not like I plan to make this possible
+any time soon).
 
 Could we use the same buffers for both the blockdev-file page mapping and the
 inode-file page mapping?  No - because the inode-file has the inode
@@ -559,6 +582,8 @@ read).  The reason we wouldn't want to pin them is to save memory.
 
 x.5: Reference Counting BHs and Pages
 --------------------
+TODO: this will change, since we are likely to need to refcnt actual BHs.
+
 So we talk about passing around BHs, both when submitting IO ops and when
 returning from things like read_block() (note: this may return pages, not
 BHs).  However, we do not kref or reference count BHs in any way.  Instead, we
index 6f623eb..ae60b37 100644 (file)
@@ -56,6 +56,13 @@ struct buffer_head {
 };
 struct kmem_cache *bh_kcache;
 
+/* Buffer Head Requests.  For now, just use these for dealing with non-file IO
+ * on a block device.  Tell it what size you think blocks are. */
+struct buffer_head *get_buffer(struct block_device *bdev, unsigned long blk_num,
+                               unsigned int blk_sz);
+void dirty_buffer(struct buffer_head *bh);
+void put_buffer(struct buffer_head *bh);
+
 /* This encapsulates the work of a request (instead of having a variety of
  * slightly-different functions for things like read/write and scatter-gather
  * ops).  Reads and writes are essentially the same, so all we need is a flag to
index 4ab62d5..9e948ad 100644 (file)
@@ -256,7 +256,9 @@ struct ext2_sb_info {
        unsigned int                            nr_bgs;
 };
 
-/* Inode in-memory data.  This stuff is in cpu-native endianness */
+/* Inode in-memory data.  This stuff is in cpu-native endianness.  If we start
+ * using the data in the actual inode and in the buffer cache, change
+ * ext2_my_bh() and its two callers.  Assume this data is dirty. */
 struct ext2_i_info {
        uint32_t                                        i_block[15];            /* list of blocks reserved*/
 };
index 9f075f8..b050f77 100644 (file)
@@ -108,75 +108,125 @@ int make_request(struct block_device *bdev, struct block_request *req)
        return 0;
 }
 
-/* Sets up the bidirectional mapping between the page and its buffer heads.
- * Note that for a block device, this is 1:1.  We have the helper in the off
- * chance we want to decouple the mapping from readpage (which we may want to
- * do for writepage, esp on FSs), and to keep readpage simple. */
-static int block_mappage(struct block_device *bdev, struct page *page)
+/* This just tells the page cache that it is 'up to date'.  Due to the nature of
+ * the blocks in the page cache, we don't actually read the items in on
+ * readpage, we read them in when a specific block is there */
+int block_readpage(struct page_map *pm, struct page *page)
 {
-       struct buffer_head *bh;
-       assert(!page->pg_private);              /* double check that we aren't bh-mapped */
-       bh = kmem_cache_alloc(bh_kcache, 0);
-       if (!bh)
-               return -ENOMEM;
-       /* Set up the BH.  bdevs do a 1:1 BH to page mapping */
-       bh->bh_page = page;                                                             /* weak ref */
-       bh->bh_buffer = page2kva(page);
-       bh->bh_flags = 0;                                                               /* whatever... */
-       bh->bh_next = 0;                                                                /* only one BH needed */
-       bh->bh_bdev = bdev;                                                             /* uncounted ref */
-       bh->bh_sector = page->pg_index * PGSIZE / bdev->b_sector_sz;
-       bh->bh_nr_sector = PGSIZE / bdev->b_sector_sz;
-       page->pg_private = bh;
+       page->pg_flags |= PG_UPTODATE;
+       unlock_page(page);
        return 0;
 }
 
-/* Fills page with the appropriate data from the block device.  There is only
- * one BH, since bdev pages must be made of contiguous blocks.  Ideally, this
- * will work for a bdev file that reads the pm via generic_file_read(), though
- * that is a ways away still.  Techincally, you could get an ENOMEM for breq and
- * have this called again with a BH already set up, but I want to see it (hence
- * the assert). */
-int block_readpage(struct page_map *pm, struct page *page)
+/* Returns a BH pointing to the buffer where blk_num from bdev is located (given
+ * blocks of size blk_sz).  This uses the page cache for the page allocations
+ * and evictions, but only caches blocks that are requested.  Check the docs for
+ * more info.  The BH isn't refcounted, but a page refcnt is returned.  Call
+ * put_block (nand/xor dirty block).
+ *
+ * Note we're using the lock_page() to sync (which is what we do with the page
+ * cache too.  It's not ideal, but keeps things simpler for now.
+ *
+ * Also note we're a little inconsistent with the use of sector sizes in certain
+ * files.  We'll sort it eventually. */
+struct buffer_head *get_buffer(struct block_device *bdev, unsigned long blk_num,
+                               unsigned int blk_sz)
 {
-       int retval;
-       unsigned long first_byte = page->pg_index * PGSIZE;
-       struct block_device *bdev = pm->pm_bdev;
+       struct page *page;
+       struct page_map *pm = &bdev->b_pm;
+       struct buffer_head *bh, *new, *prev, **next_loc;
        struct block_request *breq;
-
-       assert(pm == page->pg_mapping);         /* Should be part of a page map */
-       assert(page->pg_flags & PG_BUFFER);     /* Should be part of a page map */
-       assert(!page->pg_private);                      /* Should be no BH for this page yet */
-       /* Don't read beyond the device.  There might be an issue when using
-        * generic_file_read() with this readpage(). */
-       if (first_byte + PGSIZE > bdev->b_nr_sector * bdev->b_sector_sz) {
-               unlock_page(page);
-               return -EFBIG;
+       int error;
+       unsigned int blk_per_pg = PGSIZE / blk_sz;
+       unsigned int sct_per_blk = blk_sz / bdev->b_sector_sz;
+       unsigned int blk_offset = (blk_num % blk_per_pg) * blk_sz;
+       assert(blk_offset < PGSIZE);
+       void *my_buf;
+       /* Make sure there's a page in the page cache.  Should always be one. */
+       error = pm_load_page(pm, blk_num / blk_per_pg, &page); 
+       if (error)
+               panic("Failed to load page! (%d)", error);
+       my_buf = page2kva(page) + blk_offset;
+       assert(page->pg_flags & PG_BUFFER);             /* Should be part of a page map */
+retry:
+       bh = (struct buffer_head*)page->pg_private;
+       prev = 0;
+       /* look through all the BHs for ours, stopping if we go too far. */
+       while (bh) {
+               if (bh->bh_buffer == my_buf) {
+                       goto found;
+               } else if (bh->bh_buffer > my_buf) {
+                       break;
+               }
+               prev = bh;
+               bh = bh->bh_next;
        }
-       retval = block_mappage(bdev, page);
-       if (retval) {
-               unlock_page(page);
-               return retval;
+       /* At this point, bh points to the one beyond our space (or 0), and prev is
+        * either the one before us or 0.  We make a BH, and try to insert */
+       new = kmem_cache_alloc(bh_kcache, 0);
+       assert(new);
+       new->bh_page = page;                                    /* weak ref */
+       new->bh_buffer = my_buf;
+       new->bh_flags = 0;
+       new->bh_next = bh;
+       new->bh_bdev = bdev;                                    /* uncounted ref */
+       new->bh_sector = blk_num * sct_per_blk;
+       new->bh_nr_sector = sct_per_blk;
+       /* Try to insert the new one in place.  If it fails, retry the whole "find
+        * the bh" process.  This should be rare, so no sense optimizing it. */
+       next_loc = prev ? &prev->bh_next : (struct buffer_head**)&page->pg_private;
+       if (!atomic_comp_swap((uint32_t*)next_loc, (uint32_t)bh, (uint32_t)new)) {
+               kmem_cache_free(bh_kcache, new);
+               goto retry;
        }
-       /* Build and submit the request */
-       breq = kmem_cache_alloc(breq_kcache, 0);
-       if (!breq) {
+       bh = new;
+found:
+       /* At this point, we have the BH for our buf, but it might not be up to
+        * date, and there might be someone else trying to update it. */
+       /* is it already here and up to date?  if so, we're done */
+       if (bh->bh_flags & BH_UPTODATE)
+               return bh;
+       /* if not, try to lock the page (could BLOCK).  Using this for syncing. */
+       lock_page(page);
+       /* double check, are we up to date?  if so, we're done */
+       if (bh->bh_flags & BH_UPTODATE) {
                unlock_page(page);
-               return -ENOMEM;
+               return bh;
        }
+       /* if we're here, the page is locked by us, we need to read the block */
+       breq = kmem_cache_alloc(breq_kcache, 0);
+       assert(breq);
        breq->flags = BREQ_READ;
        breq->bhs = breq->local_bhs;
-       /* There's only one BH for a bdev page */
-       breq->bhs[0] = (struct buffer_head*)page->pg_private;
+       breq->bhs[0] = bh;
        breq->nr_bhs = 1;
-       retval = make_request(bdev, breq);
+       error = make_request(bdev, breq);
        /* TODO: (BLK) this assumes we slept til the request was done */
-       assert(!retval);
+       assert(!error);
+       kmem_cache_free(breq_kcache, breq);
        /* after the data is read, we mark it up to date and unlock the page. */
-       page->pg_flags |= PG_UPTODATE;
+       bh->bh_flags |= BH_UPTODATE;
        unlock_page(page);
-       kmem_cache_free(breq_kcache, breq);
-       return 0;
+       return bh;
+}
+
+/* Will dirty the block/BH/page for the given block/buffer.  Will have to be
+ * careful with the page reclaimer - if someone holds a reference, they can
+ * still dirty it. */
+void dirty_buffer(struct buffer_head *bh)
+{
+       struct page *page = bh->bh_page;
+       /* TODO: race on flag modification */
+       bh->bh_flags |= BH_DIRTY;
+       page->pg_flags |= PG_DIRTY;
+}
+
+/* Decrefs the buffer from get_buffer().  Call this when you no longer reference
+ * your block/buffer.  For now, we do refcnting on the page, since the
+ * reclaiming will be in page sized chunks from the page cache. */
+void put_buffer(struct buffer_head *bh)
+{
+       page_decref(bh->bh_page);
 }
 
 /* Block device page map ops: */
index 11ab6a7..d6ed1cb 100644 (file)
@@ -100,19 +100,7 @@ void ext2_init(void)
 void *__ext2_get_metablock(struct block_device *bdev, unsigned long blk_num,
                            unsigned int blk_sz)
 {
-       struct page *page;
-       struct page_map *pm = &bdev->b_pm;
-       unsigned int blk_per_pg = PGSIZE / blk_sz;
-       unsigned int blk_offset = (blk_num % blk_per_pg) * blk_sz;
-       int error;
-       assert(blk_offset < PGSIZE);
-       error = pm_load_page(pm, blk_num / blk_per_pg, &page); 
-       if (error) {
-               warn("Failed to read metablock! (%d)", error);
-               return 0;
-       }
-       /* return where we are within the page for the given block */
-       return page2kva(page) + blk_offset;
+       return get_buffer(bdev, blk_num, blk_sz)->bh_buffer;
 }
 
 /* Convenience wrapper */
@@ -121,21 +109,41 @@ void *ext2_get_metablock(struct super_block *sb, unsigned long block_num)
        return __ext2_get_metablock(sb->s_bdev, block_num, sb->s_blocksize);
 }
 
+/* Helper to figure out the BH for any address within it's buffer */
+static struct buffer_head *ext2_my_bh(struct super_block *sb, void *addr)
+{
+       struct page *page = kva2page(addr);
+       struct buffer_head *bh = (struct buffer_head*)page->pg_private;
+       /* This case is for when we try do decref a non-BH'd 'metablock'.  It's tied
+        * to e2ii->i_block[]. */
+       if (!bh)
+               return 0;
+       void *my_buf = (void*)ROUNDDOWN((uintptr_t)addr, sb->s_blocksize);
+       while (bh) {
+               if (bh->bh_buffer == my_buf)
+                       break;
+               bh = bh->bh_next;
+       }
+       assert(bh && bh->bh_buffer == my_buf);
+       return bh;
+}
+
 /* Decrefs the buffer from get_metablock().  Call this when you no longer
- * reference your metadata block/buffer */
-void ext2_put_metablock(void *buffer)
+ * reference your metadata block/buffer.  Yes, we could just decref the page,
+ * but this will work if we end up changing how put_buffer() works. */
+void ext2_put_metablock(struct super_block *sb, void *buffer)
 {
-       page_decref(kva2page(buffer));
+       struct buffer_head *bh = ext2_my_bh(sb, buffer);
+       if (bh)
+               put_buffer(bh);
 }
 
-/* Will dirty the block/BH/page for the given metadata block/buffer.  Will have
- * to be careful with the page reclaimer - if someone holds a reference, they
- * can still dirty it. */
-void ext2_dirty_metablock(void *buffer)
+/* Will dirty the block/BH/page for the given metadata block/buffer. */
+void ext2_dirty_metablock(struct super_block *sb, void *buffer)
 {
-       struct page *page = kva2page(buffer);
-       /* TODO: race on flag modification, and consider dirtying the BH. */
-       page->pg_flags |= PG_DIRTY;
+       struct buffer_head *bh = ext2_my_bh(sb, buffer);
+       if (bh)
+               dirty_buffer(bh);
 }
 
 /* Helper for alloc_block.  It will try to alloc a block from the BG, starting
@@ -159,14 +167,14 @@ static bool ext2_tryalloc(struct super_block *sb, struct ext2_block_group *bg,
                if (!(GET_BITMASK_BIT(blk_bitmap, blk_idx))) {
                        SET_BITMASK_BIT(blk_bitmap, blk_idx);
                        bg->bg_free_blocks_cnt--;
-                       ext2_dirty_metablock(blk_bitmap);
+                       ext2_dirty_metablock(sb, blk_bitmap);
                        found = TRUE;
                        break;
                }
                /* Note: the wrap-around hasn't been tested yet */
                blk_idx = (blk_idx + 1) % blks_per_bg;
        }
-       ext2_put_metablock(blk_bitmap);
+       ext2_put_metablock(sb, blk_bitmap);
        if (found)
                *block_num = ext2_bgidx2block(sb, bg, blk_idx);
        return found;
@@ -240,12 +248,12 @@ static void ext2_fill_inotable_slot(struct inode *inode, uint32_t *blk_slot)
        /* Actually read in the block we alloc'd */
        new_blk = ext2_get_metablock(inode->i_sb, new_blkid);
        memset(new_blk, 0, inode->i_sb->s_blocksize);
-       ext2_dirty_metablock(new_blk);
+       ext2_dirty_metablock(inode->i_sb, new_blk);
        /* We put it, despite it getting relooked up in the next walk */
-       ext2_put_metablock(new_blk);
+       ext2_put_metablock(inode->i_sb, new_blk);
        /* Now write the new block into its slot */
        *blk_slot = cpu_to_le32(new_blkid);
-       ext2_dirty_metablock(blk_slot);
+       ext2_dirty_metablock(inode->i_sb, blk_slot);
 }
 
 /* This walks a table stored at block 'blkid', returning which block you should
@@ -267,7 +275,7 @@ static void ext2_walk_inotable(struct inode *inode, uint32_t *blkid,
        ext2_fill_inotable_slot(inode, blk_slot);
        *blkid = le32_to_cpu(*blk_slot);
        *rel_inoblk = *rel_inoblk % reach;
-       ext2_put_metablock(blk_slot);   /* the ref for the block we looked in */
+       ext2_put_metablock(inode->i_sb, blk_slot);      /* ref for the one looked in */
 }
 
 /* Finds the slot of the FS block corresponding to a specific block number of an
@@ -276,7 +284,9 @@ static void ext2_walk_inotable(struct inode *inode, uint32_t *blkid,
  * tables (1x, 2x, or 3x (triply indirect) tables).  Block numbers start at 0.
  *
  * This returns a pointer within a metablock, which needs to be decref'd (and
- * possibly dirtied) when you are done.
+ * possibly dirtied) when you are done.  Note, it can return a pointer to
+ * something that is NOT in a metablock (e2ii->i_block[]), but put_metablock can
+ * handle it for now.
  *
  * Horrendously untested, btw. */
 uint32_t *ext2_lookup_inotable_slot(struct inode *inode, uint32_t ino_block)
@@ -322,9 +332,6 @@ uint32_t *ext2_lookup_inotable_slot(struct inode *inode, uint32_t ino_block)
        } else {
                /* Direct block, straight out of the inode */
                blk_slot = &e2ii->i_block[ino_block];
-               /* need to incref, since the i_block isn't a real metablock (it's just a
-                * random page!), and the caller is going to end up decreffing it */
-               page_incref(kva2page(blk_slot));
        }
        return blk_slot;
 }
@@ -335,7 +342,7 @@ uint32_t ext2_find_inoblock(struct inode *inode, unsigned int ino_block)
 {
        uint32_t retval, *buf = ext2_lookup_inotable_slot(inode, ino_block);
        retval = *buf;
-       ext2_put_metablock(buf);
+       ext2_put_metablock(inode->i_sb, buf);
        return retval;
 }
 
@@ -558,7 +565,7 @@ int ext2_mappage(struct page_map *pm, struct page *page)
                        fs_blk_num = ext2_alloc_block(inode, fs_blk_num + 1);
                        /* Link it, and dirty the inode indirect block */
                        *fs_blk_slot = cpu_to_le32(fs_blk_num);
-                       ext2_dirty_metablock(fs_blk_slot);
+                       ext2_dirty_metablock(inode->i_sb, fs_blk_slot);
                        /* the block is still on disk, and we don't want its contents */
                        bh->bh_flags = BH_NEEDS_ZEROED;                 /* talking to readpage */
                        /* update our num blocks, with 512B each "block" (ext2-style) */
@@ -566,7 +573,7 @@ int ext2_mappage(struct page_map *pm, struct page *page)
                } else {        /* there is a block there already */
                        fs_blk_num = *fs_blk_slot;
                }
-               ext2_put_metablock(fs_blk_slot);
+               ext2_put_metablock(inode->i_sb, fs_blk_slot);
                bh->bh_sector = fs_blk_num * sct_per_blk;
                bh->bh_nr_sector = sct_per_blk;
                /* Stop if we're the last block in the page.  We could be going beyond
@@ -733,7 +740,7 @@ void ext2_read_inode(struct inode *inode)
        /* TODO: (HASH) unused: inode->i_hash add to hash (saves on disc reading) */
        /* TODO: we could consider saving a pointer to the disk inode and pinning
         * its buffer in memory, but for now we'll just free it. */
-       ext2_put_metablock(ino_tbl_chunk);
+       ext2_put_metablock(inode->i_sb, ino_tbl_chunk);
 }
 
 /* called when an inode in memory is modified (journalling FS's care) */
@@ -853,7 +860,7 @@ struct dentry *ext2_lookup(struct inode *dir, struct dentry *dentry,
                /* On subsequent loops, we might need to advance to the next block.
                 * This is where a file abstraction for a dir might be easier. */
                if ((void*)dir_i >= (void*)dir_buf + dir->i_sb->s_blocksize) {
-                       ext2_put_metablock(dir_buf);
+                       ext2_put_metablock(dir->i_sb, dir_buf);
                        dir_buf = ext2_get_ino_metablock(dir, dir_block++);
                        dir_i = dir_buf;
                        assert(dir_buf);
@@ -865,7 +872,7 @@ struct dentry *ext2_lookup(struct inode *dir, struct dentry *dentry,
                            (dentry->d_name.name[dir_i->dir_namelen] == '\0')) {
                        load_inode(dentry, le32_to_cpu(dir_i->dir_inode));
                        /* TODO: (HASH) add dentry to dcache (maybe the caller should) */
-                       ext2_put_metablock(dir_buf);
+                       ext2_put_metablock(dir->i_sb, dir_buf);
                        return dentry;
                }
                /* Get ready for the next loop */
@@ -873,7 +880,7 @@ struct dentry *ext2_lookup(struct inode *dir, struct dentry *dentry,
                dir_i = (void*)dir_i + dir_i->dir_reclen;
        }
        printd("EXT2: Not Found, %s\n", dentry->d_name.name);   
-       ext2_put_metablock(dir_buf);
+       ext2_put_metablock(dir->i_sb, dir_buf);
        return 0;
 }
 
@@ -1068,7 +1075,7 @@ int ext2_readdir(struct file *dir, struct dirent *dirent)
        strncpy(dirent->d_name, (char*)e2dir->dir_name, e2dir->dir_namelen);
        assert(e2dir->dir_namelen <= MAX_FILENAME_SZ);
        dirent->d_name[e2dir->dir_namelen] = '\0';
-       ext2_put_metablock(blk_buf);
+       ext2_put_metablock(dir->f_dentry->d_sb, blk_buf);
        
        /* At the end of the directory, sort of.  ext2 often preallocates blocks, so
         * this will cause us to walk along til the end, which isn't quite right. */