Reset item state when retrying stale forest reads

The forest reader reads items from the fs_root and all log btrees and
gives them to the caller who tracks them to resolve version differences.

The reads can run into stale blocks which have been overwritten.  The
forest reader was implementing the retry under the item state in the
caller.  This can corrupt items that are only seen firest in an old fs
root before a merge and then only seen in the fs_root after a merge.  In
this case the item won't have any versioning and the existing version
from the old fs_root is preferred.  This is particularly bad when the
new version was deleted -- in that case we have no metadata which would
tell us to drop the old item that was read from the old fs_root.

This is fixed by pushing the retry up to callers who wipe the item state
before each retry.  Now each set of items is related to a single
snapshot of the fs_root and logs at one point in time.

I haven't seen definitive evidence of this happening in practice.  I
found this problem after putting on my craziest thinking toque and
auditing the code for places where we could lose item updates.

Signed-off-by: Zach Brown <zab@versity.com>

kmod/src/forest.c

@@ -251,10 +251,8 @@ static int forest_read_items(struct super_block *sb, struct scoutfs_key *key,
  * that covers all the blocks.  Any keys outside of this range can't be
  * trusted because we didn't visit all the trees to check their items.
  *
- * If we hit stale blocks and retry we can call the callback for
- * duplicate items.  This is harmless because the items are stable while
- * the caller holds their cluster lock and the caller has to filter out
- * item seqs anyway.
+ * We return -ESTALE if we hit stale blocks to give the caller a chance
+ * to reset their state and retry with a newer version of the btrees.
  */
 int scoutfs_forest_read_items(struct super_block *sb,
 			      struct scoutfs_lock *lock,
@@ -263,7 +261,6 @@ int scoutfs_forest_read_items(struct super_block *sb,
 			      struct scoutfs_key *end,
 			      scoutfs_forest_item_cb cb, void *arg)
 {
-	DECLARE_STALE_TRACKING_SUPER_REFS(prev_refs, refs);
 	struct forest_read_items_data rid = {
 		.cb = cb,
 		.cb_arg = arg,
@@ -281,14 +278,11 @@ int scoutfs_forest_read_items(struct super_block *sb,
 	scoutfs_inc_counter(sb, forest_read_items);
 	calc_bloom_nrs(&bloom, &lock->start);
 
-retry:
 	ret = scoutfs_client_get_roots(sb, &roots);
 	if (ret)
 		goto out;
 
 	trace_scoutfs_forest_using_roots(sb, &roots.fs_root, &roots.logs_root);
-	refs.fs_ref = roots.fs_root.ref;
-	refs.logs_ref = roots.logs_root.ref;
 
 	*start = lock->start;
 	*end = lock->end;
@@ -352,13 +346,6 @@ retry:
 
 	ret = 0;
 out:
-	if (ret == -ESTALE) {
-		if (memcmp(&prev_refs, &refs, sizeof(refs)) == 0)
-			return -EIO;
-		prev_refs = refs;
-		goto retry;
-	}
-
 	return ret;
 }
 

kmod/src/item.c

@@ -1445,6 +1445,11 @@ static int read_page_item(struct super_block *sb, struct scoutfs_key *key,
  * locks protect the stable items we read.  Invalidation is careful not
  * to drop pages that have items that we couldn't see because they were
  * dirty when we started reading.
+ *
+ * The forest item reader is reading stable trees that could be
+ * overwritten.  It can return -ESTALE which we return to the caller who
+ * will retry the operation and work with a new set of more recent
+ * btrees.
  */
 static int read_pages(struct super_block *sb, struct item_cache_info *cinf,
 		      struct scoutfs_key *key, struct scoutfs_lock *lock)
@@ -1615,7 +1620,7 @@ retry:
 					       &lock->end);
 		else
 			ret = read_pages(sb, cinf, key, lock);
-		if (ret < 0)
+		if (ret < 0 && ret != -ESTALE)
 			goto out;
 		goto retry;
 	}