[PATCH 0/6] Overhaul free objectid code

[PATCH 0/6] Overhaul free objectid code

[Nikolay Borisov]

This series aims to make the free objectid code more straighforward. Currently
the highest used objectid is used which implies that when btrfs_get_free_objectid
is called the pre-increment operator is used. At the same time when looking
at how highest_objectid is initialised in find_free_objectid it's using the,
at first looko unusual, 'BTRFS_FREE_OBJECTID - 1'. Furthermore btrfs_find_free_objectid
is badly named as it's used only in initializaion context.

With the series applied the following is achieved:
 * The 2 functions related to free objectid have better naming which describes
 their semantic meaning.

 * highest_objectid is renamed to free_objectid which clearly states what it's
 supposed to hold, also btrfs_get_free_objectid now returns the value and
 does a post-increment which seems more logical than the previous cod.

 * Now it's not necessary to re-initialize free_objectid in create_subvol
 since that member is now consistently initialized when a given root is read
 for the first time in btrfs_init_fs_root->btrfs_init_root_free_objectid.
 Additionally in btrfs_init_root_free_objectid free_objectid is now initialized
 to BTRFS_FIRST_FREE_OBJECTID so it's self-explanatory.

This series survived xfstest as well as a new xfstest which verifies precisely
this functionality.


Nikolay Borisov (6):
  btrfs: Rename btrfs_find_highest_objectid to
    btrfs_init_root_free_objectid
  btrfs: Rename btrfs_find_free_objectid to btrfs_get_free_objectid
  btrfs: Remove useless ASSERTS
  btrfs: Rename highest_objectid to free_objectid
  btrfs: Make free_objectid hold the next available objectid in the root
  btrfs: Remove new_dirid argument from btrfs_create_subvol_root

 fs/btrfs/ctree.h            |  5 ++---
 fs/btrfs/disk-io.c          | 25 ++++++++++---------------
 fs/btrfs/disk-io.h          |  4 ++--
 fs/btrfs/free-space-cache.c |  2 +-
 fs/btrfs/inode.c            | 23 +++++++++++++----------
 fs/btrfs/ioctl.c            | 11 +++--------
 fs/btrfs/relocation.c       |  2 +-
 fs/btrfs/transaction.c      |  2 +-
 fs/btrfs/tree-log.c         |  3 +--
 9 files changed, 34 insertions(+), 43 deletions(-)

--
2.25.1

[PATCH 1/6] btrfs: Rename btrfs_find_highest_objectid to btrfs_init_root_free_objectid

[Nikolay Borisov]

This function is used to initialize the in-memory
btrfs_root::highest_objectid member, which is used to get an available
objectid. Rename it to better reflect its semantics.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c  | 12 +++++-------
 fs/btrfs/disk-io.h  |  2 +-
 fs/btrfs/tree-log.c |  3 +--
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 39458baf8745..c5353767edef 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1367,8 +1367,7 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev)
 	}
 
 	mutex_lock(&root->objectid_mutex);
-	ret = btrfs_find_highest_objectid(root,
-					&root->highest_objectid);
+	ret = btrfs_init_root_free_objectid(root);
 	if (ret) {
 		mutex_unlock(&root->objectid_mutex);
 		goto fail;
@@ -2646,8 +2645,7 @@ 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
 		 */
-		ret = btrfs_find_highest_objectid(tree_root,
-						&tree_root->highest_objectid);
+		ret = btrfs_init_root_free_objectid(tree_root);
 		if (ret < 0) {
 			handle_error = true;
 			continue;
@@ -4743,7 +4741,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 	return 0;
 }
 
-int btrfs_find_highest_objectid(struct btrfs_root *root, u64 *objectid)
+int btrfs_init_root_free_objectid(struct btrfs_root *root)
 {
 	struct btrfs_path *path;
 	int ret;
@@ -4767,10 +4765,10 @@ int btrfs_find_highest_objectid(struct btrfs_root *root, u64 *objectid)
 		slot = path->slots[0] - 1;
 		l = path->nodes[0];
 		btrfs_item_key_to_cpu(l, &found_key, slot);
-		*objectid = max_t(u64, found_key.objectid,
+		root->highest_objectid = max_t(u64, found_key.objectid,
 				  BTRFS_FIRST_FREE_OBJECTID - 1);
 	} else {
-		*objectid = BTRFS_FIRST_FREE_OBJECTID - 1;
+		root->highest_objectid = BTRFS_FIRST_FREE_OBJECTID - 1;
 	}
 	ret = 0;
 error:
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index e45057c0c016..5e5bc603fbdf 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -134,7 +134,7 @@ int btree_lock_page_hook(struct page *page, void *data,
 				void (*flush_fn)(void *));
 int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags);
 int btrfs_find_free_objectid(struct btrfs_root *root, u64 *objectid);
-int btrfs_find_highest_objectid(struct btrfs_root *root, u64 *objectid);
+int btrfs_init_root_free_objectid(struct btrfs_root *root);
 int __init btrfs_end_io_wq_init(void);
 void __cold btrfs_end_io_wq_exit(void);
 
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 254c2ee43aae..8ee0700a980f 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -6307,8 +6307,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 			 * root->objectid_mutex is not acquired as log replay
 			 * could only happen during mount.
 			 */
-			ret = btrfs_find_highest_objectid(root,
-						  &root->highest_objectid);
+			ret = btrfs_init_root_free_objectid(root);
 		}
 
 		wc.replay_dest->log_root = NULL;
