[PATCH 0/7] fs_info cleanups for volume.c

[PATCH 0/7] fs_info cleanups for volume.c

[Nikolay Borisov]

Here are a bunch of patches which cleanup extraneous fs_info parameters to 
function which already take a structure that holds a reference to the fs_info. 

Except for patches 4 and 5, everything else is correct - due to those functions
always taking a transaction. 4 and 5 in turn reference the fs_info from 
struct btrfs_device. Inspecting the callers I managed to convince myself that 
those function are always called with well-formed btrfs_device i.e one which 
has its fs_info member initialised. Reviewers might want to pay extra 
attention to that but otherwise they are trivial. 

Nikolay Borisov (7):
  btrfs: Remove fs_info argument from btrfs_add_dev_item
  btrfs: Remove fs_info from btrfs_rm_dev_replace_remove_srcdev
  btrfs: remove fs_info argument from update_dev_stat_item
  btrfs: Remove fs_info from btrfs_assign_next_active_device
  btrfs: Remove fs_info from btrfs_destroy_dev_replace_tgtdev
  btrfs: Remove fs_info form btrfs_free_chunk
  btrfs: Remove fs_info from btrfs_finish_chunk_alloc

 fs/btrfs/dev-replace.c | 10 +++++-----
 fs/btrfs/extent-tree.c |  5 ++---
 fs/btrfs/volumes.c     | 49 +++++++++++++++++++++++--------------------------
 fs/btrfs/volumes.h     | 16 ++++++----------
 4 files changed, 36 insertions(+), 44 deletions(-)

-- 
2.7.4

[PATCH 1/7] btrfs: Remove fs_info argument from btrfs_add_dev_item

[Nikolay Borisov]

It can be referenced form the passed transaction handle.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e8fb9d5d976e..e73b9e9d6c10 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1666,10 +1666,8 @@ static noinline int find_next_devid(struct btrfs_fs_info *fs_info,
  * the btrfs_device struct should be fully filled in
  */
 static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
-			    struct btrfs_fs_info *fs_info,
 			    struct btrfs_device *device)
 {
-	struct btrfs_root *root = fs_info->chunk_root;
 	int ret;
 	struct btrfs_path *path;
 	struct btrfs_dev_item *dev_item;
@@ -1685,8 +1683,8 @@ static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
 	key.type = BTRFS_DEV_ITEM_KEY;
 	key.offset = device->devid;
 
-	ret = btrfs_insert_empty_item(trans, root, path, &key,
-				      sizeof(*dev_item));
+	ret = btrfs_insert_empty_item(trans, trans->fs_info->chunk_root, path,
+				      &key, sizeof(*dev_item));
 	if (ret)
 		goto out;
 
@@ -1711,7 +1709,7 @@ static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
 	ptr = btrfs_device_uuid(dev_item);
 	write_extent_buffer(leaf, device->uuid, ptr, BTRFS_UUID_SIZE);
 	ptr = btrfs_device_fsid(dev_item);
-	write_extent_buffer(leaf, fs_info->fsid, ptr, BTRFS_FSID_SIZE);
+	write_extent_buffer(leaf, trans->fs_info->fsid, ptr, BTRFS_FSID_SIZE);
 	btrfs_mark_buffer_dirty(leaf);
 
 	ret = 0;
@@ -2441,7 +2439,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		}
 	}
 
-	ret = btrfs_add_dev_item(trans, fs_info, device);
+	ret = btrfs_add_dev_item(trans, device);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
 		goto error_sysfs;
-- 
2.7.4

[PATCH 2/7] btrfs: Remove fs_info from btrfs_rm_dev_replace_remove_srcdev

[Nikolay Borisov]

It can be referenced from the passed srcdev argument.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 fs/btrfs/volumes.c     | 5 ++---
 fs/btrfs/volumes.h     | 3 +--
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 059ca3d5ddd3..df375e1a0c9f 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -667,7 +667,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 
 	btrfs_rm_dev_replace_blocked(fs_info);
 
