Skip to contentSkip to conversation

btrfs: Fix lost-data-profile caused by auto removing bg and balance bg

PATCH Superseded Zhao Lei 2 participants 4 messages Oct 3, 2015 – Oct 6, 2015 Btrfs file system

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Zhao Lei

author
#0 lore raw

v3 cover btrfs: Fix lost-data-profile caused by auto removing bg and balance bg

Sorry for sending v3 late for I'm in my holiday(until 10/7).

But it is necessary to send out in time because I wish to end this bug in v4.3 release.

Changelog v2->v3:
1: Use list_is_singular() instead of
block_group->list.next == block_group->list.prev

2: Add Reviewed-by: Filipe Manana <[email protected]>

Zhao Lei (2):
btrfs: Fix lost-data-profile caused by auto removing bg
btrfs: Fix lost-data-profile caused by balance bg

 fs/btrfs/extent-tree.c | 12 +++++++++++-
 fs/btrfs/volumes.c     | 24 ++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
Signature · 1 line
1.8.5.1
author
#1 lore raw

PATCH v3 1/2 btrfs: Fix lost-data-profile caused by auto removing bg

Reproduce:
(In integration-4.3 branch)

TEST_DEV=(/dev/vdg /dev/vdh)
TEST_DIR=/mnt/tmp

umount "$TEST_DEV" >/dev/null
mkfs.btrfs -f -d raid1 "${TEST_DEV[@]}"

mount -o nospace_cache "$TEST_DEV" "$TEST_DIR"
umount "$TEST_DEV"

mount -o nospace_cache "$TEST_DEV" "$TEST_DIR"
btrfs filesystem usage $TEST_DIR

We can see the data chunk changed from raid1 to single:

 # btrfs filesystem usage $TEST_DIR
 Data,single: Size:8.00MiB, Used:0.00B
    /dev/vdg        8.00MiB
 #

Reason:
When a empty filesystem mount with -o nospace_cache, the last data blockgroup will be auto-removed in umount.

Then if we mount it again, there is no data chunk in the filesystem, so the only available data profile is 0x0, result is all new chunks are created as single type.

Fix:
Don't auto-delete last blockgroup for a raid type.

Test:
Test by above script, and confirmed the logic by debug output.

Changelog v2->v3:
1: Use list_is_singular() instead of
block_group->list.next == block_group->list.prev

Changelog v1->v2:
1: Put code of checking block_group->list into
semaphore of space_info->groups_sem.

---
 fs/btrfs/extent-tree.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
fs/btrfs/extent-tree.c +11 -1
index 79a5bd9..c05b975 100644
@@ -10010,8 +10010,18 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		block_group = list_first_entry(&fs_info->unused_bgs,
 					       struct btrfs_block_group_cache,
 					       bg_list);
-		space_info = block_group->space_info;
 		list_del_init(&block_group->bg_list);
+
+		space_info = block_group->space_info;
+
+		down_read(&space_info->groups_sem);
+		if (list_is_singular(&block_group->list)) {
+			up_read(&space_info->groups_sem);
+			btrfs_put_block_group(block_group);
+			continue;
+		}
+		up_read(&space_info->groups_sem);
+
 		if (ret || btrfs_mixed_space_info(space_info)) {
 			btrfs_put_block_group(block_group);
 			continue;
Signature · 1 line
1.8.5.1
#2 lore raw

Re: [PATCH v3 1/2] btrfs: Fix lost-data-profile caused by auto removing bg

Hi,

Quoted 5 lines · Zhao Lei
fs/btrfs/extent-tree.c +0 -0
index 79a5bd9..c05b975 100644

fs/btrfs/extent-tree.c void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) · line 10017

Quoted 9 lines
fs/btrfs/extent-tree.c +4 -1
@@ -10010,8 +10010,18 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		block_group = list_first_entry(&fs_info->unused_bgs,
 					       struct btrfs_block_group_cache,
 					       bg_list);
-		space_info = block_group->space_info;
 		list_del_init(&block_group->bg_list);
+
+		space_info = block_group->space_info;
+
+		down_read(&space_info->groups_sem);

Thanks for reworking this, are there other patches that I'm missing that are meant to go in before this one?

The down_read() this patch adds is inside a spinlock().

-chris

author
#3 lore raw

PATCH v3 2/2 btrfs: Fix lost-data-profile caused by balance bg

In reply to Zhao Lei

Reproduce:
(In integration-4.3 branch)

TEST_DEV=(/dev/vdg /dev/vdh)
TEST_DIR=/mnt/tmp

umount "$TEST_DEV" >/dev/null mkfs.btrfs -f -d raid1 "${TEST_DEV[@]}"

