[PATCH 1/2 v2] btrfs: btrfs_rm_device() should zero mirror SB as well

[PATCH 1/2 v2] btrfs: btrfs_rm_device() should zero mirror SB as well

[Anand Jain]

From: Anand Jain <anand.jain@oracle.com>

This fix will ensure all SB copies on the disk is zeroed
when the disk is intentionally removed. This helps to
better manage disks in the user land.

Signed-off-by: Anand Jain <anand.jain@oracle.com>

btrfs: don't double brelse on device rm

Device removal currently causes bdev removal to try to double free a bh
in the bdev:

[   55.714833] WARNING: at fs/buffer.c:1160 __brelse+0x36/0x40()
[   55.714833] VFS: brelse: Trying to free free buffer

Commit 7e3d9ebb1 added a double release of the bh for a device being
removed when all the supers don't fit in the device.  In that case it
releases the bh assuming that it's going to read a new one, finds that
it won't read, and goes to a label that releases the bh again.

All it needed to do was only brelse() right before overwriting the
current bh with __bread().

Signed-off-by: Zach Brown <zab@redhat.com>
---
v1->v2: merge with Zach fix, commit update

 fs/btrfs/volumes.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b4660c4..7243196 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1681,12 +1681,43 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	 * remove it from the devices list and zero out the old super
 	 */
 	if (clear_super && disk_super) {
+		u64 bytenr;
+		int i;
+
 		/* make sure this device isn't detected as part of
 		 * the FS anymore
 		 */
 		memset(&disk_super->magic, 0, sizeof(disk_super->magic));
 		set_buffer_dirty(bh);
 		sync_dirty_buffer(bh);
+
+		/* clear the mirror copies of super block on the disk
+		 * being removed, 0th copy is been taken care above and
+		 * the below would take of the rest
+		 */
+		for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
+			bytenr = btrfs_sb_offset(i);
+			if (bytenr + BTRFS_SUPER_INFO_SIZE >=
+					i_size_read(bdev->bd_inode))
+				break;
+
+			brelse(bh);
+			bh = __bread(bdev, bytenr / 4096,
+					BTRFS_SUPER_INFO_SIZE);
+			if (!bh)
+				continue;
+
+			disk_super = (struct btrfs_super_block *)bh->b_data;
+
+			if (btrfs_super_bytenr(disk_super) != bytenr ||
+				btrfs_super_magic(disk_super) != BTRFS_MAGIC) {
+				continue;
+			}
+			memset(&disk_super->magic, 0,
+						sizeof(disk_super->magic));
+			set_buffer_dirty(bh);
+			sync_dirty_buffer(bh);
+		}
 	}
 
 	ret = 0;
-- 
1.8.5.3

[PATCH 2/2 v2] Btrfs: all super blocks of the replaced disk must be scratched

[Anand Jain]

In a normal scenario when sys-admin replaces a disk, the
expeted is btrfs will release the disk completely.

However the below test case gives a wrong impression that
replaced disk is still is in use.

$ btrfs rep start /dev/sde /dev/sdg4 /btrfs
$ mkfs.btrfs /dev/sde
/dev/sde appears to contain an existing filesystem (btrfs).
Error: Use the -f option to force overwrite.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: add missing signed-off

 fs/btrfs/volumes.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7243196..1567439 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6311,16 +6311,33 @@ int btrfs_scratch_superblock(struct btrfs_device *device)
 {
 	struct buffer_head *bh;
 	struct btrfs_super_block *disk_super;
+	int i;
+	u64 bytenr;
 
-	bh = btrfs_read_dev_super(device->bdev);
-	if (!bh)
-		return -EINVAL;
-	disk_super = (struct btrfs_super_block *)bh->b_data;
+	for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) {
+		bytenr = btrfs_sb_offset(i);
+		if (bytenr + BTRFS_SUPER_INFO_SIZE >=
+				i_size_read(device->bdev->bd_inode))
+			break;
 
-	memset(&disk_super->magic, 0, sizeof(disk_super->magic));
-	set_buffer_dirty(bh);
-	sync_dirty_buffer(bh);
-	brelse(bh);
+		bh = __bread(device->bdev, bytenr / 4096,
+					BTRFS_SUPER_INFO_SIZE);
+		if (!bh)
+			continue;
+
+		disk_super = (struct btrfs_super_block *)bh->b_data;
+		if (btrfs_super_bytenr(disk_super) != bytenr ||
+				btrfs_super_magic(disk_super) != BTRFS_MAGIC) {
+			brelse(bh);
+			continue;
+		}
+
+		memset(&disk_super->magic, 0, sizeof(disk_super->magic));
+
+		set_buffer_dirty(bh);
+		sync_dirty_buffer(bh);
+		brelse(bh);
+	}
 
 	return 0;
 }
-- 
1.8.5.3

