Merge pull request #323 from versity/auke/freed-list-accounting

Auke/freed list accounting

kmod/src/alloc.c

@@ -25,6 +25,7 @@
 #include "alloc.h"
 #include "counters.h"
 #include "scoutfs_trace.h"
+#include "triggers.h"
 
 /*
  * The core allocator uses extent items in btrees rooted in the super.
@@ -1150,13 +1151,20 @@ static bool list_has_blocks(struct super_block *sb, struct scoutfs_alloc *alloc,
 {
 	u32 tree_blocks = extent_mod_blocks(root->root.height) * extents;
 	u32 most = 1 + tree_blocks + addl_blocks;
+	u32 avail;
+	u32 freed;
 
-	if (le32_to_cpu(alloc->avail.first_nr) < most) {
+	/* use the same room accounting as the commit hold gate, including the
+	 * pending freed-head rotation, so a clean nearly-full freed head can't
+	 * stop fill_list()/empty_list() from making progress */
+	scoutfs_alloc_meta_remaining(alloc, &avail, &freed);
+
+	if (avail < most) {
 		scoutfs_inc_counter(sb, alloc_list_avail_lo);
 		return false;
 	}
 
-	if (list_block_space(alloc->freed.first_nr) < most) {
+	if (freed < most) {
 		scoutfs_inc_counter(sb, alloc_list_freed_hi);
 		return false;
 	}
@@ -1234,6 +1242,62 @@ out:
 	return ret;
 }
 
+/*
+ * Test: stuff a freed list head to nearly full with real free blocks.
+ *
+ * Destructive: bypass filesystem consistency. Claim free blocks, append
+ * them into the head block via list_block_add (bypassing free_meta and
+ * its active-head-only path), and leak whatever we don't use.
+ *
+ * The caller can stuff both meta_freed heads in a single commits, allowing
+ * to reproduce the wedge condition. A counter validates we did the stuffing
+ * on both heads (increased twice - once for each head). An unfixed kernel
+ * will hang on mount.
+ */
+int scoutfs_alloc_fill_freed_list(struct super_block *sb, struct scoutfs_alloc *alloc,
+				  struct scoutfs_block_writer *wri,
+				  struct scoutfs_alloc_root *root,
+				  struct scoutfs_alloc_list_head *lh)
+{
+	struct alloc_ext_args args = {
+		.alloc = alloc,
+		.wri = wri,
+		.root = root,
+		.zone = SCOUTFS_FREE_EXTENT_ORDER_ZONE,
+	};
+	const u32 target = SCOUTFS_ALLOC_LIST_MAX_BLOCKS - 8;
+	struct scoutfs_alloc_list_block *lblk;
+	struct scoutfs_block *bl = NULL;
+	struct scoutfs_extent ext;
+	int ret = 0;
+	u64 old; /* leaked */
+	u64 i;
+	u32 want;
+
+	while (le32_to_cpu(lh->first_nr) < target) {
+		want = target - le32_to_cpu(lh->first_nr) + 1;
+		ret = scoutfs_ext_alloc(sb, &alloc_ext_ops, &args, 0, 0, want, &ext);
+		if (ret < 0)
+			break;
+
+		ret = dirty_list_block(sb, alloc, wri, &lh->ref, ext.start, &old, &bl);
+		if (ret < 0)
+			break;
+		lblk = bl->data;
+
+		for (i = 1; i < ext.len && le32_to_cpu(lh->first_nr) < target; i++)
+			list_block_add(lh, lblk, ext.start + i);
+
+		scoutfs_block_put(sb, bl);
+		bl = NULL;
+	}
+
+	scoutfs_block_put(sb, bl);
+	if (ret == 0 && le32_to_cpu(lh->first_nr) >= target)
+		scoutfs_inc_counter(sb, alloc_freed_fill);
+	return ret < 0 ? ret : 0;
+}
+
 /*
  * Move blknos from all the blocks in the list into extents in the root,
  * removing empty blocks as we go.  This can return success and leave blocks
@@ -1384,14 +1448,35 @@ bool scoutfs_alloc_meta_low(struct super_block *sb,
 	return lo;
 }
 
+/*
+ * Report the metadata allocator room a transaction will actually have.
+ *
+ * If the freed list hasn't been dirtied yet, the first dirtying allocation
+ * rotates in a fresh head block when the current head is under
+ * EMPTY_FREED_THRESH (see dirty_alloc_blocks()).  A clean but nearly-full
+ * head then has a fresh block's worth of room as soon as it's touched, so
+ * report that; otherwise the commit hold gate and the fill/empty drains read
+ * it as no room and refuse to make progress.  The rotation frees the old
+ * avail and freed head blocks into the fresh block, so the room it leaves is
+ * MAX - 2.
+ *
+ * dirty_freed_bl isn't covered by the seqlock, but it only transitions
+ * NULL->set on a transaction's first allocation and back to NULL at
+ * prepare_commit; a stale read predicts the rotation one allocation early or
+ * late, which still gates correctly.
+ */
 void scoutfs_alloc_meta_remaining(struct scoutfs_alloc *alloc, u32 *avail_total, u32 *freed_space)
 {
 	unsigned int seq;
+	u32 fr;
 
 	do {
 		seq = read_seqbegin(&alloc->seqlock);
 		*avail_total = le32_to_cpu(alloc->avail.first_nr);
-		*freed_space = list_block_space(alloc->freed.first_nr);
+		fr = list_block_space(alloc->freed.first_nr);
+		if (!alloc->dirty_freed_bl && fr < EMPTY_FREED_THRESH)
+			fr = SCOUTFS_ALLOC_LIST_MAX_BLOCKS - 2;
+		*freed_space = fr;
 	} while (read_seqretry(&alloc->seqlock, seq));
 }
 