-	btrfs_rm_dev_replace_remove_srcdev(fs_info, src_device);
+	btrfs_rm_dev_replace_remove_srcdev(src_device);
 
 	btrfs_rm_dev_replace_unblocked(fs_info);
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e73b9e9d6c10..844e5d4fd3e4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1988,12 +1988,11 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	goto out;
 }
 
-void btrfs_rm_dev_replace_remove_srcdev(struct btrfs_fs_info *fs_info,
-					struct btrfs_device *srcdev)
+void btrfs_rm_dev_replace_remove_srcdev(struct btrfs_device *srcdev)
 {
 	struct btrfs_fs_devices *fs_devices;
 
-	lockdep_assert_held(&fs_info->fs_devices->device_list_mutex);
+	lockdep_assert_held(&srcdev->fs_info->fs_devices->device_list_mutex);
 
 	/*
 	 * in case of fs with no seed, srcdev->fs_devices will point
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6943aab9bdd7..0ae45ff1961b 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -452,8 +452,7 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info);
 int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info);
 int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
 			struct btrfs_fs_info *fs_info);
-void btrfs_rm_dev_replace_remove_srcdev(struct btrfs_fs_info *fs_info,
-					struct btrfs_device *srcdev);
+void btrfs_rm_dev_replace_remove_srcdev(struct btrfs_device *srcdev);
 void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info,
 				      struct btrfs_device *srcdev);
 void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
-- 
2.7.4

[PATCH 3/7] btrfs: remove fs_info argument from update_dev_stat_item

[Nikolay Borisov]

It can be referenced from the passed transaction handle.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 844e5d4fd3e4..35cc5b6a06e0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7035,9 +7035,9 @@ int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
 }
 
 static int update_dev_stat_item(struct btrfs_trans_handle *trans,
-				struct btrfs_fs_info *fs_info,
 				struct btrfs_device *device)
 {
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_root *dev_root = fs_info->dev_root;
 	struct btrfs_path *path;
 	struct btrfs_key key;
@@ -7130,7 +7130,7 @@ int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
 		 */
 		smp_rmb();
 
-		ret = update_dev_stat_item(trans, fs_info, device);
+		ret = update_dev_stat_item(trans, device);
 		if (!ret)
 			atomic_sub(stats_cnt, &device->dev_stats_ccnt);
 	}
-- 
2.7.4

[PATCH 4/7] btrfs: Remove fs_info from btrfs_assign_next_active_device

[Nikolay Borisov]

It can be referenced from the passed 'device' argument which is always
a well-formed device.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 fs/btrfs/volumes.c     | 9 +++++----
 fs/btrfs/volumes.h     | 4 ++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index df375e1a0c9f..dd17a4d7bea2 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -658,7 +658,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	tgt_device->commit_total_bytes = src_device->commit_total_bytes;
 	tgt_device->commit_bytes_used = src_device->bytes_used;
 
-	btrfs_assign_next_active_device(fs_info, src_device, tgt_device);
+	btrfs_assign_next_active_device(src_device, tgt_device);
 
 	list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
 	fs_info->fs_devices->rw_devices++;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 35cc5b6a06e0..99e126a0edcb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1818,9 +1818,10 @@ static struct btrfs_device * btrfs_find_next_active_device(
  * where this function called, there should be always be another device (or
  * this_dev) which is active.
  */
-void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
-		struct btrfs_device *device, struct btrfs_device *this_dev)
+void btrfs_assign_next_active_device(struct btrfs_device *device,
+				     struct btrfs_device *this_dev)
 {
+	struct btrfs_fs_info *fs_info = device->fs_info;
 	struct btrfs_device *next_device;
 
 	if (this_dev)
@@ -1936,7 +1937,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
 		cur_devices->missing_devices--;
 
-	btrfs_assign_next_active_device(fs_info, device, NULL);
+	btrfs_assign_next_active_device(device, NULL);
 
 	if (device->bdev) {
 		cur_devices->open_devices--;
@@ -2069,7 +2070,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 
 	fs_devices->num_devices--;
 
-	btrfs_assign_next_active_device(fs_info, tgtdev, NULL);
+	btrfs_assign_next_active_device(tgtdev, NULL);
 
 	list_del_rcu(&tgtdev->dev_list);
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 0ae45ff1961b..1bcf0f34e8dc 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -407,8 +407,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   fmode_t flags, void *holder);
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
-void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
-		struct btrfs_device *device, struct btrfs_device *this_dev);
+void btrfs_assign_next_active_device(struct btrfs_device *device,
+				     struct btrfs_device *this_dev);
 int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
 					 const char *device_path,
 					 struct btrfs_device **device);