Re: [PATCH 1/2 v2] btrfs: btrfs_rm_device() should zero mirror SB as well

[David Sterba]

On Mon, Mar 31, 2014 at 10:13:56PM +0800, Anand Jain wrote:
> From: Anand Jain <anand.jain@oracle.com>
> 
> This fix will ensure all SB copies on the disk is zeroed
> when the disk is intentionally removed. This helps to
> better manage disks in the user land.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> btrfs: don't double brelse on device rm
> 
> Device removal currently causes bdev removal to try to double free a bh
> in the bdev:
> 
> [   55.714833] WARNING: at fs/buffer.c:1160 __brelse+0x36/0x40()
> [   55.714833] VFS: brelse: Trying to free free buffer
> 
> Commit 7e3d9ebb1 added a double release of the bh for a device being
> removed when all the supers don't fit in the device.  In that case it
> releases the bh assuming that it's going to read a new one, finds that
> it won't read, and goes to a label that releases the bh again.
> 
> All it needed to do was only brelse() right before overwriting the
> current bh with __bread().
> 
> Signed-off-by: Zach Brown <zab@redhat.com>

This is a bit confusing, two changelogs, one patch, the referenced
commit id does not in fact exist. To keep all due credits, 2 patches
would make sense but ... up to you.

Re: [PATCH 1/2 v2] btrfs: btrfs_rm_device() should zero mirror SB as well

[Anand Jain]

On 04/04/2014 20:39, David Sterba wrote:
> On Mon, Mar 31, 2014 at 10:13:56PM +0800, Anand Jain wrote:
>> From: Anand Jain <anand.jain@oracle.com>
>>
>> This fix will ensure all SB copies on the disk is zeroed
>> when the disk is intentionally removed. This helps to
>> better manage disks in the user land.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> btrfs: don't double brelse on device rm
>>
>> Device removal currently causes bdev removal to try to double free a bh
>> in the bdev:
>>
>> [   55.714833] WARNING: at fs/buffer.c:1160 __brelse+0x36/0x40()
>> [   55.714833] VFS: brelse: Trying to free free buffer
>>
>> Commit 7e3d9ebb1 added a double release of the bh for a device being
>> removed when all the supers don't fit in the device.  In that case it
>> releases the bh assuming that it's going to read a new one, finds that
>> it won't read, and goes to a label that releases the bh again.
>>
>> All it needed to do was only brelse() right before overwriting the
>> current bh with __bread().
>>
>> Signed-off-by: Zach Brown <zab@redhat.com>
>
> This is a bit confusing, two changelogs, one patch, the referenced
> commit id does not in fact exist. To keep all due credits, 2 patches
> would make sense but ... up to you.

  Sorry to know it was confusing. I have sent out V3 hope that's better.

-Anand


> --
> 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
>

[PATCH v3] btrfs: btrfs_rm_device() should zero mirror SB as well

[Anand Jain]

From: Anand Jain <anand.jain@oracle.com>

This fix will ensure all SB copies on the disk is zeroed
when the disk is intentionally removed. This helps to
better manage disks in the user land.

This version of patch also merges the Zach patch as below.

 btrfs: don't double brelse on device rm

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Signed-off-by: Zach Brown <zab@redhat.com>
---

 v2->v3: commit rework, continues to have Zach patch merged
 v1->v2: merge with Zach fix, commit update

 fs/btrfs/volumes.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b4660c4..7243196 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1681,12 +1681,43 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	 * remove it from the devices list and zero out the old super
 	 */
 	if (clear_super && disk_super) {
+		u64 bytenr;
+		int i;
+
 		/* make sure this device isn't detected as part of
 		 * the FS anymore
 		 */
 		memset(&disk_super->magic, 0, sizeof(disk_super->magic));
 		set_buffer_dirty(bh);
 		sync_dirty_buffer(bh);
+
+		/* clear the mirror copies of super block on the disk
+		 * being removed, 0th copy is been taken care above and
+		 * the below would take of the rest
+		 */
+		for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
+			bytenr = btrfs_sb_offset(i);
+			if (bytenr + BTRFS_SUPER_INFO_SIZE >=
+					i_size_read(bdev->bd_inode))
+				break;
+
+			brelse(bh);
+			bh = __bread(bdev, bytenr / 4096,
+					BTRFS_SUPER_INFO_SIZE);
+			if (!bh)
+				continue;
+
+			disk_super = (struct btrfs_super_block *)bh->b_data;
+
+			if (btrfs_super_bytenr(disk_super) != bytenr ||
+				btrfs_super_magic(disk_super) != BTRFS_MAGIC) {
+				continue;
+			}
+			memset(&disk_super->magic, 0,
+						sizeof(disk_super->magic));
+			set_buffer_dirty(bh);
+			sync_dirty_buffer(bh);
+		}
 	}
 
 	ret = 0;
-- 
1.8.5.3