kmod/src/alloc.h

@@ -152,6 +152,10 @@ int scoutfs_alloc_splice_list(struct super_block *sb,
 			      struct scoutfs_block_writer *wri,
 			      struct scoutfs_alloc_list_head *dst,
 			      struct scoutfs_alloc_list_head *src);
+int scoutfs_alloc_fill_freed_list(struct super_block *sb, struct scoutfs_alloc *alloc,
+				  struct scoutfs_block_writer *wri,
+				  struct scoutfs_alloc_root *root,
+				  struct scoutfs_alloc_list_head *lh);
 
 bool scoutfs_alloc_meta_low(struct super_block *sb,
 			    struct scoutfs_alloc *alloc, u32 nr);

kmod/src/counters.h

@@ -16,6 +16,7 @@
 	EXPAND_COUNTER(alloc_alloc_meta)			\
 	EXPAND_COUNTER(alloc_free_data)				\
 	EXPAND_COUNTER(alloc_free_meta)				\
+	EXPAND_COUNTER(alloc_freed_fill)			\
 	EXPAND_COUNTER(alloc_list_avail_lo)			\
 	EXPAND_COUNTER(alloc_list_freed_hi)			\
 	EXPAND_COUNTER(alloc_move)				\

kmod/src/fence.c

@@ -18,6 +18,7 @@
 #include <linux/sysfs.h>
 #include <linux/device.h>
 #include <linux/timer.h>
+#include <linux/mutex.h>
 #include <asm/barrier.h>
 
 #include "super.h"
@@ -65,6 +66,7 @@ struct fence_info {
 	struct kobject fence_dir_kobj;
 	struct workqueue_struct *wq;
 	wait_queue_head_t waitq;
+	struct mutex mutex;
 	spinlock_t lock;
 	struct list_head list;
 };
