[PATCH] btrfs: s_bdev is not null after missing replace

[PATCH] btrfs: s_bdev is not null after missing replace

[Anand Jain]

Yauhen reported in the ML that s_bdev is null at mount, and
s_bdev gets updated to some device when missing device is
replaced, as because bdev is null for missing device, things
gets matched up. Fix this by checking if s_bdev is set. I
didn't want to completely remove updating s_bdev because
the future multi device support at vfs layer may need it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reported-by: Yauhen Kharuzhy <yauhen.kharuzhy@zavadatar.com>
---
 fs/btrfs/dev-replace.c | 3 ++-
 fs/btrfs/volumes.c     | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index ddc4843604df..f8ff67609c18 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -569,7 +569,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	ASSERT(list_empty(&src_device->resized_list));
 	tgt_device->commit_total_bytes = src_device->commit_total_bytes;
 	tgt_device->commit_bytes_used = src_device->bytes_used;
-	if (fs_info->sb->s_bdev == src_device->bdev)
+	if (fs_info->sb->s_bdev &&
+		(fs_info->sb->s_bdev == src_device->bdev))
 		fs_info->sb->s_bdev = tgt_device->bdev;
 	if (fs_info->fs_devices->latest_bdev == src_device->bdev)
 		fs_info->fs_devices->latest_bdev = tgt_device->bdev;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c2a87fc127a7..06de1e9b7891 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1898,7 +1898,8 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 
 	next_device = list_entry(root->fs_info->fs_devices->devices.next,
 				 struct btrfs_device, dev_list);
-	if (device->bdev == root->fs_info->sb->s_bdev)
+	if (root->fs_info->sb->s_bdev &&
+		(root->fs_info->sb->s_bdev == device->bdev))
 		root->fs_info->sb->s_bdev = next_device->bdev;
 	if (device->bdev == root->fs_info->fs_devices->latest_bdev)
 		root->fs_info->fs_devices->latest_bdev = next_device->bdev;
@@ -2084,7 +2085,8 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 
 	next_device = list_entry(fs_info->fs_devices->devices.next,
 				 struct btrfs_device, dev_list);
-	if (tgtdev->bdev == fs_info->sb->s_bdev)
+	if (fs_info->sb->s_bdev &&
+		(tgtdev->bdev == fs_info->sb->s_bdev))
 		fs_info->sb->s_bdev = next_device->bdev;
 	if (tgtdev->bdev == fs_info->fs_devices->latest_bdev)
 		fs_info->fs_devices->latest_bdev = next_device->bdev;
-- 
2.7.0

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

Re: [PATCH] btrfs: s_bdev is not null after missing replace

[David Sterba]

On Thu, Apr 14, 2016 at 06:24:10PM +0800, Anand Jain wrote:
> Yauhen reported in the ML that s_bdev is null at mount, and
> s_bdev gets updated to some device when missing device is
> replaced, as because bdev is null for missing device, things
> gets matched up. Fix this by checking if s_bdev is set. I
> didn't want to completely remove updating s_bdev because
> the future multi device support at vfs layer may need it.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reported-by: Yauhen Kharuzhy <yauhen.kharuzhy@zavadatar.com>

Do you have a testcase for that? As there are more patches touching the
device pointers I'd rather see some test coverage. Thanks.

Re: [PATCH] btrfs: s_bdev is not null after missing replace

[Yauhen Kharuzhy]

On Tue, May 03, 2016 at 07:31:48PM +0200, David Sterba wrote:
> On Thu, Apr 14, 2016 at 06:24:10PM +0800, Anand Jain wrote:
> > Yauhen reported in the ML that s_bdev is null at mount, and
> > s_bdev gets updated to some device when missing device is
> > replaced, as because bdev is null for missing device, things
> > gets matched up. Fix this by checking if s_bdev is set. I
> > didn't want to completely remove updating s_bdev because
> > the future multi device support at vfs layer may need it.
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > Reported-by: Yauhen Kharuzhy <yauhen.kharuzhy@zavadatar.com>
> 
> Do you have a testcase for that? As there are more patches touching the
> device pointers I'd rather see some test coverage. Thanks.

Testcase is ('global spare' patchset is needed for device closing
support):

1) create RAID, mount -> s_bdev is NULL
2) replace latest device by another -> s_bdev is bdev of new drive
3) remove drive added by replace
4) touch mountpount and do btrfs fi sync (device will be closed and
marked as missing here) -> s_bdev is invalid pointer to closed and freed
bdev
5) unmount FS -> oops

Something like:

mkfs.btrfs -d raid5 -m raid5 <dev1> <dev2>... <devN-1>
mount <dev1> /mnt
btrfs replace start -B <devN-1> <devN> /mnt
_devmgt_remove <devN> # _devmgt_remove is helper for detaching scsi device
touch /mnt
btrfs fi sync /mnt
umount /mnt


I will try to make xfstest scripts for this case and for other cases
reported by me.

-- 
Yauhen Kharuzhy

Re: [PATCH] btrfs: s_bdev is not null after missing replace

[Yauhen Kharuzhy]

