[PATCH resend 00/13] misc patches plus Introduce device delete by devid

[PATCH resend 00/13] misc patches plus Introduce device delete by devid

[Anand Jain]

Hi Dave,

 These patches are based on top of your for-next.

Patch 1-5 are miscellaneous fixes which were sent before as independent
patch, they aren't related to the patch set 'Introduce device delete by
devid' but they are included here as the patch apply may fail without
them.

Patch 6-12 are related to 'Introduce device delete by devid'

Patch 13 is device related which is good to be integrated. Earlier
it was withdrawn wrongly after seeing some stale device in a test
environment. But later it was found that this patch wasn't the reason
for the stale devices and this fix is good.

Thanks, Anand

Anand Jain (13):
  btrfs: pass the error code to the btrfs_std_error and log ret
  btrfs: create a helper function to read the disk super
  btrfs: maintain consistency in logging to help debugging
  btrfs: device path change must be logged
  Btrfs: fix fs logging for multi device
  btrfs: create helper function __check_raid_min_devices()
  btrfs: clean up and optimize __check_raid_min_device()
  btrfs: create helper btrfs_find_device_by_user_input()
  btrfs: make use of btrfs_find_device_by_user_input()
  btrfs: enhance btrfs_find_device_by_user_input() to check device path
  btrfs: make use of btrfs_scratch_superblocks() in btrfs_rm_device()
  btrfs: introduce device delete by devid
  btrfs: optimize check for stale device

 fs/btrfs/dev-replace.c     |  28 +---
 fs/btrfs/ioctl.c           |  65 +++++++++-
 fs/btrfs/super.c           |   4 +-
 fs/btrfs/volumes.c         | 316 ++++++++++++++++++++-------------------------
 fs/btrfs/volumes.h         |   5 +-
 include/uapi/linux/btrfs.h |  14 +-
 6 files changed, 226 insertions(+), 206 deletions(-)

-- 
2.7.0

[PATCH v2 01/13] btrfs: pass the error code to the btrfs_std_error and log ret

[Anand Jain]

This patch will log return value of add/del_qgroup_relation() and pass the
err code of btrfs_run_qgroups to the btrfs_std_error().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: fix the forgotten git commit amend, to take in compile fail, sorry

 fs/btrfs/ioctl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 293ecd0..e54a4e9 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4841,10 +4841,15 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 						sa->src, sa->dst);
 	}
 
+	if (ret)
+		btrfs_err(root->fs_info,
+			"add/del qgroup relation failed, assign %llu ret %d",
+			sa->assign, ret);
+
 	/* update qgroup status and info */
 	err = btrfs_run_qgroups(trans, root->fs_info);
 	if (err < 0)
-		btrfs_std_error(root->fs_info, ret,
+		btrfs_std_error(root->fs_info, err,
 			    "failed to update qgroup status and info\n");
 	err = btrfs_end_transaction(trans, root);
 	if (err && !ret)
-- 
2.7.0

[PATCH 02/13] btrfs: create a helper function to read the disk super

[Anand Jain]

A part of code from btrfs_scan_one_device() is moved to a new function
btrfs_read_disk_super(), so that former function looks cleaner. (In this
process it also moves the code which ensures null terminating label). So
this creates easy opportunity to merge various duplicate codes on read
disk super. Earlier attempt to merge duplicate codes highlighted that
there were some issues for which there are duplicate codes (to read disk
super), however it was not clear what was the issue. So until we figure
that out, its better to keep them in a separate functions.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 87 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 52 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6e0e439..9860b10 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -988,6 +988,56 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 	return ret;
 }
 
+void btrfs_release_disk_super(struct page *page)
+{
+	kunmap(page);
+	page_cache_release(page);
+}
+
+int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
+		struct page **page, struct btrfs_super_block **disk_super)
+{
+	void *p;
+	pgoff_t index;
+
+	/* make sure our super fits in the device */
+	if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
+		return 1;
+
+	/* make sure our super fits in the page */
+	if (sizeof(**disk_super) > PAGE_CACHE_SIZE)
+		return 1;
+
+	/* make sure our super doesn't straddle pages on disk */
+	index = bytenr >> PAGE_CACHE_SHIFT;
+	if ((bytenr + sizeof(**disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
+		return 1;
+
+	/* pull in the page with our super */
+	*page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
+				   index, GFP_NOFS);
+
+	if (IS_ERR_OR_NULL(*page))
+		return 1;
+
+	p = kmap(*page);
+
+	/* align our pointer to the offset of the super block */
+	*disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
+
+	if (btrfs_super_bytenr(*disk_super) != bytenr ||
+	    btrfs_super_magic(*disk_super) != BTRFS_MAGIC) {
+		btrfs_release_disk_super(*page);
+		return 1;
+	}
+
+	if ((*disk_super)->label[0] &&
+		(*disk_super)->label[BTRFS_LABEL_SIZE - 1])
+		(*disk_super)->label[BTRFS_LABEL_SIZE - 1] = '\0';
+
+	return 0;
+}
+
 /*
  * Look for a btrfs signature on a device. This may be called out of the mount path
  * and we are not allowed to call set_blocksize during the scan. The superblock
@@ -999,13 +1049,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	struct btrfs_super_block *disk_super;
 	struct block_device *bdev;
 	struct page *page;
-	void *p;
 	int ret = -EINVAL;
 	u64 devid;
 	u64 transid;
 	u64 total_devices;
 	u64 bytenr;
-	pgoff_t index;
 
 	/*
 	 * we would like to check all the supers, but that would make
@@ -1018,41 +1066,14 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	mutex_lock(&uuid_mutex);
 
 	bdev = blkdev_get_by_path(path, flags, holder);
-
 	if (IS_ERR(bdev)) {
 		ret = PTR_ERR(bdev);
 		goto error;
 	}
 
-	/* make sure our super fits in the device */
-	if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
+	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super))
 		goto error_bdev_put;
 
-	/* make sure our super fits in the page */
-	if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
-		goto error_bdev_put;
-
-	/* make sure our super doesn't straddle pages on disk */
-	index = bytenr >> PAGE_CACHE_SHIFT;
-	if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
-		goto error_bdev_put;
-
-	/* pull in the page with our super */
-	page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
-				   index, GFP_NOFS);
-
-	if (IS_ERR_OR_NULL(page))
-		goto error_bdev_put;
-
-	p = kmap(page);
-
-	/* align our pointer to the offset of the super block */
-	disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
-
-	if (btrfs_super_bytenr(disk_super) != bytenr ||
-	    btrfs_super_magic(disk_super) != BTRFS_MAGIC)
-		goto error_unmap;
-
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
 	transid = btrfs_super_generation(disk_super);
 	total_devices = btrfs_super_num_devices(disk_super);
