From b1b75cbe9fc93d3ed199b71d4457d35946822ef4 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Fri, 12 Feb 2021 14:07:52 -0800 Subject: [PATCH 1/7] Fix block cache shrink and read racing crash The block cache wasn't safely racing readers walking the rcu radix_tree and the shrinker walking the LRU list. A reader could get a reference to a block that had been removed from the radix and was queued for freeing. It'd clobber the free's llist_head union member by putting the block back on the lru and both the read and free would crash as they each corrupted each other's memory. We rarely saw this in heavy load testing. The fix is to clean up the use of rcu, refcounting, and freeing. First, we get rid of the LRU list. Now we don't have to worry about resolving racing accesses of blocks between two independent structures. Instead of shrinking walking the LRU list, we can mark blocks on access such that shrinking can walk all blocks randomly and expect to quickly find candidates to shrink. To make it easier to concurrently walk all the blocks we switch to the rhashtable instead of the radix tree. It also has nice per-bucket locking so we can get rid of the global lock that protected the LRU list and radix insertion. (And it isn't limited to 'long' keys so we can get rid of the check for max meta blknos that couldn't be cached.) Now we need to tighten up when read can get a reference and when shrink can remove blocks. We have presence in the hash table hold a refcount but we make it a magic high bit in the refcount so that it can be differentiated from other references. Now lookup can atomically get a reference to blocks that are in the hash table, and shrinking can atomically remove blocks when it is the only other reference. We also clean up freeing a bit. It has to wait for the rcu grace period to ensure that no other rcu readers can reference the blocks its freeing. It has to iterate over the list with _safe because it's freeing as it goes. Interestingly, when reworking the shrinker I noticed that we weren't scaling the nr_to_scan from the pages we returned in previous shrink calls back to blocks. We now divide the input from pages back into blocks. Signed-off-by: Zach Brown --- kmod/src/block.c | 448 ++++++++++++++++++++++++--------------- kmod/src/counters.h | 8 +- kmod/src/scoutfs_trace.h | 70 +++--- 3 files changed, 330 insertions(+), 196 deletions(-) diff --git a/kmod/src/block.c b/kmod/src/block.c index 146925cb..1c2e4ae8 100644 --- a/kmod/src/block.c +++ b/kmod/src/block.c @@ -19,6 +19,8 @@ #include #include #include +#include +#include #include "format.h" #include "super.h" @@ -33,22 +35,25 @@ * dirty blocks that are written together. We pin dirty blocks in * memory and only checksum them all as they're all written. * - * An LRU is maintained so the VM can reclaim the oldest presumably - * unlikely to be used blocks. But we don't maintain a perfect record - * of access order. We only move accessed blocks to the tail of the rcu - * if they weren't in the most recently moved fraction of the total - * population. This means that reclaim will walk through waves of that - * fraction of the population. It's close enough and removes lru - * maintenance locking from the fast path. + * Memory reclaim is driven by maintaining two very coarse groups of + * blocks. As we access blocks we mark them with an increasing counter + * to discourage them from being reclaimed. We then define a threshold + * at the current counter minus half the population. Recent blocks have + * a counter greater than the threshold, and all other blocks with + * counters less than it are considered older and are candidates for + * reclaim. This results in access updates rarely modifying an atomic + * counter as blocks need to be moved into the recent group, and shrink + * can randomly scan blocks looking for the half of the population that + * will be in the old group. It's reasonably effective, but is + * particularly efficient and avoids contention between concurrent + * accesses and shrinking. */ struct block_info { struct super_block *sb; - spinlock_t lock; - struct radix_tree_root radix; - struct list_head lru_list; - u64 lru_nr; - u64 lru_move_counter; + atomic_t total_inserted; + atomic64_t access_counter; + struct rhashtable ht; wait_queue_head_t waitq; struct shrinker shrinker; struct work_struct free_work; @@ -64,22 +69,33 @@ enum block_status_bits { BLOCK_BIT_DIRTY, /* dirty, writer will write */ BLOCK_BIT_IO_BUSY, /* bios are in flight */ BLOCK_BIT_ERROR, /* saw IO error */ - BLOCK_BIT_DELETED, /* has been deleted from radix tree */ BLOCK_BIT_PAGE_ALLOC, /* page (possibly high order) allocation */ BLOCK_BIT_VIRT, /* mapped virt allocation */ BLOCK_BIT_CRC_VALID, /* crc has been verified */ }; +/* + * We want to tie atomic changes in refcounts to whether or not the + * block is still visible in the hash table, so we store the hash + * table's reference up at a known high bit. We could naturally set the + * inserted bit through excessive refcount increments. We don't do + * anything about that but at least warn if we get close. + * + * We're avoiding the high byte for no real good reason, just out of a + * historical fear of implementations that don't provide the full + * precision. + */ +#define BLOCK_REF_INSERTED (1U << 23) +#define BLOCK_REF_FULL (BLOCK_REF_INSERTED >> 1) + struct block_private { struct scoutfs_block bl; struct super_block *sb; atomic_t refcount; - union { - struct list_head lru_entry; - struct llist_node free_node; - }; - u64 lru_moved; + u64 accessed; + struct rhash_head ht_head; struct list_head dirty_entry; + struct llist_node free_node; unsigned long bits; atomic_t io_count; union { @@ -88,13 +104,11 @@ struct block_private { }; }; -#define TRACE_BLOCK(which, bp) \ -do { \ - __typeof__(bp) _bp = (bp); \ - trace_scoutfs_block_##which(_bp->sb, _bp, _bp->bl.blkno, \ - atomic_read(&_bp->refcount), \ - atomic_read(&_bp->io_count), \ - _bp->bits, _bp->lru_moved); \ +#define TRACE_BLOCK(which, bp) \ +do { \ + __typeof__(bp) _bp = (bp); \ + trace_scoutfs_block_##which(_bp->sb, _bp, _bp->bl.blkno, atomic_read(&_bp->refcount), \ + atomic_read(&_bp->io_count), _bp->bits, _bp->accessed); \ } while (0) #define BLOCK_PRIVATE(_bl) \ @@ -136,7 +150,7 @@ static struct block_private *block_alloc(struct super_block *sb, u64 blkno) /* * If we had multiple blocks per page we'd need to be a little * more careful with a partial page allocator when allocating - * blocks and would make the lru per-page instead of per-block. + * blocks. */ BUILD_BUG_ON(PAGE_SIZE > SCOUTFS_BLOCK_LG_SIZE); @@ -167,7 +181,6 @@ static struct block_private *block_alloc(struct super_block *sb, u64 blkno) bp->bl.blkno = blkno; bp->sb = sb; atomic_set(&bp->refcount, 1); - INIT_LIST_HEAD(&bp->lru_entry); INIT_LIST_HEAD(&bp->dirty_entry); set_bit(BLOCK_BIT_NEW, &bp->bits); atomic_set(&bp->io_count, 0); @@ -193,7 +206,6 @@ static void block_free(struct super_block *sb, struct block_private *bp) else BUG(); - /* lru_entry could have been clobbered by union member free_node */ WARN_ON_ONCE(!list_empty(&bp->dirty_entry)); WARN_ON_ONCE(atomic_read(&bp->refcount)); WARN_ON_ONCE(atomic_read(&bp->io_count)); @@ -201,109 +213,179 @@ static void block_free(struct super_block *sb, struct block_private *bp) } /* - * We free blocks in task context so we can free kernel virtual mappings. + * Free all the blocks that were put in the free_llist. We have to wait + * for rcu grace periods to expire to ensure that no more rcu hash list + * lookups can see the blocks. */ static void block_free_work(struct work_struct *work) { - struct block_info *binf = container_of(work, struct block_info, - free_work); + struct block_info *binf = container_of(work, struct block_info, free_work); struct super_block *sb = binf->sb; struct block_private *bp; + struct block_private *tmp; struct llist_node *deleted; - deleted = llist_del_all(&binf->free_llist); + scoutfs_inc_counter(sb, block_cache_free_work); - llist_for_each_entry(bp, deleted, free_node) { + deleted = llist_del_all(&binf->free_llist); + synchronize_rcu(); + + llist_for_each_entry_safe(bp, tmp, deleted, free_node) { block_free(sb, bp); } } /* - * After we've dropped the final ref kick off the final free in task - * context. This happens in the relatively rare cases of IO errors, - * stale cached data, memory pressure, and unmount. + * Get a reference to a block while holding an existing reference. + */ +static void block_get(struct block_private *bp) +{ + WARN_ON_ONCE((atomic_read(&bp->refcount) & ~BLOCK_REF_INSERTED) <= 0); + + atomic_inc(&bp->refcount); +} + +/* + * Get a reference to a block as long as it's been inserted in the hash + * table and hasn't been removed. + */ +static struct block_private *block_get_if_inserted(struct block_private *bp) +{ + int cnt; + + do { + cnt = atomic_read(&bp->refcount); + WARN_ON_ONCE(cnt & BLOCK_REF_FULL); + if (!(cnt & BLOCK_REF_INSERTED)) + return NULL; + + } while (atomic_cmpxchg(&bp->refcount, cnt, cnt + 1) != cnt); + + return bp; +} + +/* + * Drop the caller's reference. If this was the final reference we + * queue the block to be freed once the rcu period ends. Readers can be + * racing to try to get references to these blocks, but they won't get a + * reference because the block isn't present in the hash table any more. */ static void block_put(struct super_block *sb, struct block_private *bp) { DECLARE_BLOCK_INFO(sb, binf); + int cnt; - if (!IS_ERR_OR_NULL(bp) && atomic_dec_and_test(&bp->refcount)) { - WARN_ON_ONCE(!list_empty(&bp->lru_entry)); - llist_add(&bp->free_node, &binf->free_llist); - schedule_work(&binf->free_work); + if (!IS_ERR_OR_NULL(bp)) { + cnt = atomic_dec_return(&bp->refcount); + if (cnt == 0) { + llist_add(&bp->free_node, &binf->free_llist); + schedule_work(&binf->free_work); + } else { + WARN_ON_ONCE(cnt < 0); + } } } +static const struct rhashtable_params block_ht_params = { + .key_len = member_sizeof(struct block_private, bl.blkno), + .key_offset = offsetof(struct block_private, bl.blkno), + .head_offset = offsetof(struct block_private, ht_head), +}; + /* - * Add a new block into the cache. The caller holds the lock and has - * preloaded the radix. + * Insert a new block into the hash table. Once it is inserted in the + * hash table readers can start getting references. The caller only has + * its initial ref so inserted can't be set and there can be no other + * references. */ -static void block_insert(struct super_block *sb, struct block_private *bp, - u64 blkno) +static int block_insert(struct super_block *sb, struct block_private *bp) { DECLARE_BLOCK_INFO(sb, binf); + int ret; - assert_spin_locked(&binf->lock); - BUG_ON(!list_empty(&bp->lru_entry)); + WARN_ON_ONCE(atomic_read(&bp->refcount) != 1); - atomic_inc(&bp->refcount); - radix_tree_insert(&binf->radix, blkno, bp); - list_add_tail(&bp->lru_entry, &binf->lru_list); - bp->lru_moved = ++binf->lru_move_counter; - binf->lru_nr++; + atomic_add(BLOCK_REF_INSERTED, &bp->refcount); + ret = rhashtable_insert_fast(&binf->ht, &bp->ht_head, block_ht_params); + if (ret < 0) { + atomic_sub(BLOCK_REF_INSERTED, &bp->refcount); + } else { + atomic_inc(&binf->total_inserted); + TRACE_BLOCK(insert, bp); + } - TRACE_BLOCK(insert, bp); + return ret; +} + +static u64 accessed_recently(struct block_info *binf) +{ + return atomic64_read(&binf->access_counter) - (atomic_read(&binf->total_inserted) >> 1); } /* - * Only move the block to the tail of the LRU if it's outside of the - * small fraction of the lru population that has been most recently - * used. This gives us a reasonable number of most recently accessed - * blocks which will be reclaimed after the rest of the least recently - * used blocks while reducing per-access locking overhead of maintaining - * the LRU. We don't care about unlikely non-atomic u64 accesses racing - * and messing up LRU position. - * - * This can race with blocks being removed from the cache (shrinking, - * stale, errors) so we're careful to only move the entry if it's still - * on the list after we acquire the lock. We still hold a reference so it's - * lru_entry hasn't transitioned to being used as the free_node. + * Make sure that a block that is being accessed is less likely to be + * reclaimed if it is seen by the shrinker. If the block hasn't been + * accessed recently we update its accessed value. */ static void block_accessed(struct super_block *sb, struct block_private *bp) { DECLARE_BLOCK_INFO(sb, binf); - u64 recent = binf->lru_nr >> 3; - scoutfs_inc_counter(sb, block_cache_access); - - if (bp->lru_moved < (binf->lru_move_counter - recent)) { - spin_lock(&binf->lock); - if (!list_empty(&bp->lru_entry)) { - list_move_tail(&bp->lru_entry, &binf->lru_list); - bp->lru_moved = ++binf->lru_move_counter; - scoutfs_inc_counter(sb, block_cache_lru_move); - } - spin_unlock(&binf->lock); + if (bp->accessed == 0 || bp->accessed < accessed_recently(binf)) { + scoutfs_inc_counter(sb, block_cache_access_update); + bp->accessed = atomic64_inc_return(&binf->access_counter); } } /* - * Remove a block from the cache and drop its reference. We only remove - * the block once as the deleted bit is first set. + * The caller wants to remove the block from the hash table and has an + * idea what the refcount should be. If the refcount does still + * indicate that the block is hashed, and we're able to clear that bit, + * then we can remove it from the hash table. + * + * The caller makes sure that it's safe to be referencing this block, + * either with their own held reference (most everything) or by being in + * an rcu grace period (shrink). + */ +static bool block_remove_cnt(struct super_block *sb, struct block_private *bp, int cnt) +{ + DECLARE_BLOCK_INFO(sb, binf); + int ret; + + if ((cnt & BLOCK_REF_INSERTED) && + (atomic_cmpxchg(&bp->refcount, cnt, cnt & ~BLOCK_REF_INSERTED) == cnt)) { + + TRACE_BLOCK(remove, bp); + ret = rhashtable_remove_fast(&binf->ht, &bp->ht_head, block_ht_params); + WARN_ON_ONCE(ret); /* must have been inserted */ + atomic_dec(&binf->total_inserted); + return true; + } + + return false; +} + +/* + * Try to remove the block from the hash table as long as the refcount + * indicates that it is still in the hash table. This can be racing + * with normal refcount changes so it might have to retry. */ static void block_remove(struct super_block *sb, struct block_private *bp) { - DECLARE_BLOCK_INFO(sb, binf); + int cnt; - assert_spin_locked(&binf->lock); + do { + cnt = atomic_read(&bp->refcount); + } while ((cnt & BLOCK_REF_INSERTED) && !block_remove_cnt(sb, bp, cnt)); +} - if (!test_and_set_bit(BLOCK_BIT_DELETED, &bp->bits)) { - BUG_ON(list_empty(&bp->lru_entry)); - radix_tree_delete(&binf->radix, bp->bl.blkno); - list_del_init(&bp->lru_entry); - binf->lru_nr--; - block_put(sb, bp); - } +/* + * Take one shot at removing the block from the hash table if it's still + * in the hash table and the caller has the only other reference. + */ +static bool block_remove_solo(struct super_block *sb, struct block_private *bp) +{ + return block_remove_cnt(sb, bp, BLOCK_REF_INSERTED | 1); } static bool io_busy(struct block_private *bp) @@ -318,20 +400,29 @@ static bool io_busy(struct block_private *bp) static void block_remove_all(struct super_block *sb) { DECLARE_BLOCK_INFO(sb, binf); + struct rhashtable_iter iter; struct block_private *bp; - spin_lock(&binf->lock); + rhashtable_walk_enter(&binf->ht, &iter); + rhashtable_walk_start(&iter); - while (radix_tree_gang_lookup(&binf->radix, (void **)&bp, 0, 1) == 1) { - wait_event(binf->waitq, !io_busy(bp)); - block_remove(sb, bp); + for (;;) { + bp = rhashtable_walk_next(&iter); + if (bp == NULL) + break; + if (bp == ERR_PTR(-EAGAIN)) + continue; + + if (block_get_if_inserted(bp)) { + block_remove(sb, bp); + block_put(sb, bp); + } } - spin_unlock(&binf->lock); + rhashtable_walk_stop(&iter); + rhashtable_walk_exit(&iter); - WARN_ON_ONCE(!list_empty(&binf->lru_list)); - WARN_ON_ONCE(binf->lru_nr != 0); - WARN_ON_ONCE(binf->radix.rnode != NULL); + WARN_ON_ONCE(atomic_read(&binf->total_inserted) != 0); } /* @@ -402,7 +493,7 @@ static int block_submit_bio(struct super_block *sb, struct block_private *bp, /* don't let racing end_io during submission think block is complete */ atomic_inc(&bp->io_count); set_bit(BLOCK_BIT_IO_BUSY, &bp->bits); - atomic_inc(&bp->refcount); + block_get(bp); blk_start_plug(&plug); @@ -449,29 +540,33 @@ static int block_submit_bio(struct super_block *sb, struct block_private *bp, } /* - * Return a reference to a cached block in the system, allocating a new - * block if one isn't found in the radix. Its contents are undefined if - * it's newly allocated. + * Return a reference to a cached block found in the hash table. If one + * isn't found then we try and allocate and insert a new one. Its + * contents are undefined if it's newly allocated. + * + * Our hash table lookups during rcu can be racing with shrinking and + * removal from the hash table. We only atomically get a reference if + * the refcount indicates that the block is still present in the hash + * table. */ -static struct block_private *block_get(struct super_block *sb, u64 blkno) +static struct block_private *block_lookup_create(struct super_block *sb, + u64 blkno) { DECLARE_BLOCK_INFO(sb, binf); - struct block_private *found; struct block_private *bp; int ret; +restart: rcu_read_lock(); - bp = radix_tree_lookup(&binf->radix, blkno); + bp = rhashtable_lookup(&binf->ht, &blkno, block_ht_params); if (bp) - atomic_inc(&bp->refcount); + bp = block_get_if_inserted(bp); rcu_read_unlock(); /* drop failed reads that interrupted waiters abandoned */ if (bp && (test_bit(BLOCK_BIT_ERROR, &bp->bits) && !test_bit(BLOCK_BIT_DIRTY, &bp->bits))) { - spin_lock(&binf->lock); block_remove(sb, bp); - spin_unlock(&binf->lock); block_put(sb, bp); bp = NULL; } @@ -483,24 +578,13 @@ static struct block_private *block_get(struct super_block *sb, u64 blkno) goto out; } - ret = radix_tree_preload(GFP_NOFS); - if (ret) + ret = block_insert(sb, bp); + if (ret < 0) { + if (ret == -EEXIST) { + block_put(sb, bp); + goto restart; + } goto out; - - /* could use slot instead of lookup/insert */ - spin_lock(&binf->lock); - found = radix_tree_lookup(&binf->radix, blkno); - if (found) { - atomic_inc(&found->refcount); - } else { - block_insert(sb, bp, blkno); - } - spin_unlock(&binf->lock); - radix_tree_preload_end(); - - if (found) { - block_put(sb, bp); - bp = found; } } @@ -524,7 +608,7 @@ struct scoutfs_block *scoutfs_block_create(struct super_block *sb, u64 blkno) { struct block_private *bp; - bp = block_get(sb, blkno); + bp = block_lookup_create(sb, blkno); if (IS_ERR(bp)) return ERR_CAST(bp); @@ -547,7 +631,7 @@ struct scoutfs_block *scoutfs_block_read(struct super_block *sb, u64 blkno) struct block_private *bp = NULL; int ret; - bp = block_get(sb, blkno); + bp = block_lookup_create(sb, blkno); if (IS_ERR(bp)) { ret = PTR_ERR(bp); goto out; @@ -580,14 +664,11 @@ out: */ void scoutfs_block_invalidate(struct super_block *sb, struct scoutfs_block *bl) { - DECLARE_BLOCK_INFO(sb, binf); struct block_private *bp = BLOCK_PRIVATE(bl); if (!WARN_ON_ONCE(test_bit(BLOCK_BIT_DIRTY, &bp->bits))) { scoutfs_inc_counter(sb, block_cache_invalidate); - spin_lock(&binf->lock); block_remove(sb, bp); - spin_unlock(&binf->lock); TRACE_BLOCK(invalidate, bp); } } @@ -642,12 +723,11 @@ void scoutfs_block_writer_mark_dirty(struct super_block *sb, if (!test_and_set_bit(BLOCK_BIT_DIRTY, &bp->bits)) { BUG_ON(!list_empty(&bp->dirty_entry)); - atomic_inc(&bp->refcount); + block_get(bp); spin_lock(&wri->lock); list_add_tail(&bp->dirty_entry, &wri->dirty_list); wri->nr_dirty_blocks++; spin_unlock(&wri->lock); - TRACE_BLOCK(mark_dirty, bp); } } @@ -792,53 +872,94 @@ u64 scoutfs_block_writer_dirty_bytes(struct super_block *sb, } /* - * Remove a number of least recently accessed blocks and free them. We - * don't take locking hit of removing blocks from the lru as they're - * used so this is racing with accesses holding an elevated refcount. - * We check the refcount to attempt to not free a block that snuck in - * and is being accessed while the block is still at the head of the - * LRU. + * Remove a number of cached blocks that haven't been used recently. * - * Dirty blocks will always have an elevated refcount (and will be - * likely be towards the tail of the LRU). Even if we do remove them - * from the LRU their dirty refcount will keep them live until IO - * completes and their dirty refcount is dropped. + * We don't maintain a strictly ordered LRU to avoid the contention of + * accesses always moving blocks around in some precise global + * structure. + * + * Instead we use counters to divide the blocks into two roughly equal + * groups by how recently they were accessed. We randomly walk all + * inserted blocks looking for any blocks in the older half to remove + * and free. The random walk and there being two groups means that we + * typically only walk a small multiple of the number we're looking for + * before we find them all. + * + * Our rcu walk of blocks can see blocks in all stages of their life + * cycle, from dirty blocks to those with 0 references that are queued + * for freeing. We only want to free idle inserted blocks so we + * atomically remove blocks when the only references are ours and the + * hash table. */ static int block_shrink(struct shrinker *shrink, struct shrink_control *sc) { struct block_info *binf = container_of(shrink, struct block_info, shrinker); struct super_block *sb = binf->sb; - struct block_private *tmp; + struct rhashtable_iter iter; struct block_private *bp; unsigned long nr; - LIST_HEAD(list); + u64 recently; nr = sc->nr_to_scan; - if (!nr) + if (nr == 0) goto out; - spin_lock(&binf->lock); + scoutfs_inc_counter(sb, block_cache_shrink); - list_for_each_entry_safe(bp, tmp, &binf->lru_list, lru_entry) { + nr = DIV_ROUND_UP(nr, SCOUTFS_BLOCK_LG_PAGES_PER); - if (atomic_read(&bp->refcount) > 1) - continue; +restart: + recently = accessed_recently(binf); + rhashtable_walk_enter(&binf->ht, &iter); + rhashtable_walk_start(&iter); - if (nr-- == 0) + /* + * This isn't great but I don't see a better way. We want to + * walk the hash from a random point so that we're not + * constantly walking over the same region that we've already + * freed old blocks within. The interface doesn't let us do + * this explicitly, but this seems to work? The difference this + * makes is enormous, around a few orders of magnitude fewer + * _nexts per shrink. + */ + if (iter.walker.tbl) + iter.slot = prandom_u32_max(iter.walker.tbl->size); + + while (nr > 0) { + bp = rhashtable_walk_next(&iter); + if (bp == NULL) break; + if (bp == ERR_PTR(-EAGAIN)) { + /* hard reset to not hold rcu grace period across retries */ + rhashtable_walk_stop(&iter); + rhashtable_walk_exit(&iter); + scoutfs_inc_counter(sb, block_cache_shrink_restart); + goto restart; + } - TRACE_BLOCK(shrink, bp); + scoutfs_inc_counter(sb, block_cache_shrink_next); - scoutfs_inc_counter(sb, block_cache_shrink); - block_remove(sb, bp); + if (bp->accessed >= recently) { + scoutfs_inc_counter(sb, block_cache_shrink_recent); + continue; + } + if (block_get_if_inserted(bp)) { + if (block_remove_solo(sb, bp)) { + scoutfs_inc_counter(sb, block_cache_shrink_remove); + TRACE_BLOCK(shrink, bp); + nr--; + } + block_put(sb, bp); + } } - spin_unlock(&binf->lock); - + rhashtable_walk_stop(&iter); + rhashtable_walk_exit(&iter); out: - return min_t(u64, binf->lru_nr * SCOUTFS_BLOCK_LG_PAGES_PER, INT_MAX); + return min_t(u64, (u64)atomic_read(&binf->total_inserted) * SCOUTFS_BLOCK_LG_PAGES_PER, + INT_MAX); } struct sm_block_completion { @@ -945,27 +1066,23 @@ int scoutfs_block_setup(struct super_block *sb) { struct scoutfs_sb_info *sbi = SCOUTFS_SB(sb); struct block_info *binf; - loff_t size; int ret; - /* we store blknos in longs in the radix */ - size = i_size_read(sb->s_bdev->bd_inode); - if ((size >> SCOUTFS_BLOCK_LG_SHIFT) >= LONG_MAX) { - scoutfs_err(sb, "Cant reference all blocks in %llu byte device with %u bit long radix tree indexes", - size, BITS_PER_LONG); - return -EINVAL; - } - binf = kzalloc(sizeof(struct block_info), GFP_KERNEL); if (!binf) { ret = -ENOMEM; goto out; } + ret = rhashtable_init(&binf->ht, &block_ht_params); + if (ret < 0) { + kfree(binf); + goto out; + } + binf->sb = sb; - spin_lock_init(&binf->lock); - INIT_RADIX_TREE(&binf->radix, GFP_ATOMIC); /* insertion preloads */ - INIT_LIST_HEAD(&binf->lru_list); + atomic_set(&binf->total_inserted, 0); + atomic64_set(&binf->access_counter, 0); init_waitqueue_head(&binf->waitq); binf->shrinker.shrink = block_shrink; binf->shrinker.seeks = DEFAULT_SEEKS; @@ -992,10 +1109,9 @@ void scoutfs_block_destroy(struct super_block *sb) unregister_shrinker(&binf->shrinker); block_remove_all(sb); flush_work(&binf->free_work); + rhashtable_destroy(&binf->ht); - WARN_ON_ONCE(!llist_empty(&binf->free_llist)); kfree(binf); - sbi->block_info = NULL; } } diff --git a/kmod/src/counters.h b/kmod/src/counters.h index f6aa6b3b..37d08191 100644 --- a/kmod/src/counters.h +++ b/kmod/src/counters.h @@ -21,16 +21,20 @@ EXPAND_COUNTER(alloc_move) \ EXPAND_COUNTER(alloc_moved_extent) \ EXPAND_COUNTER(alloc_stale_cached_list_block) \ - EXPAND_COUNTER(block_cache_access) \ + EXPAND_COUNTER(block_cache_access_update) \ EXPAND_COUNTER(block_cache_alloc_failure) \ EXPAND_COUNTER(block_cache_alloc_page_order) \ EXPAND_COUNTER(block_cache_alloc_virt) \ EXPAND_COUNTER(block_cache_end_io_error) \ EXPAND_COUNTER(block_cache_forget) \ EXPAND_COUNTER(block_cache_free) \ + EXPAND_COUNTER(block_cache_free_work) \ EXPAND_COUNTER(block_cache_invalidate) \ - EXPAND_COUNTER(block_cache_lru_move) \ EXPAND_COUNTER(block_cache_shrink) \ + EXPAND_COUNTER(block_cache_shrink_next) \ + EXPAND_COUNTER(block_cache_shrink_recent) \ + EXPAND_COUNTER(block_cache_shrink_remove) \ + EXPAND_COUNTER(block_cache_shrink_restart) \ EXPAND_COUNTER(btree_compact_values) \ EXPAND_COUNTER(btree_compact_values_enomem) \ EXPAND_COUNTER(btree_delete) \ diff --git a/kmod/src/scoutfs_trace.h b/kmod/src/scoutfs_trace.h index d440a228..563f95e3 100644 --- a/kmod/src/scoutfs_trace.h +++ b/kmod/src/scoutfs_trace.h @@ -2057,17 +2057,17 @@ TRACE_EVENT(scoutfs_forest_init_our_log, ); DECLARE_EVENT_CLASS(scoutfs_block_class, - TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved), + TP_PROTO(struct super_block *sb, void *bp, u64 blkno, int refcount, int io_count, + unsigned long bits, __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed), TP_STRUCT__entry( SCSB_TRACE_FIELDS __field(void *, bp) __field(__u64, blkno) __field(int, refcount) __field(int, io_count) - __field(unsigned long, bits) - __field(__u64, lru_moved) + __field(long, bits) + __field(__u64, accessed) ), TP_fast_assign( SCSB_TRACE_ASSIGN(sb); @@ -2076,57 +2076,71 @@ DECLARE_EVENT_CLASS(scoutfs_block_class, __entry->refcount = refcount; __entry->io_count = io_count; __entry->bits = bits; - __entry->lru_moved = lru_moved; + __entry->accessed = accessed; ), - TP_printk(SCSBF" bp %p blkno %llu refcount %d io_count %d bits 0x%lx lru_moved %llu", - SCSB_TRACE_ARGS, __entry->bp, __entry->blkno, - __entry->refcount, __entry->io_count, __entry->bits, - __entry->lru_moved) + TP_printk(SCSBF" bp %p blkno %llu refcount %d io_count %d bits 0x%lx accessed %llu", + SCSB_TRACE_ARGS, __entry->bp, __entry->blkno, __entry->refcount, + __entry->io_count, __entry->bits, __entry->accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_allocate, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_free, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_insert, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) +); +DEFINE_EVENT(scoutfs_block_class, scoutfs_block_remove, + TP_PROTO(struct super_block *sb, void *bp, u64 blkno, + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_end_io, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_submit, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_invalidate, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_mark_dirty, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_forget, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DEFINE_EVENT(scoutfs_block_class, scoutfs_block_shrink, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, - int refcount, int io_count, unsigned long bits, u64 lru_moved), - TP_ARGS(sb, bp, blkno, refcount, io_count, bits, lru_moved) + int refcount, int io_count, unsigned long bits, + __u64 accessed), + TP_ARGS(sb, bp, blkno, refcount, io_count, bits, accessed) ); DECLARE_EVENT_CLASS(scoutfs_ext_next_class, From 0969a94bfc8dcd2ab9a969aed75617c6d63efdda Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Sun, 21 Feb 2021 10:49:07 -0800 Subject: [PATCH 2/7] Check one block_ref struct in block core Each of the different block types had a reading function that read a block and then checked their reference struct for their block type. This gets rid of each block reference type and has a single block_ref type which is then checked by a single ref reading function in the block core. By putting ref checking in the core we no longer have to export checking the block header crc, verifying headers, invalidating blocks, or even reading raw blocks themseves. Everyone reads refs and leaves the checking up to the core. The changes don't have a significant functional effect. This is mostly just changing types and moving code around. (There are some changes to visible counters.) This shares code, which is nice, but this is putting the block reference checking in one place in the block core so that in a few patches we can fix problems with writers dirtying blocks that are being read. Signed-off-by: Zach Brown --- kmod/src/alloc.c | 43 +++++++-------- kmod/src/block.c | 112 ++++++++++++++++++++------------------- kmod/src/block.h | 13 +---- kmod/src/btree.c | 53 +++++------------- kmod/src/counters.h | 7 ++- kmod/src/forest.c | 24 ++++----- kmod/src/format.h | 38 ++++++------- kmod/src/scoutfs_trace.h | 8 +-- kmod/src/srch.c | 38 +++---------- 9 files changed, 129 insertions(+), 207 deletions(-) diff --git a/kmod/src/alloc.c b/kmod/src/alloc.c index 530e261f..53965eec 100644 --- a/kmod/src/alloc.c +++ b/kmod/src/alloc.c @@ -358,31 +358,24 @@ static void list_block_sort(struct scoutfs_alloc_list_block *lblk) /* * We're always reading blocks that we own, so we shouldn't see stale - * references. But the cached block can be stale and we can need to - * invalidate it. + * references but we could retry reads after dropping stale cached + * blocks. If we do see a stale error then we've hit persistent + * corruption. */ -static int read_list_block(struct super_block *sb, - struct scoutfs_alloc_list_ref *ref, +static int read_list_block(struct super_block *sb, struct scoutfs_block_ref *ref, struct scoutfs_block **bl_ret) { - struct scoutfs_block *bl = NULL; + int ret; - bl = scoutfs_block_read(sb, le64_to_cpu(ref->blkno)); - if (!IS_ERR_OR_NULL(bl) && - !scoutfs_block_consistent_ref(sb, bl, ref->seq, ref->blkno, - SCOUTFS_BLOCK_MAGIC_ALLOC_LIST)) { - scoutfs_inc_counter(sb, alloc_stale_cached_list_block); - scoutfs_block_invalidate(sb, bl); - scoutfs_block_put(sb, bl); - bl = scoutfs_block_read(sb, le64_to_cpu(ref->blkno)); - } - if (IS_ERR(bl)) { - *bl_ret = NULL; - return PTR_ERR(bl); - } + ret = scoutfs_block_read_ref(sb, ref, SCOUTFS_BLOCK_MAGIC_ALLOC_LIST, bl_ret); + if (ret < 0) { + if (ret == -ESTALE) { + scoutfs_inc_counter(sb, alloc_stale_list_block); + ret = -EIO; + } + }; - *bl_ret = bl; - return 0; + return ret; } /* @@ -396,7 +389,7 @@ static int read_list_block(struct super_block *sb, static int dirty_list_block(struct super_block *sb, struct scoutfs_alloc *alloc, struct scoutfs_block_writer *wri, - struct scoutfs_alloc_list_ref *ref, + struct scoutfs_block_ref *ref, u64 dirty, u64 *old, struct scoutfs_block **bl_ret) { @@ -497,7 +490,7 @@ static int dirty_alloc_blocks(struct super_block *sb, struct scoutfs_alloc *alloc, struct scoutfs_block_writer *wri) { - struct scoutfs_alloc_list_ref orig_freed; + struct scoutfs_block_ref orig_freed; struct scoutfs_alloc_list_block *lblk; struct scoutfs_block *av_bl = NULL; struct scoutfs_block *fr_bl = NULL; @@ -1106,7 +1099,7 @@ int scoutfs_alloc_splice_list(struct super_block *sb, struct scoutfs_alloc_list_head *src) { struct scoutfs_alloc_list_block *lblk; - struct scoutfs_alloc_list_ref *ref; + struct scoutfs_block_ref *ref; struct scoutfs_block *prev = NULL; struct scoutfs_block *bl = NULL; int ret = 0; @@ -1169,8 +1162,8 @@ bool scoutfs_alloc_meta_low(struct super_block *sb, int scoutfs_alloc_foreach(struct super_block *sb, scoutfs_alloc_foreach_cb_t cb, void *arg) { - struct scoutfs_btree_ref stale_refs[2] = {{0,}}; - struct scoutfs_btree_ref refs[2] = {{0,}}; + struct scoutfs_block_ref stale_refs[2] = {{0,}}; + struct scoutfs_block_ref refs[2] = {{0,}}; struct scoutfs_super_block *super = NULL; struct scoutfs_srch_compact *sc; struct scoutfs_log_trees lt; diff --git a/kmod/src/block.c b/kmod/src/block.c index 1c2e4ae8..8e0ed42f 100644 --- a/kmod/src/block.c +++ b/kmod/src/block.c @@ -114,12 +114,7 @@ do { \ #define BLOCK_PRIVATE(_bl) \ container_of((_bl), struct block_private, bl) -/* - * These _block_header helpers are from a previous generation and may - * be refactored away. - */ - -__le32 scoutfs_block_calc_crc(struct scoutfs_block_header *hdr, u32 size) +static __le32 block_calc_crc(struct scoutfs_block_header *hdr, u32 size) { int off = offsetof(struct scoutfs_block_header, crc) + FIELD_SIZEOF(struct scoutfs_block_header, crc); @@ -128,21 +123,6 @@ __le32 scoutfs_block_calc_crc(struct scoutfs_block_header *hdr, u32 size) return cpu_to_le32(calc); } -bool scoutfs_block_valid_crc(struct scoutfs_block_header *hdr, u32 size) -{ - return hdr->crc == scoutfs_block_calc_crc(hdr, size); -} - -bool scoutfs_block_valid_ref(struct super_block *sb, - struct scoutfs_block_header *hdr, - __le64 seq, __le64 blkno) -{ - struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; - - return hdr->fsid == super->hdr.fsid && hdr->seq == seq && - hdr->blkno == blkno; -} - static struct block_private *block_alloc(struct super_block *sb, u64 blkno) { struct block_private *bp; @@ -625,7 +605,7 @@ static bool uptodate_or_error(struct block_private *bp) test_bit(BLOCK_BIT_ERROR, &bp->bits); } -struct scoutfs_block *scoutfs_block_read(struct super_block *sb, u64 blkno) +static struct block_private *block_read(struct super_block *sb, u64 blkno) { DECLARE_BLOCK_INFO(sb, binf); struct block_private *bp = NULL; @@ -654,45 +634,69 @@ out: return ERR_PTR(ret); } - return &bp->bl; + return bp; } /* - * Drop a stale cached read block from the cache. A future read will - * re-read the block from the device. This doesn't drop the caller's reference, - * they still have to call _put. + * Btree blocks don't have rigid cache consistency. We can be following + * block references into cached blocks that are now stale or can be + * following a stale root into blocks that have been overwritten. If we + * hit a block that looks stale we first invalidate the cache and retry, + * returning -ESTALE if it still looks wrong. The caller can retry the + * read from a more current root or decide that this is a persistent + * error. */ -void scoutfs_block_invalidate(struct super_block *sb, struct scoutfs_block *bl) -{ - struct block_private *bp = BLOCK_PRIVATE(bl); - - if (!WARN_ON_ONCE(test_bit(BLOCK_BIT_DIRTY, &bp->bits))) { - scoutfs_inc_counter(sb, block_cache_invalidate); - block_remove(sb, bp); - TRACE_BLOCK(invalidate, bp); - } -} - -/* This is only used for large metadata blocks */ -bool scoutfs_block_consistent_ref(struct super_block *sb, - struct scoutfs_block *bl, - __le64 seq, __le64 blkno, u32 magic) +int scoutfs_block_read_ref(struct super_block *sb, struct scoutfs_block_ref *ref, u32 magic, + struct scoutfs_block **bl_ret) { struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; - struct block_private *bp = BLOCK_PRIVATE(bl); - struct scoutfs_block_header *hdr = bl->data; + struct scoutfs_block_header *hdr; + struct block_private *bp = NULL; + bool retried = false; + int ret; +retry: + bp = block_read(sb, le64_to_cpu(ref->blkno)); + if (IS_ERR(bp)) { + ret = PTR_ERR(bp); + goto out; + } + hdr = bp->bl.data; + + /* corrupted writes might be a sign of a stale reference */ if (!test_bit(BLOCK_BIT_CRC_VALID, &bp->bits)) { - if (hdr->crc != - scoutfs_block_calc_crc(hdr, SCOUTFS_BLOCK_LG_SIZE)) - return false; + if (hdr->crc != block_calc_crc(hdr, SCOUTFS_BLOCK_LG_SIZE)) { + ret = -ESTALE; + goto out; + } + set_bit(BLOCK_BIT_CRC_VALID, &bp->bits); } - return hdr->magic == cpu_to_le32(magic) && - hdr->fsid == super->hdr.fsid && - hdr->seq == seq && - hdr->blkno == blkno; + if (hdr->magic != cpu_to_le32(magic) || hdr->fsid != super->hdr.fsid || + hdr->seq != ref->seq || hdr->blkno != ref->blkno) { + ret = -ESTALE; + goto out; + } + + ret = 0; +out: + if (ret == -ESTALE && !retried) { + retried = true; + scoutfs_inc_counter(sb, block_cache_remove_stale); + block_remove(sb, bp); + block_put(sb, bp); + bp = NULL; + goto retry; + } + + if (ret < 0) { + block_put(sb, bp); + bp = NULL; + } + + *bl_ret = bp ? &bp->bl : NULL; + return ret; } void scoutfs_block_put(struct super_block *sb, struct scoutfs_block *bl) @@ -762,7 +766,7 @@ int scoutfs_block_writer_write(struct super_block *sb, /* checksum everything to reduce time between io submission merging */ list_for_each_entry(bp, &wri->dirty_list, dirty_entry) { hdr = bp->bl.data; - hdr->crc = scoutfs_block_calc_crc(hdr, SCOUTFS_BLOCK_LG_SIZE); + hdr->crc = block_calc_crc(hdr, SCOUTFS_BLOCK_LG_SIZE); } blk_start_plug(&plug); @@ -1013,8 +1017,7 @@ static int sm_block_io(struct block_device *bdev, int rw, u64 blkno, if (len < SCOUTFS_BLOCK_SM_SIZE) memset((char *)pg_hdr + len, 0, SCOUTFS_BLOCK_SM_SIZE - len); - pg_hdr->crc = scoutfs_block_calc_crc(pg_hdr, - SCOUTFS_BLOCK_SM_SIZE); + pg_hdr->crc = block_calc_crc(pg_hdr, SCOUTFS_BLOCK_SM_SIZE); } bio = bio_alloc(GFP_NOFS, 1); @@ -1039,8 +1042,7 @@ static int sm_block_io(struct block_device *bdev, int rw, u64 blkno, if (ret == 0 && !(rw & WRITE)) { memcpy(hdr, pg_hdr, len); - *blk_crc = scoutfs_block_calc_crc(pg_hdr, - SCOUTFS_BLOCK_SM_SIZE); + *blk_crc = block_calc_crc(pg_hdr, SCOUTFS_BLOCK_SM_SIZE); } out: __free_page(page); diff --git a/kmod/src/block.h b/kmod/src/block.h index 79a859d7..7276a61c 100644 --- a/kmod/src/block.h +++ b/kmod/src/block.h @@ -13,18 +13,9 @@ struct scoutfs_block { void *priv; }; -__le32 scoutfs_block_calc_crc(struct scoutfs_block_header *hdr, u32 size); -bool scoutfs_block_valid_crc(struct scoutfs_block_header *hdr, u32 size); -bool scoutfs_block_valid_ref(struct super_block *sb, - struct scoutfs_block_header *hdr, - __le64 seq, __le64 blkno); - struct scoutfs_block *scoutfs_block_create(struct super_block *sb, u64 blkno); -struct scoutfs_block *scoutfs_block_read(struct super_block *sb, u64 blkno); -void scoutfs_block_invalidate(struct super_block *sb, struct scoutfs_block *bl); -bool scoutfs_block_consistent_ref(struct super_block *sb, - struct scoutfs_block *bl, - __le64 seq, __le64 blkno, u32 magic); +int scoutfs_block_read_ref(struct super_block *sb, struct scoutfs_block_ref *ref, u32 magic, + struct scoutfs_block **bl_ret); void scoutfs_block_put(struct super_block *sb, struct scoutfs_block *bl); void scoutfs_block_writer_init(struct super_block *sb, diff --git a/kmod/src/btree.c b/kmod/src/btree.c index d4eee1cc..78643961 100644 --- a/kmod/src/btree.c +++ b/kmod/src/btree.c @@ -619,18 +619,13 @@ static void move_items(struct scoutfs_btree_block *dst, * This is used to lookup cached blocks, read blocks, cow blocks for * dirtying, and allocate new blocks. * - * Btree blocks don't have rigid cache consistency. We can be following - * block references into cached blocks that are now stale or can be - * following a stale root into blocks that have been overwritten. If we - * hit a block that looks stale we first invalidate the cache and retry, - * returning -ESTALE if it still looks wrong. The caller can retry the - * read from a more current root or decide that this is a persistent - * error. + * If we read a stale block we return stale so the caller can retry with + * a newer root or return an error. */ static int get_ref_block(struct super_block *sb, struct scoutfs_alloc *alloc, struct scoutfs_block_writer *wri, int flags, - struct scoutfs_btree_ref *ref, + struct scoutfs_block_ref *ref, struct scoutfs_block **bl_ret) { struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; @@ -638,40 +633,16 @@ static int get_ref_block(struct super_block *sb, struct scoutfs_btree_block *new; struct scoutfs_block *new_bl = NULL; struct scoutfs_block *bl = NULL; - bool retried = false; u64 blkno; u64 seq; int ret; /* always get the current block, either to return or cow from */ if (ref && ref->blkno) { -retry: - - bl = scoutfs_block_read(sb, le64_to_cpu(ref->blkno)); - if (IS_ERR(bl)) { - trace_scoutfs_btree_read_error(sb, ref); - scoutfs_inc_counter(sb, btree_read_error); - ret = PTR_ERR(bl); - goto out; - } - bt = (void *)bl->data; - - if (!scoutfs_block_consistent_ref(sb, bl, ref->seq, ref->blkno, - SCOUTFS_BLOCK_MAGIC_BTREE) || - scoutfs_trigger(sb, BTREE_STALE_READ)) { - - scoutfs_inc_counter(sb, btree_stale_read); - - scoutfs_block_invalidate(sb, bl); - scoutfs_block_put(sb, bl); - bl = NULL; - - if (!retried) { - retried = true; - goto retry; - } - - ret = -ESTALE; + ret = scoutfs_block_read_ref(sb, ref, SCOUTFS_BLOCK_MAGIC_BTREE, &bl); + if (ret < 0) { + if (ret == -ESTALE) + scoutfs_inc_counter(sb, btree_stale_read); goto out; } @@ -766,7 +737,7 @@ static void create_parent_item(struct scoutfs_btree_block *parent, { struct scoutfs_avl_node *par; int cmp; - struct scoutfs_btree_ref ref = { + struct scoutfs_block_ref ref = { .blkno = child->hdr.blkno, .seq = child->hdr.seq, }; @@ -784,7 +755,7 @@ static void update_parent_item(struct scoutfs_btree_block *parent, struct scoutfs_btree_item *par_item, struct scoutfs_btree_block *child) { - struct scoutfs_btree_ref *ref = item_val(parent, par_item); + struct scoutfs_block_ref *ref = item_val(parent, par_item); par_item->key = *item_key(last_item(child)); ref->blkno = child->hdr.blkno; @@ -837,7 +808,7 @@ static int try_split(struct super_block *sb, /* parents need to leave room for child references */ if (right->level) - val_len = sizeof(struct scoutfs_btree_ref); + val_len = sizeof(struct scoutfs_block_ref); /* don't need to split if there's enough space for the item */ if (mid_free_item_room(right, val_len)) @@ -905,7 +876,7 @@ static int try_join(struct super_block *sb, struct scoutfs_btree_item *sib_par_item; struct scoutfs_btree_block *sib; struct scoutfs_block *sib_bl; - struct scoutfs_btree_ref *ref; + struct scoutfs_block_ref *ref; unsigned int sib_tot; bool move_right; int to_move; @@ -1194,7 +1165,7 @@ static int btree_walk(struct super_block *sb, struct scoutfs_btree_item *prev; struct scoutfs_avl_node *next_node; struct scoutfs_avl_node *node; - struct scoutfs_btree_ref *ref; + struct scoutfs_block_ref *ref; unsigned int level; unsigned int nr; int ret; diff --git a/kmod/src/counters.h b/kmod/src/counters.h index 37d08191..7cb5a331 100644 --- a/kmod/src/counters.h +++ b/kmod/src/counters.h @@ -20,7 +20,7 @@ EXPAND_COUNTER(alloc_list_freed_hi) \ EXPAND_COUNTER(alloc_move) \ EXPAND_COUNTER(alloc_moved_extent) \ - EXPAND_COUNTER(alloc_stale_cached_list_block) \ + EXPAND_COUNTER(alloc_stale_list_block) \ EXPAND_COUNTER(block_cache_access_update) \ EXPAND_COUNTER(block_cache_alloc_failure) \ EXPAND_COUNTER(block_cache_alloc_page_order) \ @@ -29,7 +29,7 @@ EXPAND_COUNTER(block_cache_forget) \ EXPAND_COUNTER(block_cache_free) \ EXPAND_COUNTER(block_cache_free_work) \ - EXPAND_COUNTER(block_cache_invalidate) \ + EXPAND_COUNTER(block_cache_remove_stale) \ EXPAND_COUNTER(block_cache_shrink) \ EXPAND_COUNTER(block_cache_shrink_next) \ EXPAND_COUNTER(block_cache_shrink_recent) \ @@ -46,7 +46,6 @@ EXPAND_COUNTER(btree_lookup) \ EXPAND_COUNTER(btree_next) \ EXPAND_COUNTER(btree_prev) \ - EXPAND_COUNTER(btree_read_error) \ EXPAND_COUNTER(btree_split) \ EXPAND_COUNTER(btree_stale_read) \ EXPAND_COUNTER(btree_update) \ @@ -77,6 +76,7 @@ EXPAND_COUNTER(ext_op_remove) \ EXPAND_COUNTER(forest_bloom_fail) \ EXPAND_COUNTER(forest_bloom_pass) \ + EXPAND_COUNTER(forest_bloom_stale) \ EXPAND_COUNTER(forest_read_items) \ EXPAND_COUNTER(forest_roots_next_hint) \ EXPAND_COUNTER(forest_set_bloom_bits) \ @@ -167,7 +167,6 @@ EXPAND_COUNTER(srch_compact_flush) \ EXPAND_COUNTER(srch_compact_log_page) \ EXPAND_COUNTER(srch_compact_removed_entry) \ - EXPAND_COUNTER(srch_inconsistent_ref) \ EXPAND_COUNTER(srch_rotate_log) \ EXPAND_COUNTER(srch_search_log) \ EXPAND_COUNTER(srch_search_log_block) \ diff --git a/kmod/src/forest.c b/kmod/src/forest.c index f5f259c0..a832b2dd 100644 --- a/kmod/src/forest.c +++ b/kmod/src/forest.c @@ -66,8 +66,8 @@ struct forest_info { struct forest_info *name = SCOUTFS_SB(sb)->forest_info struct forest_refs { - struct scoutfs_btree_ref fs_ref; - struct scoutfs_btree_ref logs_ref; + struct scoutfs_block_ref fs_ref; + struct scoutfs_block_ref logs_ref; }; /* initialize some refs that initially aren't equal */ @@ -96,20 +96,16 @@ static void calc_bloom_nrs(struct forest_bloom_nrs *bloom, } } -static struct scoutfs_block *read_bloom_ref(struct super_block *sb, - struct scoutfs_btree_ref *ref) +static struct scoutfs_block *read_bloom_ref(struct super_block *sb, struct scoutfs_block_ref *ref) { struct scoutfs_block *bl; + int ret; - bl = scoutfs_block_read(sb, le64_to_cpu(ref->blkno)); - if (IS_ERR(bl)) - return bl; - - if (!scoutfs_block_consistent_ref(sb, bl, ref->seq, ref->blkno, - SCOUTFS_BLOCK_MAGIC_BLOOM)) { - scoutfs_block_invalidate(sb, bl); - scoutfs_block_put(sb, bl); - return ERR_PTR(-ESTALE); + ret = scoutfs_block_read_ref(sb, ref, SCOUTFS_BLOCK_MAGIC_BLOOM, &bl); + if (ret < 0) { + if (ret == -ESTALE) + scoutfs_inc_counter(sb, forest_bloom_stale); + bl = ERR_PTR(ret); } return bl; @@ -386,7 +382,7 @@ int scoutfs_forest_set_bloom_bits(struct super_block *sb, struct scoutfs_block *new_bl = NULL; struct scoutfs_block *bl = NULL; struct scoutfs_bloom_block *bb; - struct scoutfs_btree_ref *ref; + struct scoutfs_block_ref *ref; struct forest_bloom_nrs bloom; int nr_set = 0; u64 blkno; diff --git a/kmod/src/format.h b/kmod/src/format.h index 9578586f..28a15175 100644 --- a/kmod/src/format.h +++ b/kmod/src/format.h @@ -106,6 +106,15 @@ struct scoutfs_block_header { __le64 blkno; }; +/* + * A reference to a block. The corresponding fields in the block_header + * must match after having read the block contents. + */ +struct scoutfs_block_ref { + __le64 blkno; + __le64 seq; +}; + /* * scoutfs identifies all file system metadata items by a small key * struct. @@ -202,17 +211,12 @@ struct scoutfs_avl_node { */ #define SCOUTFS_BTREE_MAX_HEIGHT 20 -struct scoutfs_btree_ref { - __le64 blkno; - __le64 seq; -}; - /* * A height of X means that the first block read will have level X-1 and * the leaves will have level 0. */ struct scoutfs_btree_root { - struct scoutfs_btree_ref ref; + struct scoutfs_block_ref ref; __u8 height; __u8 __pad[7]; }; @@ -253,18 +257,13 @@ struct scoutfs_btree_block { #define SCOUTFS_BTREE_LEAF_ITEM_HASH_BYTES \ (SCOUTFS_BTREE_LEAF_ITEM_HASH_NR * sizeof(__le16)) -struct scoutfs_alloc_list_ref { - __le64 blkno; - __le64 seq; -}; - /* * first_nr tracks the nr of the first block in the list and is used for * allocation sizing. total_nr is the sum of the nr of all the blocks in * the list and is used for calculating total free block counts. */ struct scoutfs_alloc_list_head { - struct scoutfs_alloc_list_ref ref; + struct scoutfs_block_ref ref; __le64 total_nr; __le32 first_nr; __u8 __pad[4]; @@ -283,7 +282,7 @@ struct scoutfs_alloc_list_head { */ struct scoutfs_alloc_list_block { struct scoutfs_block_header hdr; - struct scoutfs_alloc_list_ref next; + struct scoutfs_block_ref next; __le32 start; __le32 nr; __le64 blknos[0]; /* naturally aligned for sorting */ @@ -329,15 +328,10 @@ struct scoutfs_srch_entry { #define SCOUTFS_SRCH_ENTRY_MAX_BYTES (2 + (sizeof(__u64) * 3)) -struct scoutfs_srch_ref { - __le64 blkno; - __le64 seq; -}; - struct scoutfs_srch_file { struct scoutfs_srch_entry first; struct scoutfs_srch_entry last; - struct scoutfs_srch_ref ref; + struct scoutfs_block_ref ref; __le64 blocks; __le64 entries; __u8 height; @@ -346,13 +340,13 @@ struct scoutfs_srch_file { struct scoutfs_srch_parent { struct scoutfs_block_header hdr; - struct scoutfs_srch_ref refs[0]; + struct scoutfs_block_ref refs[0]; }; #define SCOUTFS_SRCH_PARENT_REFS \ ((SCOUTFS_BLOCK_LG_SIZE - \ offsetof(struct scoutfs_srch_parent, refs)) / \ - sizeof(struct scoutfs_srch_ref)) + sizeof(struct scoutfs_block_ref)) struct scoutfs_srch_block { struct scoutfs_block_header hdr; @@ -423,7 +417,7 @@ struct scoutfs_log_trees { struct scoutfs_alloc_list_head meta_avail; struct scoutfs_alloc_list_head meta_freed; struct scoutfs_btree_root item_root; - struct scoutfs_btree_ref bloom_ref; + struct scoutfs_block_ref bloom_ref; struct scoutfs_alloc_root data_avail; struct scoutfs_alloc_root data_freed; struct scoutfs_srch_file srch_file; diff --git a/kmod/src/scoutfs_trace.h b/kmod/src/scoutfs_trace.h index 563f95e3..ecd2080f 100644 --- a/kmod/src/scoutfs_trace.h +++ b/kmod/src/scoutfs_trace.h @@ -1611,7 +1611,7 @@ TRACE_EVENT(scoutfs_get_name, ); TRACE_EVENT(scoutfs_btree_read_error, - TP_PROTO(struct super_block *sb, struct scoutfs_btree_ref *ref), + TP_PROTO(struct super_block *sb, struct scoutfs_block_ref *ref), TP_ARGS(sb, ref), @@ -1661,7 +1661,7 @@ TRACE_EVENT(scoutfs_btree_dirty_block, TRACE_EVENT(scoutfs_btree_walk, TP_PROTO(struct super_block *sb, struct scoutfs_btree_root *root, struct scoutfs_key *key, int flags, int level, - struct scoutfs_btree_ref *ref), + struct scoutfs_block_ref *ref), TP_ARGS(sb, root, key, flags, level, ref), @@ -1989,8 +1989,8 @@ DEFINE_EVENT(scoutfs_forest_bloom_class, scoutfs_forest_bloom_search, ); TRACE_EVENT(scoutfs_forest_prepare_commit, - TP_PROTO(struct super_block *sb, struct scoutfs_btree_ref *item_ref, - struct scoutfs_btree_ref *bloom_ref), + TP_PROTO(struct super_block *sb, struct scoutfs_block_ref *item_ref, + struct scoutfs_block_ref *bloom_ref), TP_ARGS(sb, item_ref, bloom_ref), TP_STRUCT__entry( SCSB_TRACE_FIELDS diff --git a/kmod/src/srch.c b/kmod/src/srch.c index 00c4f0fd..41cb11b1 100644 --- a/kmod/src/srch.c +++ b/kmod/src/srch.c @@ -284,39 +284,15 @@ static void init_file_block(struct super_block *sb, struct scoutfs_block *bl, */ static int read_srch_block(struct super_block *sb, struct scoutfs_block_writer *wri, int level, - struct scoutfs_srch_ref *ref, + struct scoutfs_block_ref *ref, struct scoutfs_block **bl_ret) { - struct scoutfs_block *bl; - int retries = 0; - int ret = 0; - int mag; + u32 magic = level ? SCOUTFS_BLOCK_MAGIC_SRCH_PARENT : SCOUTFS_BLOCK_MAGIC_SRCH_BLOCK; + int ret; - mag = level ? SCOUTFS_BLOCK_MAGIC_SRCH_PARENT : - SCOUTFS_BLOCK_MAGIC_SRCH_BLOCK; -retry: - bl = scoutfs_block_read(sb, le64_to_cpu(ref->blkno)); - if (!IS_ERR_OR_NULL(bl) && - !scoutfs_block_consistent_ref(sb, bl, ref->seq, ref->blkno, mag)) { - - scoutfs_inc_counter(sb, srch_inconsistent_ref); - scoutfs_block_writer_forget(sb, wri, bl); - scoutfs_block_invalidate(sb, bl); - scoutfs_block_put(sb, bl); - bl = NULL; - - if (retries++ == 0) - goto retry; - - bl = ERR_PTR(-ESTALE); + ret = scoutfs_block_read_ref(sb, ref, magic, bl_ret); + if (ret == -ESTALE) scoutfs_inc_counter(sb, srch_read_stale); - } - if (IS_ERR(bl)) { - ret = PTR_ERR(bl); - bl = NULL; - } - - *bl_ret = bl; return ret; } @@ -333,7 +309,7 @@ static int read_path_block(struct super_block *sb, { struct scoutfs_block *bl = NULL; struct scoutfs_srch_parent *srp; - struct scoutfs_srch_ref ref; + struct scoutfs_block_ref ref; int level; int ind; int ret; @@ -393,7 +369,7 @@ static int get_file_block(struct super_block *sb, struct scoutfs_block *bl = NULL; struct scoutfs_srch_parent *srp; struct scoutfs_block *new_bl; - struct scoutfs_srch_ref *ref; + struct scoutfs_block_ref *ref; u64 blkno = 0; int level; int ind; From f18fa0e97adc50647fb0d62661dce75a743af161 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Mon, 22 Feb 2021 13:40:20 -0800 Subject: [PATCH 3/7] Update scoutfs print for centralized block_ref Update scoutfs print to use the new block_ref struct instead of the handful of per-block type ref structs that we had accumulated. Signed-off-by: Zach Brown --- utils/src/print.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/utils/src/print.c b/utils/src/print.c index ce89c12c..084626f2 100644 --- a/utils/src/print.c +++ b/utils/src/print.c @@ -388,10 +388,10 @@ static int print_alloc_item(struct scoutfs_key *key, void *val, typedef int (*print_item_func)(struct scoutfs_key *key, void *val, unsigned val_len, void *arg); -static int print_btree_ref(struct scoutfs_key *key, void *val, +static int print_block_ref(struct scoutfs_key *key, void *val, unsigned val_len, print_item_func func, void *arg) { - struct scoutfs_btree_ref *ref = val; + struct scoutfs_block_ref *ref = val; func(key, NULL, 0, arg); printf(" ref blkno %llu seq %llu\n", @@ -433,7 +433,7 @@ static void print_leaf_item_hash(struct scoutfs_btree_block *bt) } static int print_btree_block(int fd, struct scoutfs_super_block *super, - char *which, struct scoutfs_btree_ref *ref, + char *which, struct scoutfs_block_ref *ref, print_item_func func, void *arg, u8 level) { struct scoutfs_btree_item *item; @@ -500,7 +500,7 @@ static int print_btree_block(int fd, struct scoutfs_super_block *super, val_len); if (level) - print_btree_ref(key, val, val_len, func, arg); + print_block_ref(key, val, val_len, func, arg); else func(key, val, val_len, arg); } @@ -531,11 +531,10 @@ static int print_btree(int fd, struct scoutfs_super_block *super, char *which, return ret; } -static int print_alloc_list_block(int fd, char *str, - struct scoutfs_alloc_list_ref *ref) +static int print_alloc_list_block(int fd, char *str, struct scoutfs_block_ref *ref) { struct scoutfs_alloc_list_block *lblk; - struct scoutfs_alloc_list_ref next; + struct scoutfs_block_ref next; u64 blkno; u64 start; u64 len; @@ -583,7 +582,7 @@ static int print_alloc_list_block(int fd, char *str, return print_alloc_list_block(fd, str, &next); } -static int print_srch_block(int fd, struct scoutfs_srch_ref *ref, int level) +static int print_srch_block(int fd, struct scoutfs_block_ref *ref, int level) { struct scoutfs_srch_parent *srp; struct scoutfs_srch_block *srb; @@ -729,7 +728,7 @@ static int print_srch_root_files(struct scoutfs_key *key, void *val, } static int print_btree_leaf_items(int fd, struct scoutfs_super_block *super, - struct scoutfs_btree_ref *ref, + struct scoutfs_block_ref *ref, print_item_func func, void *arg) { struct scoutfs_btree_item *item; From 6237f0adc5d32884b8b5ca21092286b810725932 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Sun, 21 Feb 2021 11:26:11 -0800 Subject: [PATCH 4/7] Add _block_dirty_ref to dirty blocks in one place To create dirty blocks in memory each block type caller currently gets a reference on a created block and then dirties it. The reference it gets could be an existing cached block that stale readers are currently using. This creates a problem with our block consistency protocol where writers can dirty and modify cached blocks that readers are currently reading in memory, leading to read corruption. This commit is the first step in addressing that problem. We add a scoutfs_block_dirty_ref() call which returns a reference to a dirtied block from the block core in one call. We're only changing the callers in this patch but we'll be reworking the dirtying mechanism in an upcoming patch to avoid corrupting readers. Signed-off-by: Zach Brown --- kmod/src/alloc.c | 78 +------------------------ kmod/src/block.c | 119 ++++++++++++++++++++++++++++++++++++--- kmod/src/block.h | 9 ++- kmod/src/btree.c | 109 ++++++----------------------------- kmod/src/forest.c | 56 ++---------------- kmod/src/scoutfs_trace.h | 54 +++++++++--------- kmod/src/srch.c | 110 +++++++----------------------------- 7 files changed, 187 insertions(+), 348 deletions(-) diff --git a/kmod/src/alloc.c b/kmod/src/alloc.c index 53965eec..80980cfa 100644 --- a/kmod/src/alloc.c +++ b/kmod/src/alloc.c @@ -393,82 +393,8 @@ static int dirty_list_block(struct super_block *sb, u64 dirty, u64 *old, struct scoutfs_block **bl_ret) { - struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; - struct scoutfs_block *cow_bl = NULL; - struct scoutfs_block *bl = NULL; - struct scoutfs_alloc_list_block *lblk; - bool undo_alloc = false; - u64 blkno; - int ret; - int err; - - blkno = le64_to_cpu(ref->blkno); - if (blkno) { - ret = read_list_block(sb, ref, &bl); - if (ret < 0) - goto out; - - if (scoutfs_block_writer_is_dirty(sb, bl)) { - ret = 0; - goto out; - } - } - - if (dirty == 0) { - ret = scoutfs_alloc_meta(sb, alloc, wri, &dirty); - if (ret < 0) - goto out; - undo_alloc = true; - } - - cow_bl = scoutfs_block_create(sb, dirty); - if (IS_ERR(cow_bl)) { - ret = PTR_ERR(cow_bl); - goto out; - } - - if (old) { - *old = blkno; - } else if (blkno) { - ret = scoutfs_free_meta(sb, alloc, wri, blkno); - if (ret < 0) - goto out; - } - - if (bl) - memcpy(cow_bl->data, bl->data, SCOUTFS_BLOCK_LG_SIZE); - else - memset(cow_bl->data, 0, SCOUTFS_BLOCK_LG_SIZE); - scoutfs_block_put(sb, bl); - bl = cow_bl; - cow_bl = NULL; - - lblk = bl->data; - lblk->hdr.magic = cpu_to_le32(SCOUTFS_BLOCK_MAGIC_ALLOC_LIST); - lblk->hdr.fsid = super->hdr.fsid; - lblk->hdr.blkno = cpu_to_le64(bl->blkno); - prandom_bytes(&lblk->hdr.seq, sizeof(lblk->hdr.seq)); - - ref->blkno = lblk->hdr.blkno; - ref->seq = lblk->hdr.seq; - - scoutfs_block_writer_mark_dirty(sb, wri, bl); - ret = 0; - -out: - scoutfs_block_put(sb, cow_bl); - if (ret < 0 && undo_alloc) { - err = scoutfs_free_meta(sb, alloc, wri, dirty); - BUG_ON(err); /* inconsistent */ - } - - if (ret < 0) { - scoutfs_block_put(sb, bl); - bl = NULL; - } - *bl_ret = bl; - - return ret; + return scoutfs_block_dirty_ref(sb, alloc, wri, ref, SCOUTFS_BLOCK_MAGIC_ALLOC_LIST, + bl_ret, dirty, old); } /* Allocate a new dirty list block if we fill up more than 3/4 of the block. */ diff --git a/kmod/src/block.c b/kmod/src/block.c index 8e0ed42f..7b199c9f 100644 --- a/kmod/src/block.c +++ b/kmod/src/block.c @@ -28,6 +28,7 @@ #include "counters.h" #include "msg.h" #include "scoutfs_trace.h" +#include "alloc.h" /* * The scoutfs block cache manages metadata blocks that can be larger @@ -719,9 +720,8 @@ void scoutfs_block_writer_init(struct super_block *sb, * and allocate with an advancing cursor so we always dirty in block * offset order and can walk our list to submit nice ordered IO. */ -void scoutfs_block_writer_mark_dirty(struct super_block *sb, - struct scoutfs_block_writer *wri, - struct scoutfs_block *bl) +static void block_mark_dirty(struct super_block *sb, struct scoutfs_block_writer *wri, + struct scoutfs_block *bl) { struct block_private *bp = BLOCK_PRIVATE(bl); @@ -736,14 +736,119 @@ void scoutfs_block_writer_mark_dirty(struct super_block *sb, } } -bool scoutfs_block_writer_is_dirty(struct super_block *sb, - struct scoutfs_block *bl) +static bool block_is_dirty(struct block_private *bp) { - struct block_private *bp = BLOCK_PRIVATE(bl); - return test_bit(BLOCK_BIT_DIRTY, &bp->bits) != 0; } +/* + * Give the caller a dirty block that is pointed to by their ref. + * + * The ref may already refer to a cached dirty block. In that case the + * dirty block is returned. + * + * If the ref doesn't refer to a dirty block, then a new block is always + * allocated and returned. If the ref refers to an existing block then + * its contents are copied into the new block. + * + * If a new blkno is allocated then the ref is updated and any existing + * blkno is freed. + * + * The dirty_blkno and ref_blkno arguments are used by the metadata + * allocator to avoid recursing into itself. dirty_blkno provides the + * blkno of the new dirty block to avoid calling _alloc_meta and + * ref_blkno is set to the old blkno instead of freeing it with + * _free_meta. + */ +int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, + struct scoutfs_block_writer *wri, struct scoutfs_block_ref *ref, + u32 magic, struct scoutfs_block **bl_ret, + u64 dirty_blkno, u64 *ref_blkno) +{ + struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; + struct scoutfs_block *cow_bl = NULL; + struct scoutfs_block *bl = NULL; + struct block_private *bp = NULL; + struct scoutfs_block_header *hdr; + bool undo_alloc = false; + u64 blkno; + int ret; + int err; + + blkno = le64_to_cpu(ref->blkno); + if (blkno) { + ret = scoutfs_block_read_ref(sb, ref, magic, bl_ret); + if (ret < 0) + goto out; + bl = *bl_ret; + bp = BLOCK_PRIVATE(bl); + + if (block_is_dirty(bp)) { + ret = 0; + goto out; + } + } + + if (dirty_blkno == 0) { + ret = scoutfs_alloc_meta(sb, alloc, wri, &dirty_blkno); + if (ret < 0) + goto out; + undo_alloc = true; + } + + cow_bl = scoutfs_block_create(sb, dirty_blkno); + if (IS_ERR(cow_bl)) { + ret = PTR_ERR(cow_bl); + goto out; + } + + if (ref_blkno) { + *ref_blkno = blkno; + } else if (blkno) { + ret = scoutfs_free_meta(sb, alloc, wri, blkno); + if (ret < 0) + goto out; + } + + if (bl) + memcpy(cow_bl->data, bl->data, SCOUTFS_BLOCK_LG_SIZE); + else + memset(cow_bl->data, 0, SCOUTFS_BLOCK_LG_SIZE); + scoutfs_block_put(sb, bl); + bl = cow_bl; + cow_bl = NULL; + + hdr = bl->data; + hdr->magic = cpu_to_le32(magic); + hdr->fsid = super->hdr.fsid; + hdr->blkno = cpu_to_le64(bl->blkno); + prandom_bytes(&hdr->seq, sizeof(hdr->seq)); + + trace_scoutfs_block_dirty_ref(sb, le64_to_cpu(ref->blkno), le64_to_cpu(ref->seq), + le64_to_cpu(hdr->blkno), le64_to_cpu(hdr->seq)); + + ref->blkno = hdr->blkno; + ref->seq = hdr->seq; + + block_mark_dirty(sb, wri, bl); + ret = 0; + +out: + scoutfs_block_put(sb, cow_bl); + if (ret < 0 && undo_alloc) { + err = scoutfs_free_meta(sb, alloc, wri, dirty_blkno); + BUG_ON(err); /* inconsistent */ + } + + if (ret < 0) { + scoutfs_block_put(sb, bl); + bl = NULL; + } + *bl_ret = bl; + + return ret; +} + /* * Submit writes for all the dirty blocks in the writer's dirty list and * wait for them to complete. The caller must serialize this with diff --git a/kmod/src/block.h b/kmod/src/block.h index 7276a61c..8278f3d3 100644 --- a/kmod/src/block.h +++ b/kmod/src/block.h @@ -20,11 +20,10 @@ void scoutfs_block_put(struct super_block *sb, struct scoutfs_block *bl); void scoutfs_block_writer_init(struct super_block *sb, struct scoutfs_block_writer *wri); -void scoutfs_block_writer_mark_dirty(struct super_block *sb, - struct scoutfs_block_writer *wri, - struct scoutfs_block *bl); -bool scoutfs_block_writer_is_dirty(struct super_block *sb, - struct scoutfs_block *bl); +int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, + struct scoutfs_block_writer *wri, struct scoutfs_block_ref *ref, + u32 magic, struct scoutfs_block **bl_ret, + u64 dirty_blkno, u64 *ref_blkno); int scoutfs_block_writer_write(struct super_block *sb, struct scoutfs_block_writer *wri); void scoutfs_block_writer_forget_all(struct super_block *sb, diff --git a/kmod/src/btree.c b/kmod/src/btree.c index 78643961..97778352 100644 --- a/kmod/src/btree.c +++ b/kmod/src/btree.c @@ -80,7 +80,7 @@ enum btree_walk_flags { BTW_NEXT = (1 << 0), /* return >= key */ BTW_PREV = (1 << 1), /* return <= key */ BTW_DIRTY = (1 << 2), /* cow stable blocks */ - BTW_ALLOC = (1 << 3), /* allocate a new block for 0 ref */ + BTW_ALLOC = (1 << 3), /* allocate a new block for 0 ref, requires dirty */ BTW_INSERT = (1 << 4), /* walking to insert, try splitting */ BTW_DELETE = (1 << 5), /* walking to delete, try joining */ }; @@ -628,102 +628,27 @@ static int get_ref_block(struct super_block *sb, struct scoutfs_block_ref *ref, struct scoutfs_block **bl_ret) { - struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; - struct scoutfs_btree_block *bt = NULL; - struct scoutfs_btree_block *new; - struct scoutfs_block *new_bl = NULL; - struct scoutfs_block *bl = NULL; - u64 blkno; - u64 seq; int ret; - /* always get the current block, either to return or cow from */ - if (ref && ref->blkno) { - ret = scoutfs_block_read_ref(sb, ref, SCOUTFS_BLOCK_MAGIC_BTREE, &bl); - if (ret < 0) { - if (ret == -ESTALE) - scoutfs_inc_counter(sb, btree_stale_read); - goto out; - } + if (WARN_ON_ONCE((flags & BTW_ALLOC) && !(flags & BTW_DIRTY))) + return -EINVAL; - /* - * We need to create a new dirty copy of the block if - * the caller asked for it. If the block is already - * dirty then we can return it. - */ - if (!(flags & BTW_DIRTY) || - scoutfs_block_writer_is_dirty(sb, bl)) { - ret = 0; - goto out; - } - - } else if (!(flags & BTW_ALLOC)) { + if (ref->blkno == 0 && !(flags & BTW_ALLOC)) { ret = -ENOENT; goto out; } - ret = scoutfs_alloc_meta(sb, alloc, wri, &blkno); - if (ret < 0) - goto out; - - prandom_bytes(&seq, sizeof(seq)); - - new_bl = scoutfs_block_create(sb, blkno); - if (IS_ERR(new_bl)) { - ret = scoutfs_free_meta(sb, alloc, wri, blkno); - BUG_ON(ret); - ret = PTR_ERR(new_bl); - goto out; - } - new = (void *)new_bl->data; - - /* free old stable blkno we're about to overwrite */ - if (ref && ref->blkno) { - ret = scoutfs_free_meta(sb, alloc, wri, - le64_to_cpu(ref->blkno)); - if (ret) { - ret = scoutfs_free_meta(sb, alloc, wri, blkno); - BUG_ON(ret); - scoutfs_block_put(sb, new_bl); - new_bl = NULL; - goto out; - } - } - - scoutfs_block_writer_mark_dirty(sb, wri, new_bl); - - trace_scoutfs_btree_dirty_block(sb, blkno, seq, - bt ? le64_to_cpu(bt->hdr.blkno) : 0, - bt ? le64_to_cpu(bt->hdr.seq) : 0); - - if (bt) { - /* returning a cow of an existing block */ - memcpy(new, bt, SCOUTFS_BLOCK_LG_SIZE); - scoutfs_block_put(sb, bl); - } else { - /* returning a newly allocated block */ - memset(new, 0, SCOUTFS_BLOCK_LG_SIZE); - new->hdr.fsid = super->hdr.fsid; - } - bl = new_bl; - bt = new; - - bt->hdr.magic = cpu_to_le32(SCOUTFS_BLOCK_MAGIC_BTREE); - bt->hdr.blkno = cpu_to_le64(blkno); - bt->hdr.seq = cpu_to_le64(seq); - if (ref) { - ref->blkno = bt->hdr.blkno; - ref->seq = bt->hdr.seq; - } - ret = 0; - + if (flags & BTW_DIRTY) + ret = scoutfs_block_dirty_ref(sb, alloc, wri, ref, SCOUTFS_BLOCK_MAGIC_BTREE, + bl_ret, 0, NULL); + else + ret = scoutfs_block_read_ref(sb, ref, SCOUTFS_BLOCK_MAGIC_BTREE, bl_ret); out: - if (ret) { - scoutfs_block_put(sb, bl); - bl = NULL; + if (ret < 0) { + if (ret == -ESTALE) + scoutfs_inc_counter(sb, btree_stale_read); } - *bl_ret = bl; return ret; } @@ -803,6 +728,7 @@ static int try_split(struct super_block *sb, struct scoutfs_block *par_bl = NULL; struct scoutfs_btree_block *left; struct scoutfs_key max_key; + struct scoutfs_block_ref zeros; int ret; int err; @@ -820,7 +746,8 @@ static int try_split(struct super_block *sb, scoutfs_inc_counter(sb, btree_split); /* alloc split neighbour first to avoid unwinding tree growth */ - ret = get_ref_block(sb, alloc, wri, BTW_ALLOC, NULL, &left_bl); + memset(&zeros, 0, sizeof(zeros)); + ret = get_ref_block(sb, alloc, wri, BTW_ALLOC | BTW_DIRTY, &zeros, &left_bl); if (ret) return ret; left = left_bl->data; @@ -828,7 +755,8 @@ static int try_split(struct super_block *sb, init_btree_block(left, right->level); if (!parent) { - ret = get_ref_block(sb, alloc, wri, BTW_ALLOC, NULL, &par_bl); + memset(&zeros, 0, sizeof(zeros)); + ret = get_ref_block(sb, alloc, wri, BTW_ALLOC | BTW_DIRTY, &zeros, &par_bl); if (ret) { err = scoutfs_free_meta(sb, alloc, wri, le64_to_cpu(left->hdr.blkno)); @@ -1196,8 +1124,7 @@ restart: if (!(flags & BTW_INSERT)) { ret = -ENOENT; } else { - ret = get_ref_block(sb, alloc, wri, BTW_ALLOC, - &root->ref, &bl); + ret = get_ref_block(sb, alloc, wri, BTW_ALLOC | BTW_DIRTY, &root->ref, &bl); if (ret == 0) { bt = bl->data; init_btree_block(bt, 0); diff --git a/kmod/src/forest.c b/kmod/src/forest.c index a832b2dd..c967c15a 100644 --- a/kmod/src/forest.c +++ b/kmod/src/forest.c @@ -377,18 +377,14 @@ out: int scoutfs_forest_set_bloom_bits(struct super_block *sb, struct scoutfs_lock *lock) { - struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; DECLARE_FOREST_INFO(sb, finf); - struct scoutfs_block *new_bl = NULL; struct scoutfs_block *bl = NULL; struct scoutfs_bloom_block *bb; struct scoutfs_block_ref *ref; struct forest_bloom_nrs bloom; int nr_set = 0; - u64 blkno; u64 nr; int ret; - int err; int i; nr = le64_to_cpu(finf->our_log.nr); @@ -406,53 +402,11 @@ int scoutfs_forest_set_bloom_bits(struct super_block *sb, ref = &finf->our_log.bloom_ref; - if (ref->blkno) { - bl = read_bloom_ref(sb, ref); - if (IS_ERR(bl)) { - ret = PTR_ERR(bl); - goto unlock; - } - bb = bl->data; - } - - if (!ref->blkno || !scoutfs_block_writer_is_dirty(sb, bl)) { - - ret = scoutfs_alloc_meta(sb, finf->alloc, finf->wri, &blkno); - if (ret < 0) - goto unlock; - - new_bl = scoutfs_block_create(sb, blkno); - if (IS_ERR(new_bl)) { - err = scoutfs_free_meta(sb, finf->alloc, finf->wri, - blkno); - BUG_ON(err); /* could have dirtied */ - ret = PTR_ERR(new_bl); - goto unlock; - } - - if (bl) { - err = scoutfs_free_meta(sb, finf->alloc, finf->wri, - le64_to_cpu(ref->blkno)); - BUG_ON(err); /* could have dirtied */ - memcpy(new_bl->data, bl->data, SCOUTFS_BLOCK_LG_SIZE); - } else { - memset(new_bl->data, 0, SCOUTFS_BLOCK_LG_SIZE); - } - - scoutfs_block_writer_mark_dirty(sb, finf->wri, new_bl); - - scoutfs_block_put(sb, bl); - bl = new_bl; - bb = bl->data; - new_bl = NULL; - - bb->hdr.magic = cpu_to_le32(SCOUTFS_BLOCK_MAGIC_BLOOM); - bb->hdr.fsid = super->hdr.fsid; - bb->hdr.blkno = cpu_to_le64(blkno); - prandom_bytes(&bb->hdr.seq, sizeof(bb->hdr.seq)); - ref->blkno = bb->hdr.blkno; - ref->seq = bb->hdr.seq; - } + ret = scoutfs_block_dirty_ref(sb, finf->alloc, finf->wri, ref, SCOUTFS_BLOCK_MAGIC_BLOOM, + &bl, 0, NULL); + if (ret < 0) + goto unlock; + bb = bl->data; for (i = 0; i < ARRAY_SIZE(bloom.nrs); i++) { if (!test_and_set_bit_le(bloom.nrs[i], bb->bits)) { diff --git a/kmod/src/scoutfs_trace.h b/kmod/src/scoutfs_trace.h index ecd2080f..ae99a994 100644 --- a/kmod/src/scoutfs_trace.h +++ b/kmod/src/scoutfs_trace.h @@ -1631,33 +1631,6 @@ TRACE_EVENT(scoutfs_btree_read_error, SCSB_TRACE_ARGS, __entry->blkno, __entry->seq) ); -TRACE_EVENT(scoutfs_btree_dirty_block, - TP_PROTO(struct super_block *sb, u64 blkno, u64 seq, - u64 bt_blkno, u64 bt_seq), - - TP_ARGS(sb, blkno, seq, bt_blkno, bt_seq), - - TP_STRUCT__entry( - SCSB_TRACE_FIELDS - __field(__u64, blkno) - __field(__u64, seq) - __field(__u64, bt_blkno) - __field(__u64, bt_seq) - ), - - TP_fast_assign( - SCSB_TRACE_ASSIGN(sb); - __entry->blkno = blkno; - __entry->seq = seq; - __entry->bt_blkno = bt_blkno; - __entry->bt_seq = bt_seq; - ), - - TP_printk(SCSBF" blkno %llu seq %llu bt_blkno %llu bt_seq %llu", - SCSB_TRACE_ARGS, __entry->blkno, __entry->seq, - __entry->bt_blkno, __entry->bt_seq) -); - TRACE_EVENT(scoutfs_btree_walk, TP_PROTO(struct super_block *sb, struct scoutfs_btree_root *root, struct scoutfs_key *key, int flags, int level, @@ -2056,6 +2029,33 @@ TRACE_EVENT(scoutfs_forest_init_our_log, __entry->blkno, __entry->seq) ); +TRACE_EVENT(scoutfs_block_dirty_ref, + TP_PROTO(struct super_block *sb, u64 ref_blkno, u64 ref_seq, + u64 block_blkno, u64 block_seq), + + TP_ARGS(sb, ref_blkno, ref_seq, block_blkno, block_seq), + + TP_STRUCT__entry( + SCSB_TRACE_FIELDS + __field(__u64, ref_blkno) + __field(__u64, ref_seq) + __field(__u64, block_blkno) + __field(__u64, block_seq) + ), + + TP_fast_assign( + SCSB_TRACE_ASSIGN(sb); + __entry->ref_blkno = ref_blkno; + __entry->ref_seq = ref_seq; + __entry->block_blkno = block_blkno; + __entry->block_seq = block_seq; + ), + + TP_printk(SCSBF" ref_blkno %llu ref_seq %llu block_blkno %llu block_seq %llu", + SCSB_TRACE_ARGS, __entry->ref_blkno, __entry->ref_seq, + __entry->block_blkno, __entry->block_seq) +); + DECLARE_EVENT_CLASS(scoutfs_block_class, TP_PROTO(struct super_block *sb, void *bp, u64 blkno, int refcount, int io_count, unsigned long bits, __u64 accessed), diff --git a/kmod/src/srch.c b/kmod/src/srch.c index 41cb11b1..4cca2b0c 100644 --- a/kmod/src/srch.c +++ b/kmod/src/srch.c @@ -255,24 +255,9 @@ static u8 height_for_blk(u64 blk) return hei; } -static void init_file_block(struct super_block *sb, struct scoutfs_block *bl, - int level) +static inline u32 srch_level_magic(int level) { - struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; - struct scoutfs_block_header *hdr; - - /* don't leak uninit kernel mem.. block should do this for us? */ - memset(bl->data, 0, SCOUTFS_BLOCK_LG_SIZE); - - hdr = bl->data; - hdr->fsid = super->hdr.fsid; - hdr->blkno = cpu_to_le64(bl->blkno); - prandom_bytes(&hdr->seq, sizeof(hdr->seq)); - - if (level) - hdr->magic = cpu_to_le32(SCOUTFS_BLOCK_MAGIC_SRCH_PARENT); - else - hdr->magic = cpu_to_le32(SCOUTFS_BLOCK_MAGIC_SRCH_BLOCK); + return level ? SCOUTFS_BLOCK_MAGIC_SRCH_PARENT : SCOUTFS_BLOCK_MAGIC_SRCH_BLOCK; } /* @@ -287,7 +272,7 @@ static int read_srch_block(struct super_block *sb, struct scoutfs_block_ref *ref, struct scoutfs_block **bl_ret) { - u32 magic = level ? SCOUTFS_BLOCK_MAGIC_SRCH_PARENT : SCOUTFS_BLOCK_MAGIC_SRCH_BLOCK; + u32 magic = srch_level_magic(level); int ret; ret = scoutfs_block_read_ref(sb, ref, magic, bl_ret); @@ -368,12 +353,10 @@ static int get_file_block(struct super_block *sb, struct scoutfs_block_header *hdr; struct scoutfs_block *bl = NULL; struct scoutfs_srch_parent *srp; - struct scoutfs_block *new_bl; + struct scoutfs_block_ref new_root_ref; struct scoutfs_block_ref *ref; - u64 blkno = 0; int level; int ind; - int err; int ret; u8 hei; @@ -385,29 +368,21 @@ static int get_file_block(struct super_block *sb, goto out; } - ret = scoutfs_alloc_meta(sb, alloc, wri, &blkno); + memset(&new_root_ref, 0, sizeof(new_root_ref)); + level = sfl->height; + + ret = scoutfs_block_dirty_ref(sb, alloc, wri, &new_root_ref, + srch_level_magic(level), &bl, 0, NULL); if (ret < 0) goto out; - bl = scoutfs_block_create(sb, blkno); - if (IS_ERR(bl)) { - ret = PTR_ERR(bl); - goto out; - } - blkno = 0; - - scoutfs_block_writer_mark_dirty(sb, wri, bl); - - init_file_block(sb, bl, sfl->height); - if (sfl->height) { + if (level) { srp = bl->data; - srp->refs[0].blkno = sfl->ref.blkno; - srp->refs[0].seq = sfl->ref.seq; + srp->refs[0] = sfl->ref; } hdr = bl->data; - sfl->ref.blkno = hdr->blkno; - sfl->ref.seq = hdr->seq; + sfl->ref = new_root_ref; sfl->height++; scoutfs_block_put(sb, bl); bl = NULL; @@ -423,54 +398,13 @@ static int get_file_block(struct super_block *sb, goto out; } - /* read an existing block */ - if (ref->blkno) { - ret = read_srch_block(sb, wri, level, ref, &bl); - if (ret < 0) - goto out; - } - - /* allocate a new block if we need it */ - if (!ref->blkno || ((flags & GFB_DIRTY) && - !scoutfs_block_writer_is_dirty(sb, bl))) { - ret = scoutfs_alloc_meta(sb, alloc, wri, &blkno); - if (ret < 0) - goto out; - - new_bl = scoutfs_block_create(sb, blkno); - if (IS_ERR(new_bl)) { - ret = PTR_ERR(new_bl); - goto out; - } - - if (bl) { - /* cow old block if we have one */ - ret = scoutfs_free_meta(sb, alloc, wri, - bl->blkno); - if (ret) - goto out; - - memcpy(new_bl->data, bl->data, - SCOUTFS_BLOCK_LG_SIZE); - scoutfs_block_put(sb, bl); - bl = new_bl; - hdr = bl->data; - hdr->blkno = cpu_to_le64(bl->blkno); - prandom_bytes(&hdr->seq, sizeof(hdr->seq)); - } else { - /* init new allocated block */ - bl = new_bl; - init_file_block(sb, bl, level); - } - - blkno = 0; - scoutfs_block_writer_mark_dirty(sb, wri, bl); - - /* update file or parent block ref */ - hdr = bl->data; - ref->blkno = hdr->blkno; - ref->seq = hdr->seq; - } + if (flags & GFB_DIRTY) + ret = scoutfs_block_dirty_ref(sb, alloc, wri, ref, srch_level_magic(level), + &bl, 0, NULL); + else + ret = scoutfs_block_read_ref(sb, ref, srch_level_magic(level), &bl); + if (ret < 0) + goto out; if (level == 0) { ret = 0; @@ -490,12 +424,6 @@ static int get_file_block(struct super_block *sb, out: scoutfs_block_put(sb, parent); - /* return allocated blkno on error */ - if (blkno > 0) { - err = scoutfs_free_meta(sb, alloc, wri, blkno); - BUG_ON(err); /* radix should have been dirty */ - } - if (ret < 0) { scoutfs_block_put(sb, bl); bl = NULL; From 9450959ca41a00f736f5356ec895ef0939bd8890 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Mon, 22 Feb 2021 13:26:46 -0800 Subject: [PATCH 5/7] Protect stale block readers from local dirtying Our block cache consistency mechanism allows readers to try and read stale block references. They check block headers of the block they read to discover if it has been modified and they should retry the read with newer block references. For this to be correct the block contents can't change under the readers. That's obviously true in the simple imagined case of one node writing and another node reading. But we also have the case where the stale reader and dirtying writer can be concurrent tasks in the same mount which share a block cache. There were a two failure cases that derive from the order of readers and writers working with blocks. If the reader goes first, the writer could find the existing block in the cache and modify it while the reader assumes that it is read only. The fix is to have the writer always remove any existing cached block and insert a newly allocated block into the cache with the header fields already changed. Any existing readers will still have their cached block references and any new readers will see the modified headers and return -ESTALE. The next failure comes from readers trying to invalidate dirty blocks when they see modified headers. They assumed that the existing cached block was old and could be dropped so that a new current version could be read. But in this case a local writer has clobbered the reader's stale block and the reader should immediately return -ESTALE. Signed-off-by: Zach Brown --- kmod/src/block.c | 121 +++++++++++++++++++++++++++++------------------ kmod/src/block.h | 1 - 2 files changed, 76 insertions(+), 46 deletions(-) diff --git a/kmod/src/block.c b/kmod/src/block.c index 7b199c9f..6d93e116 100644 --- a/kmod/src/block.c +++ b/kmod/src/block.c @@ -275,16 +275,15 @@ static const struct rhashtable_params block_ht_params = { /* * Insert a new block into the hash table. Once it is inserted in the - * hash table readers can start getting references. The caller only has - * its initial ref so inserted can't be set and there can be no other - * references. + * hash table readers can start getting references. The caller may have + * multiple refs but the block can't already be inserted. */ static int block_insert(struct super_block *sb, struct block_private *bp) { DECLARE_BLOCK_INFO(sb, binf); int ret; - WARN_ON_ONCE(atomic_read(&bp->refcount) != 1); + WARN_ON_ONCE(atomic_read(&bp->refcount) & BLOCK_REF_INSERTED); atomic_add(BLOCK_REF_INSERTED, &bp->refcount); ret = rhashtable_insert_fast(&binf->ht, &bp->ht_head, block_ht_params); @@ -520,6 +519,20 @@ static int block_submit_bio(struct super_block *sb, struct block_private *bp, return ret; } +static struct block_private *block_lookup(struct super_block *sb, u64 blkno) +{ + DECLARE_BLOCK_INFO(sb, binf); + struct block_private *bp; + + rcu_read_lock(); + bp = rhashtable_lookup(&binf->ht, &blkno, block_ht_params); + if (bp) + bp = block_get_if_inserted(bp); + rcu_read_unlock(); + + return bp; +} + /* * Return a reference to a cached block found in the hash table. If one * isn't found then we try and allocate and insert a new one. Its @@ -533,16 +546,11 @@ static int block_submit_bio(struct super_block *sb, struct block_private *bp, static struct block_private *block_lookup_create(struct super_block *sb, u64 blkno) { - DECLARE_BLOCK_INFO(sb, binf); struct block_private *bp; int ret; restart: - rcu_read_lock(); - bp = rhashtable_lookup(&binf->ht, &blkno, block_ht_params); - if (bp) - bp = block_get_if_inserted(bp); - rcu_read_unlock(); + bp = block_lookup(sb, blkno); /* drop failed reads that interrupted waiters abandoned */ if (bp && (test_bit(BLOCK_BIT_ERROR, &bp->bits) && @@ -581,24 +589,6 @@ out: return bp; } -/* - * Return a cached block or a newly allocated block whose contents are - * undefined. The caller is going to initialize the block contents. - */ -struct scoutfs_block *scoutfs_block_create(struct super_block *sb, u64 blkno) -{ - struct block_private *bp; - - bp = block_lookup_create(sb, blkno); - if (IS_ERR(bp)) - return ERR_CAST(bp); - - set_bit(BLOCK_BIT_UPTODATE, &bp->bits); - set_bit(BLOCK_BIT_CRC_VALID, &bp->bits); - - return &bp->bl; -} - static bool uptodate_or_error(struct block_private *bp) { smp_rmb(); /* test after adding to wait queue */ @@ -606,6 +596,11 @@ static bool uptodate_or_error(struct block_private *bp) test_bit(BLOCK_BIT_ERROR, &bp->bits); } +static bool block_is_dirty(struct block_private *bp) +{ + return test_bit(BLOCK_BIT_DIRTY, &bp->bits) != 0; +} + static struct block_private *block_read(struct super_block *sb, u64 blkno) { DECLARE_BLOCK_INFO(sb, binf); @@ -639,13 +634,18 @@ out: } /* - * Btree blocks don't have rigid cache consistency. We can be following - * block references into cached blocks that are now stale or can be - * following a stale root into blocks that have been overwritten. If we - * hit a block that looks stale we first invalidate the cache and retry, - * returning -ESTALE if it still looks wrong. The caller can retry the - * read from a more current root or decide that this is a persistent - * error. + * Read a referenced metadata block. + * + * The caller may be following a stale reference to a block location + * that has since been rewritten. We check that destination block + * header fields match the reference. If they don't, we return -ESTALE + * and the caller can chose to retry with newer references or return an + * error. -ESTALE can be a sign of block corruption when the refs are + * current. + * + * Once the caller has the cached block it won't be modified. A writer + * trying to dirty a block at that location will remove our existing + * block and insert a new block with modified headers. */ int scoutfs_block_read_ref(struct super_block *sb, struct scoutfs_block_ref *ref, u32 magic, struct scoutfs_block **bl_ret) @@ -682,7 +682,7 @@ retry: ret = 0; out: - if (ret == -ESTALE && !retried) { + if (ret == -ESTALE && !retried && !block_is_dirty(bp)) { retried = true; scoutfs_inc_counter(sb, block_cache_remove_stale); block_remove(sb, bp); @@ -736,11 +736,6 @@ static void block_mark_dirty(struct super_block *sb, struct scoutfs_block_writer } } -static bool block_is_dirty(struct block_private *bp) -{ - return test_bit(BLOCK_BIT_DIRTY, &bp->bits) != 0; -} - /* * Give the caller a dirty block that is pointed to by their ref. * @@ -754,6 +749,15 @@ static bool block_is_dirty(struct block_private *bp) * If a new blkno is allocated then the ref is updated and any existing * blkno is freed. * + * A newly allocated block that we insert into the cache and return + * might already have an old stale copy inserted in the cache and it + * might be actively in use by readers. Future readers may also try to + * read their old block from our newly allocated block. We always + * remove any existing blocks and insert our new block only after + * modifying its headers and marking it dirty. Readers will never have + * their blocks modified and they can always identify new mismatched + * cached blocks. + * * The dirty_blkno and ref_blkno arguments are used by the metadata * allocator to avoid recursing into itself. dirty_blkno provides the * blkno of the new dirty block to avoid calling _alloc_meta and @@ -768,6 +772,8 @@ int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, struct scoutfs_super_block *super = &SCOUTFS_SB(sb)->super; struct scoutfs_block *cow_bl = NULL; struct scoutfs_block *bl = NULL; + struct block_private *exist_bp = NULL; + struct block_private *cow_bp = NULL; struct block_private *bp = NULL; struct scoutfs_block_header *hdr; bool undo_alloc = false; @@ -775,6 +781,7 @@ int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, int ret; int err; + /* read existing referenced block, if any */ blkno = le64_to_cpu(ref->blkno); if (blkno) { ret = scoutfs_block_read_ref(sb, ref, magic, bl_ret); @@ -789,6 +796,7 @@ int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, } } + /* allocate new blkno if caller didn't give us one */ if (dirty_blkno == 0) { ret = scoutfs_alloc_meta(sb, alloc, wri, &dirty_blkno); if (ret < 0) @@ -796,12 +804,18 @@ int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, undo_alloc = true; } - cow_bl = scoutfs_block_create(sb, dirty_blkno); - if (IS_ERR(cow_bl)) { - ret = PTR_ERR(cow_bl); + /* allocate new cached block */ + cow_bp = block_alloc(sb, dirty_blkno); + if (cow_bp == NULL) { + ret = -ENOMEM; goto out; } + cow_bl = &cow_bp->bl; + set_bit(BLOCK_BIT_UPTODATE, &cow_bp->bits); + set_bit(BLOCK_BIT_CRC_VALID, &cow_bp->bits); + + /* free original referenced blkno, or give it to the caller to deal with */ if (ref_blkno) { *ref_blkno = blkno; } else if (blkno) { @@ -810,6 +824,7 @@ int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, goto out; } + /* copy original block contents, or initialize */ if (bl) memcpy(cow_bl->data, bl->data, SCOUTFS_BLOCK_LG_SIZE); else @@ -817,6 +832,8 @@ int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, scoutfs_block_put(sb, bl); bl = cow_bl; cow_bl = NULL; + bp = cow_bp; + cow_bp = NULL; hdr = bl->data; hdr->magic = cpu_to_le32(magic); @@ -827,10 +844,24 @@ int scoutfs_block_dirty_ref(struct super_block *sb, struct scoutfs_alloc *alloc, trace_scoutfs_block_dirty_ref(sb, le64_to_cpu(ref->blkno), le64_to_cpu(ref->seq), le64_to_cpu(hdr->blkno), le64_to_cpu(hdr->seq)); + /* mark the block dirty before it's visible */ + block_mark_dirty(sb, wri, bl); + + /* insert the new block, maybe removing any existing blocks */ + while ((ret = block_insert(sb, bp)) == -EEXIST) { + exist_bp = block_lookup(sb, dirty_blkno); + if (exist_bp) { + block_remove(sb, exist_bp); + block_put(sb, exist_bp); + } + } + if (ret < 0) + goto out; + + /* set the ref now that the block is visible */ ref->blkno = hdr->blkno; ref->seq = hdr->seq; - block_mark_dirty(sb, wri, bl); ret = 0; out: diff --git a/kmod/src/block.h b/kmod/src/block.h index 8278f3d3..93d88731 100644 --- a/kmod/src/block.h +++ b/kmod/src/block.h @@ -13,7 +13,6 @@ struct scoutfs_block { void *priv; }; -struct scoutfs_block *scoutfs_block_create(struct super_block *sb, u64 blkno); int scoutfs_block_read_ref(struct super_block *sb, struct scoutfs_block_ref *ref, u32 magic, struct scoutfs_block **bl_ret); void scoutfs_block_put(struct super_block *sb, struct scoutfs_block *bl); From 208c51d1d24fb59c881c4c67b8ab535d761e4728 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Wed, 24 Feb 2021 12:41:14 -0800 Subject: [PATCH 6/7] Update stale block reading test The previous test that triggered re-reading blocks, as though they were stale, was written in the era where it only hit btree blocks and everything else was stored in LSM segments. This reworks the test to make it clear that it affects all our block readers today. The test only exercise the core read retry path, but it could be expanded to test callers retrying with newer references after they get -ESTALE errors. Signed-off-by: Zach Brown --- kmod/src/block.c | 4 +++- kmod/src/triggers.c | 1 + kmod/src/triggers.h | 1 + tests/golden/block-stale-reads | 29 +++++++++++++++++++++++ tests/golden/stale-btree-read | 11 --------- tests/sequence | 2 +- tests/tests/block-stale-reads.sh | 40 ++++++++++++++++++++++++++++++++ tests/tests/stale-btree-read.sh | 40 -------------------------------- 8 files changed, 75 insertions(+), 53 deletions(-) create mode 100644 tests/golden/block-stale-reads delete mode 100644 tests/golden/stale-btree-read create mode 100644 tests/tests/block-stale-reads.sh delete mode 100644 tests/tests/stale-btree-read.sh diff --git a/kmod/src/block.c b/kmod/src/block.c index 6d93e116..e13aa8e0 100644 --- a/kmod/src/block.c +++ b/kmod/src/block.c @@ -29,6 +29,7 @@ #include "msg.h" #include "scoutfs_trace.h" #include "alloc.h" +#include "triggers.h" /* * The scoutfs block cache manages metadata blocks that can be larger @@ -682,7 +683,8 @@ retry: ret = 0; out: - if (ret == -ESTALE && !retried && !block_is_dirty(bp)) { + if ((ret == -ESTALE || scoutfs_trigger(sb, BLOCK_REMOVE_STALE)) && + !retried && !block_is_dirty(bp)) { retried = true; scoutfs_inc_counter(sb, block_cache_remove_stale); block_remove(sb, bp); diff --git a/kmod/src/triggers.c b/kmod/src/triggers.c index a94f2b65..75ba07d7 100644 --- a/kmod/src/triggers.c +++ b/kmod/src/triggers.c @@ -38,6 +38,7 @@ struct scoutfs_triggers { struct scoutfs_triggers *name = SCOUTFS_SB(sb)->triggers static char *names[] = { + [SCOUTFS_TRIGGER_BLOCK_REMOVE_STALE] = "block_remove_stale", [SCOUTFS_TRIGGER_BTREE_STALE_READ] = "btree_stale_read", [SCOUTFS_TRIGGER_BTREE_ADVANCE_RING_HALF] = "btree_advance_ring_half", [SCOUTFS_TRIGGER_HARD_STALE_ERROR] = "hard_stale_error", diff --git a/kmod/src/triggers.h b/kmod/src/triggers.h index 8796cd18..d3d2e0f6 100644 --- a/kmod/src/triggers.h +++ b/kmod/src/triggers.h @@ -2,6 +2,7 @@ #define _SCOUTFS_TRIGGERS_H_ enum scoutfs_trigger { + SCOUTFS_TRIGGER_BLOCK_REMOVE_STALE, SCOUTFS_TRIGGER_BTREE_STALE_READ, SCOUTFS_TRIGGER_BTREE_ADVANCE_RING_HALF, SCOUTFS_TRIGGER_HARD_STALE_ERROR, diff --git a/tests/golden/block-stale-reads b/tests/golden/block-stale-reads new file mode 100644 index 00000000..51f11b28 --- /dev/null +++ b/tests/golden/block-stale-reads @@ -0,0 +1,29 @@ +== create file for xattr ping pong +# file: /mnt/test/test/block-stale-reads/file +user.xat="initial" + +== retry btree forest reads between mounts +trigger block_remove_stale armed: 0 +# file: /mnt/test/test/block-stale-reads/file +user.xat="1" + +trigger block_remove_stale after: 0 +counter block_cache_remove_stale diff 1 +trigger block_remove_stale armed: 0 +# file: /mnt/test/test/block-stale-reads/file +user.xat="2" + +trigger block_remove_stale after: 0 +counter block_cache_remove_stale diff 2 +trigger block_remove_stale armed: 0 +# file: /mnt/test/test/block-stale-reads/file +user.xat="3" + +trigger block_remove_stale after: 0 +counter block_cache_remove_stale diff 3 +trigger block_remove_stale armed: 0 +# file: /mnt/test/test/block-stale-reads/file +user.xat="4" + +trigger block_remove_stale after: 0 +counter block_cache_remove_stale diff 4 diff --git a/tests/golden/stale-btree-read b/tests/golden/stale-btree-read deleted file mode 100644 index 07cb3bf1..00000000 --- a/tests/golden/stale-btree-read +++ /dev/null @@ -1,11 +0,0 @@ -== create file for xattr ping pong -# file: /mnt/test/test/stale-btree-read/file -user.xat="initial" - -== retry btree block read -trigger btree_stale_read armed: 1 -# file: /mnt/test/test/stale-btree-read/file -user.xat="btree" - -trigger btree_stale_read after: 0 -counter btree_stale_read diff 1 diff --git a/tests/sequence b/tests/sequence index 8cd44c87..764ec501 100644 --- a/tests/sequence +++ b/tests/sequence @@ -28,5 +28,5 @@ setup-error-teardown.sh mount-unmount-race.sh createmany-parallel-mounts.sh archive-light-cycle.sh -stale-btree-read.sh +block-stale-reads.sh xfstests.sh diff --git a/tests/tests/block-stale-reads.sh b/tests/tests/block-stale-reads.sh new file mode 100644 index 00000000..a41d821d --- /dev/null +++ b/tests/tests/block-stale-reads.sh @@ -0,0 +1,40 @@ +# +# exercise stale block reading. +# +# It would be very difficult to manipulate the allocators, cache, and +# persistent blocks to create stable block reading scenarios. Instead +# we use triggers to exercise how readers encounter stale blocks. +# + +t_require_commands touch setfattr getfattr +t_require_mounts 2 + +GETFATTR="getfattr --absolute-names" +SETFATTR="setfattr" + +# +# force re-reading forest btree blocks as each mount reads the items +# written by the other. +# +set_file="$T_D0/file" +get_file="$T_D1/file" +echo "== create file for xattr ping pong" +touch "$set_file" +$SETFATTR -n user.xat -v initial "$set_file" +$GETFATTR -n user.xat "$get_file" 2>&1 | t_filter_fs + +echo "== retry btree forest reads between mounts" +for i in $(seq 1 4); do + tmp="$set_file" + set_file="$get_file" + get_file="$tmp" + + $SETFATTR -n user.xat -v $i "$set_file" + t_trigger_arm block_remove_stale $cl + old=$(t_counter btree_stale_read $cl) + $GETFATTR -n user.xat "$get_file" 2>&1 | t_filter_fs + t_trigger_show block_remove_stale "after" $cl + t_counter_diff block_cache_remove_stale $old $cl +done + +t_pass diff --git a/tests/tests/stale-btree-read.sh b/tests/tests/stale-btree-read.sh deleted file mode 100644 index 36c43cb7..00000000 --- a/tests/tests/stale-btree-read.sh +++ /dev/null @@ -1,40 +0,0 @@ -# -# verify stale btree block reading -# - -t_require_commands touch stat setfattr getfattr createmany -t_require_mounts 2 - -GETFATTR="getfattr --absolute-names" -SETFATTR="setfattr" - -# -# This exercises the soft retry of btree blocks when -# inconsistent cached versions are found. It ensures that basic hard -# error returning turns into EIO in the case where the persistent reread -# blocks and segments really are inconsistent. -# -# The triggers apply across all execution in the file system. So to -# trigger btree block retries in the client we make sure that the server -# is running on the other node. -# - -cl=$(t_first_client_nr) -sv=$(t_server_nr) -eval cl_dir="\$T_D${cl}" -eval sv_dir="\$T_D${sv}" - -echo "== create file for xattr ping pong" -touch "$sv_dir/file" -$SETFATTR -n user.xat -v initial "$sv_dir/file" -$GETFATTR -n user.xat "$sv_dir/file" 2>&1 | t_filter_fs - -echo "== retry btree block read" -$SETFATTR -n user.xat -v btree "$sv_dir/file" -t_trigger_arm btree_stale_read $cl -old=$(t_counter btree_stale_read $cl) -$GETFATTR -n user.xat "$cl_dir/file" 2>&1 | t_filter_fs -t_trigger_show btree_stale_read "after" $cl -t_counter_diff btree_stale_read $old $cl - -t_pass From a508baae76a694ad2c3640f767293811b5b50597 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Wed, 24 Feb 2021 14:27:35 -0800 Subject: [PATCH 7/7] Remove unused triggers As the implementation shifted away from the ring of btree blocks and LSM segments we lost callers to all these triggers. They're unused and can be removed. Signed-off-by: Zach Brown --- kmod/src/triggers.c | 4 ---- kmod/src/triggers.h | 4 ---- 2 files changed, 8 deletions(-) diff --git a/kmod/src/triggers.c b/kmod/src/triggers.c index 75ba07d7..0dcbfb7d 100644 --- a/kmod/src/triggers.c +++ b/kmod/src/triggers.c @@ -39,10 +39,6 @@ struct scoutfs_triggers { static char *names[] = { [SCOUTFS_TRIGGER_BLOCK_REMOVE_STALE] = "block_remove_stale", - [SCOUTFS_TRIGGER_BTREE_STALE_READ] = "btree_stale_read", - [SCOUTFS_TRIGGER_BTREE_ADVANCE_RING_HALF] = "btree_advance_ring_half", - [SCOUTFS_TRIGGER_HARD_STALE_ERROR] = "hard_stale_error", - [SCOUTFS_TRIGGER_SEG_STALE_READ] = "seg_stale_read", [SCOUTFS_TRIGGER_STATFS_LOCK_PURGE] = "statfs_lock_purge", }; diff --git a/kmod/src/triggers.h b/kmod/src/triggers.h index d3d2e0f6..40b40626 100644 --- a/kmod/src/triggers.h +++ b/kmod/src/triggers.h @@ -3,10 +3,6 @@ enum scoutfs_trigger { SCOUTFS_TRIGGER_BLOCK_REMOVE_STALE, - SCOUTFS_TRIGGER_BTREE_STALE_READ, - SCOUTFS_TRIGGER_BTREE_ADVANCE_RING_HALF, - SCOUTFS_TRIGGER_HARD_STALE_ERROR, - SCOUTFS_TRIGGER_SEG_STALE_READ, SCOUTFS_TRIGGER_STATFS_LOCK_PURGE, SCOUTFS_TRIGGER_NR, };