On 08/07/2018 11:02 PM, David Sterba wrote:
On Thu, Aug 02, 2018 at 09:07:00PM +0800, Anand Jain wrote:
- 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.
Assert makes sense to me. Patch [1] shall be dropped
[1]
[PATCH] btrfs: handle the BUG_ON in btrfs_num_devices()
Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