-- 
2.25.1

[PATCH 2/6] btrfs: Rename btrfs_find_free_objectid to btrfs_get_free_objectid

[Nikolay Borisov]

This better reflects the semantics of the function i.e no search is
performed whatsoever.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c          |  2 +-
 fs/btrfs/disk-io.h          |  2 +-
 fs/btrfs/free-space-cache.c |  2 +-
 fs/btrfs/inode.c            | 12 ++++++------
 fs/btrfs/ioctl.c            |  2 +-
 fs/btrfs/relocation.c       |  2 +-
 fs/btrfs/transaction.c      |  2 +-
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c5353767edef..4c3dda0034b5 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4776,7 +4776,7 @@ int btrfs_init_root_free_objectid(struct btrfs_root *root)
 	return ret;
 }
 
-int btrfs_find_free_objectid(struct btrfs_root *root, u64 *objectid)
+int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
 {
 	int ret;
 	mutex_lock(&root->objectid_mutex);
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 5e5bc603fbdf..9f4a2a1e3d36 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -133,7 +133,7 @@ struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
 int btree_lock_page_hook(struct page *page, void *data,
 				void (*flush_fn)(void *));
 int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags);
-int btrfs_find_free_objectid(struct btrfs_root *root, u64 *objectid);
+int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid);
 int btrfs_init_root_free_objectid(struct btrfs_root *root);
 int __init btrfs_end_io_wq_init(void);
 void __cold btrfs_end_io_wq_exit(void);
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index a62fdfa1de39..ebc3f0417f54 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -198,7 +198,7 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
 	int ret;
 	u64 ino;
 
-	ret = btrfs_find_free_objectid(trans->fs_info->tree_root, &ino);
+	ret = btrfs_get_free_objectid(trans->fs_info->tree_root, &ino);
 	if (ret < 0)
 		return ret;
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 393c1232d949..0dee1fa29207 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6356,7 +6356,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry,
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	err = btrfs_find_free_objectid(root, &objectid);
+	err = btrfs_get_free_objectid(root, &objectid);
 	if (err)
 		goto out_unlock;
 
@@ -6420,7 +6420,7 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry,
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	err = btrfs_find_free_objectid(root, &objectid);
+	err = btrfs_get_free_objectid(root, &objectid);
 	if (err)
 		goto out_unlock;
 
@@ -6564,7 +6564,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	err = btrfs_find_free_objectid(root, &objectid);
+	err = btrfs_get_free_objectid(root, &objectid);
 	if (err)
 		goto out_fail;
 