@@ -1060,8 +1081,6 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
 	if (ret > 0) {
 		if (disk_super->label[0]) {
-			if (disk_super->label[BTRFS_LABEL_SIZE - 1])
-				disk_super->label[BTRFS_LABEL_SIZE - 1] = '\0';
 			printk(KERN_INFO "BTRFS: device label %s ", disk_super->label);
 		} else {
 			printk(KERN_INFO "BTRFS: device fsid %pU ", disk_super->fsid);
@@ -1073,9 +1092,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	if (!ret && fs_devices_ret)
 		(*fs_devices_ret)->total_devices = total_devices;
 
-error_unmap:
-	kunmap(page);
-	page_cache_release(page);
+	btrfs_release_disk_super(page);
 
 error_bdev_put:
 	blkdev_put(bdev, flags);
-- 
2.7.0

[PATCH v2 03/13] btrfs: maintain consistency in logging to help debugging

[Anand Jain]

Optional Label may or may not be set, or it might be set at some time
later. However while debugging to search through the kernel logs the
scripts would need the logs to be consistent, so logs search key words
shouldn't depend on the optional variables, instead fsid is better.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: fix commit log

 fs/btrfs/volumes.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9860b10..36108e9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1080,13 +1080,8 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
 	if (ret > 0) {
-		if (disk_super->label[0]) {
-			printk(KERN_INFO "BTRFS: device label %s ", disk_super->label);
-		} else {
-			printk(KERN_INFO "BTRFS: device fsid %pU ", disk_super->fsid);
-		}
-
-		printk(KERN_CONT "devid %llu transid %llu %s\n", devid, transid, path);
+		printk(KERN_INFO "BTRFS: device fsid %pU devid %llu transid %llu %s\n",
+						disk_super->fsid, devid, transid, path);
 		ret = 0;
 	}
 	if (!ret && fs_devices_ret)
-- 
2.7.0

[PATCH v2 04/13] btrfs: device path change must be logged

[Anand Jain]

>From the issue diagnosable point of view, log if the device path is changed.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 V2: Accepts David's review comment and adds a new ret value 2
     for device_list_add() and based on that the caller function
     would indicate if the path is overwritten (thanks David).

 fs/btrfs/volumes.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 36108e9..76133e0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -585,6 +585,7 @@ void btrfs_free_stale_device(struct btrfs_device *cur_dev)
  *
  * Returns:
  * 1   - first time device is seen
+ * 2   - device already known but now is overwritten with new path
  * 0   - device already known
  * < 0 - error
  */
@@ -684,6 +685,7 @@ static noinline int device_list_add(const char *path,
 			fs_devices->missing_devices--;
 			device->missing = 0;
 		}
+		ret = 2;
 	}
 
 	/*
@@ -1080,8 +1082,9 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
 	if (ret > 0) {
-		printk(KERN_INFO "BTRFS: device fsid %pU devid %llu transid %llu %s\n",
-						disk_super->fsid, devid, transid, path);
+		printk(KERN_INFO "BTRFS: device fsid %pU devid %llu transid %llu %s %s\n",
+						disk_super->fsid, devid, transid, path,
+						ret == 2 ? "(overwritten)":"");
 		ret = 0;
 	}
 	if (!ret && fs_devices_ret)
-- 
2.7.0

[PATCH 05/13] Btrfs: fix fs logging for multi device

[Anand Jain]

In case of multi device btrfs fs, using one of device for
the logging purpose it quite confusing, instead use the
fsid. FSID is bit long, but the device path can be long
as well in some cases.

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

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index bf75200..f0bbfa9 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -190,12 +190,12 @@ static const char * const logtypes[] = {
 
 void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
 {
-	struct super_block *sb = fs_info->sb;
 	char lvl[4];
 	struct va_format vaf;
 	va_list args;
 	const char *type = logtypes[4];
 	int kern_level;
+	struct btrfs_fs_devices *fs_devs = fs_info->fs_devices;
 
 	va_start(args, fmt);
 
@@ -212,7 +212,7 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	printk("%sBTRFS %s (device %s): %pV\n", lvl, type, sb->s_id, &vaf);
+	printk("%sBTRFS: %pU %s: %pV\n", lvl, fs_devs->fsid, type, &vaf);
 
 	va_end(args);
 }
-- 
2.7.0

[PATCH v2 06/13] btrfs: create helper function __check_raid_min_devices()

[Anand Jain]

move a section of btrfs_rm_device() code to check for min number of the
devices into the function __check_raid_min_devices()

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: commit update and title renamed from
    Btrfs: move check for min number of devices to a function
 fs/btrfs/volumes.c | 51 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 76133e0..506d1fd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1703,23 +1703,20 @@ out:
 	return ret;
 }
 
-int btrfs_rm_device(struct btrfs_root *root, char *device_path)
+static int __check_raid_min_devices(struct btrfs_root *root)
 {
-	struct btrfs_device *device;
-	struct btrfs_device *next_device;
-	struct block_device *bdev;
-	struct buffer_head *bh = NULL;
-	struct btrfs_super_block *disk_super;
-	struct btrfs_fs_devices *cur_devices;
 	u64 all_avail;
-	u64 devid;
 	u64 num_devices;
-	u8 *dev_uuid;
 	unsigned seq;
 	int ret = 0;
-	bool clear_super = false;
 
-	mutex_lock(&uuid_mutex);
+	num_devices = root->fs_info->fs_devices->num_devices;
+	btrfs_dev_replace_lock(&root->fs_info->dev_replace);
+	if (btrfs_dev_replace_is_ongoing(&root->fs_info->dev_replace)) {
+		WARN_ON(num_devices < 1);
+		num_devices--;
+	}
+	btrfs_dev_replace_unlock(&root->fs_info->dev_replace);
 
 	do {
 		seq = read_seqbegin(&root->fs_info->profiles_lock);
@@ -1729,14 +1726,6 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 			    root->fs_info->avail_metadata_alloc_bits;
 	} while (read_seqretry(&root->fs_info->profiles_lock, seq));
 
-	num_devices = root->fs_info->fs_devices->num_devices;
-	btrfs_dev_replace_lock(&root->fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&root->fs_info->dev_replace)) {
-		WARN_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_unlock(&root->fs_info->dev_replace);
-
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) && num_devices <= 4) {
 		ret = BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET;
 		goto out;
@@ -1758,6 +1747,30 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 		goto out;
 	}
 
+out:
+	return ret;
+}
+
+int btrfs_rm_device(struct btrfs_root *root, char *device_path)
+{
+	struct btrfs_device *device;
+	struct btrfs_device *next_device;
+	struct block_device *bdev;
+	struct buffer_head *bh = NULL;
+	struct btrfs_super_block *disk_super;
+	struct btrfs_fs_devices *cur_devices;
+	u64 devid;
+	u64 num_devices;
+	u8 *dev_uuid;
+	int ret = 0;
+	bool clear_super = false;
+
+	mutex_lock(&uuid_mutex);
+
+	ret = __check_raid_min_devices(root);
+	if (ret)
+		goto out;
+
 	if (strcmp(device_path, "missing") == 0) {
 		struct list_head *devices;
 		struct btrfs_device *tmp;
-- 
2.7.0

[PATCH 07/13] btrfs: clean up and optimize __check_raid_min_device()

[Anand Jain]

__check_raid_min_device() which was pealed from btrfs_rm_device()
maintianed its original code to show the block move. This patch cleans up
__check_raid_min_device().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 506d1fd..191eb82 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1703,52 +1703,47 @@ out:
 	return ret;
 }
 
-static int __check_raid_min_devices(struct btrfs_root *root)
+static int __check_raid_min_devices(struct btrfs_fs_info *fs_info)
 {
 	u64 all_avail;
 	u64 num_devices;
 	unsigned seq;
-	int ret = 0;
 
-	num_devices = root->fs_info->fs_devices->num_devices;
-	btrfs_dev_replace_lock(&root->fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&root->fs_info->dev_replace)) {
+	num_devices = fs_info->fs_devices->num_devices;
+	btrfs_dev_replace_lock(&fs_info->dev_replace);
+	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
 		WARN_ON(num_devices < 1);
 		num_devices--;
 	}
-	btrfs_dev_replace_unlock(&root->fs_info->dev_replace);
+	btrfs_dev_replace_unlock(&fs_info->dev_replace);
 
 	do {
-		seq = read_seqbegin(&root->fs_info->profiles_lock);
+		seq = read_seqbegin(&fs_info->profiles_lock);
 
-		all_avail = root->fs_info->avail_data_alloc_bits |
-			    root->fs_info->avail_system_alloc_bits |
-			    root->fs_info->avail_metadata_alloc_bits;
-	} while (read_seqretry(&root->fs_info->profiles_lock, seq));
+		all_avail = fs_info->avail_data_alloc_bits |
+			    fs_info->avail_system_alloc_bits |
+			    fs_info->avail_metadata_alloc_bits;
+	} while (read_seqretry(&fs_info->profiles_lock, seq));
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) && num_devices <= 4) {
-		ret = BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET;
-		goto out;
+		return BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) && num_devices <= 2) {
-		ret = BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET;
-		goto out;
+		return BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID5) &&
-	    root->fs_info->fs_devices->rw_devices <= 2) {
-		ret = BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET;
-		goto out;
+	    fs_info->fs_devices->rw_devices <= 2) {
+		return BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET;
 	}
+
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID6) &&
-	    root->fs_info->fs_devices->rw_devices <= 3) {
-		ret = BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET;
-		goto out;
+	    fs_info->fs_devices->rw_devices <= 3) {
+		return BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET;
 	}
 
-out:
-	return ret;
+	return 0;
 }
 
 int btrfs_rm_device(struct btrfs_root *root, char *device_path)
@@ -1767,7 +1762,7 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 
 	mutex_lock(&uuid_mutex);
 
-	ret = __check_raid_min_devices(root);
+	ret = __check_raid_min_devices(root->fs_info);
 	if (ret)
 		goto out;
 
-- 
2.7.0

[PATCH v2 08/13] btrfs: create helper btrfs_find_device_by_user_input()

[Anand Jain]

The patch renames btrfs_dev_replace_find_srcdev() to
btrfs_find_device_by_user_input() and moves it to volumes.c, so that
delete device can use it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: changed title from
    'Btrfs: rename btrfs_dev_replace_find_srcdev()'
    and commit update
 fs/btrfs/dev-replace.c | 24 +-----------------------
 fs/btrfs/volumes.c     | 19 +++++++++++++++++++
 fs/btrfs/volumes.h     |  3 +++
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 01ce5fc..b3e3f94 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -44,9 +44,6 @@ static void btrfs_dev_replace_update_device_in_mapping_tree(
 						struct btrfs_fs_info *fs_info,
 						struct btrfs_device *srcdev,
 						struct btrfs_device *tgtdev);
-static int btrfs_dev_replace_find_srcdev(struct btrfs_root *root, u64 srcdevid,
-					 char *srcdev_name,
-					 struct btrfs_device **device);
 static u64 __btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
 static int btrfs_dev_replace_kthread(void *data);
 static int btrfs_dev_replace_continue_on_mount(struct btrfs_fs_info *fs_info);
@@ -329,7 +326,7 @@ int btrfs_dev_replace_start(struct btrfs_root *root,
 
 	/* the disk copy procedure reuses the scrub code */
 	mutex_lock(&fs_info->volume_mutex);
-	ret = btrfs_dev_replace_find_srcdev(root, args->start.srcdevid,
+	ret = btrfs_find_device_by_user_input(root, args->start.srcdevid,
 					    args->start.srcdev_name,
 					    &src_device);
 	if (ret) {
@@ -624,25 +621,6 @@ static void btrfs_dev_replace_update_device_in_mapping_tree(
 	write_unlock(&em_tree->lock);
 }
 
-static int btrfs_dev_replace_find_srcdev(struct btrfs_root *root, u64 srcdevid,
-					 char *srcdev_name,
-					 struct btrfs_device **device)
-{
-	int ret;
-
-	if (srcdevid) {
-		ret = 0;
-		*device = btrfs_find_device(root->fs_info, srcdevid, NULL,
-					    NULL);
-		if (!*device)
-			ret = -ENOENT;
-	} else {
-		ret = btrfs_find_device_missing_or_by_path(root, srcdev_name,
-							   device);
-	}
-	return ret;
-}
-
 void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
 			      struct btrfs_ioctl_dev_replace_args *args)
 {
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 191eb82..889fc0c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2124,6 +2124,25 @@ int btrfs_find_device_missing_or_by_path(struct btrfs_root *root,
 	}
 }
 
+int btrfs_find_device_by_user_input(struct btrfs_root *root, u64 srcdevid,
+					 char *srcdev_name,
+					 struct btrfs_device **device)
+{
+	int ret;
+
+	if (srcdevid) {
+		ret = 0;
+		*device = btrfs_find_device(root->fs_info, srcdevid, NULL,
+					    NULL);
+		if (!*device)
+			ret = -ENOENT;
+	} else {
+		ret = btrfs_find_device_missing_or_by_path(root, srcdev_name,
+							   device);
+	}
+	return ret;
+}
+
 /*
  * does all the dirty work required for changing file system's UUID.
  */
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 1939ebd..5087393 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -448,6 +448,9 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step);
 int btrfs_find_device_missing_or_by_path(struct btrfs_root *root,
 					 char *device_path,
 					 struct btrfs_device **device);
+int btrfs_find_device_by_user_input(struct btrfs_root *root, u64 srcdevid,
+					 char *srcdev_name,
+					 struct btrfs_device **device);
 struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 					const u64 *devid,
 					const u8 *uuid);