-- 
2.7.4

[PATCH 5/7] btrfs: Remove fs_info from btrfs_destroy_dev_replace_tgtdev

[Nikolay Borisov]

This function is always passed a well-formed tgtdevice so the fs_info
can be referenced from there.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c | 6 +++---
 fs/btrfs/volumes.c     | 5 ++---
 fs/btrfs/volumes.h     | 3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index dd17a4d7bea2..5a72f9933e58 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -506,7 +506,7 @@ int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	dev_replace->srcdev = NULL;
 	dev_replace->tgtdev = NULL;
 	btrfs_dev_replace_write_unlock(dev_replace);
-	btrfs_destroy_dev_replace_tgtdev(fs_info, tgt_device);
+	btrfs_destroy_dev_replace_tgtdev(tgt_device);
 	return ret;
 }
 
@@ -632,7 +632,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 		btrfs_rm_dev_replace_blocked(fs_info);
 		if (tgt_device)
-			btrfs_destroy_dev_replace_tgtdev(fs_info, tgt_device);
+			btrfs_destroy_dev_replace_tgtdev(tgt_device);
 		btrfs_rm_dev_replace_unblocked(fs_info);
 		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
 
@@ -821,7 +821,7 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 		btrfs_dev_name(tgt_device));
 
 	if (tgt_device)
-		btrfs_destroy_dev_replace_tgtdev(fs_info, tgt_device);
+		btrfs_destroy_dev_replace_tgtdev(tgt_device);
 
 leave:
 	mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 99e126a0edcb..241875277a7e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2055,10 +2055,9 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info,
 	}
 }
 
-void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
-				      struct btrfs_device *tgtdev)
+void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
 {
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_fs_devices *fs_devices = tgtdev->fs_info->fs_devices;
 
 	WARN_ON(!tgtdev);
 	mutex_lock(&fs_devices->device_list_mutex);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 1bcf0f34e8dc..69a028058c43 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -455,8 +455,7 @@ int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
 void btrfs_rm_dev_replace_remove_srcdev(struct btrfs_device *srcdev);
 void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info,
 				      struct btrfs_device *srcdev);
-void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
-				      struct btrfs_device *tgtdev);
+void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev);
 void btrfs_scratch_superblocks(struct block_device *bdev, const char *device_path);
 int btrfs_is_parity_mirror(struct btrfs_fs_info *fs_info,
 			   u64 logical, u64 len);
-- 
2.7.4

[PATCH 6/7] btrfs: Remove fs_info form btrfs_free_chunk

[Nikolay Borisov]

It can be referenced from the passed transaction handle.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 241875277a7e..7376c83708e4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2597,9 +2597,9 @@ int btrfs_grow_device(struct btrfs_trans_handle *trans,
 	return btrfs_update_device(trans, device);
 }
 
-static int btrfs_free_chunk(struct btrfs_trans_handle *trans,
-			    struct btrfs_fs_info *fs_info, u64 chunk_offset)
+static int btrfs_free_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 {
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_root *root = fs_info->chunk_root;
 	int ret;
 	struct btrfs_path *path;
@@ -2769,7 +2769,7 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans,
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
 
-	ret = btrfs_free_chunk(trans, fs_info, chunk_offset);
+	ret = btrfs_free_chunk(trans, chunk_offset);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
 		goto out;
-- 
2.7.4

[PATCH 7/7] btrfs: Remove fs_info from btrfs_finish_chunk_alloc

[Nikolay Borisov]

It can be referenced from the passed transaction handle.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c |  5 ++---
 fs/btrfs/volumes.c     | 10 +++++-----
 fs/btrfs/volumes.h     |  6 ++----
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index fd109bfd528d..7469fc7e3f1a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -10023,7 +10023,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
 					sizeof(item));
 		if (ret)
 			btrfs_abort_transaction(trans, ret);