@@ -9064,7 +9064,7 @@ static int btrfs_whiteout_for_rename(struct btrfs_trans_handle *trans,
 	u64 objectid;
 	u64 index;
 
-	ret = btrfs_find_free_objectid(root, &objectid);
+	ret = btrfs_get_free_objectid(root, &objectid);
 	if (ret)
 		return ret;
 
@@ -9534,7 +9534,7 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry,
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	err = btrfs_find_free_objectid(root, &objectid);
+	err = btrfs_get_free_objectid(root, &objectid);
 	if (err)
 		goto out_unlock;
 
@@ -9868,7 +9868,7 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	ret = btrfs_find_free_objectid(root, &objectid);
+	ret = btrfs_get_free_objectid(root, &objectid);
 	if (ret)
 		goto out;
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index c23243c7800c..8ae38806211c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -613,7 +613,7 @@ static noinline int create_subvol(struct inode *dir,
 	if (!root_item)
 		return -ENOMEM;
 
-	ret = btrfs_find_free_objectid(fs_info->tree_root, &objectid);
+	ret = btrfs_get_free_objectid(fs_info->tree_root, &objectid);
 	if (ret)
 		goto fail_free;
 
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 19b7db8b2117..30a80669647f 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3429,7 +3429,7 @@ struct inode *create_reloc_inode(struct btrfs_fs_info *fs_info,
 		return ERR_CAST(trans);
 	}
 
-	err = btrfs_find_free_objectid(root, &objectid);
+	err = btrfs_get_free_objectid(root, &objectid);
 	if (err)
 		goto out;
 
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index b08283829abf..85cb619ba7d5 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1526,7 +1526,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	ASSERT(pending->root_item);
 	new_root_item = pending->root_item;
 
-	pending->error = btrfs_find_free_objectid(tree_root, &objectid);
+	pending->error = btrfs_get_free_objectid(tree_root, &objectid);
 	if (pending->error)
 		goto no_free_objectid;
 
-- 
2.25.1

[PATCH 3/6] btrfs: Remove useless ASSERTS

[Nikolay Borisov]

The invariants the asserts are checking are already verified by the
tree checker, just remove them.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4c3dda0034b5..eaaece2bf0c8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1373,8 +1373,6 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev)
 		goto fail;
 	}
 
-	ASSERT(root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
-
 	mutex_unlock(&root->objectid_mutex);
 
 	return 0;
@@ -2651,7 +2649,6 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 			continue;
 		}
 