-- 
2.7.0

[PATCH 09/13] btrfs: make use of btrfs_find_device_by_user_input()

[Anand Jain]

btrfs_rm_device() has a section of the code which can be replaced
btrfs_find_device_by_user_input()

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 100 ++++++++++++++++++++---------------------------------
 1 file changed, 37 insertions(+), 63 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 889fc0c..1324a9f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1750,13 +1750,11 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 {
 	struct btrfs_device *device;
 	struct btrfs_device *next_device;
-	struct block_device *bdev;
+	struct block_device *bdev = NULL;
 	struct buffer_head *bh = NULL;
-	struct btrfs_super_block *disk_super;
+	struct btrfs_super_block *disk_super = NULL;
 	struct btrfs_fs_devices *cur_devices;
-	u64 devid;
 	u64 num_devices;
-	u8 *dev_uuid;
 	int ret = 0;
 	bool clear_super = false;
 
@@ -1766,57 +1764,19 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	if (ret)
 		goto out;
 
-	if (strcmp(device_path, "missing") == 0) {
-		struct list_head *devices;
-		struct btrfs_device *tmp;
-
-		device = NULL;
-		devices = &root->fs_info->fs_devices->devices;
-		/*
-		 * It is safe to read the devices since the volume_mutex
-		 * is held.
-		 */
-		list_for_each_entry(tmp, devices, dev_list) {
-			if (tmp->in_fs_metadata &&
-			    !tmp->is_tgtdev_for_dev_replace &&
-			    !tmp->bdev) {
-				device = tmp;
-				break;
-			}
-		}
-		bdev = NULL;
-		bh = NULL;
-		disk_super = NULL;
-		if (!device) {
-			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
-			goto out;
-		}
-	} else {
-		ret = btrfs_get_bdev_and_sb(device_path,
-					    FMODE_WRITE | FMODE_EXCL,
-					    root->fs_info->bdev_holder, 0,
-					    &bdev, &bh);
-		if (ret)
-			goto out;
-		disk_super = (struct btrfs_super_block *)bh->b_data;
-		devid = btrfs_stack_device_id(&disk_super->dev_item);
-		dev_uuid = disk_super->dev_item.uuid;
-		device = btrfs_find_device(root->fs_info, devid, dev_uuid,
-					   disk_super->fsid);
-		if (!device) {
-			ret = -ENOENT;
-			goto error_brelse;
-		}
-	}
+	ret = btrfs_find_device_by_user_input(root, 0, device_path,
+				&device);
+	if (ret)
+		goto out;
 
 	if (device->is_tgtdev_for_dev_replace) {
 		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
-		goto error_brelse;
+		goto out;
 	}
 
 	if (device->writeable && root->fs_info->fs_devices->rw_devices == 1) {
 		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
-		goto error_brelse;
+		goto out;
 	}
 
 	if (device->writeable) {
@@ -1906,16 +1866,33 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	 * at this point, the device is zero sized.  We want to
 	 * remove it from the devices list and zero out the old super
 	 */
-	if (clear_super && disk_super) {
+	if (clear_super) {
 		u64 bytenr;
 		int i;
 
+		if (!disk_super) {
+			ret = btrfs_get_bdev_and_sb(device_path,
+					FMODE_WRITE | FMODE_EXCL,
+					root->fs_info->bdev_holder, 0,
+					&bdev, &bh);
+			if (ret) {
+				/*
+				 * It could be a failed device ok for clear_super
+				 * to fail. So return success
+				 */
+				ret = 0;
+				goto out;
+			}
+
+			disk_super = (struct btrfs_super_block *)bh->b_data;
+		}
 		/* 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);
+		brelse(bh);
 
 		/* clear the mirror copies of super block on the disk
 		 * being removed, 0th copy is been taken care above and
@@ -1927,7 +1904,6 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 					i_size_read(bdev->bd_inode))
 				break;
 
-			brelse(bh);
 			bh = __bread(bdev, bytenr / 4096,
 					BTRFS_SUPER_INFO_SIZE);
 			if (!bh)
@@ -1937,32 +1913,30 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 
 			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);
 		}
-	}
 
-	ret = 0;
-
-	if (bdev) {
-		/* Notify udev that device has changed */
-		btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
+		if (bdev) {
+			/* Notify udev that device has changed */
+			btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
 
-		/* Update ctime/mtime for device path for libblkid */
-		update_dev_time(device_path);
+			/* Update ctime/mtime for device path for libblkid */
+			update_dev_time(device_path);
+			blkdev_put(bdev, FMODE_READ | FMODE_EXCL);
+		}
 	}
 
-error_brelse:
-	brelse(bh);
-	if (bdev)
-		blkdev_put(bdev, FMODE_READ | FMODE_EXCL);
 out:
 	mutex_unlock(&uuid_mutex);
 	return ret;
+
 error_undo:
 	if (device->writeable) {
 		lock_chunks(root);
@@ -1971,7 +1945,7 @@ error_undo:
 		device->fs_devices->rw_devices++;
 		unlock_chunks(root);
 	}
-	goto error_brelse;
+	goto out;
 }
 
 void btrfs_rm_dev_replace_remove_srcdev(struct btrfs_fs_info *fs_info,
-- 
2.7.0

[PATCH v2 10/13] btrfs: enhance btrfs_find_device_by_user_input() to check device path

[Anand Jain]

The operation of device replace and device delete follows same steps upto
some depth with in btrfs kernel, however they don't share codes. This
enhancement will help replace and delete to share codes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: reword subject from
    Btrfs: check device_path in btrfs_find_device_by_user_input()
 fs/btrfs/dev-replace.c | 4 ----
 fs/btrfs/volumes.c     | 3 +++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index b3e3f94..9700a60 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -320,10 +320,6 @@ int btrfs_dev_replace_start(struct btrfs_root *root,
 		return -EINVAL;
 	}
 
-	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
-	    args->start.tgtdev_name[0] == '\0')
-		return -EINVAL;
-
 	/* the disk copy procedure reuses the scrub code */
 	mutex_lock(&fs_info->volume_mutex);
 	ret = btrfs_find_device_by_user_input(root, args->start.srcdevid,
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1324a9f..5dc8b7a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2111,6 +2111,9 @@ int btrfs_find_device_by_user_input(struct btrfs_root *root, u64 srcdevid,
 		if (!*device)
 			ret = -ENOENT;
 	} else {
+		if (!srcdev_name || !srcdev_name[0])
+			return -EINVAL;
+
 		ret = btrfs_find_device_missing_or_by_path(root, srcdev_name,
 							   device);
 	}
-- 
2.7.0

[PATCH v2 11/13] btrfs: make use of btrfs_scratch_superblocks() in btrfs_rm_device()

[Anand Jain]

With the previous patches now the btrfs_scratch_superblocks() is ready to
be used in btrfs_rm_device() so use it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: changelog update
 fs/btrfs/volumes.c | 78 ++++++++++--------------------------------------------
 1 file changed, 14 insertions(+), 64 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5dc8b7a..b24cff2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1750,13 +1750,11 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 {
 	struct btrfs_device *device;
 	struct btrfs_device *next_device;
-	struct block_device *bdev = NULL;
-	struct buffer_head *bh = NULL;
-	struct btrfs_super_block *disk_super = NULL;
 	struct btrfs_fs_devices *cur_devices;
 	u64 num_devices;
 	int ret = 0;
 	bool clear_super = false;
+	char *dev_name = NULL;
 
 	mutex_lock(&uuid_mutex);
 
@@ -1784,6 +1782,11 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 		list_del_init(&device->dev_alloc_list);
 		device->fs_devices->rw_devices--;
 		unlock_chunks(root);
+		dev_name = kstrdup(device->name->str, GFP_NOFS);
+		if (!dev_name) {
+			ret = -ENOMEM;
+			goto error_undo;
+		}
 		clear_super = true;
 	}
 
@@ -1867,73 +1870,20 @@ 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) {
-		u64 bytenr;
-		int i;
-
-		if (!disk_super) {
-			ret = btrfs_get_bdev_and_sb(device_path,
-					FMODE_WRITE | FMODE_EXCL,
-					root->fs_info->bdev_holder, 0,
-					&bdev, &bh);
-			if (ret) {
-				/*
-				 * It could be a failed device ok for clear_super
-				 * to fail. So return success
-				 */
-				ret = 0;
-				goto out;
-			}
-
-			disk_super = (struct btrfs_super_block *)bh->b_data;
-		}
-		/* 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);
-		brelse(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;
-
-			bh = __bread(bdev, bytenr / 4096,
-					BTRFS_SUPER_INFO_SIZE);
-			if (!bh)
-				continue;
+		struct block_device *bdev;
 
-			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);
-		}
-
-		if (bdev) {
-			/* Notify udev that device has changed */
-			btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
-
-			/* Update ctime/mtime for device path for libblkid */
-			update_dev_time(device_path);
+		bdev = blkdev_get_by_path(dev_name, FMODE_READ | FMODE_EXCL,
+						root->fs_info->bdev_holder);
+		if (!IS_ERR(bdev)) {
+			btrfs_scratch_superblocks(bdev, dev_name);
 			blkdev_put(bdev, FMODE_READ | FMODE_EXCL);
 		}
 	}
 
 out:
+	if (dev_name)
+		kfree(dev_name);
+
 	mutex_unlock(&uuid_mutex);
 	return ret;
 
-- 
2.7.0

[PATCH v4 12/13] btrfs: introduce device delete by devid

[Anand Jain]

This introduces new ioctl BTRFS_IOC_RM_DEV_V2, which uses enhanced struct
btrfs_ioctl_vol_args_v2 to carry devid as an user argument.

The patch won't delete the old ioctl interface and so kernel remains
backward compatible with user land progs.

Test case/script:
echo "0 $(blockdev --getsz /dev/sdf) linear /dev/sdf 0" | dmsetup create bad_disk
mkfs.btrfs -f -d raid1 -m raid1 /dev/sdd /dev/sde /dev/mapper/bad_disk
mount /dev/sdd /btrfs
dmsetup suspend bad_disk
echo "0 $(blockdev --getsz /dev/sdf) error /dev/sdf 0" | dmsetup load bad_disk
dmsetup resume bad_disk
echo "bad disk failed. now deleting/replacing"
btrfs dev del  3  /btrfs
echo $?
btrfs fi show /btrfs
umount /btrfs
btrfs-show-super /dev/sdd | egrep num_device
dmsetup remove bad_disk
wipefs -a /dev/sdf

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reported-by: Martin <m_btrfs@ml1.co.uk>
---
v4: enahnced btrfs_ioctl_vol_args_v2 to accept devid instead of
    creating a new structure. Thanks to David.
    commit update and title changed from..
    Btrfs: device delete by devid

v3: commit update, included test case

v2: don't use device->name after free

 fs/btrfs/ioctl.c           | 58 +++++++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/volumes.c         |  4 ++--
 fs/btrfs/volumes.h         |  2 +-
 include/uapi/linux/btrfs.h | 14 ++++++++++-
 4 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e54a4e9..0585d2d 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2667,6 +2667,60 @@ out:
 	return ret;
 }
 
+static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
+{
+	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
+	struct btrfs_ioctl_vol_args_v2 *vol_args;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		return ret;
+
+	vol_args = memdup_user(arg, sizeof(*vol_args));
+	if (IS_ERR(vol_args)) {
+		ret = PTR_ERR(vol_args);
+		goto err_drop;
+	}
+
+	/* Check for compatibility reject unknown flags */
+	if (vol_args->flags & ~BTRFS_VOL_ARG_V2_FLAGS)
+		return -ENOTTY;
+
+	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
+			1)) {
+		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
+		goto out;
+	}
+
+	mutex_lock(&root->fs_info->volume_mutex);
+	if (vol_args->flags & BTRFS_DEVICE_BY_ID) {
+		ret = btrfs_rm_device(root, NULL, vol_args->devid);
+	} else {
+		vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
+		ret = btrfs_rm_device(root, vol_args->name, 0);
+	}
+
+	mutex_unlock(&root->fs_info->volume_mutex);
+	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
+
+	if (!ret) {
+		if (vol_args->flags & BTRFS_DEVICE_BY_ID)
+			btrfs_info(root->fs_info, "disk devid %llu deleted",
+								vol_args->devid);
+		else
+			btrfs_info(root->fs_info, "disk deleted - %s", vol_args->name);
+	}
+out:
+	kfree(vol_args);
+err_drop:
+	mnt_drop_write_file(file);
+	return ret;
+}
+
 static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 {
 	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
@@ -2695,7 +2749,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	}
 
 	mutex_lock(&root->fs_info->volume_mutex);