mount -o nospace_cache "$TEST_DEV" "$TEST_DIR" btrfs balance start -dusage=0 $TEST_DIR btrfs filesystem usage $TEST_DIR

dd if=/dev/zero of="$TEST_DIR"/file count=100 btrfs filesystem usage $TEST_DIR

Result:
We can see "no data chunk" in first "btrfs filesystem usage":

 # btrfs filesystem usage $TEST_DIR
 Overall:
    ...
 Metadata,single: Size:8.00MiB, Used:0.00B
    /dev/vdg        8.00MiB
 Metadata,RAID1: Size:122.88MiB, Used:112.00KiB
    /dev/vdg      122.88MiB
    /dev/vdh      122.88MiB
 System,single: Size:4.00MiB, Used:0.00B
    /dev/vdg        4.00MiB
 System,RAID1: Size:8.00MiB, Used:16.00KiB
    /dev/vdg        8.00MiB
    /dev/vdh        8.00MiB
 Unallocated:
    /dev/vdg        1.06GiB
    /dev/vdh        1.07GiB

And "data chunks changed from raid1 to single" in second "btrfs filesystem usage":

 # btrfs filesystem usage $TEST_DIR
 Overall:
    ...
 Data,single: Size:256.00MiB, Used:0.00B
    /dev/vdh      256.00MiB
 Metadata,single: Size:8.00MiB, Used:0.00B
    /dev/vdg        8.00MiB
 Metadata,RAID1: Size:122.88MiB, Used:112.00KiB
    /dev/vdg      122.88MiB
    /dev/vdh      122.88MiB
 System,single: Size:4.00MiB, Used:0.00B
    /dev/vdg        4.00MiB
 System,RAID1: Size:8.00MiB, Used:16.00KiB
    /dev/vdg        8.00MiB
    /dev/vdh        8.00MiB
 Unallocated:
    /dev/vdg        1.06GiB
    /dev/vdh      841.92MiB

Reason:
btrfs balance delete last data chunk in case of no data in the filesystem, then we can see "no data chunk" by "fi usage" command.

And when we do write operation to fs, the only available data profile is 0x0, result is all new chunks are allocated single type.

Fix:
Allocate a data chunk explicitly to ensure we don't lose the raid profile for data.

Test:
Test by above script, and confirmed the logic by debug output.

Changelog v1->v2:
1: Update patch description of "Fix" field
2: Use BTRFS_BLOCK_GROUP_DATA for btrfs_force_chunk_alloc instead
of 1
3: Only reserve chunk if balance data chunk.
All suggested-by: Filipe Manana <[email protected]>

---
 fs/btrfs/volumes.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
fs/btrfs/volumes.c +24 -0
index 6fc73586..cd9e5bd 100644
@@ -3277,6 +3277,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	u64 limit_data = bctl->data.limit;
 	u64 limit_meta = bctl->meta.limit;
 	u64 limit_sys = bctl->sys.limit;
+	int chunk_reserved = 0;
 
 	/* step one make some room on all the devices */
 	devices = &fs_info->fs_devices->devices;
@@ -3326,6 +3327,8 @@ again:
 	key.type = BTRFS_CHUNK_ITEM_KEY;
 
 	while (1) {
+		u64 chunk_type;
+
 		if ((!counting && atomic_read(&fs_info->balance_pause_req)) ||
 		    atomic_read(&fs_info->balance_cancel_req)) {
 			ret = -ECANCELED;
@@ -3371,8 +3374,10 @@ again:
 			spin_unlock(&fs_info->balance_lock);
 		}
 
+		chunk_type = btrfs_chunk_type(leaf, chunk);
 		ret = should_balance_chunk(chunk_root, leaf, chunk,
 					   found_key.offset);
+
 		btrfs_release_path(path);
 		if (!ret) {
 			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
@@ -3387,6 +3392,25 @@ again:
 			goto loop;
 		}
 
+		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) && !chunk_reserved) {
+			trans = btrfs_start_transaction(chunk_root, 0);
+			if (IS_ERR(trans)) {
+				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+				ret = PTR_ERR(trans);
+				goto error;
+			}
+
+			ret = btrfs_force_chunk_alloc(trans, chunk_root,
+						      BTRFS_BLOCK_GROUP_DATA);
+			if (ret < 0) {
+				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+				goto error;
+			}
+
+			btrfs_end_transaction(trans, chunk_root);
+			chunk_reserved = 1;
+		}
+
 		ret = btrfs_relocate_chunk(chunk_root,
 					   found_key.offset);
 		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
Signature · 1 line
1.8.5.1