-		ret = btrfs_finish_chunk_alloc(trans, fs_info, key.objectid,
+		ret = btrfs_finish_chunk_alloc(trans, key.objectid,
 					       key.offset);
 		if (ret)
 			btrfs_abort_transaction(trans, ret);
@@ -10595,8 +10595,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		 * Btrfs_remove_chunk will abort the transaction if things go
 		 * horribly wrong.
 		 */
-		ret = btrfs_remove_chunk(trans, fs_info,
-					 block_group->key.objectid);
+		ret = btrfs_remove_chunk(trans, block_group->key.objectid);
 
 		if (ret) {
 			if (trimming)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7376c83708e4..418843b8f8e8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2708,9 +2708,9 @@ static struct extent_map *get_chunk_map(struct btrfs_fs_info *fs_info,
 	return em;
 }
 
-int btrfs_remove_chunk(struct btrfs_trans_handle *trans,
-		       struct btrfs_fs_info *fs_info, u64 chunk_offset)
+int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 {
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct extent_map *em;
 	struct map_lookup *map;
 	u64 dev_extent_len = 0;
@@ -2850,7 +2850,7 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset)
 	 * step two, delete the device extents and the
 	 * chunk tree entries
 	 */
-	ret = btrfs_remove_chunk(trans, fs_info, chunk_offset);
+	ret = btrfs_remove_chunk(trans, chunk_offset);
 	btrfs_end_transaction(trans);
 	return ret;
 }
@@ -4834,9 +4834,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 }
 
 int btrfs_finish_chunk_alloc(struct btrfs_trans_handle *trans,
-				struct btrfs_fs_info *fs_info,
-				u64 chunk_offset, u64 chunk_size)
+			     u64 chunk_offset, u64 chunk_size)
 {
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_root *extent_root = fs_info->extent_root;
 	struct btrfs_root *chunk_root = fs_info->chunk_root;
 	struct btrfs_key key;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 69a028058c43..049619176831 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -462,10 +462,8 @@ int btrfs_is_parity_mirror(struct btrfs_fs_info *fs_info,
 unsigned long btrfs_full_stripe_len(struct btrfs_fs_info *fs_info,
 				    u64 logical);
 int btrfs_finish_chunk_alloc(struct btrfs_trans_handle *trans,
-				struct btrfs_fs_info *fs_info,
-				u64 chunk_offset, u64 chunk_size);
-int btrfs_remove_chunk(struct btrfs_trans_handle *trans,
-		       struct btrfs_fs_info *fs_info, u64 chunk_offset);
+			     u64 chunk_offset, u64 chunk_size);
+int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset);
 
 static inline void btrfs_dev_stat_inc(struct btrfs_device *dev,
 				      int index)
-- 
2.7.4

Re: [PATCH 0/7] fs_info cleanups for volume.c

[Lu Fengqi]

On Fri, Jul 20, 2018 at 07:37:46PM +0300, Nikolay Borisov wrote:
>Here are a bunch of patches which cleanup extraneous fs_info parameters to 
>function which already take a structure that holds a reference to the fs_info. 
>
>Except for patches 4 and 5, everything else is correct - due to those functions
>always taking a transaction. 4 and 5 in turn reference the fs_info from 
>struct btrfs_device. Inspecting the callers I managed to convince myself that 
>those function are always called with well-formed btrfs_device i.e one which 
>has its fs_info member initialised. Reviewers might want to pay extra 
>attention to that but otherwise they are trivial. 
>
>Nikolay Borisov (7):
>  btrfs: Remove fs_info argument from btrfs_add_dev_item
>  btrfs: Remove fs_info from btrfs_rm_dev_replace_remove_srcdev
>  btrfs: remove fs_info argument from update_dev_stat_item
>  btrfs: Remove fs_info from btrfs_assign_next_active_device
>  btrfs: Remove fs_info from btrfs_destroy_dev_replace_tgtdev
>  btrfs: Remove fs_info form btrfs_free_chunk
>  btrfs: Remove fs_info from btrfs_finish_chunk_alloc