On Tue, May 03, 2016 at 11:34:50PM +0300, Yauhen Kharuzhy wrote:
> On Tue, May 03, 2016 at 07:31:48PM +0200, David Sterba wrote:
> > On Thu, Apr 14, 2016 at 06:24:10PM +0800, Anand Jain wrote:
> > > Yauhen reported in the ML that s_bdev is null at mount, and
> > > s_bdev gets updated to some device when missing device is
> > > replaced, as because bdev is null for missing device, things
> > > gets matched up. Fix this by checking if s_bdev is set. I
> > > didn't want to completely remove updating s_bdev because
> > > the future multi device support at vfs layer may need it.
> > > 
> > > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > > Reported-by: Yauhen Kharuzhy <yauhen.kharuzhy@zavadatar.com>
> > 
> > Do you have a testcase for that? As there are more patches touching the
> > device pointers I'd rather see some test coverage. Thanks.
> 
> Testcase is ('global spare' patchset is needed for device closing
> support):
> 
> 1) create RAID, mount -> s_bdev is NULL
sorry, for match old s_bdev (NULL) with replaced device:

1.1) remove latest drive and touch FS to make device missing -> device->bdev is NULL

> 2) replace latest device by another -> s_bdev is bdev of new drive
> 3) remove drive added by replace
> 4) touch mountpount and do btrfs fi sync (device will be closed and
> marked as missing here) -> s_bdev is invalid pointer to closed and freed
> bdev
> 5) unmount FS -> oops
> 
> Something like:
> 
> mkfs.btrfs -d raid5 -m raid5 <dev1> <dev2>... <devN-1>
> mount <dev1> /mnt
_devmgt_remove <devN-1>
touch /mnt
btrfs fi sync /mnt

btrfs replace start -B <missing devid> <devN> /mnt
> _devmgt_remove <devN> # _devmgt_remove is helper for detaching scsi device
> touch /mnt
> btrfs fi sync /mnt
> umount /mnt
> 
> 
> I will try to make xfstest scripts for this case and for other cases
> reported by me.
> 
> -- 
> Yauhen Kharuzhy

-- 
Yauhen Kharuzhy

Re: [PATCH] btrfs: s_bdev is not null after missing replace

[Anand Jain]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]




On 05/04/2016 01:31 AM, David Sterba wrote:
> On Thu, Apr 14, 2016 at 06:24:10PM +0800, Anand Jain wrote:
>> Yauhen reported in the ML that s_bdev is null at mount, and
>> s_bdev gets updated to some device when missing device is
>> replaced, as because bdev is null for missing device, things
>> gets matched up. Fix this by checking if s_bdev is set. I
>> didn't want to completely remove updating s_bdev because
>> the future multi device support at vfs layer may need it.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Reported-by: Yauhen Kharuzhy <yauhen.kharuzhy@zavadatar.com>
>
> Do you have a testcase for that?

  For this particular bug/fix no. As I went through the vfs
  layer to understand the s_bdev usage. It appears that s_bdev
  is used for fsync.
  But btrfs kept s_bdev NULL, which will make vfs layer to fail
  safe in that context.
  We need to continue to keep s_bdev null, even after the
  replace missing. The bug is that it didn't. As shown below.

  mkfs.btrfs -f -draid1 -mraid1 /dev/sdc /dev/sdd /dev/sde
  modprobe -r btrfs && modprobe btrfs
  mount -o degraded,device=/dev/sdc /dev/sde /btrfs
-----
  cat /proc/fs/btrfs/fsinfo  [1]
  [fsid: 60ca666b-83e8-4a28-a0cb-9b7ddb0451e2]
	sb->s_bdev:		null
	latest_bdev:		sde
-----
  btrfs repl start 2 /dev/sdf /btrfs -f

-----
cat /proc/fs/btrfs/fsinfo
[fsid: 60ca666b-83e8-4a28-a0cb-9b7ddb0451e2]
	sb->s_bdev:		sdf
	latest_bdev:		sde
-----


> As there are more patches touching the
> device pointers I'd rather see some test coverage. Thanks.

  Hm, yes the btrfs-vm test coverage needs a review at xfstests.
  Let me try.

  For my quick verifications, I have been using..
    https://github.com/asj/exercise-btrfs-volume-mgt.git
  has around quick 30+ btrfs-vm related tests. But very raw.

  Omar added test case 027 to test replace missing. The for-next
  plus 3 patches[2] passes 027 as well.

  Do you have any particular test coverage idea in mind ?

Thanks, Anand


[1]
For /proc/fs/btrfs/fsinfo see attached which should be applied
on top of the patch.
[PATCH] btrfs: procfs: devlist: introduce /proc/fs/btrfs/devlist

[2]
86eb5d598d58 btrfs: cleanup assigning next active device with a check
7b09eb01d80c btrfs: s_bdev is not null after missing replace
6237a895bc31 btrfs: fix lock dep warning move scratch super outside of 
chunk_mutex

