Re: [PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices
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
---
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(_info->dev_replace);
+ if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
+ WARN_ON(num_devices < 1);
+ num_devices--;
+ }
+ btrfs_dev_replace_read_unlock(_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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices
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
> >> ---
> >> 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(_info->dev_replace);
> >> + if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
> >> + WARN_ON(num_devices < 1);
> >> + num_devices--;
> >> + }
> >> + btrfs_dev_replace_read_unlock(_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
Re: [PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices
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
---
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(_info->dev_replace);
+ if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
+ WARN_ON(num_devices < 1);
+ num_devices--;
+ }
+ btrfs_dev_replace_read_unlock(_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(_mutex);
- num_devices = fs_devices->num_devices;
- btrfs_dev_replace_read_lock(_info->dev_replace);
- if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
- WARN_ON(num_devices < 1);
- num_devices--;
- }
- btrfs_dev_replace_read_unlock(_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(_info->dev_replace);
- if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
- WARN_ON(num_devices < 1);
- num_devices--;
- }
- btrfs_dev_replace_read_unlock(_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 majord...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices
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
---
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(_info->dev_replace);
+ if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
+ WARN_ON(num_devices < 1);
+ num_devices--;
+ }
+ btrfs_dev_replace_read_unlock(_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(_mutex);
- num_devices = fs_devices->num_devices;
- btrfs_dev_replace_read_lock(_info->dev_replace);
- if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
- WARN_ON(num_devices < 1);
- num_devices--;
- }
- btrfs_dev_replace_read_unlock(_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(_info->dev_replace);
- if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
- WARN_ON(num_devices < 1);
- num_devices--;
- }
- btrfs_dev_replace_read_unlock(_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 majord...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices
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
> ---
> 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(_info->dev_replace);
> + if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
> + WARN_ON(num_devices < 1);
> + num_devices--;
> + }
> + btrfs_dev_replace_read_unlock(_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(_mutex);
>
> - num_devices = fs_devices->num_devices;
> - btrfs_dev_replace_read_lock(_info->dev_replace);
> - if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
> - WARN_ON(num_devices < 1);
> - num_devices--;
> - }
> - btrfs_dev_replace_read_unlock(_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(_info->dev_replace);
> - if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
> - WARN_ON(num_devices < 1);
> - num_devices--;
> - }
> - btrfs_dev_replace_read_unlock(_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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] btrfs: add helper btrfs_num_devices() to deduce num_devices
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
---
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(_info->dev_replace);
+ if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
+ WARN_ON(num_devices < 1);
+ num_devices--;
+ }
+ btrfs_dev_replace_read_unlock(_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(_mutex);
- num_devices = fs_devices->num_devices;
- btrfs_dev_replace_read_lock(_info->dev_replace);
- if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
- WARN_ON(num_devices < 1);
- num_devices--;
- }
- btrfs_dev_replace_read_unlock(_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(_info->dev_replace);
- if (btrfs_dev_replace_is_ongoing(_info->dev_replace)) {
- WARN_ON(num_devices < 1);
- num_devices--;
- }
- btrfs_dev_replace_read_unlock(_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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