The series looks good to me.

Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

-- 
Thanks,
Lu

>
> fs/btrfs/dev-replace.c | 10 +++++-----
> fs/btrfs/extent-tree.c |  5 ++---
> fs/btrfs/volumes.c     | 49 +++++++++++++++++++++++--------------------------
> fs/btrfs/volumes.h     | 16 ++++++----------
> 4 files changed, 36 insertions(+), 44 deletions(-)
>
>-- 
>2.7.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 0/7] fs_info cleanups for volume.c

[David Sterba]

On Fri, Jul 20, 2018 at 07:37:46PM +0300, Nikolay Borisov wrote:
> Here are a bunch of patches which cleanup extraneous fs_info parameters to 
> function which already take a structure that holds a reference to the fs_info. 
> 
> Except for patches 4 and 5, everything else is correct - due to those functions
> always taking a transaction. 4 and 5 in turn reference the fs_info from 
> struct btrfs_device. Inspecting the callers I managed to convince myself that 
> those function are always called with well-formed btrfs_device i.e one which 
> has its fs_info member initialised. Reviewers might want to pay extra 
> attention to that but otherwise they are trivial. 

4 and 5 look good to me, a device without a valid fs_info has a short
timespan and should not appear anywhere besides the helpers that set up
fs_devices etc. Series added to misc-next, thanks.

Re: [PATCH 0/7] fs_info cleanups for volume.c

[David Sterba]

On Mon, Jul 23, 2018 at 03:25:01PM +0200, David Sterba wrote:
> On Fri, Jul 20, 2018 at 07:37:46PM +0300, Nikolay Borisov wrote:
> > Here are a bunch of patches which cleanup extraneous fs_info parameters to 
> > function which already take a structure that holds a reference to the fs_info. 
> > 
> > Except for patches 4 and 5, everything else is correct - due to those functions
> > always taking a transaction. 4 and 5 in turn reference the fs_info from 
> > struct btrfs_device. Inspecting the callers I managed to convince myself that 
> > those function are always called with well-formed btrfs_device i.e one which 
> > has its fs_info member initialised. Reviewers might want to pay extra 
> > attention to that but otherwise they are trivial. 
> 
> 4 and 5 look good to me, a device without a valid fs_info has a short
> timespan and should not appear anywhere besides the helpers that set up
> fs_devices etc. Series added to misc-next, thanks.

btrfs/164 crashed in device rm that could be related to the patches, I
haven't analyzed that but this series looks most suspicious since the
last round of tests that did not see that crash:

