Re: [PATCH v2] btrfs: use atomic64_t for free_objectid

[David Sterba <dsterba@suse.cz>]

On Tue, Mar 04, 2025 at 10:52:56AM +0100, David Sterba wrote:
> > -	ASSERT(root->free_objectid <= BTRFS_LAST_FREE_OBJECTID);
> > -
> > -	mutex_unlock(&root->objectid_mutex);
> > +	ASSERT((u64)atomic64_read(&root->free_objectid) <=
> > +		BTRFS_LAST_FREE_OBJECTID);
> 
> I'm not sure if this was mentioned in the previous discussion. This
> assert will be always true, the atomic is signed 64 and cast to
> unsigned. Under normal circumstances the atomic will not be negative so
> it won't translate to a huge unsigned number by the cast.
> 
> What we need is an unsigned atomic type. The atomic64_t is a natural
> choice and it probably has enough margin for the simple increment
> allocation. But still I think we should make it a "u64".
> 
> The simplest implementation is to use spin lock around the updates,
> seqlock_t is also possible but it effectively uses a spin lock too and
> we don't need the read side protection.
> 
> Sorry, it's another change right before the code freeze so we may want
> to postpone it to let us think it through. I'll prototype the idea and
> do some tests, we can still target 6.15.

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 075a06db43a1..2620403fd4c9 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -179,8 +179,6 @@ struct btrfs_root {
 	struct btrfs_fs_info *fs_info;
 	struct extent_io_tree dirty_log_pages;
 
-	struct mutex objectid_mutex;
-
 	spinlock_t accounting_lock;
 	struct btrfs_block_rsv *block_rsv;
 
@@ -214,7 +212,9 @@ struct btrfs_root {
 
 	u64 last_trans;
 
+	/* Locking is done only when incremented, read size relies on u64. */
 	u64 free_objectid;
+	spinlock_t objectid_lock;
 
 	struct btrfs_key defrag_progress;
 	struct btrfs_key defrag_max;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0cb559448933..e5569cec0547 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -676,7 +676,7 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
 	spin_lock_init(&root->ordered_extent_lock);
 	spin_lock_init(&root->accounting_lock);
 	spin_lock_init(&root->qgroup_meta_rsv_lock);
-	mutex_init(&root->objectid_mutex);
+	spin_lock_init(&root->objectid_lock);
 	mutex_init(&root->log_mutex);
 	mutex_init(&root->ordered_extent_mutex);
 	mutex_init(&root->delalloc_mutex);
@@ -1132,17 +1132,12 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev)
 		}
 	}
 
-	mutex_lock(&root->objectid_mutex);
 	ret = btrfs_init_root_free_objectid(root);
-	if (ret) {
-		mutex_unlock(&root->objectid_mutex);
+	if (ret)
 		return ret;
-	}
 
 	ASSERT(root->free_objectid <= BTRFS_LAST_FREE_OBJECTID);
 
-	mutex_unlock(&root->objectid_mutex);
-
 	return 0;
 }
 
@@ -2725,8 +2720,8 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 		}
 
 		/*
-		 * No need to hold btrfs_root::objectid_mutex since the fs
-		 * hasn't been fully initialised and we are the only user
+		 * No need to lock btrfs_root::free_objectid since the fs
+		 * hasn't been fully initialised and we are the only user.
 		 */
 		ret = btrfs_init_root_free_objectid(tree_root);
 		if (ret < 0) {
@@ -4930,20 +4925,21 @@ int btrfs_init_root_free_objectid(struct btrfs_root *root)
 
 int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
 {
-	int ret;
-	mutex_lock(&root->objectid_mutex);
+	u64 val;
 
-	if (unlikely(root->free_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
+	spin_lock(&root->objectid_lock);
+	val = root->free_objectid;
+	if (unlikely(val >= BTRFS_LAST_FREE_OBJECTID)) {
+		spin_unlock(&root->objectid_lock);
 		btrfs_warn(root->fs_info,
 			   "the objectid of root %llu reaches its highest value",
 			   btrfs_root_id(root));
-		ret = -ENOSPC;
-		goto out;
+		return -ENOSPC;
 	}
+	root->free_objectid = val + 1;
+	spin_unlock(&root->objectid_lock);
 
-	*objectid = root->free_objectid++;
-	ret = 0;
-out:
-	mutex_unlock(&root->objectid_mutex);
-	return ret;
+	*objectid = val;
+
+	return 0;
 }
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index d6fa36674270..1ce84bf59a09 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -472,9 +472,9 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 			 *
 			 * Ensure that we skip any such subvol ids.
 			 *
-			 * We don't need to lock because this is only called
-			 * during mount before we start doing things like creating
-			 * subvolumes.
+			 * We don't need to worry about updates to free_objectid,
+			 * this is only called during mount before we start
+			 * doing things like creating subvolumes.
 			 */
 			if (is_fstree(qgroup->qgroupid) &&
 			    qgroup->qgroupid > tree_root->free_objectid)
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index fc5c761181eb..97e608b251fa 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -7325,9 +7325,6 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 			 * We have just replayed everything, and the highest
 			 * objectid of fs roots probably has changed in case
 			 * some inode_item's got replayed.
-			 *
-			 * root->objectid_mutex is not acquired as log replay
-			 * could only happen during mount.
 			 */
 			ret = btrfs_init_root_free_objectid(root);
 			if (ret)
-- 
2.47.1