On Fri, Jul 20, 2018 at 07:18:54PM +0800, Anand Jain wrote:
>
>
> On 07/19/2018 07:53 PM, David Sterba wrote:
> > On Mon, Jul 16, 2018 at 10:58:11PM +0800, Anand Jain wrote:
> >> When the replace is running the fs_devices::num_devices also includes
> >> the replace device, however in some operations like device delete and
> >> balance it needs the actual num_devices without the repalce devices, so
> >> now the function btrfs_num_devices() just provides that.
> >
> > We can't run any two from device delete, device replace or balance at
> > the same time.
> >
> >>
> >> Signed-off-by: Anand Jain <anand.j...@oracle.com>
> >> ---
> >> v2: add comments. Thanks Nikolay.
> >>
> >> fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
> >> 1 file changed, 18 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index 0f4c512aa6b4..1c0b56374992 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -1844,6 +1844,21 @@ void btrfs_assign_next_active_device(struct
> >> btrfs_fs_info *fs_info,
> >> fs_info->fs_devices->latest_bdev = next_device->bdev;
> >> }
> >>
> >> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any
> >> */
> >> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> >> +{
> >> + u64 num_devices = fs_info->fs_devices->num_devices;
> >> +
> >> + btrfs_dev_replace_read_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_read_unlock(&fs_info->dev_replace);
>
>
>
> > This does not make sense, besides that > btrfs_dev_replace_is_ongoing is
> > always going to be false here,
>
> No. There is a way how balance and replace could co-exists.
> (theoretically, I didn't experiment it yet)
> . Start balance and pause it
> . Now start the replace
> . power-fail
> . The open_ctree() first starts the balance so it must check
> for the replace device otherwise our num_devices calculation will
> be wrong. IMO its not a good idea to remove the replace check here.
I see, the paused states can lead to balance that sees device replace
ongoing as true. This would be good to add to the function comment as
it's not quite obvious why the helper is needed.
> For now a consolidation as in this patch is better.
Yeah, for this context it would be good. The function name could be
more descriptive what devices it actually counts.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