[ 6712.084324] general protection fault: 0000 [#1] PREEMPT SMP
[ 6712.090096] CPU: 0 PID: 2694 Comm: btrfs Not tainted4.18.0-rc6-1.ge195904-vanilla+ #279
[ 6712.098398] Hardware name: empty empty/S3993, BIOS PAQEX0-302/24/2008
[ 6712.105072] RIP: 0010:__list_del_entry_valid+0x25/0xc0
[ 6712.129603] RSP: 0018:ffffb01281e2bbd0 EFLAGS: 00010a83
[ 6712.134969] RAX: 6b6b6b6b6b6b6b6b RBX: ffff9972756dafd8 RCX:dead000000000200
[ 6712.142246] RDX: 6b6b6b6b6b6b6b6b RSI: ffffffffc10252cf RDI:ffff9972756db100
[ 6712.149507] RBP: ffff9972756db100 R08: 0000000000000000 R09:0000000000000001
[ 6712.156788] R10: ffffb01281e2bbd8 R11: 0000000000000000 R12:ffff99728a2d7500
[ 6712.164059] R13: ffff9972756c0910 R14: ffff99728a2d7450 R15:6b6b6b6b6b6b6a43
[ 6712.171332] FS:  00007f3100bb58c0(0000) GS:ffff9972a6600000(0000)knlGS:0000000000000000
[ 6712.179615] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6712.185507] CR2: 00007f336c989000 CR3: 00000001f6868000 CR4:00000000000006f0
[ 6712.192779] Call Trace:
[ 6712.195423]  btrfs_update_commit_device_size+0x75/0xf0 [btrfs]
[ 6712.201424]  btrfs_commit_transaction+0x57d/0xa90 [btrfs]
[ 6712.206999]  btrfs_rm_device+0x627/0x850 [btrfs]
[ 6712.211800]  btrfs_ioctl+0x2b03/0x3120 [btrfs]
[ 6712.216404]  ? __might_fault+0x3e/0x90
[ 6712.220277]  ? lock_acquire+0x9f/0x270
[ 6712.224156]  ? __audit_syscall_entry+0xd6/0x170
[ 6712.228835]  ? ktime_get_coarse_real_ts64+0x67/0x100
[ 6712.233940]  ? do_vfs_ioctl+0xa5/0x6f0
[ 6712.237836]  do_vfs_ioctl+0xa5/0x6f0
[ 6712.241554]  ? syscall_trace_enter+0x1e8/0x3e0
[ 6712.246130]  ksys_ioctl+0x70/0x80
[ 6712.249593]  __x64_sys_ioctl+0x16/0x20
[ 6712.253484]  do_syscall_64+0x62/0x1c0
[ 6712.257291]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 6712.262482] RIP: 0033:0x7f30ffc52417
[ 6712.285413] RSP: 002b:00007ffd636f32c8 EFLAGS: 00000202 ORIG_RAX:0000000000000010
[ 6712.293191] RAX: ffffffffffffffda RBX: 00007ffd636f5460 RCX:00007f30ffc52417
[ 6712.300462] RDX: 00007ffd636f4300 RSI: 000000005000943a RDI:0000000000000003
[ 6712.307742] RBP: 00007ffd636f4300 R08: 0000000000000000 R09:0000000000100000
[ 6712.315005] R10: 000000000fa99fa0 R11: 0000000000000202 R12:0000000000000000
[ 6712.322286] R13: 0000000000000000 R14: 0000000000000003 R15:00007ffd636f5468
[ 6712.391596] ---[ end trace f05bf71894e4ee4d ]---

Re: [PATCH 0/7] fs_info cleanups for volume.c

[Lu Fengqi]