[-- Attachment #2: 0003-d-btrfs-procfs-fsinfo-introduce-proc-fs-btrfs-fsinfo-f.patch --]
[-- Type: text/x-patch, Size: 3730 bytes --]

>From 0411476bdf720556945457f63032aca8b3f5390e Mon Sep 17 00:00:00 2001
From: Anand Jain <anand.jain@oracle.com>
Date: Thu, 14 Apr 2016 11:28:41 +0800
Subject: [PATCH 2/2] btrfs: procfs: fsinfo: introduce /proc/fs/btrfs/fsinfo
 for debugging

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/procfs.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/procfs.c b/fs/btrfs/procfs.c
index 99edcad8a825..337ae1212130 100644
--- a/fs/btrfs/procfs.c
+++ b/fs/btrfs/procfs.c
@@ -7,9 +7,48 @@
 
 #define BTRFS_PROC_PATH		"fs/btrfs"
 #define BTRFS_PROC_DEVLIST	"devlist"
+#define BTRFS_PROC_FSINFO	"fsinfo"
 
 struct proc_dir_entry	*btrfs_proc_root;
 
+void btrfs_print_fsinfo(struct seq_file *seq)
+{
+
+	/* Btrfs Procfs String Len */
+#define BPSL	256
+#define BTRFS_SEQ_PRINT2(plist, arg)\
+		snprintf(str, BPSL, plist, arg);\
+		seq_printf(seq, str)
+
+	char str[BPSL];
+	char b[BDEVNAME_SIZE];
+	struct list_head *cur_uuid;
+	struct btrfs_fs_info *fs_info;
+	struct btrfs_fs_devices *fs_devices;
+	struct list_head *fs_uuids = btrfs_get_fs_uuids();
+
+	seq_printf(seq, "\n#Its Experimental, parameters may change without notice.\n\n");
+
+	mutex_lock(&uuid_mutex);
+	list_for_each(cur_uuid, fs_uuids) {
+		fs_devices  = list_entry(cur_uuid, struct btrfs_fs_devices, list);
+		fs_info = fs_devices->fs_info;
+		if (!fs_info)
+			continue;
+
+		BTRFS_SEQ_PRINT2("[fsid: %pU]\n", fs_devices->fsid);
+		BTRFS_SEQ_PRINT2("\tsb->s_bdev:\t\t%s\n",
+				fs_info->sb->s_bdev ?
+				bdevname(fs_info->sb->s_bdev, b):
+				"null");
+		BTRFS_SEQ_PRINT2("\tlatest_bdev:\t\t%s\n",
+				fs_devices->latest_bdev ?
+				bdevname(fs_devices->latest_bdev, b):
+				"null");
+	}
+	mutex_unlock(&uuid_mutex);
+}
+
 void btrfs_print_devlist(struct seq_file *seq)
 {
 
@@ -120,20 +159,40 @@ again_fs_devs:
 	}
 	mutex_unlock(&uuid_mutex);
 }
+
+static int btrfs_fsinfo_show(struct seq_file *seq, void *offset)
+{
+	btrfs_print_fsinfo(seq);
+	return 0;
+}
+
 static int btrfs_devlist_show(struct seq_file *seq, void *offset)
 {
 	btrfs_print_devlist(seq);
 	return 0;
 }
 
-static int btrfs_seq_open(struct inode *inode, struct file *file)
+static int btrfs_seq_fsinfo_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, btrfs_fsinfo_show, PDE_DATA(inode));
+}
+
+static int btrfs_seq_devlist_open(struct inode *inode, struct file *file)
 {
 	return single_open(file, btrfs_devlist_show, PDE_DATA(inode));
 }
 
-static const struct file_operations btrfs_seq_fops = {
+static const struct file_operations btrfs_seq_devlist_fops = {
 	.owner   = THIS_MODULE,
-	.open    = btrfs_seq_open,
+	.open    = btrfs_seq_devlist_open,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = single_release,
+};
+
+static const struct file_operations btrfs_seq_fsinfo_fops = {
+	.owner   = THIS_MODULE,
+	.open    = btrfs_seq_fsinfo_open,
 	.read    = seq_read,
 	.llseek  = seq_lseek,
 	.release = single_release,
@@ -142,16 +201,21 @@ static const struct file_operations btrfs_seq_fops = {
 void btrfs_init_procfs(void)
 {
 	btrfs_proc_root = proc_mkdir(BTRFS_PROC_PATH, NULL);
-	if (btrfs_proc_root)
+	if (btrfs_proc_root) {
 		proc_create_data(BTRFS_PROC_DEVLIST, S_IRUGO, btrfs_proc_root,
-					&btrfs_seq_fops, NULL);
+					&btrfs_seq_devlist_fops, NULL);
+		proc_create_data(BTRFS_PROC_FSINFO, S_IRUGO, btrfs_proc_root,
+					&btrfs_seq_fsinfo_fops, NULL);
+	}
 	return;
 }
 
 void btrfs_exit_procfs(void)
 {
-	if (btrfs_proc_root)
+	if (btrfs_proc_root) {
 		remove_proc_entry(BTRFS_PROC_DEVLIST, btrfs_proc_root);
+		remove_proc_entry(BTRFS_PROC_FSINFO, btrfs_proc_root);
+	}
 	remove_proc_entry(BTRFS_PROC_PATH, NULL);
 }
 
-- 
2.7.0