-		ASSERT(tree_root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
 
 		ret = btrfs_read_roots(fs_info);
 		if (ret < 0) {
-- 
2.25.1

Re: [PATCH 3/6] btrfs: Remove useless ASSERTS

[David Sterba]

On Mon, Dec 07, 2020 at 05:32:34PM +0200, Nikolay Borisov wrote:
> The invariants the asserts are checking are already verified by the
> tree checker, just remove them.

I haven't found where exactly does tree-checker verify the invariant and
also think that we can safely leave the asserts there. Even if it's for
a normally impossible case, assertions usually catch bugs after changing
some other code.

Re: [PATCH 3/6] btrfs: Remove useless ASSERTS

[Nikolay Borisov]

On 15.12.20 г. 18:58 ч., David Sterba wrote:
> On Mon, Dec 07, 2020 at 05:32:34PM +0200, Nikolay Borisov wrote:
>> The invariants the asserts are checking are already verified by the
>> tree checker, just remove them.
> 
> I haven't found where exactly does tree-checker verify the invariant and
> also think that we can safely leave the asserts there. Even if it's for
> a normally impossible case, assertions usually catch bugs after changing
> some other code.
> 

   2         if (unlikely((key->objectid < BTRFS_                                           
     1                       key->objectid > BTRFS_ #define BTRFS_ROOT_TREE_DIR_OBJECTID 6ULL 
  402                       key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&           
     1                      key->objectid != BTRFS_FREE_INO_OBJECTID)) { 


in check_inode_key. We verify that for every inode its objectid is within range, transitively this assures highest_objectid is also within range. But If you want to leave it - I'm fine with it. 

Re: [PATCH 3/6] btrfs: Remove useless ASSERTS

[David Sterba]

On Tue, Dec 15, 2020 at 07:48:18PM +0200, Nikolay Borisov wrote:
> 
> 
> On 15.12.20 г. 18:58 ч., David Sterba wrote:
> > On Mon, Dec 07, 2020 at 05:32:34PM +0200, Nikolay Borisov wrote:
> >> The invariants the asserts are checking are already verified by the
> >> tree checker, just remove them.
> > 
> > I haven't found where exactly does tree-checker verify the invariant and
> > also think that we can safely leave the asserts there. Even if it's for
> > a normally impossible case, assertions usually catch bugs after changing
> > some other code.
> > 
> 
>    2         if (unlikely((key->objectid < BTRFS_                                           
>      1                       key->objectid > BTRFS_ #define BTRFS_ROOT_TREE_DIR_OBJECTID 6ULL 
>   402                       key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&           
>      1                      key->objectid != BTRFS_FREE_INO_OBJECTID)) { 
> 
> 
> in check_inode_key. We verify that for every inode its objectid is
> within range, transitively

Ah so it's only indirectly implied.

> this assures highest_objectid is also
> within range. But If you want to leave it - I'm fine with it. 

Tree checker verifies that any inode that is read has the object id
within the bounds, that's fine. The highest free objectid is obtained
by doing reverse search, without reading (and checking) any existing
inode.

btrfs_init_root_free_objectid checks only object ids in the tree, not
necessarily inodes (though technically we use the objectids only for
inode-like items).

Things can be improved by doing proper checks inside
btrfs_init_root_free_objectid and drop the asserts, I can imagine a
crafted image that would trigger the asserts so we'd better handle that.

[PATCH 4/6] btrfs: Rename highest_objectid to free_objectid

[Nikolay Borisov]

This reflects the true purpose of the member as it's being used solely
in context where a new objectid is being allocated. Future changes will
also change the way it's being used to closely follow this semantics.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h   |  2 +-
 fs/btrfs/disk-io.c | 10 +++++-----
 fs/btrfs/ioctl.c   |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index dac0f7b70a68..e8652c73c189 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1101,7 +1101,7 @@ struct btrfs_root {
 
 	u32 type;
 
-	u64 highest_objectid;
+	u64 free_objectid;
 
 	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 eaaece2bf0c8..e96103b1c5db 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1016,7 +1016,7 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
 	root->orphan_cleanup_state = 0;
 
 	root->last_trans = 0;
-	root->highest_objectid = 0;
+	root->free_objectid = 0;
 	root->nr_delalloc_inodes = 0;
 	root->nr_ordered_extents = 0;
 	root->inode_tree = RB_ROOT;
@@ -4762,10 +4762,10 @@ int btrfs_init_root_free_objectid(struct btrfs_root *root)
 		slot = path->slots[0] - 1;
 		l = path->nodes[0];
 		btrfs_item_key_to_cpu(l, &found_key, slot);
-		root->highest_objectid = max_t(u64, found_key.objectid,
+		root->free_objectid = max_t(u64, found_key.objectid,
 				  BTRFS_FIRST_FREE_OBJECTID - 1);
 	} else {
-		root->highest_objectid = BTRFS_FIRST_FREE_OBJECTID - 1;
+		root->free_objectid = BTRFS_FIRST_FREE_OBJECTID - 1;
 	}
 	ret = 0;
 error:
@@ -4778,7 +4778,7 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
 	int ret;
 	mutex_lock(&root->objectid_mutex);
 
-	if (unlikely(root->highest_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
+	if (unlikely(root->free_objectid >= BTRFS_LAST_FREE_OBJECTID)) {
 		btrfs_warn(root->fs_info,
 			   "the objectid of root %llu reaches its highest value",
 			   root->root_key.objectid);
@@ -4786,7 +4786,7 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
 		goto out;
 	}
 
-	*objectid = ++root->highest_objectid;
+	*objectid = ++root->free_objectid;
 	ret = 0;
 out:
 	mutex_unlock(&root->objectid_mutex);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 8ae38806211c..21c0215a0fd0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -725,7 +725,7 @@ static noinline int create_subvol(struct inode *dir,
 	}
 
 	mutex_lock(&new_root->objectid_mutex);
-	new_root->highest_objectid = new_dirid;
+	new_root->free_objectid = new_dirid;
 	mutex_unlock(&new_root->objectid_mutex);
 
 	/*
-- 
2.25.1

[PATCH 5/6] btrfs: Make free_objectid hold the next available objectid in the root

[Nikolay Borisov]

Adjust the way free_objectid is being initialized, it now stores
BTRFS_FIRST_FREE_OBJECTID rather than the, somewhat arbitrary,
BTRFS_FIRST_FREE_OBJECTID - 1. This change also has the added benefit
that now it becomes unnecessary to explicitly initialize free_objectid
for a newly create fs root.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 8 ++++----
 fs/btrfs/inode.c   | 8 ++++++--
 fs/btrfs/ioctl.c   | 4 ----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index e96103b1c5db..6a4d7ea61903 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4762,10 +4762,10 @@ int btrfs_init_root_free_objectid(struct btrfs_root *root)
 		slot = path->slots[0] - 1;
 		l = path->nodes[0];
 		btrfs_item_key_to_cpu(l, &found_key, slot);
-		root->free_objectid = max_t(u64, found_key.objectid,
-				  BTRFS_FIRST_FREE_OBJECTID - 1);
+		root->free_objectid = max_t(u64, found_key.objectid + 1,
+				  BTRFS_FIRST_FREE_OBJECTID);
 	} else {
-		root->free_objectid = BTRFS_FIRST_FREE_OBJECTID - 1;
+		root->free_objectid = BTRFS_FIRST_FREE_OBJECTID;
 	}
 	ret = 0;
 error:
@@ -4786,7 +4786,7 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
 		goto out;
 	}
 
-	*objectid = ++root->free_objectid;
+	*objectid = root->free_objectid++;
 	ret = 0;
 out:
 	mutex_unlock(&root->objectid_mutex);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0dee1fa29207..aea76abd83ba 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8583,9 +8583,13 @@ int btrfs_create_subvol_root(struct btrfs_trans_handle *trans,
 	struct inode *inode;
 	int err;
 	u64 index = 0;
+	u64 ino;
+
+	err = btrfs_get_free_objectid(new_root, &ino);
+	if (err < 0)
+		return err;
 
-	inode = btrfs_new_inode(trans, new_root, NULL, "..", 2,
-				new_dirid, new_dirid,
+	inode = btrfs_new_inode(trans, new_root, NULL, "..", 2, ino, ino,
 				S_IFDIR | (~current_umask() & S_IRWXUGO),
 				&index);
 	if (IS_ERR(inode))
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 21c0215a0fd0..58b9a27559c0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -724,10 +724,6 @@ static noinline int create_subvol(struct inode *dir,
 		goto fail;
 	}
 
-	mutex_lock(&new_root->objectid_mutex);
-	new_root->free_objectid = new_dirid;
-	mutex_unlock(&new_root->objectid_mutex);
-
 	/*
 	 * insert the directory item
 	 */
-- 
2.25.1

[PATCH 6/6] btrfs: Remove new_dirid argument from btrfs_create_subvol_root

[Nikolay Borisov]

It's no longer used. While at it also remove new_dirid in create_subvol
as it's used in a single place and open code it. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h | 3 +--
 fs/btrfs/inode.c | 3 +--
 fs/btrfs/ioctl.c | 5 ++---
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index e8652c73c189..616a4c6fefc3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3089,8 +3089,7 @@ int btrfs_set_extent_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
 			      struct extent_state **cached_state);
 int btrfs_create_subvol_root(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *new_root,
-			     struct btrfs_root *parent_root,
-			     u64 new_dirid);
+			     struct btrfs_root *parent_root);
  void btrfs_set_delalloc_extent(struct inode *inode, struct extent_state *state,
 			       unsigned *bits);
 void btrfs_clear_delalloc_extent(struct inode *inode,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index aea76abd83ba..b82c5bde6a3e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8577,8 +8577,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
  */
 int btrfs_create_subvol_root(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *new_root,
-			     struct btrfs_root *parent_root,
-			     u64 new_dirid)
+			     struct btrfs_root *parent_root)
 {
 	struct inode *inode;
 	int err;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 58b9a27559c0..db29c1b54daa 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -606,7 +606,6 @@ static noinline int create_subvol(struct inode *dir,
 	int err;
 	dev_t anon_dev = 0;
 	u64 objectid;
-	u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
 	u64 index = 0;
 
 	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
@@ -693,7 +692,7 @@ static noinline int create_subvol(struct inode *dir,
 	free_extent_buffer(leaf);
 	leaf = NULL;
 
-	btrfs_set_root_dirid(root_item, new_dirid);
+	btrfs_set_root_dirid(root_item, BTRFS_FIRST_FREE_OBJECTID);
 
 	key.objectid = objectid;
 	key.offset = 0;
@@ -716,7 +715,7 @@ static noinline int create_subvol(struct inode *dir,
 
 	btrfs_record_root_in_trans(trans, new_root);
 
-	ret = btrfs_create_subvol_root(trans, new_root, root, new_dirid);
+	ret = btrfs_create_subvol_root(trans, new_root, root);
 	btrfs_put_root(new_root);
 	if (ret) {
 		/* We potentially lose an unused inode item here */
-- 
2.25.1

[PATCH] btrfs: Add test 154

[Nikolay Borisov]

This test verifies btrfs' free objectid management. I.e it ensures that
the first objectid is always 256 in an fs tree.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tests/btrfs/154     | 80 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/154.out |  2 ++
 tests/btrfs/group   |  1 +
 3 files changed, 83 insertions(+)
 create mode 100755 tests/btrfs/154
 create mode 100644 tests/btrfs/154.out

diff --git a/tests/btrfs/154 b/tests/btrfs/154
new file mode 100755
index 000000000000..6aee204e05cb
--- /dev/null
+++ b/tests/btrfs/154
@@ -0,0 +1,80 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2020 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 154
+#
+# Test correct operation of free objectid related functionality
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+
+
+_scratch_mkfs > /dev/null
+_scratch_mount
+
+# create a new subvolume to validate its objectid is initialized accordingly
+$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/newvol >> $seqres.full 2>&1 \
+	|| _fail "couldn't create subvol"
+
+$BTRFS_UTIL_PROG inspect-internal dump-tree -t1 $SCRATCH_DEV \
+	| grep -q "256 ROOT_ITEM"  ||	_fail "First subvol with id 256 doesn't exist"
+
+# create new file in the new subvolume to validate its objectid is set as
+# expected
+touch $SCRATCH_MNT/newvol/file1
+
+# ensure we have consistent view on-disk
+sync
+
+# get output related to the new root's dir entry
+output=$($BTRFS_UTIL_PROG inspect-internal dump-tree -t5 $SCRATCH_DEV | grep -A2 "256 DIR_ITEM 1903355334")
+
+# get the objectid of the new root
+new_root_id=$(echo "$output" | awk '/location key/{printf $3}' | tr -d  '(')
+[ $new_root_id -eq 256 ] || _fail "New root id not equal to 256"
+
+# the given root should always be item number 2, since it's the only item
+item_seq=$(echo "$output" | awk '/item/ {printf $2}')
+[ $item_seq -eq 2 ] || _fail "New root not at item idx 2"
+
+# now parse the structure of the new subvol's tree
+output=$($BTRFS_UTIL_PROG inspect-internal dump-tree -t256 $SCRATCH_DEV)
+
+# this is the subvol's own ino
+first_ino=$(echo "$output" | awk '/item 0/{printf $4}' | tr -d '(')
+[ $first_ino -eq 256 ] || _fail "First ino objectid in subvol not 256"
+
+# this is ino of first file in subvol
+second_ino=$(echo "$output" | awk '/item 4/{printf $4}' | tr -d '(')
+[ $second_ino -eq 257 ] || _fail "Second ino objectid in subvol not 257"
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/btrfs/154.out b/tests/btrfs/154.out
new file mode 100644
index 000000000000..a18c304305c4
--- /dev/null
+++ b/tests/btrfs/154.out
@@ -0,0 +1,2 @@
+QA output created by 154
+Silence is golden
diff --git a/tests/btrfs/group b/tests/btrfs/group
index d18450c7552e..44d33222def0 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -156,6 +156,7 @@
 151 auto quick volume
 152 auto quick metadata qgroup send
 153 auto quick qgroup limit
+154 auto quick
 155 auto quick send
 156 auto quick trim balance
 157 auto quick raid
-- 
2.17.1

[RESEND PATCH] btrfs: Add test 154

[Nikolay Borisov]

This test verifies btrfs' free objectid management. I.e it ensures that
the first objectid is always 256 in an fs tree.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

Resend it as I fudged btrfs' mailing list address so the patch didn't get to it.
 tests/btrfs/154     | 80 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/154.out |  2 ++
 tests/btrfs/group   |  1 +
 3 files changed, 83 insertions(+)
 create mode 100755 tests/btrfs/154
 create mode 100644 tests/btrfs/154.out

diff --git a/tests/btrfs/154 b/tests/btrfs/154
new file mode 100755
index 000000000000..6aee204e05cb
--- /dev/null
+++ b/tests/btrfs/154
@@ -0,0 +1,80 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2020 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 154
+#
+# Test correct operation of free objectid related functionality
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+
+
+_scratch_mkfs > /dev/null
+_scratch_mount
+
+# create a new subvolume to validate its objectid is initialized accordingly
+$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/newvol >> $seqres.full 2>&1 \
+	|| _fail "couldn't create subvol"
+
+$BTRFS_UTIL_PROG inspect-internal dump-tree -t1 $SCRATCH_DEV \
+	| grep -q "256 ROOT_ITEM"  ||	_fail "First subvol with id 256 doesn't exist"
+
+# create new file in the new subvolume to validate its objectid is set as
+# expected
+touch $SCRATCH_MNT/newvol/file1
+
+# ensure we have consistent view on-disk
+sync
+
+# get output related to the new root's dir entry
+output=$($BTRFS_UTIL_PROG inspect-internal dump-tree -t5 $SCRATCH_DEV | grep -A2 "256 DIR_ITEM 1903355334")
+
+# get the objectid of the new root
+new_root_id=$(echo "$output" | awk '/location key/{printf $3}' | tr -d  '(')
+[ $new_root_id -eq 256 ] || _fail "New root id not equal to 256"
+
+# the given root should always be item number 2, since it's the only item
+item_seq=$(echo "$output" | awk '/item/ {printf $2}')
+[ $item_seq -eq 2 ] || _fail "New root not at item idx 2"
+
+# now parse the structure of the new subvol's tree
+output=$($BTRFS_UTIL_PROG inspect-internal dump-tree -t256 $SCRATCH_DEV)
+
+# this is the subvol's own ino
+first_ino=$(echo "$output" | awk '/item 0/{printf $4}' | tr -d '(')
+[ $first_ino -eq 256 ] || _fail "First ino objectid in subvol not 256"
+
+# this is ino of first file in subvol
+second_ino=$(echo "$output" | awk '/item 4/{printf $4}' | tr -d '(')
+[ $second_ino -eq 257 ] || _fail "Second ino objectid in subvol not 257"
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/btrfs/154.out b/tests/btrfs/154.out
new file mode 100644
index 000000000000..a18c304305c4
--- /dev/null
+++ b/tests/btrfs/154.out
@@ -0,0 +1,2 @@
+QA output created by 154
+Silence is golden
diff --git a/tests/btrfs/group b/tests/btrfs/group
index d18450c7552e..44d33222def0 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -156,6 +156,7 @@
 151 auto quick volume
 152 auto quick metadata qgroup send
 153 auto quick qgroup limit
+154 auto quick
 155 auto quick send
 156 auto quick trim balance
 157 auto quick raid
--
2.17.1

Re: [RESEND PATCH] btrfs: Add test 154

[Eryu Guan]

On Mon, Dec 07, 2020 at 06:19:00PM +0200, Nikolay Borisov wrote:
> This test verifies btrfs' free objectid management. I.e it ensures that
> the first objectid is always 256 in an fs tree.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Some minor issues below, but I'd like btrfs folks to help review to see
if the free objectid management test is reasonable.

> ---
> 
> Resend it as I fudged btrfs' mailing list address so the patch didn't get to it.
>  tests/btrfs/154     | 80 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/154.out |  2 ++
>  tests/btrfs/group   |  1 +
>  3 files changed, 83 insertions(+)
>  create mode 100755 tests/btrfs/154
>  create mode 100644 tests/btrfs/154.out
> 
> diff --git a/tests/btrfs/154 b/tests/btrfs/154
> new file mode 100755
> index 000000000000..6aee204e05cb
> --- /dev/null
> +++ b/tests/btrfs/154
> @@ -0,0 +1,80 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2020 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 154
> +#
> +# Test correct operation of free objectid related functionality
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
> +
> +
> +_scratch_mkfs > /dev/null
> +_scratch_mount
> +
> +# create a new subvolume to validate its objectid is initialized accordingly
> +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/newvol >> $seqres.full 2>&1 \
> +	|| _fail "couldn't create subvol"
> +
> +$BTRFS_UTIL_PROG inspect-internal dump-tree -t1 $SCRATCH_DEV \
> +	| grep -q "256 ROOT_ITEM"  ||	_fail "First subvol with id 256 doesn't exist"

This also requires

_require_btrfs_command inspect-internal dump-tree

And it's better to explain why '256' is the expected value, where does
it come from.

> +
> +# create new file in the new subvolume to validate its objectid is set as
> +# expected
> +touch $SCRATCH_MNT/newvol/file1
> +
> +# ensure we have consistent view on-disk
> +sync
> +
> +# get output related to the new root's dir entry
> +output=$($BTRFS_UTIL_PROG inspect-internal dump-tree -t5 $SCRATCH_DEV | grep -A2 "256 DIR_ITEM 1903355334")
> +
> +# get the objectid of the new root
> +new_root_id=$(echo "$output" | awk '/location key/{printf $3}' | tr -d  '(')

I'd dump the output to a tmp file (and the following outputs in the
test), as saving the output in a variable may cause unexpected results,
something like ignoring "\n", and make it harder to debug.

> +[ $new_root_id -eq 256 ] || _fail "New root id not equal to 256"
> +
> +# the given root should always be item number 2, since it's the only item
> +item_seq=$(echo "$output" | awk '/item/ {printf $2}')

$AWK_PROG

Thanks,
Eryu

> +[ $item_seq -eq 2 ] || _fail "New root not at item idx 2"
> +
> +# now parse the structure of the new subvol's tree
> +output=$($BTRFS_UTIL_PROG inspect-internal dump-tree -t256 $SCRATCH_DEV)
> +
> +# this is the subvol's own ino
> +first_ino=$(echo "$output" | awk '/item 0/{printf $4}' | tr -d '(')
> +[ $first_ino -eq 256 ] || _fail "First ino objectid in subvol not 256"
> +
> +# this is ino of first file in subvol
> +second_ino=$(echo "$output" | awk '/item 4/{printf $4}' | tr -d '(')
> +[ $second_ino -eq 257 ] || _fail "Second ino objectid in subvol not 257"
> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/btrfs/154.out b/tests/btrfs/154.out
> new file mode 100644
> index 000000000000..a18c304305c4
> --- /dev/null
> +++ b/tests/btrfs/154.out
> @@ -0,0 +1,2 @@
> +QA output created by 154
> +Silence is golden
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index d18450c7552e..44d33222def0 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -156,6 +156,7 @@
>  151 auto quick volume
>  152 auto quick metadata qgroup send
>  153 auto quick qgroup limit
> +154 auto quick
>  155 auto quick send
>  156 auto quick trim balance
>  157 auto quick raid
> --
> 2.17.1

Re: [PATCH 0/6] Overhaul free objectid code

[Josef Bacik]

On 12/7/20 10:32 AM, Nikolay Borisov wrote:
> This series aims to make the free objectid code more straighforward. Currently
> the highest used objectid is used which implies that when btrfs_get_free_objectid
> is called the pre-increment operator is used. At the same time when looking
> at how highest_objectid is initialised in find_free_objectid it's using the,
> at first looko unusual, 'BTRFS_FREE_OBJECTID - 1'. Furthermore btrfs_find_free_objectid
> is badly named as it's used only in initializaion context.
> 
> With the series applied the following is achieved:
>   * The 2 functions related to free objectid have better naming which describes
>   their semantic meaning.
> 
>   * highest_objectid is renamed to free_objectid which clearly states what it's
>   supposed to hold, also btrfs_get_free_objectid now returns the value and
>   does a post-increment which seems more logical than the previous cod.
> 
>   * Now it's not necessary to re-initialize free_objectid in create_subvol
>   since that member is now consistently initialized when a given root is read
>   for the first time in btrfs_init_fs_root->btrfs_init_root_free_objectid.
>   Additionally in btrfs_init_root_free_objectid free_objectid is now initialized
>   to BTRFS_FIRST_FREE_OBJECTID so it's self-explanatory.
> 
> This series survived xfstest as well as a new xfstest which verifies precisely
> this functionality.
> 

You can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the series, thanks,

Josef

Re: [PATCH 0/6] Overhaul free objectid code

[David Sterba]

On Mon, Dec 07, 2020 at 05:32:31PM +0200, Nikolay Borisov wrote:
> This series aims to make the free objectid code more straighforward. Currently
> the highest used objectid is used which implies that when btrfs_get_free_objectid
> is called the pre-increment operator is used. At the same time when looking
> at how highest_objectid is initialised in find_free_objectid it's using the,
> at first looko unusual, 'BTRFS_FREE_OBJECTID - 1'. Furthermore btrfs_find_free_objectid
> is badly named as it's used only in initializaion context.
> 
> With the series applied the following is achieved:
>  * The 2 functions related to free objectid have better naming which describes
>  their semantic meaning.
> 
>  * highest_objectid is renamed to free_objectid which clearly states what it's
>  supposed to hold, also btrfs_get_free_objectid now returns the value and
>  does a post-increment which seems more logical than the previous cod.
> 
>  * Now it's not necessary to re-initialize free_objectid in create_subvol
>  since that member is now consistently initialized when a given root is read
>  for the first time in btrfs_init_fs_root->btrfs_init_root_free_objectid.
>  Additionally in btrfs_init_root_free_objectid free_objectid is now initialized
>  to BTRFS_FIRST_FREE_OBJECTID so it's self-explanatory.
> 
> This series survived xfstest as well as a new xfstest which verifies precisely
> this functionality.
> 
> 
> Nikolay Borisov (6):
>   btrfs: Rename btrfs_find_highest_objectid to
>     btrfs_init_root_free_objectid
>   btrfs: Rename btrfs_find_free_objectid to btrfs_get_free_objectid
>   btrfs: Remove useless ASSERTS
>   btrfs: Rename highest_objectid to free_objectid
>   btrfs: Make free_objectid hold the next available objectid in the root
>   btrfs: Remove new_dirid argument from btrfs_create_subvol_root

Except patch 3 (the assert) added to misc-next, thanks.