@@ -235,8 +237,10 @@ static void fence_timeout(struct timer_list *timer)
 int scoutfs_fence_start(struct super_block *sb, u64 rid, __be32 ipv4_addr, int reason)
 {
 	DECLARE_FENCE_INFO(sb, fi);
+	struct pending_fence *existing;
 	struct pending_fence *fence;
-	int ret;
+	bool duplicate = false;
+	int ret = 0;
 
 	fence = kzalloc(sizeof(struct pending_fence), GFP_NOFS);
 	if (!fence) {
@@ -246,6 +250,7 @@ int scoutfs_fence_start(struct super_block *sb, u64 rid, __be32 ipv4_addr, int r
 
 	fence->sb = sb;
 	scoutfs_sysfs_init_attrs(sb, &fence->ssa);
+	timer_setup(&fence->timer, fence_timeout, 0);
 
 	fence->start_kt = ktime_get();
 	fence->ipv4_addr = ipv4_addr;
@@ -254,22 +259,39 @@ int scoutfs_fence_start(struct super_block *sb, u64 rid, __be32 ipv4_addr, int r
 	fence->reason = reason;
 	fence->rid = rid;
 
+	mutex_lock(&fi->mutex);
+
+	spin_lock(&fi->lock);
+	list_for_each_entry(existing, &fi->list, entry) {
+		if (existing->rid == rid) {
+			duplicate = true;
+			break;
+		}
+	}
+	spin_unlock(&fi->lock);
+
+	if (duplicate)
+		goto unlock;
+
 	ret = scoutfs_sysfs_create_attrs_parent(sb, &fi->kset->kobj,
 						&fence->ssa, fence_attrs,
 						"%016llx", rid);
-	if (ret < 0) {
-		kfree(fence);
-		goto out;
-	}
+	if (ret < 0)
+		goto unlock;
 
-	timer_setup(&fence->timer, fence_timeout, 0);
 	fence->timer.expires = jiffies + msecs_to_jiffies(FENCE_TIMEOUT_MS);
 	add_timer(&fence->timer);
 
 	spin_lock(&fi->lock);
 	list_add_tail(&fence->entry, &fi->list);
 	spin_unlock(&fi->lock);
+
+	fence = NULL;
+unlock:
+	mutex_unlock(&fi->mutex);
 out:
+	if (fence)
+		destroy_fence(fence);
 	return ret;
 }
 
@@ -324,6 +346,8 @@ int scoutfs_fence_free(struct super_block *sb, u64 rid)
 	struct pending_fence *fence;
 	int ret = -ENOENT;
 
+	mutex_lock(&fi->mutex);
+
 	spin_lock(&fi->lock);
 	list_for_each_entry(fence, &fi->list, entry) {
 		if (fence->rid == rid) {
@@ -339,6 +363,8 @@ int scoutfs_fence_free(struct super_block *sb, u64 rid)
 		wake_up(&fi->waitq);
 	}
 
+	mutex_unlock(&fi->mutex);
+
 	return ret;
 }
 
@@ -413,6 +439,7 @@ int scoutfs_fence_setup(struct super_block *sb)
 	}
 
 	init_waitqueue_head(&fi->waitq);
+	mutex_init(&fi->mutex);
 	spin_lock_init(&fi->lock);
 	INIT_LIST_HEAD(&fi->list);
 
@@ -446,6 +473,7 @@ void scoutfs_fence_stop(struct super_block *sb)
 	DECLARE_FENCE_INFO(sb, fi);
 	struct pending_fence *fence;
 
+	mutex_lock(&fi->mutex);
 	do {
 		spin_lock(&fi->lock);
 		fence = list_first_entry_or_null(&fi->list, struct pending_fence, entry);
@@ -458,20 +486,18 @@ void scoutfs_fence_stop(struct super_block *sb)
 			wake_up(&fi->waitq);
 		}
 	} while (fence);
+	mutex_unlock(&fi->mutex);
 }
 
 void scoutfs_fence_destroy(struct super_block *sb)
 {
 	struct scoutfs_sb_info *sbi = SCOUTFS_SB(sb);
 	struct fence_info *fi = SCOUTFS_SB(sb)->fence_info;
-	struct pending_fence *fence;
-	struct pending_fence *tmp;
 
 	if (fi) {
 		if (fi->wq)
 			destroy_workqueue(fi->wq);
-		list_for_each_entry_safe(fence, tmp, &fi->list, entry)
-			destroy_fence(fence);
+		scoutfs_fence_stop(sb);
 		if (fi->kset)
 			kset_unregister(fi->kset);
 		kfree(fi);

kmod/src/server.c

@@ -623,6 +623,16 @@ static void scoutfs_server_commit_func(struct work_struct *work)
 		goto out;
 	}
 
+	/* test-only: manufacture the deadlock by stuffing both freed heads full
+	 * in this one commit, so there's no window for empty_list to drain one
+	 * before the other fills */
+	if (scoutfs_trigger(sb, ALLOC_FILL_FREED_LIST)) {
+		scoutfs_alloc_fill_freed_list(sb, &server->alloc, &server->wri,
+					      server->meta_avail, &server->alloc.freed);
+		scoutfs_alloc_fill_freed_list(sb, &server->alloc, &server->wri,
+					      server->meta_avail, server->other_freed);
+	}
+
 	/* make sure next avail has sufficient blocks */
 	ret = scoutfs_alloc_fill_list(sb, &server->alloc, &server->wri,
 				      server->other_avail,

kmod/src/super.c

@@ -627,6 +627,19 @@ static int scoutfs_fill_super(struct super_block *sb, void *data, int silent)
 	scoutfs_trans_restart_sync_deadline(sb);
 	ret = 0;
 out:
+	if (ret) {
+		/*
+		 * The mount failed and we're about to tear down, either here
+		 * or via generic_shutdown_super if s_root was set. Any worker
+		 * started during fill_super can be blocked in an uninterruptible
+		 * net request to a server that will never respond. Force the
+		 * client connection down so those pending requests abort with
+		 * -ECONNABORTED before teardown.
+		 */
+		SCOUTFS_SB(sb)->forced_unmount = true;
+		scoutfs_client_net_shutdown(sb);
+	}
+
 	/* on error, generic_shutdown_super calls put_super if s_root */
 	if (ret && !sb->s_root)
 		scoutfs_put_super(sb);

kmod/src/triggers.c

@@ -47,6 +47,7 @@ static char *names[] = {
 	[SCOUTFS_TRIGGER_STATFS_LOCK_PURGE] = "statfs_lock_purge",
 	[SCOUTFS_TRIGGER_RECLAIM_SKIP_FINALIZE] = "reclaim_skip_finalize",
 	[SCOUTFS_TRIGGER_LOG_MERGE_FORCE_PARTIAL] = "log_merge_force_partial",
+	[SCOUTFS_TRIGGER_ALLOC_FILL_FREED_LIST] = "alloc_fill_freed_list",
 };
 
 bool scoutfs_trigger_test_and_clear(struct super_block *sb, unsigned int t)

kmod/src/triggers.h

@@ -10,6 +10,7 @@ enum scoutfs_trigger {
 	SCOUTFS_TRIGGER_STATFS_LOCK_PURGE,
 	SCOUTFS_TRIGGER_RECLAIM_SKIP_FINALIZE,
 	SCOUTFS_TRIGGER_LOG_MERGE_FORCE_PARTIAL,
+	SCOUTFS_TRIGGER_ALLOC_FILL_FREED_LIST,
 	SCOUTFS_TRIGGER_NR,
 };
 

tests/golden/freed-list-wedge

@@ -0,0 +1,8 @@
+== make throwaway scratch fs
+== stuff both server freed heads full in one commit
+== confirm both heads were stuffed (not a no-op)
+both freed heads stuffed
+== a fixed server drains them and keeps making progress
+one
+two
+== cleanup scratch fs

tests/sequence

@@ -62,6 +62,7 @@ client-unmount-recovery.sh
 createmany-parallel-mounts.sh
 archive-light-cycle.sh
 block-stale-reads.sh
+freed-list-wedge.sh
 inode-deletion.sh
 renameat2-noreplace.sh
 xfstests.sh

tests/tests/freed-list-wedge.sh

@@ -0,0 +1,39 @@
+#
+# Destructive: the alloc_fill_freed_list trigger claims free blocks and leaks
+# them into both server_meta_freed heads in one commit, filling them near-full.
+# Runs on a scratch fs; mkfs before re-use.
+#
+# A fixed server drains the full heads and stays live; an unfixed server wedges
+# on the next commit and hangs until the harness times it out.
+#
+
+scr_counter() {
+	cat "$(t_sysfs_path_from_mnt "$T_MSCR")/counters/$1"
+}
+
+echo "== make throwaway scratch fs"
+t_scratch_mkfs
+t_scratch_mount
+
+echo "== stuff both server freed heads full in one commit"
+old=$(scr_counter alloc_freed_fill)
+
+echo 1 > "/sys/kernel/debug/scoutfs/$(t_ident_from_mnt "$T_MSCR")/trigger/alloc_fill_freed_list"
+echo one > "$T_MSCR/one"; sync
+
+echo "== confirm both heads were stuffed (not a no-op)"
+filled=$(($(scr_counter alloc_freed_fill) - old))
+test "$filled" -ge 2 && echo "both freed heads stuffed" || \
+	echo "stuff was a no-op ($filled heads)"
+
+echo "== a fixed server drains them and keeps making progress"
+# an unfixed server wedges on this commit
+echo two > "$T_MSCR/two"; sync
+cat "$T_MSCR/one" "$T_MSCR/two"
+
+rm -f "$T_MSCR/one" "$T_MSCR/two"; sync
+
+echo "== cleanup scratch fs"
+t_scratch_umount
+
+t_pass

utils/src/print.c

@@ -375,6 +375,11 @@ static int print_srch_root_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 	if (val) {
 		if (key->sk_type == SCOUTFS_SRCH_PENDING_TYPE ||
 		    key->sk_type == SCOUTFS_SRCH_BUSY_TYPE) {
+			if (val_len < sizeof(*sc)) {
+				printf("      (short srch compact value: val_len %u)\n",
+				       val_len);
+				return 0;
+			}
 			sc = val;
 			printf("      compact %s: nr %u flags 0x%x\n",
 			       key->sk_type == SCOUTFS_SRCH_PENDING_TYPE ?
@@ -387,6 +392,11 @@ static int print_srch_root_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 				       SRF_A(&sc->in[i].sfl));
 			}
 		} else {
+			if (val_len < sizeof(*sfl)) {
+				printf("      (short srch file value: val_len %u)\n",
+				       val_len);
+				return 0;
+			}
 			sfl = val;
 			printf("      "SRF_FMT"\n", SRF_A(sfl));
 		}
@@ -398,9 +408,25 @@ static int print_srch_root_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 static int print_mounted_client_entry(struct scoutfs_key *key, u64 seq, u8 flags, void *val,
 				      unsigned val_len, void *arg)
 {
-	struct scoutfs_mounted_client_btree_val *mcv = val;
+	struct scoutfs_mounted_client_btree_val *mcv;
 	struct in_addr in;
 
+	/*
+	 * Parent block items reference child blocks and have no value;
+	 * print_block_ref() calls us with a NULL val just to print the key.
+	 */
+	if (!val) {
+		printf("    rid %016llx\n", le64_to_cpu(key->skmc_rid));
+		return 0;
+	}
+
+	if (val_len < sizeof(*mcv)) {
+		printf("    rid %016llx (short mounted client value: val_len %u)\n",
+		       le64_to_cpu(key->skmc_rid), val_len);
+		return 0;
+	}
+
+	mcv = val;
 	memset(&in, 0, sizeof(in));
 	in.s_addr = htonl(le32_to_cpu(mcv->addr.v4.addr));
 
@@ -419,8 +445,17 @@ static int print_log_merge_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 	struct scoutfs_log_merge_complete *comp;
 	struct scoutfs_log_merge_freeing *fr;
 
+	/*
+	 * Parent block items reference child blocks and have no value;
+	 * print_block_ref() calls us with a NULL val just to print the key.
+	 */
+	if (!val)
+		return 0;
+
 	switch (key->sk_zone) {
 	case SCOUTFS_LOG_MERGE_STATUS_ZONE:
+		if (val_len < sizeof(*stat))
+			goto bad_len;
 		stat = val;
 		printf("    status: next_range_key "SK_FMT" nr_req %llu nr_comp %llu seq %llu\n",
 		       SK_ARG(&stat->next_range_key),
@@ -429,12 +464,16 @@ static int print_log_merge_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 		       le64_to_cpu(stat->seq));
 		break;
 	case SCOUTFS_LOG_MERGE_RANGE_ZONE:
+		if (val_len < sizeof(*rng))
+			goto bad_len;
 		rng = val;
 		printf("    range: start "SK_FMT" end "SK_FMT"\n",
 		       SK_ARG(&rng->start),
 		       SK_ARG(&rng->end));
 		break;
 	case SCOUTFS_LOG_MERGE_REQUEST_ZONE:
+		if (val_len < sizeof(*req))
+			goto bad_len;
 		req = val;
 		printf("    request: logs_root "BTROOT_F" logs_root "BTROOT_F" start "SK_FMT
 		       " end "SK_FMT" input_seq %llu rid %016llx seq %llu flags 0x%llx\n",
@@ -448,6 +487,8 @@ static int print_log_merge_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 		       le64_to_cpu(req->flags));
 		break;
 	case SCOUTFS_LOG_MERGE_COMPLETE_ZONE:
+		if (val_len < sizeof(*comp))
+			goto bad_len;
 		comp = val;
 		printf("    complete: root "BTROOT_F" start "SK_FMT" end "SK_FMT
 		       " remain "SK_FMT" rid %016llx seq %llu flags %llx\n",
@@ -460,6 +501,8 @@ static int print_log_merge_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 		       le64_to_cpu(comp->flags));
 		break;
 	case SCOUTFS_LOG_MERGE_FREEING_ZONE:
+		if (val_len < sizeof(*fr))
+			goto bad_len;
 		fr = val;
 		printf("    freeing: root "BTROOT_F" key "SK_FMT" seq %llu\n",
 		       BTROOT_A(&fr->root),
@@ -472,6 +515,11 @@ static int print_log_merge_item(struct scoutfs_key *key, u64 seq, u8 flags, void
 	}
 
 	return 0;
+
+bad_len:
+	printf("    (short log merge value: zone %u val_len %u)\n",
+	       key->sk_zone, val_len);
+	return 0;
 }
 
 static int print_alloc_item(struct scoutfs_key *key, u64 seq, u8 flags, void *val,