* [PATCH 1/4] btrfs: dev delete should remove sysfs entry
2014-05-26 9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
@ 2014-05-26 9:30 ` Anand Jain
2014-05-29 13:04 ` David Sterba
2014-05-26 9:30 ` [PATCH 2/4] btrfs: dev add should add its " Anand Jain
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26 9:30 UTC (permalink / raw)
To: linux-btrfs; +Cc: jeffm, dsterba, clm
From: Anand Jain <Anand.Jain@oracle.com>
when we delete the device from the mounted btrfs,
we would need its corresponding sysfs enty to
be removed as well.
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
fs/btrfs/sysfs.c | 20 ++++++++++++++++++++
fs/btrfs/sysfs.h | 2 ++
fs/btrfs/volumes.c | 4 ++++
3 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 522d023..4fe3f0b 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -572,6 +572,26 @@ static void init_feature_attrs(void)
}
}
+int rm_device_membership(struct btrfs_fs_info *fs_info,
+ struct btrfs_device *one_device)
+{
+ struct hd_struct *disk;
+ struct kobject *disk_kobj;
+
+ if (!fs_info->device_dir_kobj)
+ return -EINVAL;
+
+ if (one_device) {
+ disk = one_device->bdev->bd_part;
+ disk_kobj = &part_to_dev(disk)->kobj;
+
+ sysfs_remove_link(fs_info->device_dir_kobj,
+ disk_kobj->name);
+ }
+
+ return 0;
+}
+
static int add_device_membership(struct btrfs_fs_info *fs_info)
{
int error = 0;
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index 9ab5763..eaed810 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -66,4 +66,6 @@ char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
extern const char * const btrfs_feature_set_names[3];
extern struct kobj_type space_info_ktype;
extern struct kobj_type btrfs_raid_ktype;
+int rm_device_membership(struct btrfs_fs_info *fs_info,
+ struct btrfs_device *one_device);
#endif /* _BTRFS_SYSFS_H_ */
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 15a8934..3ceb28c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -40,6 +40,7 @@
#include "rcu-string.h"
#include "math.h"
#include "dev-replace.h"
+#include "sysfs.h"
static int init_first_rw_device(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
@@ -1651,6 +1652,9 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
if (device->bdev)
device->fs_devices->open_devices--;
+ /* remove sysfs entry */
+ rm_device_membership(root->fs_info, device);
+
call_rcu(&device->rcu, free_device);
num_devices = btrfs_super_num_devices(root->fs_info->super_copy) - 1;
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry
2014-05-26 9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
@ 2014-05-29 13:04 ` David Sterba
2014-05-30 6:10 ` Anand Jain
0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-05-29 13:04 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm
On Mon, May 26, 2014 at 05:30:23PM +0800, Anand Jain wrote:
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -572,6 +572,26 @@ static void init_feature_attrs(void)
> }
> }
>
> +int rm_device_membership(struct btrfs_fs_info *fs_info,
> + struct btrfs_device *one_device)
The name is too generic for a non-static function, so it would be good
to add at least the btrfs_ prefix. Same applies to the change of
'add_device_membership' in the next patch.
As this is only a trivial change, consider this
Reviewed-by: David Sterba <dsterba@suse.cz>
> +{
> + struct hd_struct *disk;
> + struct kobject *disk_kobj;
> +
> + if (!fs_info->device_dir_kobj)
> + return -EINVAL;
> +
> + if (one_device) {
> + disk = one_device->bdev->bd_part;
> + disk_kobj = &part_to_dev(disk)->kobj;
> +
> + sysfs_remove_link(fs_info->device_dir_kobj,
> + disk_kobj->name);
> + }
> +
> + return 0;
> +}
> +
> static int add_device_membership(struct btrfs_fs_info *fs_info)
> {
> int error = 0;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry
2014-05-29 13:04 ` David Sterba
@ 2014-05-30 6:10 ` Anand Jain
2014-05-30 14:10 ` David Sterba
0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-30 6:10 UTC (permalink / raw)
To: dsterba, linux-btrfs, jeffm, clm
Thanks for the review David.
On 29/05/14 21:04, David Sterba wrote:
> On Mon, May 26, 2014 at 05:30:23PM +0800, Anand Jain wrote:
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -572,6 +572,26 @@ static void init_feature_attrs(void)
>> }
>> }
>>
>> +int rm_device_membership(struct btrfs_fs_info *fs_info,
>> + struct btrfs_device *one_device)
>
> The name is too generic for a non-static function, so it would be good
> to add at least the btrfs_ prefix. Same applies to the change of
> 'add_device_membership' in the next patch.
Yes indeed. I wanted it to change as well. But the diff became
unfriendly to read. I shall do that in a separate patch.
> As this is only a trivial change, consider this
> Reviewed-by: David Sterba <dsterba@suse.cz>
>
>> +{
>> + struct hd_struct *disk;
>> + struct kobject *disk_kobj;
>> +
>> + if (!fs_info->device_dir_kobj)
>> + return -EINVAL;
>> +
>> + if (one_device) {
>> + disk = one_device->bdev->bd_part;
>> + disk_kobj = &part_to_dev(disk)->kobj;
>> +
>> + sysfs_remove_link(fs_info->device_dir_kobj,
>> + disk_kobj->name);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int add_device_membership(struct btrfs_fs_info *fs_info)
>> {
>> int error = 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] btrfs: dev delete should remove sysfs entry
2014-05-30 6:10 ` Anand Jain
@ 2014-05-30 14:10 ` David Sterba
0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2014-05-30 14:10 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, jeffm, clm
On Fri, May 30, 2014 at 02:10:28PM +0800, Anand Jain wrote:
> >>@@ -572,6 +572,26 @@ static void init_feature_attrs(void)
> >> }
> >> }
> >>
> >>+int rm_device_membership(struct btrfs_fs_info *fs_info,
> >>+ struct btrfs_device *one_device)
> >
> >The name is too generic for a non-static function, so it would be good
> >to add at least the btrfs_ prefix. Same applies to the change of
> >'add_device_membership' in the next patch.
>
> Yes indeed. I wanted it to change as well. But the diff became
> unfriendly to read. I shall do that in a separate patch.
No problem with doing the rename changes in a separate patch, but here
you introduce a new function, that should get the right name from the
beginning. If this would look inconsitent with the add_ counterpart,
then do the rename of add_ in advance.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/4] btrfs: dev add should add its sysfs entry
2014-05-26 9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
2014-05-26 9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
@ 2014-05-26 9:30 ` Anand Jain
2014-05-29 14:49 ` David Sterba
2014-05-26 9:30 ` [PATCH 3/4] btrfs: dev replace should replace the " Anand Jain
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26 9:30 UTC (permalink / raw)
To: linux-btrfs; +Cc: jeffm, dsterba, clm
From: Anand Jain <Anand.Jain@oracle.com>
we would need the device links to be created,
when device is added.
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
fs/btrfs/sysfs.c | 12 +++++++++---
fs/btrfs/sysfs.h | 2 ++
fs/btrfs/volumes.c | 5 +++++
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 4fe3f0b..0f2ca33 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -592,14 +592,17 @@ int rm_device_membership(struct btrfs_fs_info *fs_info,
return 0;
}
-static int add_device_membership(struct btrfs_fs_info *fs_info)
+int add_device_membership(struct btrfs_fs_info *fs_info,
+ struct btrfs_device *one_device)
{
int error = 0;
struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
struct btrfs_device *dev;
- fs_info->device_dir_kobj = kobject_create_and_add("devices",
+ if (!fs_info->device_dir_kobj)
+ fs_info->device_dir_kobj = kobject_create_and_add("devices",
&fs_info->super_kobj);
+
if (!fs_info->device_dir_kobj)
return -ENOMEM;
@@ -610,6 +613,9 @@ static int add_device_membership(struct btrfs_fs_info *fs_info)
if (!dev->bdev)
continue;
+ if (one_device && one_device != dev)
+ continue;
+
disk = dev->bdev->bd_part;
disk_kobj = &part_to_dev(disk)->kobj;
@@ -653,7 +659,7 @@ int btrfs_sysfs_add_one(struct btrfs_fs_info *fs_info)
if (error)
goto failure;
- error = add_device_membership(fs_info);
+ error = add_device_membership(fs_info, NULL);
if (error)
goto failure;
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index eaed810..2de1314 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -66,6 +66,8 @@ char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
extern const char * const btrfs_feature_set_names[3];
extern struct kobj_type space_info_ktype;
extern struct kobj_type btrfs_raid_ktype;
+int add_device_membership(struct btrfs_fs_info *fs_info,
+ struct btrfs_device *one_device);
int rm_device_membership(struct btrfs_fs_info *fs_info,
struct btrfs_device *one_device);
#endif /* _BTRFS_SYSFS_H_ */
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3ceb28c..5a577ab 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2077,6 +2077,10 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
total_bytes = btrfs_super_num_devices(root->fs_info->super_copy);
btrfs_set_super_num_devices(root->fs_info->super_copy,
total_bytes + 1);
+
+ /* add sysfs device entry */
+ add_device_membership(root->fs_info, device);
+
mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
if (seeding_dev) {
@@ -2137,6 +2141,7 @@ error_trans:
unlock_chunks(root);
btrfs_end_transaction(trans, root);
rcu_string_free(device->name);
+ rm_device_membership(root->fs_info, device);
kfree(device);
error:
blkdev_put(bdev, FMODE_EXCL);
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] btrfs: dev add should add its sysfs entry
2014-05-26 9:30 ` [PATCH 2/4] btrfs: dev add should add its " Anand Jain
@ 2014-05-29 14:49 ` David Sterba
0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2014-05-29 14:49 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm
On Mon, May 26, 2014 at 05:30:24PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
>
> we would need the device links to be created,
> when device is added.
Looks good (plus the btrfs_ prefix mentioned before).
Reviewed-by: David Sterba <dsterba@suse.cz>
I was comparing it to Jeff's patch and I think Anand's version is
cleaner because it uses helpers that conveniently wrap the
bdev/part_to_dev trickery.
There's one difference in btrfs_init_new_device, where the
add_membership is called, but both locations look correct to me.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
2014-05-26 9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
2014-05-26 9:30 ` [PATCH 1/4] btrfs: dev delete should remove sysfs entry Anand Jain
2014-05-26 9:30 ` [PATCH 2/4] btrfs: dev add should add its " Anand Jain
@ 2014-05-26 9:30 ` Anand Jain
2014-05-29 13:29 ` David Sterba
2014-05-26 9:30 ` [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well Anand Jain
2014-05-28 8:30 ` [PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices Anand Jain
4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26 9:30 UTC (permalink / raw)
To: linux-btrfs; +Cc: jeffm, dsterba, clm
when we replace the device its corresponding sysfs
entry has to be replaced as well
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/dev-replace.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 9f22905..f4f8728 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -36,6 +36,7 @@
#include "check-integrity.h"
#include "rcu-string.h"
#include "dev-replace.h"
+#include "sysfs.h"
static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
int scrub_ret);
@@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
fs_info->fs_devices->latest_bdev = tgt_device->bdev;
list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
+ /* replace the sysfs entry */
+ rm_device_membership(fs_info, src_device);
+ add_device_membership(fs_info, tgt_device);
+
btrfs_rm_dev_replace_blocked(fs_info);
btrfs_rm_dev_replace_srcdev(fs_info, src_device);
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
2014-05-26 9:30 ` [PATCH 3/4] btrfs: dev replace should replace the " Anand Jain
@ 2014-05-29 13:29 ` David Sterba
2014-05-30 7:40 ` Anand Jain
0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-05-29 13:29 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm
On Mon, May 26, 2014 at 05:30:25PM +0800, Anand Jain wrote:
> when we replace the device its corresponding sysfs
> entry has to be replaced as well
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/dev-replace.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 9f22905..f4f8728 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -36,6 +36,7 @@
> #include "check-integrity.h"
> #include "rcu-string.h"
> #include "dev-replace.h"
> +#include "sysfs.h"
>
> static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> int scrub_ret);
> @@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> fs_info->fs_devices->latest_bdev = tgt_device->bdev;
> list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
>
> + /* replace the sysfs entry */
> + rm_device_membership(fs_info, src_device);
> + add_device_membership(fs_info, tgt_device);
> +
> btrfs_rm_dev_replace_blocked(fs_info);
>
> btrfs_rm_dev_replace_srcdev(fs_info, src_device);
569 btrfs_rm_dev_replace_unblocked(fs_info);
570
The comment that follows says
571 /*
572 * this is again a consistent state where no dev_replace procedure
573 * is running, the target device is part of the filesystem, the
574 * source device is not part of the filesystem anymore and its 1st
575 * superblock is scratched out so that it is no longer marked to
576 * belong to this filesystem.
577 */
and I think this is the right place to put the sysfs updates, because the
srcdev is processed.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
2014-05-29 13:29 ` David Sterba
@ 2014-05-30 7:40 ` Anand Jain
2014-06-03 3:47 ` Anand Jain
0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-30 7:40 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs, jeffm, clm
On 29/05/14 21:29, David Sterba wrote:
> On Mon, May 26, 2014 at 05:30:25PM +0800, Anand Jain wrote:
>> when we replace the device its corresponding sysfs
>> entry has to be replaced as well
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> fs/btrfs/dev-replace.c | 5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 9f22905..f4f8728 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -36,6 +36,7 @@
>> #include "check-integrity.h"
>> #include "rcu-string.h"
>> #include "dev-replace.h"
>> +#include "sysfs.h"
>>
>> static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>> int scrub_ret);
>> @@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>> fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>> list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
>>
>> + /* replace the sysfs entry */
>> + rm_device_membership(fs_info, src_device);
>> + add_device_membership(fs_info, tgt_device);
>> +
>> btrfs_rm_dev_replace_blocked(fs_info);
>>
>> btrfs_rm_dev_replace_srcdev(fs_info, src_device);
>
> 569 btrfs_rm_dev_replace_unblocked(fs_info);
> 570
>
> The comment that follows says
>
> 571 /*
> 572 * this is again a consistent state where no dev_replace procedure
> 573 * is running, the target device is part of the filesystem, the
> 574 * source device is not part of the filesystem anymore and its 1st
> 575 * superblock is scratched out so that it is no longer marked to
> 576 * belong to this filesystem.
> 577 */
>
> and I think this is the right place to put the sysfs updates, because the
> srcdev is processed.
Looking into this, will update. Thanks for the review.
> --
> 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
2014-05-30 7:40 ` Anand Jain
@ 2014-06-03 3:47 ` Anand Jain
2014-06-03 13:39 ` David Sterba
0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-06-03 3:47 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs, jeffm, clm
inline below.
On 30/05/2014 15:40, Anand Jain wrote:
>
>
>
> On 29/05/14 21:29, David Sterba wrote:
>> On Mon, May 26, 2014 at 05:30:25PM +0800, Anand Jain wrote:
>>> when we replace the device its corresponding sysfs
>>> entry has to be replaced as well
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>> fs/btrfs/dev-replace.c | 5 +++++
>>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>>> index 9f22905..f4f8728 100644
>>> --- a/fs/btrfs/dev-replace.c
>>> +++ b/fs/btrfs/dev-replace.c
>>> @@ -36,6 +36,7 @@
>>> #include "check-integrity.h"
>>> #include "rcu-string.h"
>>> #include "dev-replace.h"
>>> +#include "sysfs.h"
>>>
>>> static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>> int scrub_ret);
>>> @@ -562,6 +563,10 @@ static int btrfs_dev_replace_finishing(struct
>>> btrfs_fs_info *fs_info,
>>> fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>>> list_add(&tgt_device->dev_alloc_list,
>>> &fs_info->fs_devices->alloc_list);
>>>
>>> + /* replace the sysfs entry */
>>> + rm_device_membership(fs_info, src_device);
>>> + add_device_membership(fs_info, tgt_device);
>>> +
>>> btrfs_rm_dev_replace_blocked(fs_info);
>>>
>>> btrfs_rm_dev_replace_srcdev(fs_info, src_device);
>>
>> 569 btrfs_rm_dev_replace_unblocked(fs_info);
>> 570
>>
>> The comment that follows says
>>
>> 571 /*
>> 572 * this is again a consistent state where no dev_replace
>> procedure
>> 573 * is running, the target device is part of the
>> filesystem, the
>> 574 * source device is not part of the filesystem anymore and
>> its 1st
>> 575 * superblock is scratched out so that it is no longer
>> marked to
>> 576 * belong to this filesystem.
>> 577 */
>>
>> and I think this is the right place to put the sysfs updates, because the
>> srcdev is processed.
>
> Looking into this, will update. Thanks for the review.
btrfs_rm_dev_replace_srcdev() would destroy the btrfs_device of
src_device, and I am removing its sys/fs entry before we destroy
btrfs_device of src_device. Which is logically correct.
Further, RFC like 6/6 would depend on the btrfs_device struct,
so we have to call btrfs_kobj_rm_device() before
btrfs_rm_dev_replace_srcdev()
Also I did some extra tests and code walk I don't see any case
which it would fail by calling btrfs_rm_dev_replace_srcdev()
before btrfs_rm_dev_replace_srcdev().
V2 for this patch-set has been sent out.
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] btrfs: dev replace should replace the sysfs entry
2014-06-03 3:47 ` Anand Jain
@ 2014-06-03 13:39 ` David Sterba
0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2014-06-03 13:39 UTC (permalink / raw)
To: Anand Jain; +Cc: dsterba, linux-btrfs, jeffm, clm
On Tue, Jun 03, 2014 at 11:47:42AM +0800, Anand Jain wrote:
> >>>+ /* replace the sysfs entry */
> >>>+ rm_device_membership(fs_info, src_device);
> >>>+ add_device_membership(fs_info, tgt_device);
> >>>+
> >>> btrfs_rm_dev_replace_blocked(fs_info);
> >>>
> >>> btrfs_rm_dev_replace_srcdev(fs_info, src_device);
> >>
> >>569 btrfs_rm_dev_replace_unblocked(fs_info);
> >>570
> >>
> >>The comment that follows says
> >>
> >>571 /*
> >>572 * this is again a consistent state where no dev_replace
> >>procedure
> >>573 * is running, the target device is part of the
> >>filesystem, the
> >>574 * source device is not part of the filesystem anymore and
> >>its 1st
> >>575 * superblock is scratched out so that it is no longer
> >>marked to
> >>576 * belong to this filesystem.
> >>577 */
> >>
> >>and I think this is the right place to put the sysfs updates, because the
> >>srcdev is processed.
> >
> >Looking into this, will update. Thanks for the review.
>
> btrfs_rm_dev_replace_srcdev() would destroy the btrfs_device of
> src_device, and I am removing its sys/fs entry before we destroy
> btrfs_device of src_device. Which is logically correct.
I agree, my analysis was wrong.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
2014-05-26 9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
` (2 preceding siblings ...)
2014-05-26 9:30 ` [PATCH 3/4] btrfs: dev replace should replace the " Anand Jain
@ 2014-05-26 9:30 ` Anand Jain
2014-05-29 12:54 ` David Sterba
2014-05-28 8:30 ` [PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices Anand Jain
4 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-05-26 9:30 UTC (permalink / raw)
To: linux-btrfs; +Cc: jeffm, dsterba, clm
Creating sprout will change the fsid of the mounted root.
do the same on the sysfs as well.
reproducer:
mount /dev/sdb /btrfs (seed disk)
btrfs dev add /dev/sdc /btrfs
mount -o rw,remount /btrfs
btrfs dev del /dev/sdb /btrfs
mount /dev/sdb /btrfs
Error:
kobject_add_internal failed for fe350492-dc28-4051-a601-e017b17e6145 with -EEXIST, don't try to register things with the same name in the same directory.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5a577ab..e6cf3a7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
if (seeding_dev) {
+ char fsid_buf[37];
ret = init_first_rw_device(trans, root, device);
if (ret) {
btrfs_abort_transaction(trans, root, ret);
@@ -2094,6 +2095,13 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
btrfs_abort_transaction(trans, root, ret);
goto error_trans;
}
+
+ /* Sprouting would change fsid of the mounted root,
+ * so rename the fsid on the sysfs
+ */
+ sprintf(fsid_buf, "%pU", root->fs_info->fsid);
+ if (kobject_rename(&root->fs_info->super_kobj, fsid_buf))
+ goto error_trans;
} else {
ret = btrfs_add_device(trans, root, device);
if (ret) {
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
2014-05-26 9:30 ` [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well Anand Jain
@ 2014-05-29 12:54 ` David Sterba
2014-06-02 8:22 ` Anand Jain
0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-05-29 12:54 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, jeffm, dsterba, clm
On Mon, May 26, 2014 at 05:30:26PM +0800, Anand Jain wrote:
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
> mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
>
> if (seeding_dev) {
> + char fsid_buf[37];
Is there a symbolic constant available? We have one in userspace, but I
can't find one for kernel, only a few locally defined.
> ret = init_first_rw_device(trans, root, device);
> if (ret) {
> btrfs_abort_transaction(trans, root, ret);
> @@ -2094,6 +2095,13 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
> btrfs_abort_transaction(trans, root, ret);
> goto error_trans;
> }
> +
> + /* Sprouting would change fsid of the mounted root,
> + * so rename the fsid on the sysfs
> + */
> + sprintf(fsid_buf, "%pU", root->fs_info->fsid);
Would be better do use snprintf explicitly.
> + if (kobject_rename(&root->fs_info->super_kobj, fsid_buf))
> + goto error_trans;
> } else {
Otherwise ok.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
2014-05-29 12:54 ` David Sterba
@ 2014-06-02 8:22 ` Anand Jain
2014-06-02 15:39 ` David Sterba
0 siblings, 1 reply; 18+ messages in thread
From: Anand Jain @ 2014-06-02 8:22 UTC (permalink / raw)
To: dsterba, linux-btrfs, jeffm, clm
On 29/05/14 20:54, David Sterba wrote:
> On Mon, May 26, 2014 at 05:30:26PM +0800, Anand Jain wrote:
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>> mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
>>
>> if (seeding_dev) {
>> + char fsid_buf[37];
>
> Is there a symbolic constant available? We have one in userspace, but I
> can't find one for kernel, only a few locally defined.
now defined the same (as in progs) in kernel as well.
>> ret = init_first_rw_device(trans, root, device);
>> if (ret) {
>> btrfs_abort_transaction(trans, root, ret);
>> @@ -2094,6 +2095,13 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>> btrfs_abort_transaction(trans, root, ret);
>> goto error_trans;
>> }
>> +
>> + /* Sprouting would change fsid of the mounted root,
>> + * so rename the fsid on the sysfs
>> + */
>> + sprintf(fsid_buf, "%pU", root->fs_info->fsid);
>
> Would be better do use snprintf explicitly.
added. Thanks for commenting.
Anand
>> + if (kobject_rename(&root->fs_info->super_kobj, fsid_buf))
>> + goto error_trans;
>> } else {
>
> Otherwise ok.
> --
> 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
2014-06-02 8:22 ` Anand Jain
@ 2014-06-02 15:39 ` David Sterba
2014-06-03 0:27 ` Anand Jain
0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-06-02 15:39 UTC (permalink / raw)
To: Anand Jain; +Cc: dsterba, linux-btrfs, jeffm, clm
On Mon, Jun 02, 2014 at 04:22:20PM +0800, Anand Jain wrote:
> >>--- a/fs/btrfs/volumes.c
> >>+++ b/fs/btrfs/volumes.c
> >>@@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
> >> mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
> >>
> >> if (seeding_dev) {
> >>+ char fsid_buf[37];
> >
> >Is there a symbolic constant available? We have one in userspace, but I
> >can't find one for kernel, only a few locally defined.
>
> now defined the same (as in progs) in kernel as well.
In progs it's in utils.h
40 #define BTRFS_UUID_UNPARSED_SIZE 37
but I don't see where it's defined in kernel. Can you please give me a
pointer?
On the other hand, progs have a local define, we can do the same, I
don't see any other potential users.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well
2014-06-02 15:39 ` David Sterba
@ 2014-06-03 0:27 ` Anand Jain
0 siblings, 0 replies; 18+ messages in thread
From: Anand Jain @ 2014-06-03 0:27 UTC (permalink / raw)
To: dsterba, linux-btrfs, jeffm, clm
On 02/06/2014 23:39, David Sterba wrote:
> On Mon, Jun 02, 2014 at 04:22:20PM +0800, Anand Jain wrote:
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -2084,6 +2084,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>>>> mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
>>>>
>>>> if (seeding_dev) {
>>>> + char fsid_buf[37];
>>>
>>> Is there a symbolic constant available? We have one in userspace, but I
>>> can't find one for kernel, only a few locally defined.
>>
>> now defined the same (as in progs) in kernel as well.
>
> In progs it's in utils.h
>
> 40 #define BTRFS_UUID_UNPARSED_SIZE 37
>
> but I don't see where it's defined in kernel. Can you please give me a
> pointer?
>
> On the other hand, progs have a local define, we can do the same, I
> don't see any other potential users.
David,
What I have as of now (in my workspace) is a local define in volume.h.
but in the 2nd thought I am thinking if it is better to be at
./include/uapi/linux/btrfs.h
::
#define BTRFS_FSID_SIZE 16
#define BTRFS_UUID_SIZE 16
#define BTRFS_UUID_UNPARSED_SIZE 37 <--
Eventually in the long run, when we clean up btrfs-progs it could
just include ./include/uapi/linux/btrfs.h
Also there is this driver, who has defined it but its local
#define MAXUUIDLEN 37
./drivers/staging/tidspbridge/include/dspbridge/uuidutil.h
How do you like the idea of define at include/uapi/linux/btrfs.h
let me know.
Thxs, 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH RFC v2] btrfs: revamp /sys/fs/btrfs/<fsid>/devices
2014-05-26 9:30 [PATCH 0/4] update sysfs per btrfs device operations Anand Jain
` (3 preceding siblings ...)
2014-05-26 9:30 ` [PATCH 4/4] btrfs: create sprout should rename fsid on the sysfs as well Anand Jain
@ 2014-05-28 8:30 ` Anand Jain
4 siblings, 0 replies; 18+ messages in thread
From: Anand Jain @ 2014-05-28 8:30 UTC (permalink / raw)
To: linux-btrfs; +Cc: jeffm, dsterba, clm
As of now with out this patch the sysfs interface under dir
/sys/fs/btrfs/<fsid>/devices is just link to the block devs.
Moving forward we would need the above btrfs sysfs path to contain more
info about the btrfs devices. So this patch provides a framework for
the same.
And as of now a probe interface is added, which can be used to notify
btrfs for any change in the device status.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
V2: On the 2nd thought I kept the device link, but under the device name.
eg: /sys/fs/btrfs/<fsid>/devices/<sdx>/sdx-> link to blk device
/sys/fs/btrfs/<fsid>/devices/<sdx>/probe
and commit update accordingly
fs/btrfs/sysfs.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++----
fs/btrfs/volumes.h | 2 +
2 files changed, 69 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 0f2ca33..a8d367c 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -31,8 +31,11 @@
#include "transaction.h"
#include "sysfs.h"
#include "volumes.h"
+#include "rcu-string.h"
static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj);
+int rm_device_membership(struct btrfs_fs_info *fs_info,
+ struct btrfs_device *one_device);
static u64 get_features(struct btrfs_fs_info *fs_info,
enum btrfs_feature_set set)
@@ -490,8 +493,13 @@ void btrfs_sysfs_remove_one(struct btrfs_fs_info *fs_info)
kobject_del(fs_info->space_info_kobj);
kobject_put(fs_info->space_info_kobj);
}
- kobject_del(fs_info->device_dir_kobj);
- kobject_put(fs_info->device_dir_kobj);
+
+ if (fs_info->device_dir_kobj) {
+ rm_device_membership(fs_info, NULL);
+ kobject_del(fs_info->device_dir_kobj);
+ kobject_put(fs_info->device_dir_kobj);
+ }
+
addrm_unknown_feature_attrs(fs_info, false);
sysfs_remove_group(&fs_info->super_kobj, &btrfs_feature_attr_group);
__btrfs_sysfs_remove_one(fs_info);
@@ -577,21 +585,68 @@ int rm_device_membership(struct btrfs_fs_info *fs_info,
{
struct hd_struct *disk;
struct kobject *disk_kobj;
+ struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+ struct btrfs_device *dev;
if (!fs_info->device_dir_kobj)
return -EINVAL;
if (one_device) {
+ if (!one_device->device_kobj.parent)
+ return -EINVAL;
+
disk = one_device->bdev->bd_part;
disk_kobj = &part_to_dev(disk)->kobj;
- sysfs_remove_link(fs_info->device_dir_kobj,
+ sysfs_remove_link(&one_device->device_kobj,
disk_kobj->name);
+ kobject_del(&one_device->device_kobj);
+ kobject_put(&one_device->device_kobj);
+ return 0;
}
+ list_for_each_entry(dev, &fs_devices->devices, dev_list) {
+ if (!dev->device_kobj.parent)
+ continue;
+
+ disk = dev->bdev->bd_part;
+ disk_kobj = &part_to_dev(disk)->kobj;
+
+ sysfs_remove_link(&dev->device_kobj, disk_kobj->name);
+ kobject_del(&dev->device_kobj);
+ kobject_put(&dev->device_kobj);
+ }
return 0;
}
+#define to_btrfs_device(_kobj) container_of(_kobj, struct btrfs_device, device_kobj)
+
+static ssize_t device_kobj_probe_store(struct kobject *dev_kobj,
+ struct kobj_attribute *a, const char *buf, size_t len)
+{
+ /* Fixme: Call appropriate device check status handler */
+
+ return len;
+}
+
+BTRFS_ATTR_RW(probe, 0200, NULL, device_kobj_probe_store);
+
+static struct attribute *device_kobj_attrs[] = {
+ BTRFS_ATTR_PTR(probe),
+ NULL,
+};
+
+static void device_kobj_release(struct kobject *dev_kobj)
+{
+ /* nothing to free as of now */
+}
+
+struct kobj_type device_ktype = {
+ .sysfs_ops = &kobj_sysfs_ops,
+ .release = device_kobj_release,
+ .default_attrs = device_kobj_attrs,
+};
+
int add_device_membership(struct btrfs_fs_info *fs_info,
struct btrfs_device *one_device)
{
@@ -610,19 +665,25 @@ int add_device_membership(struct btrfs_fs_info *fs_info,
struct hd_struct *disk;
struct kobject *disk_kobj;
- if (!dev->bdev)
+ if (!dev->bdev || dev->missing || dev->device_kobj.parent)
continue;
if (one_device && one_device != dev)
continue;
+ error = kobject_init_and_add(&dev->device_kobj, &device_ktype,
+ fs_info->device_dir_kobj, "%s",
+ strrchr(rcu_str_deref(dev->name), '/') + 1);
+ if (error)
+ break;
+
disk = dev->bdev->bd_part;
disk_kobj = &part_to_dev(disk)->kobj;
-
- error = sysfs_create_link(fs_info->device_dir_kobj,
+ error = sysfs_create_link(&dev->device_kobj,
disk_kobj, disk_kobj->name);
if (error)
break;
+
}
return error;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 25f0505..d0c9c32 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -113,6 +113,8 @@ struct btrfs_device {
int dev_stats_valid;
int dev_stats_dirty; /* counters need to be written to disk */
atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX];
+
+ struct kobject device_kobj;
};
struct btrfs_fs_devices {
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread