Re: [PATCH v2 0/3] Misc volume patch set part2
On 08/15/2018 08:20 PM, David Sterba wrote: On Fri, Aug 10, 2018 at 01:53:18PM +0800, Anand Jain wrote: The patch set (2/3 and 3/3) adds helper function to deduce the num_device (which is the number of the devices when device is mounted, this excludes the seed device however includes the replacing target). We need to know the num_device that actually belongs to the FSID without the replacing target (if it exists) when we are balancing and or when we are trying to device delete. In both of these cases, it would be wrong to consider replacing target when reallocating the blockgroups. This patch as such does not change any of the logic which is already in the original code, but instead it would bring such a logic at two locations into a single place/function. But to do that as these two location uses either WARN_ON or BUG_ON to perform the safety catch so the first patch does the cleanup and uses ASSERT at both of these places. And now since these two sections of codes are identical, the 2nd patch replaces them into a helper function. Also though the check for num_device > 0 should actually be num_device > 1 OR instead, the check of num_device > 0 should have been after the num_device--, I updated the patch following that logic now. Right thanks. since we have had already too many comments and added confusions, I shall leave that part to be fixed at sometime later. So that the cleanup here is a really a cleanup instead of chaning the logic in a rough scale. Last but not least, this patch has gone through a lot of comments on either to use BUG or return -EINVAL for accidental fall of num_device below 1, which I wasn't expecting so much of the concerns about the use of BUG_ON/-EINVAL as its trivial because fundamentally a FS can't be in the mounted state when num_device < 1. Anand Jain (3): btrfs: drop uuid_mutex in btrfs_free_extra_devids() btrfs: assert for num_devices below 0 btrfs: add helper btrfs_num_devices() to deduce num_devices Patches 2 and 3 are applied, with some adjustments. Adjustments at [1] are correct. [1] gitlab.com/kdave/btrfs-devel.git misc-next Thanks, Anand. The first patch needs another round of review as there were many changes regarding the device locking and uuid_mutex. I need a break from this code as the wounds haven't healed yet, but will add the patch to a topic branch to next so I don't forget about it.
Re: [PATCH v2 0/3] Misc volume patch set part2
On Fri, Aug 10, 2018 at 01:53:18PM +0800, Anand Jain wrote: > The patch set (2/3 and 3/3) adds helper function to deduce the num_device > (which is the number of the devices when device is mounted, this excludes > the seed device however includes the replacing target). We need to know > the num_device that actually belongs to the FSID without the replacing > target (if it exists) when we are balancing and or when we are trying to > device delete. In both of these cases, it would be wrong to consider > replacing target when reallocating the blockgroups. > > This patch as such does not change any of the logic which is already in > the original code, but instead it would bring such a logic at two > locations into a single place/function. But to do that as these two > location uses either WARN_ON or BUG_ON to perform the safety catch so > the first patch does the cleanup and uses ASSERT at both of these > places. And now since these two sections of codes are identical, the 2nd > patch replaces them into a helper function. > > Also though the check for num_device > 0 should actually be num_device > > 1 OR instead, the check of num_device > 0 should have been after the > num_device--, I updated the patch following that logic now. > since we have had already too many comments and added > confusions, I shall leave that part to be fixed at sometime later. So > that the cleanup here is a really a cleanup instead of chaning the > logic in a rough scale. > > Last but not least, this patch has gone through a lot of comments on > either to use BUG or return -EINVAL for accidental fall of num_device > below 1, which I wasn't expecting so much of the concerns about > the use of BUG_ON/-EINVAL as its trivial because fundamentally a FS > can't be in the mounted state when num_device < 1. > > > Anand Jain (3): > btrfs: drop uuid_mutex in btrfs_free_extra_devids() > btrfs: assert for num_devices below 0 > btrfs: add helper btrfs_num_devices() to deduce num_devices Patches 2 and 3 are applied, with some adjustments. The first patch needs another round of review as there were many changes regarding the device locking and uuid_mutex. I need a break from this code as the wounds haven't healed yet, but will add the patch to a topic branch to next so I don't forget about it.
[PATCH v2 0/3] Misc volume patch set part2
The patch set (2/3 and 3/3) adds helper function to deduce the num_device (which is the number of the devices when device is mounted, this excludes the seed device however includes the replacing target). We need to know the num_device that actually belongs to the FSID without the replacing target (if it exists) when we are balancing and or when we are trying to device delete. In both of these cases, it would be wrong to consider replacing target when reallocating the blockgroups. This patch as such does not change any of the logic which is already in the original code, but instead it would bring such a logic at two locations into a single place/function. But to do that as these two location uses either WARN_ON or BUG_ON to perform the safety catch so the first patch does the cleanup and uses ASSERT at both of these places. And now since these two sections of codes are identical, the 2nd patch replaces them into a helper function. Also though the check for num_device > 0 should actually be num_device > 1 OR instead, the check of num_device > 0 should have been after the num_device--, since we have had already too many comments and added confusions, I shall leave that part to be fixed at sometime later. So that the cleanup here is a really a cleanup instead of chaning the logic in a rough scale. Last but not least, this patch has gone through a lot of comments on either to use BUG or return -EINVAL for accidental fall of num_device below 1, which I wasn't expecting so much of the concerns about the use of BUG_ON/-EINVAL as its trivial because fundamentally a FS can't be in the mounted state when num_device < 1. Anand Jain (3): btrfs: drop uuid_mutex in btrfs_free_extra_devids() btrfs: assert for num_devices below 0 btrfs: add helper btrfs_num_devices() to deduce num_devices fs/btrfs/volumes.c | 32 ++-- 1 file changed, 18 insertions(+), 14 deletions(-) -- 2.7.0
