| Message ID | 20180716145812.20836-7-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.jain@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, the locking would need to cover the whole range where we want the num_devices to remain unchanged by other operatons. > + > + return num_devices; > +} > + > int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, > u64 devid) > { > @@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, > > mutex_lock(&uuid_mutex); > > - 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)) { > - WARN_ON(num_devices < 1); > - num_devices--; > - } > - btrfs_dev_replace_read_unlock(&fs_info->dev_replace); > + num_devices = btrfs_num_devices(fs_info); > > ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1); > if (ret) > @@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, > } > } > > - 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); > + num_devices = btrfs_num_devices(fs_info); > + > allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP; > if (num_devices > 1) > allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1); > -- > 2.7.0 > > -- > 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 -- 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
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. You are right. Will fix it in a separate patch. As, here in this patch my intention was to de-duplicate a section of the code. >> >> Signed-off-by: Anand Jain <anand.jain@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, That's in the original code I did not add it. > besides that btrfs_dev_replace_is_ongoing is > always going to be false here, Right. Will fix in a separate patch. > the locking would need to cover the whole > range where we want the num_devices to remain unchanged by other > operatons. Will review this part. Thanks, Anand >> + >> + return num_devices; >> +} >> + >> int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, >> u64 devid) >> { >> @@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, >> >> mutex_lock(&uuid_mutex); >> >> - 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)) { >> - WARN_ON(num_devices < 1); >> - num_devices--; >> - } >> - btrfs_dev_replace_read_unlock(&fs_info->dev_replace); >> + num_devices = btrfs_num_devices(fs_info); >> >> ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1); >> if (ret) >> @@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, >> } >> } >> >> - 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); >> + num_devices = btrfs_num_devices(fs_info); >> + >> allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP; >> if (num_devices > 1) >> allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1); >> -- >> 2.7.0 >> >> -- >> 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 > -- > 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 > -- 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
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.jain@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. For now a consolidation as in this patch is better. Thanks, Anand > the locking would need to cover the whole > range where we want the num_devices to remain unchanged by other > operatons. > >> + >> + return num_devices; >> +} >> + >> int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, >> u64 devid) >> { >> @@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, >> >> mutex_lock(&uuid_mutex); >> >> - 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)) { >> - WARN_ON(num_devices < 1); >> - num_devices--; >> - } >> - btrfs_dev_replace_read_unlock(&fs_info->dev_replace); >> + num_devices = btrfs_num_devices(fs_info); >> >> ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1); >> if (ret) >> @@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, >> } >> } >> >> - 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); >> + num_devices = btrfs_num_devices(fs_info); >> + >> allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP; >> if (num_devices > 1) >> allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1); >> -- >> 2.7.0 >> >> -- >> 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 > -- > 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 > -- 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
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.jain@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/23/2018 09:57 PM, David Sterba wrote: > 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.jain@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. Regarding function names its tough to convince in a short form. As of now I have the following choices, if there is anything better I don't mind using it though. btrfs_num_devices_minus_replace() btrfs_get_num_devices_raw() btrfs_num_devices_raw() 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 > -- 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
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); + + return num_devices; +} + int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, u64 devid) { @@ -1857,13 +1872,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, mutex_lock(&uuid_mutex); - 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)) { - WARN_ON(num_devices < 1); - num_devices--; - } - btrfs_dev_replace_read_unlock(&fs_info->dev_replace); + num_devices = btrfs_num_devices(fs_info); ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1); if (ret) @@ -3723,13 +3732,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, } } - 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); + num_devices = btrfs_num_devices(fs_info); + allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP; if (num_devices > 1) allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
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. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v2: add comments. Thanks Nikolay. fs/btrfs/volumes.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)