On Tue, Jul 24, 2018 at 10:28:04AM +0200, David Sterba wrote:
>On Mon, Jul 23, 2018 at 03:25:01PM +0200, David Sterba wrote:
>> On Fri, Jul 20, 2018 at 07:37:46PM +0300, Nikolay Borisov wrote:
>> > Here are a bunch of patches which cleanup extraneous fs_info parameters to 
>> > function which already take a structure that holds a reference to the fs_info. 
>> > 
>> > Except for patches 4 and 5, everything else is correct - due to those functions
>> > always taking a transaction. 4 and 5 in turn reference the fs_info from 
>> > struct btrfs_device. Inspecting the callers I managed to convince myself that 
>> > those function are always called with well-formed btrfs_device i.e one which 
>> > has its fs_info member initialised. Reviewers might want to pay extra 
>> > attention to that but otherwise they are trivial. 
>> 
>> 4 and 5 look good to me, a device without a valid fs_info has a short
>> timespan and should not appear anywhere besides the helpers that set up
>> fs_devices etc. Series added to misc-next, thanks.
>
>btrfs/164 crashed in device rm that could be related to the patches, I
>haven't analyzed that but this series looks most suspicious since the
>last round of tests that did not see that crash:
>
>[ 6712.084324] general protection fault: 0000 [#1] PREEMPT SMP
>[ 6712.090096] CPU: 0 PID: 2694 Comm: btrfs Not tainted4.18.0-rc6-1.ge195904-vanilla+ #279
>[ 6712.098398] Hardware name: empty empty/S3993, BIOS PAQEX0-302/24/2008
>[ 6712.105072] RIP: 0010:__list_del_entry_valid+0x25/0xc0
>[ 6712.129603] RSP: 0018:ffffb01281e2bbd0 EFLAGS: 00010a83
>[ 6712.134969] RAX: 6b6b6b6b6b6b6b6b RBX: ffff9972756dafd8 RCX:dead000000000200
>[ 6712.142246] RDX: 6b6b6b6b6b6b6b6b RSI: ffffffffc10252cf RDI:ffff9972756db100
>[ 6712.149507] RBP: ffff9972756db100 R08: 0000000000000000 R09:0000000000000001
>[ 6712.156788] R10: ffffb01281e2bbd8 R11: 0000000000000000 R12:ffff99728a2d7500
>[ 6712.164059] R13: ffff9972756c0910 R14: ffff99728a2d7450 R15:6b6b6b6b6b6b6a43
>[ 6712.171332] FS:  00007f3100bb58c0(0000) GS:ffff9972a6600000(0000)knlGS:0000000000000000
>[ 6712.179615] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>[ 6712.185507] CR2: 00007f336c989000 CR3: 00000001f6868000 CR4:00000000000006f0
>[ 6712.192779] Call Trace:
>[ 6712.195423]  btrfs_update_commit_device_size+0x75/0xf0 [btrfs]
>[ 6712.201424]  btrfs_commit_transaction+0x57d/0xa90 [btrfs]
>[ 6712.206999]  btrfs_rm_device+0x627/0x850 [btrfs]
>[ 6712.211800]  btrfs_ioctl+0x2b03/0x3120 [btrfs]
>[ 6712.216404]  ? __might_fault+0x3e/0x90
>[ 6712.220277]  ? lock_acquire+0x9f/0x270
>[ 6712.224156]  ? __audit_syscall_entry+0xd6/0x170
>[ 6712.228835]  ? ktime_get_coarse_real_ts64+0x67/0x100
>[ 6712.233940]  ? do_vfs_ioctl+0xa5/0x6f0
>[ 6712.237836]  do_vfs_ioctl+0xa5/0x6f0
>[ 6712.241554]  ? syscall_trace_enter+0x1e8/0x3e0
>[ 6712.246130]  ksys_ioctl+0x70/0x80
>[ 6712.249593]  __x64_sys_ioctl+0x16/0x20
>[ 6712.253484]  do_syscall_64+0x62/0x1c0
>[ 6712.257291]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>[ 6712.262482] RIP: 0033:0x7f30ffc52417
>[ 6712.285413] RSP: 002b:00007ffd636f32c8 EFLAGS: 00000202 ORIG_RAX:0000000000000010
>[ 6712.293191] RAX: ffffffffffffffda RBX: 00007ffd636f5460 RCX:00007f30ffc52417
>[ 6712.300462] RDX: 00007ffd636f4300 RSI: 000000005000943a RDI:0000000000000003
>[ 6712.307742] RBP: 00007ffd636f4300 R08: 0000000000000000 R09:0000000000100000
>[ 6712.315005] R10: 000000000fa99fa0 R11: 0000000000000202 R12:0000000000000000
>[ 6712.322286] R13: 0000000000000000 R14: 0000000000000003 R15:00007ffd636f5468
>[ 6712.391596] ---[ end trace f05bf71894e4ee4d ]---
>
>

Hi, David

After I applied this patchset, my test also encountered this crash.
However, the result of git bisect shows the cause may be the
"[PATCH 2/2] btrfs: fix missing superblock update in the device delete commit transaction".
Moreover, I have reported this to Anand Jain. I will send him the updated
log(kasan enabled), and tell him why he can't reproduce before. Please
see the thread I will cc you.

-- 
Thanks,
Lu

Re: [PATCH 0/7] fs_info cleanups for volume.c

[David Sterba]

On Tue, Jul 24, 2018 at 04:59:00PM +0800, Lu Fengqi wrote:
> After I applied this patchset, my test also encountered this crash.
> However, the result of git bisect shows the cause may be the
> "[PATCH 2/2] btrfs: fix missing superblock update in the device delete commit transaction".
> Moreover, I have reported this to Anand Jain. I will send him the updated
> log(kasan enabled), and tell him why he can't reproduce before. Please
> see the thread I will cc you.

Thanks for bisecting it, I'll remove the patch until it's resolved and
re-add Nikolai's patches.