On Thu, Aug 02, 2018 at 09:07:00PM +0800, Anand Jain wrote:

quoted

quoted

quoted

-	num_devices = fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);

How about lifting the BUG_ON from btrfs_num_devices into a check in this
function, so if num_devices < 1 then we just exit with -EINVAL or some
such. We should be aiming at eliminating BUG_ONs.

Right, in both cases it's possible to return with an error instead of
the BUG_ON.

Actually we should just remove it as its a logical bug if num_devices < 
1, so long we didn't hit this bug which means its stable OR keep BUG_ON 
it until we add RAID-N. ?

I think the BUG_ON is redundant, all the sanity checks at mount time or
ioctl remove/replace make sure that there are enough devices to perform
the action. Still, it can be an assert, such things do not hurt and may
be catch other errors someday.