-	ret = btrfs_rm_device(root, vol_args->name);
+	ret = btrfs_rm_device(root, vol_args->name, 0);
 	mutex_unlock(&root->fs_info->volume_mutex);
 	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
 
@@ -5462,6 +5516,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_add_dev(root, argp);
 	case BTRFS_IOC_RM_DEV:
 		return btrfs_ioctl_rm_dev(file, argp);
+	case BTRFS_IOC_RM_DEV_V2:
+		return btrfs_ioctl_rm_dev_v2(file, argp);
 	case BTRFS_IOC_FS_INFO:
 		return btrfs_ioctl_fs_info(root, argp);
 	case BTRFS_IOC_DEV_INFO:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b24cff2..f47bd0b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1746,7 +1746,7 @@ static int __check_raid_min_devices(struct btrfs_fs_info *fs_info)
 	return 0;
 }
 
-int btrfs_rm_device(struct btrfs_root *root, char *device_path)
+int btrfs_rm_device(struct btrfs_root *root, char *device_path, u64 devid)
 {
 	struct btrfs_device *device;
 	struct btrfs_device *next_device;
@@ -1762,7 +1762,7 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	if (ret)
 		goto out;
 
-	ret = btrfs_find_device_by_user_input(root, 0, device_path,
+	ret = btrfs_find_device_by_user_input(root, devid, device_path,
 				&device);
 	if (ret)
 		goto out;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 5087393..c73d027 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -454,7 +454,7 @@ int btrfs_find_device_by_user_input(struct btrfs_root *root, u64 srcdevid,
 struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 					const u64 *devid,
 					const u8 *uuid);
-int btrfs_rm_device(struct btrfs_root *root, char *device_path);
+int btrfs_rm_device(struct btrfs_root *root, char *device_path, u64 devid);
 void btrfs_cleanup_fs_uuids(void);
 int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
 int btrfs_grow_device(struct btrfs_trans_handle *trans,
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index dea8931..396a4ef 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -36,6 +36,13 @@ struct btrfs_ioctl_vol_args {
 #define BTRFS_SUBVOL_CREATE_ASYNC	(1ULL << 0)
 #define BTRFS_SUBVOL_RDONLY		(1ULL << 1)
 #define BTRFS_SUBVOL_QGROUP_INHERIT	(1ULL << 2)
+#define BTRFS_DEVICE_BY_ID		(1ULL << 3)
+#define BTRFS_VOL_ARG_V2_FLAGS				\
+			(BTRFS_SUBVOL_CREATE_ASYNC |	\
+			BTRFS_SUBVOL_RDONLY |		\
+			BTRFS_SUBVOL_QGROUP_INHERIT |	\
+			BTRFS_DEVICE_BY_ID)
+
 #define BTRFS_FSID_SIZE 16
 #define BTRFS_UUID_SIZE 16
 #define BTRFS_UUID_UNPARSED_SIZE	37
@@ -76,7 +83,10 @@ struct btrfs_ioctl_vol_args_v2 {
 		};
 		__u64 unused[4];
 	};
-	char name[BTRFS_SUBVOL_NAME_MAX + 1];
+	union {
+		char name[BTRFS_SUBVOL_NAME_MAX + 1];
+		u64 devid;
+	};
 };
 
 /*
@@ -659,5 +669,7 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
 				   struct btrfs_ioctl_feature_flags[2])
 #define BTRFS_IOC_GET_SUPPORTED_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 57, \
 				   struct btrfs_ioctl_feature_flags[3])
+#define BTRFS_IOC_RM_DEV_V2 _IOW(BTRFS_IOCTL_MAGIC, 58, \
+				   struct btrfs_ioctl_vol_args_v2)
 
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
2.7.0

[PATCH 13/13] btrfs: optimize check for stale device

[Anand Jain]

Optimize check for stale device to only be checked when there is device
added or changed. If there is no update to the device, there is no need
to call btrfs_free_stale_device().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 This is good to go. The there were some stale devices while testing
 and now I have confirmed it wasn't due to this. Sorry that I was bit
 jumpy on concluding this patch as bad.

 fs/btrfs/volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f47bd0b..b7cbb31 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -701,7 +701,8 @@ static noinline int device_list_add(const char *path,
 	 * if there is new btrfs on an already registered device,
 	 * then remove the stale device entry.
 	 */
-	btrfs_free_stale_device(device);
+	if (ret > 0)
+		btrfs_free_stale_device(device);
 
 	*fs_devices_ret = fs_devices;
 
-- 
2.7.0

Re: [PATCH v2 06/13] btrfs: create helper function __check_raid_min_devices()

[David Sterba]

On Sat, Feb 13, 2016 at 10:01:33AM +0800, Anand Jain wrote:
> move a section of btrfs_rm_device() code to check for min number of the
> devices into the function __check_raid_min_devices()

But this also changes the semantics a bit. In context of device remove
we check if there are going to be enough devices after removal, while
the new helper __check_raid_min_devices should verify that a given
number of devices is OK.

In a pseudo code:

__check_raid_min_devices(fs_info, num_devices) {
	/* check the device constraints */
}

btrfs_rm_device() {
	num_devices = <read number of devices>;

	if (!__check_raid_min_devices(num_devices - 1))
		goto error;
}

As there's only one user of the __check_raid_min_devices function, this
would not break bisection etc, but I'm going to clean it up in followup
patches. There are more changes, I'll send it to the mailing list when
done.

Re: [PATCH 09/13] btrfs: make use of btrfs_find_device_by_user_input()

[David Sterba]

On Sat, Feb 13, 2016 at 10:01:36AM +0800, Anand Jain wrote:
> btrfs_rm_device() has a section of the code which can be replaced
> btrfs_find_device_by_user_input()
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 100 ++++++++++++++++++++---------------------------------
>  1 file changed, 37 insertions(+), 63 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 889fc0c..1324a9f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1750,13 +1750,11 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  {
>  	struct btrfs_device *device;
>  	struct btrfs_device *next_device;
> -	struct block_device *bdev;
> +	struct block_device *bdev = NULL;
>  	struct buffer_head *bh = NULL;
> -	struct btrfs_super_block *disk_super;
> +	struct btrfs_super_block *disk_super = NULL;

disk_super is NULL here and unchanged until ...

>  	struct btrfs_fs_devices *cur_devices;
> -	u64 devid;
>  	u64 num_devices;
> -	u8 *dev_uuid;
>  	int ret = 0;
>  	bool clear_super = false;
>  
> @@ -1766,57 +1764,19 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  	if (ret)
>  		goto out;
>  
> -	if (strcmp(device_path, "missing") == 0) {
> -		struct list_head *devices;
> -		struct btrfs_device *tmp;
> -
> -		device = NULL;
> -		devices = &root->fs_info->fs_devices->devices;
> -		/*
> -		 * It is safe to read the devices since the volume_mutex
> -		 * is held.
> -		 */
> -		list_for_each_entry(tmp, devices, dev_list) {
> -			if (tmp->in_fs_metadata &&
> -			    !tmp->is_tgtdev_for_dev_replace &&
> -			    !tmp->bdev) {
> -				device = tmp;
> -				break;
> -			}
> -		}
> -		bdev = NULL;
> -		bh = NULL;
> -		disk_super = NULL;
> -		if (!device) {
> -			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
> -			goto out;
> -		}
> -	} else {
> -		ret = btrfs_get_bdev_and_sb(device_path,
> -					    FMODE_WRITE | FMODE_EXCL,
> -					    root->fs_info->bdev_holder, 0,
> -					    &bdev, &bh);
> -		if (ret)
> -			goto out;
> -		disk_super = (struct btrfs_super_block *)bh->b_data;
> -		devid = btrfs_stack_device_id(&disk_super->dev_item);
> -		dev_uuid = disk_super->dev_item.uuid;
> -		device = btrfs_find_device(root->fs_info, devid, dev_uuid,
> -					   disk_super->fsid);
> -		if (!device) {
> -			ret = -ENOENT;
> -			goto error_brelse;
> -		}
> -	}
> +	ret = btrfs_find_device_by_user_input(root, 0, device_path,
> +				&device);
> +	if (ret)
> +		goto out;
>  
>  	if (device->is_tgtdev_for_dev_replace) {
>  		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
> -		goto error_brelse;
> +		goto out;
>  	}
>  
>  	if (device->writeable && root->fs_info->fs_devices->rw_devices == 1) {
>  		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
> -		goto error_brelse;
> +		goto out;
>  	}
>  
>  	if (device->writeable) {
> @@ -1906,16 +1866,33 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  	 * at this point, the device is zero sized.  We want to
>  	 * remove it from the devices list and zero out the old super
>  	 */
> -	if (clear_super && disk_super) {
> +	if (clear_super) {
>  		u64 bytenr;
>  		int i;
>  
> +		if (!disk_super) {

... here, so this will always take this branch

> +			ret = btrfs_get_bdev_and_sb(device_path,
> +					FMODE_WRITE | FMODE_EXCL,
> +					root->fs_info->bdev_holder, 0,
> +					&bdev, &bh);
> +			if (ret) {
> +				/*
> +				 * It could be a failed device ok for clear_super
> +				 * to fail. So return success
> +				 */
> +				ret = 0;
> +				goto out;
> +			}
> +
> +			disk_super = (struct btrfs_super_block *)bh->b_data;
> +		}

The patch looks otherwise good so I'm curious if this is a leftover or
I'm missing some logic behind that.

Re: [PATCH 09/13] btrfs: make use of btrfs_find_device_by_user_input()

[David Sterba]

On Mon, Feb 15, 2016 at 05:47:46PM +0100, David Sterba wrote:
> > +	struct btrfs_super_block *disk_super = NULL;
> 
> disk_super is NULL here and unchanged until ...
> 

> > +		if (!disk_super) {
> 
> ... here, so this will always take this branch
> 
> > +			ret = btrfs_get_bdev_and_sb(device_path,
> > +					FMODE_WRITE | FMODE_EXCL,
> > +					root->fs_info->bdev_holder, 0,
> > +					&bdev, &bh);
> > +			if (ret) {
> > +				/*
> > +				 * It could be a failed device ok for clear_super
> > +				 * to fail. So return success
> > +				 */
> > +				ret = 0;
> > +				goto out;
> > +			}
> > +
> > +			disk_super = (struct btrfs_super_block *)bh->b_data;
> > +		}
> 
> The patch looks otherwise good so I'm curious if this is a leftover or
> I'm missing some logic behind that.

Never mind, the code gets removed anyway. Bisectability is not broken.

Re: [PATCH v4 12/13] btrfs: introduce device delete by devid

[David Sterba]

On Sat, Feb 13, 2016 at 10:01:39AM +0800, Anand Jain wrote:
> +	if (vol_args->flags & BTRFS_DEVICE_BY_ID) {
> +		ret = btrfs_rm_device(root, NULL, vol_args->devid);
> +	} else {
> +		vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';

			       BTRFS_SUBVOL_NAME_MAX

Spotted by Chris,

fs/btrfs/ioctl.c:2703: warning: array subscript is above array bounds

my gcc version does not report that. Fixed and for-next pushed.

Re: [PATCH v4 12/13] btrfs: introduce device delete by devid

[Anand Jain]

On 02/17/2016 06:49 PM, David Sterba wrote:
> On Sat, Feb 13, 2016 at 10:01:39AM +0800, Anand Jain wrote:
>> +	if (vol_args->flags & BTRFS_DEVICE_BY_ID) {
>> +		ret = btrfs_rm_device(root, NULL, vol_args->devid);
>> +	} else {
>> +		vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
>
> 			       BTRFS_SUBVOL_NAME_MAX
>
> Spotted by Chris,
>
> fs/btrfs/ioctl.c:2703: warning: array subscript is above array bounds
>
> my gcc version does not report that. Fixed and for-next pushed.

  mine either. Sorry about that, thanks for the catch.

#define BTRFS_PATH_NAME_MAX 4087
#define BTRFS_SUBVOL_NAME_MAX 4039

  I am fine with using BTRFS_SUBVOL_NAME_MAX for now. But theoretical
  anomaly is that add-device code path will use BTRFS_PATH_NAME_MAX and
  delete device will use BTRFS_SUBVOL_NAME_MAX.. its only theoretical
  as most of the devices path are well below 4k IMO. So its a good
  trade off than other solutions like.. (just for the understanding),

    - Update add device code as well to use btrfs_ioctl_vol_args_v2
      Which means we need to introduce BTRFS_IOC_ADD_DEV_V2 (system
      PATH_MAX is 4096).

    OR

    - Create new btrfs_ioctl_vol_args_v3 with name[BTRFS_PATH_NAME_MAX+1]
    (instead of name[BTRFS_SUBVOL_NAME_MAX+1]) and BTRFS_IOC_RM_DEV_V2
    will be the only consumer of btrfs_ioctl_vol_args_v3 as of now.


Thanks, Anand

Re: [PATCH v4 12/13] btrfs: introduce device delete by devid

[David Sterba]

On Thu, Feb 18, 2016 at 02:59:26PM +0800, Anand Jain wrote:
> #define BTRFS_PATH_NAME_MAX 4087
> #define BTRFS_SUBVOL_NAME_MAX 4039
> 
>   I am fine with using BTRFS_SUBVOL_NAME_MAX for now. But theoretical
>   anomaly is that add-device code path will use BTRFS_PATH_NAME_MAX and
>   delete device will use BTRFS_SUBVOL_NAME_MAX.. its only theoretical
>   as most of the devices path are well below 4k IMO. So its a good
>   trade off than other solutions like.. (just for the understanding),
> 
>     - Update add device code as well to use btrfs_ioctl_vol_args_v2
>       Which means we need to introduce BTRFS_IOC_ADD_DEV_V2 (system
>       PATH_MAX is 4096).
> 
>     OR
> 
>     - Create new btrfs_ioctl_vol_args_v3 with name[BTRFS_PATH_NAME_MAX+1]
>     (instead of name[BTRFS_SUBVOL_NAME_MAX+1]) and BTRFS_IOC_RM_DEV_V2
>     will be the only consumer of btrfs_ioctl_vol_args_v3 as of now.

I'd rather not introduce any new structures or ioctls, the problem seems
to be marginal. As mentioned, paths are way shorter than 4k. A symlink
can be used as a workaround. We can document that.

The use of BTRFS_SUBVOL_NAME_MAX is confusing for devices, we can do

#define BTRFS_DEVICE_PATH_MAX	BTRFS_SUBVOL_NAME_MAX

Re: [PATCH 13/13] btrfs: optimize check for stale device

[David Sterba]

On Sat, Feb 13, 2016 at 10:01:40AM +0800, Anand Jain wrote:
> Optimize check for stale device to only be checked when there is device
> added or changed. If there is no update to the device, there is no need
> to call btrfs_free_stale_device().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

http://thread.gmane.org/gmane.comp.file-systems.btrfs/48909/focus=48976

So why did you include the patch in this series?

I see crashes with btrfs/011 on a non-debugging config

[  641.714363] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
[  641.716057] IP: [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
[  641.717036] PGD 720c1067 PUD 720c2067 PMD 0
[  641.717749] Oops: 0000 [#1] PREEMPT SMP
[  641.718432] Modules linked in: af_packet iscsi_ibft iscsi_boot_sysfs xfs libcrc32c ppdev acpi_cpufreq 8250_fintek parport_pc parport bochs_drm ttm drm_kms_helper drm fb_sys_fops syscopyarea sysfillrect sysimgblt button joydev tpm_tis tpm i2c_piix4 serio_raw pcspkr dm_mod btrfs xor raid6_pq sr_mod cdrom ata_generic ata_piix sym53c8xx e1000 scsi_transport_spi floppy sg
[  641.723163] CPU: 0 PID: 27766 Comm: btrfs Not tainted 4.5.0-rc3-next-20160212-1.g38290f0-vanilla #1
[  641.724420] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
[  641.725723] task: ffff8800742481c0 ti: ffff880071d10000 task.ti: ffff880071d10000
[  641.726954] RIP: 0010:[<ffffffffa0152eb6>]  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
[  641.728404] RSP: 0018:ffff880071d13ce8  EFLAGS: 00010202
[  641.729413] RAX: ffff88007231e800 RBX: ffff88007231e800 RCX: 0000000000000000
[  641.730610] RDX: ffffffffa0195638 RSI: ffffffffa017c5a8 RDI: ffff88007231ea80
[  641.731832] RBP: ffff880071d13d18 R08: 0000000000000000 R09: ffff88007204ea00
[  641.733085] R10: 0000000000000008 R11: 0000000000000000 R12: 0000000000000000
[  641.734307] R13: 0000000000000001 R14: ffff88007231e9f8 R15: 000000000000003f
[  641.735544] FS:  00007f03ed36d8c0(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
[  641.736883] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  641.738022] CR2: 0000000000000068 CR3: 00000000720c0000 CR4: 00000000000006f0
[  641.739325] Stack:
[  641.740156]  ffff8800724d4000 ffff8800724d4000 0000000000000000 ffff8800722ef000
[  641.741735]  0000000000000000 ffff8800724d4fc8 ffff880071d13d98 ffffffffa01566fd
[  641.743163]  ffff88007b127000 0000001900000000 ffff8800724d4ce8 0000000000000000
[  641.744599] Call Trace:
[  641.745553]  [<ffffffffa01566fd>] btrfs_scrub_dev+0x13d/0x510 [btrfs]
[  641.746894]  [<ffffffffa0169ca9>] btrfs_dev_replace_start+0x279/0x3f0 [btrfs]
[  641.748282]  [<ffffffffa0132839>] btrfs_ioctl+0x1869/0x2070 [btrfs]
[  641.749587]  [<ffffffff8106d553>] ? pte_alloc_one+0x33/0x40
[  641.750850]  [<ffffffff81222516>] do_vfs_ioctl+0x96/0x590
[  641.752128]  [<ffffffff810682d1>] ? __do_page_fault+0x181/0x450
[  641.753432]  [<ffffffff81222a89>] SyS_ioctl+0x79/0x90
[  641.754663]  [<ffffffff816d4336>] entry_SYSCALL_64_fastpath+0x1e/0xa8
[  641.756037] Code: 00 48 c7 c2 38 56 19 a0 48 c7 c6 a8 c5 17 a0 e8 21 39 f7 e0 45 85 ed 48 c7 83 68 02 00 00 00 00 00 00 48 89 d8 0f 84 03 ff ff ff <49> 83 7c 24 68 00 74 40 c7 83 78 02 00 00 20 00 00 00 4c 89 a3
[  641.760392] RIP  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
[  641.761970]  RSP <ffff880071d13ce8>
[  641.763190] CR2: 0000000000000068
[  641.767218] ---[ end trace f46d4e6a90bda310 ]---

the dereference happens at offset 0x68 which matches bdev in
btrfs_device, so this patch is my best guess at the moment. I'm not able
to reproduce it directly so I need to wait for a rebuild and repeat.

Re: [PATCH 13/13] btrfs: optimize check for stale device

[Anand Jain]

On 02/18/2016 11:13 PM, David Sterba wrote:
> On Sat, Feb 13, 2016 at 10:01:40AM +0800, Anand Jain wrote:
>> Optimize check for stale device to only be checked when there is device
>> added or changed. If there is no update to the device, there is no need
>> to call btrfs_free_stale_device().
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>
> http://thread.gmane.org/gmane.comp.file-systems.btrfs/48909/focus=48976
>
> So why did you include the patch in this series?

  Non technical. Getting miscellaneous device management related patches
  through.


> I see crashes with btrfs/011 on a non-debugging config
>
> [  641.714363] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
> [  641.716057] IP: [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
> [  641.717036] PGD 720c1067 PUD 720c2067 PMD 0
> [  641.717749] Oops: 0000 [#1] PREEMPT SMP
::
> [  641.723163] CPU: 0 PID: 27766 Comm: btrfs Not tainted 4.5.0-rc3-next-20160212-1.g38290f0-vanilla #1
> [  641.724420] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
> [  641.725723] task: ffff8800742481c0 ti: ffff880071d10000 task.ti: ffff880071d10000
> [  641.726954] RIP: 0010:[<ffffffffa0152eb6>]  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
> [  641.728404] RSP: 0018:ffff880071d13ce8  EFLAGS: 00010202
> [  641.729413] RAX: ffff88007231e800 RBX: ffff88007231e800 RCX: 0000000000000000
> [  641.730610] RDX: ffffffffa0195638 RSI: ffffffffa017c5a8 RDI: ffff88007231ea80
> [  641.731832] RBP: ffff880071d13d18 R08: 0000000000000000 R09: ffff88007204ea00
> [  641.733085] R10: 0000000000000008 R11: 0000000000000000 R12: 0000000000000000
> [  641.734307] R13: 0000000000000001 R14: ffff88007231e9f8 R15: 000000000000003f
> [  641.735544] FS:  00007f03ed36d8c0(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
> [  641.736883] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  641.738022] CR2: 0000000000000068 CR3: 00000000720c0000 CR4: 00000000000006f0
> [  641.739325] Stack:
> [  641.740156]  ffff8800724d4000 ffff8800724d4000 0000000000000000 ffff8800722ef000
> [  641.741735]  0000000000000000 ffff8800724d4fc8 ffff880071d13d98 ffffffffa01566fd
> [  641.743163]  ffff88007b127000 0000001900000000 ffff8800724d4ce8 0000000000000000
> [  641.744599] Call Trace:
> [  641.745553]  [<ffffffffa01566fd>] btrfs_scrub_dev+0x13d/0x510 [btrfs]
> [  641.746894]  [<ffffffffa0169ca9>] btrfs_dev_replace_start+0x279/0x3f0 [btrfs]
> [  641.748282]  [<ffffffffa0132839>] btrfs_ioctl+0x1869/0x2070 [btrfs]
> [  641.749587]  [<ffffffff8106d553>] ? pte_alloc_one+0x33/0x40
> [  641.750850]  [<ffffffff81222516>] do_vfs_ioctl+0x96/0x590
> [  641.752128]  [<ffffffff810682d1>] ? __do_page_fault+0x181/0x450
> [  641.753432]  [<ffffffff81222a89>] SyS_ioctl+0x79/0x90
> [  641.754663]  [<ffffffff816d4336>] entry_SYSCALL_64_fastpath+0x1e/0xa8
> [  641.756037] Code: 00 48 c7 c2 38 56 19 a0 48 c7 c6 a8 c5 17 a0 e8 21 39 f7 e0 45 85 ed 48 c7 83 68 02 00 00 00 00 00 00 48 89 d8 0f 84 03 ff ff ff <49> 83 7c 24 68 00 74 40 c7 83 78 02 00 00 20 00 00 00 4c 89 a3
> [  641.760392] RIP  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
> [  641.761970]  RSP <ffff880071d13ce8>
> [  641.763190] CR2: 0000000000000068
> [  641.767218] ---[ end trace f46d4e6a90bda310 ]---
>
> the dereference happens at offset 0x68 which matches bdev in
> btrfs_device, so this patch is my best guess at the moment. I'm not able
> to reproduce it directly so I need to wait for a rebuild and repeat.


   Looks like dev was fine when find_device was called, but
   later it was null when ->bdev was accessed.

   I couldn't reproduce here. There are 10 workouts within btrfs/011
   any idea workout caused this? As of now I am guessing..

   workout "-m dup -d single" 1 cancel quick

   digging more.

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

Re: [PATCH 13/13] btrfs: optimize check for stale device

[Anand Jain]

Dave,

>> I see crashes with btrfs/011 on a non-debugging config

  Could you share your xfstests config file ? especially the
  devices config part.

Thanks, Anand

Re: [PATCH 13/13] btrfs: optimize check for stale device

[Anand Jain]

Dave,

> I see crashes with btrfs/011 on a non-debugging config
>
> [  641.714363] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
> [  641.716057] IP: [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
::
> [  641.744599] Call Trace:
> [  641.745553]  [<ffffffffa01566fd>] btrfs_scrub_dev+0x13d/0x510 [btrfs]
> [  641.746894]  [<ffffffffa0169ca9>] btrfs_dev_replace_start+0x279/0x3f0 [btrfs]
> [  641.748282]  [<ffffffffa0132839>] btrfs_ioctl+0x1869/0x2070 [btrfs]
> [  641.749587]  [<ffffffff8106d553>] ? pte_alloc_one+0x33/0x40
> [  641.750850]  [<ffffffff81222516>] do_vfs_ioctl+0x96/0x590
> [  641.752128]  [<ffffffff810682d1>] ? __do_page_fault+0x181/0x450
> [  641.753432]  [<ffffffff81222a89>] SyS_ioctl+0x79/0x90
> [  641.754663]  [<ffffffff816d4336>] entry_SYSCALL_64_fastpath+0x1e/0xa8
> [  641.756037] Code: 00 48 c7 c2 38 56 19 a0 48 c7 c6 a8 c5 17 a0 e8 21 39 f7 e0 45 85 ed 48 c7 83 68 02 00 00 00 00 00 00 48 89 d8 0f 84 03 ff ff ff <49> 83 7c 24 68 00 74 40 c7 83 78 02 00 00 20 00 00 00 4c 89 a3
> [  641.760392] RIP  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
> [  641.761970]  RSP <ffff880071d13ce8>
> [  641.763190] CR2: 0000000000000068
> [  641.767218] ---[ end trace f46d4e6a90bda310 ]---
>
> the dereference happens at offset 0x68 which matches bdev in
> btrfs_device, so this patch is my best guess at the moment. I'm not able
> to reproduce it directly so I need to wait for a rebuild and repeat.


  As of now,
  There is nothing that tells me the above crash is due to this patch.

  By any chance were you running multiple instance of fstests ? If that's
  possible ?

Thanks, Anand

Re: [PATCH 13/13] btrfs: optimize check for stale device

[David Sterba]

On Wed, Mar 09, 2016 at 05:54:47PM +0800, Anand Jain wrote:
> > the dereference happens at offset 0x68 which matches bdev in
> > btrfs_device, so this patch is my best guess at the moment. I'm not able
> > to reproduce it directly so I need to wait for a rebuild and repeat.
> 
> 
>   As of now,
>   There is nothing that tells me the above crash is due to this patch.

That was my suspicion but so far does not point to this patch.

>   By any chance were you running multiple instance of fstests ? If that's
>   possible ?

No, just a single instance.

Re: [PATCH 13/13] btrfs: optimize check for stale device

[David Sterba]

On Fri, Feb 19, 2016 at 03:10:16PM +0800, Anand Jain wrote:
> > I see crashes with btrfs/011 on a non-debugging config
> >
> > [  641.714363] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
> > [  641.716057] IP: [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
> > [  641.717036] PGD 720c1067 PUD 720c2067 PMD 0
> > [  641.717749] Oops: 0000 [#1] PREEMPT SMP
> ::
> > [  641.723163] CPU: 0 PID: 27766 Comm: btrfs Not tainted 4.5.0-rc3-next-20160212-1.g38290f0-vanilla #1
> > [  641.724420] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
> > [  641.725723] task: ffff8800742481c0 ti: ffff880071d10000 task.ti: ffff880071d10000
> > [  641.726954] RIP: 0010:[<ffffffffa0152eb6>]  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
> > [  641.728404] RSP: 0018:ffff880071d13ce8  EFLAGS: 00010202
> > [  641.729413] RAX: ffff88007231e800 RBX: ffff88007231e800 RCX: 0000000000000000
> > [  641.730610] RDX: ffffffffa0195638 RSI: ffffffffa017c5a8 RDI: ffff88007231ea80
> > [  641.731832] RBP: ffff880071d13d18 R08: 0000000000000000 R09: ffff88007204ea00
> > [  641.733085] R10: 0000000000000008 R11: 0000000000000000 R12: 0000000000000000
> > [  641.734307] R13: 0000000000000001 R14: ffff88007231e9f8 R15: 000000000000003f
> > [  641.735544] FS:  00007f03ed36d8c0(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
> > [  641.736883] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [  641.738022] CR2: 0000000000000068 CR3: 00000000720c0000 CR4: 00000000000006f0
> > [  641.739325] Stack:
> > [  641.740156]  ffff8800724d4000 ffff8800724d4000 0000000000000000 ffff8800722ef000
> > [  641.741735]  0000000000000000 ffff8800724d4fc8 ffff880071d13d98 ffffffffa01566fd
> > [  641.743163]  ffff88007b127000 0000001900000000 ffff8800724d4ce8 0000000000000000
> > [  641.744599] Call Trace:
> > [  641.745553]  [<ffffffffa01566fd>] btrfs_scrub_dev+0x13d/0x510 [btrfs]
> > [  641.746894]  [<ffffffffa0169ca9>] btrfs_dev_replace_start+0x279/0x3f0 [btrfs]
> > [  641.748282]  [<ffffffffa0132839>] btrfs_ioctl+0x1869/0x2070 [btrfs]
> > [  641.749587]  [<ffffffff8106d553>] ? pte_alloc_one+0x33/0x40
> > [  641.750850]  [<ffffffff81222516>] do_vfs_ioctl+0x96/0x590
> > [  641.752128]  [<ffffffff810682d1>] ? __do_page_fault+0x181/0x450
> > [  641.753432]  [<ffffffff81222a89>] SyS_ioctl+0x79/0x90
> > [  641.754663]  [<ffffffff816d4336>] entry_SYSCALL_64_fastpath+0x1e/0xa8
> > [  641.756037] Code: 00 48 c7 c2 38 56 19 a0 48 c7 c6 a8 c5 17 a0 e8 21 39 f7 e0 45 85 ed 48 c7 83 68 02 00 00 00 00 00 00 48 89 d8 0f 84 03 ff ff ff <49> 83 7c 24 68 00 74 40 c7 83 78 02 00 00 20 00 00 00 4c 89 a3
> > [  641.760392] RIP  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
> > [  641.761970]  RSP <ffff880071d13ce8>
> > [  641.763190] CR2: 0000000000000068
> > [  641.767218] ---[ end trace f46d4e6a90bda310 ]---
> >
> > the dereference happens at offset 0x68 which matches bdev in
> > btrfs_device, so this patch is my best guess at the moment. I'm not able
> > to reproduce it directly so I need to wait for a rebuild and repeat.
> 
> 
>    Looks like dev was fine when find_device was called, but
>    later it was null when ->bdev was accessed.
> 
>    I couldn't reproduce here. There are 10 workouts within btrfs/011
>    any idea workout caused this? As of now I am guessing..
> 
>    workout "-m dup -d single" 1 cancel quick
> 
>    digging more.

I was not able reproduce the crash since. All ok on a physical machine,
in a virtual machine in kvm the test runs for a long time and then
freezes (serial console, ssh). The kvm process eats 100% cpu, not
possible to debug it directly. The branch stays in my for-next and is
on the way to 4.7, we'll see if we can reproduce it.

Re: [PATCH 13/13] btrfs: optimize check for stale device

[Anand Jain]

On 03/22/2016 08:21 PM, David Sterba wrote:
> On Fri, Feb 19, 2016 at 03:10:16PM +0800, Anand Jain wrote:
>>> I see crashes with btrfs/011 on a non-debugging config
>>>
>>> [  641.714363] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
>>> [  641.716057] IP: [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
>>> [  641.717036] PGD 720c1067 PUD 720c2067 PMD 0
>>> [  641.717749] Oops: 0000 [#1] PREEMPT SMP
>> ::
>>> [  641.723163] CPU: 0 PID: 27766 Comm: btrfs Not tainted 4.5.0-rc3-next-20160212-1.g38290f0-vanilla #1
>>> [  641.724420] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
>>> [  641.725723] task: ffff8800742481c0 ti: ffff880071d10000 task.ti: ffff880071d10000
>>> [  641.726954] RIP: 0010:[<ffffffffa0152eb6>]  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
>>> [  641.728404] RSP: 0018:ffff880071d13ce8  EFLAGS: 00010202
>>> [  641.729413] RAX: ffff88007231e800 RBX: ffff88007231e800 RCX: 0000000000000000
>>> [  641.730610] RDX: ffffffffa0195638 RSI: ffffffffa017c5a8 RDI: ffff88007231ea80
>>> [  641.731832] RBP: ffff880071d13d18 R08: 0000000000000000 R09: ffff88007204ea00
>>> [  641.733085] R10: 0000000000000008 R11: 0000000000000000 R12: 0000000000000000
>>> [  641.734307] R13: 0000000000000001 R14: ffff88007231e9f8 R15: 000000000000003f
>>> [  641.735544] FS:  00007f03ed36d8c0(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
>>> [  641.736883] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [  641.738022] CR2: 0000000000000068 CR3: 00000000720c0000 CR4: 00000000000006f0
>>> [  641.739325] Stack:
>>> [  641.740156]  ffff8800724d4000 ffff8800724d4000 0000000000000000 ffff8800722ef000
>>> [  641.741735]  0000000000000000 ffff8800724d4fc8 ffff880071d13d98 ffffffffa01566fd
>>> [  641.743163]  ffff88007b127000 0000001900000000 ffff8800724d4ce8 0000000000000000
>>> [  641.744599] Call Trace:
>>> [  641.745553]  [<ffffffffa01566fd>] btrfs_scrub_dev+0x13d/0x510 [btrfs]
>>> [  641.746894]  [<ffffffffa0169ca9>] btrfs_dev_replace_start+0x279/0x3f0 [btrfs]
>>> [  641.748282]  [<ffffffffa0132839>] btrfs_ioctl+0x1869/0x2070 [btrfs]
>>> [  641.749587]  [<ffffffff8106d553>] ? pte_alloc_one+0x33/0x40
>>> [  641.750850]  [<ffffffff81222516>] do_vfs_ioctl+0x96/0x590
>>> [  641.752128]  [<ffffffff810682d1>] ? __do_page_fault+0x181/0x450
>>> [  641.753432]  [<ffffffff81222a89>] SyS_ioctl+0x79/0x90
>>> [  641.754663]  [<ffffffff816d4336>] entry_SYSCALL_64_fastpath+0x1e/0xa8
>>> [  641.756037] Code: 00 48 c7 c2 38 56 19 a0 48 c7 c6 a8 c5 17 a0 e8 21 39 f7 e0 45 85 ed 48 c7 83 68 02 00 00 00 00 00 00 48 89 d8 0f 84 03 ff ff ff <49> 83 7c 24 68 00 74 40 c7 83 78 02 00 00 20 00 00 00 4c 89 a3
>>> [  641.760392] RIP  [<ffffffffa0152eb6>] scrub_setup_ctx.isra.19+0x1f6/0x260 [btrfs]
>>> [  641.761970]  RSP <ffff880071d13ce8>
>>> [  641.763190] CR2: 0000000000000068
>>> [  641.767218] ---[ end trace f46d4e6a90bda310 ]---
>>>
>>> the dereference happens at offset 0x68 which matches bdev in
>>> btrfs_device, so this patch is my best guess at the moment. I'm not able
>>> to reproduce it directly so I need to wait for a rebuild and repeat.
>>
>>
>>     Looks like dev was fine when find_device was called, but
>>     later it was null when ->bdev was accessed.
>>
>>     I couldn't reproduce here. There are 10 workouts within btrfs/011
>>     any idea workout caused this? As of now I am guessing..
>>
>>     workout "-m dup -d single" 1 cancel quick
>>
>>     digging more.
>
> I was not able reproduce the crash since. All ok on a physical machine,
> in a virtual machine in kvm the test runs for a long time and then
> freezes (serial console, ssh). The kvm process eats 100% cpu, not
> possible to debug it directly. The branch stays in my for-next and is
> on the way to 4.7, we'll see if we can reproduce it.

Agreed. Thanks Dave.