Skip to contentSkip to conversation

btrfs: use list_for_each_entry_safe() when delete items

PATCH In review Azat Khuzhin 2 participants 4 messages Jul 27, 2013 – Aug 31, 2013 Btrfs file system

lore
Share & tools
lore

Azat Khuzhin

author
#0 lore raw

PATCH 1/1 btrfs: use list_for_each_entry_safe() when delete items

Replace list_for_each_entry() by list_for_each_entry_safe() in __btrfs_close_devices()

There is another place that delete items lock_stripe_add(), but there we don't need safe version, because after deleting we exit from loop.

Signed-off-by Azat Khuzhin <[email protected]>
---
 fs/btrfs/volumes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/volumes.c +2 -2
index 78b8717..1d1b595 100644
@@ -616,13 +616,13 @@ static void free_device(struct rcu_head *head)
 
 static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
-	struct btrfs_device *device;
+	struct btrfs_device *device, *next;
 
 	if (--fs_devices->opened > 0)
 		return 0;
 
 	mutex_lock(&fs_devices->device_list_mutex);
-	list_for_each_entry(device, &fs_devices->devices, dev_list) {
+	list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
 		struct btrfs_device *new_device;
 		struct rcu_string *name;
Signature · 1 line
1.7.10.4
author
#1 lore raw
Quoted 20 lines · Azat Khuzhin

Replace list_for_each_entry() by list_for_each_entry_safe() in __btrfs_close_devices()

There is another place that delete items lock_stripe_add(), but there we don't need safe version, because after deleting we exit from loop.

Signed-off-by Azat Khuzhin <[email protected]>
---
 fs/btrfs/volumes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/volumes.c +0 -1
index 78b8717..1d1b595 100644
@@ -616,13 +616,13 @@ static void free_device(struct rcu_head *head)
 
 static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
-       struct btrfs_device *device;

fs/btrfs/volumes.c static void free_device(struct rcu_head *head) · line 627

Quoted 10 lines
fs/btrfs/volumes.c +2 -1
+       struct btrfs_device *device, *next;
 
        if (--fs_devices->opened > 0)
                return 0;
 
        mutex_lock(&fs_devices->device_list_mutex);
-       list_for_each_entry(device, &fs_devices->devices, dev_list) {
+       list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
                struct btrfs_device *new_device;
                struct rcu_string *name;

There is "kfree(device);" at the end of loop, maybe there must "goto again;" after it?
(instead of this patch)

Quoted 4 lines
fs/btrfs/volumes.c +0 -1
--

1.7.10.4

Signature · 2 lines
Respectfully
Azat Khuzhin
Reviewed-by
#2 lore raw
Quoted 26 lines · Azat Khuzhin
Quoted 25 lines · Azat Khuzhin

Replace list_for_each_entry() by list_for_each_entry_safe() in __btrfs_close_devices()

There is another place that delete items lock_stripe_add(), but there we don't need safe version, because after deleting we exit from loop.

Signed-off-by Azat Khuzhin <[email protected]>
---
 fs/btrfs/volumes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/volumes.c +1 -1
index 78b8717..1d1b595 100644
@@ -616,13 +616,13 @@ static void free_device(struct rcu_head *head)
 
 static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
-       struct btrfs_device *device;
+       struct btrfs_device *device, *next;
 
        if (--fs_devices->opened > 0)
                return 0;
Quoted 9 lines

fs/btrfs/volumes.c static void free_device(struct rcu_head *head) · line 627

Quoted 5 lines
fs/btrfs/volumes.c +1 -1
        mutex_lock(&fs_devices->device_list_mutex);
-       list_for_each_entry(device, &fs_devices->devices, dev_list) {
+       list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
                struct btrfs_device *new_device;
                struct rcu_string *name;

There is "kfree(device);" at the end of loop, maybe there must "goto again;" after it?
(instead of this patch)

Your fix is right, we needn't search from the head once again.

The other fix way is:

	call_rcu(&device->rcu, free_device);

+ device = new_device;
}
but from the viewpoint of the readability, this way is not so good.

Reviewed-by Miao Xie <[email protected]>
Quoted 8 lines
Quoted 5 lines
fs/btrfs/volumes.c +0 -1
--

1.7.10.4

author
#3 lore raw
Quoted 27 lines · Miao Xie
Quoted 26 lines · Azat Khuzhin
Quoted 25 lines · Azat Khuzhin

Replace list_for_each_entry() by list_for_each_entry_safe() in __btrfs_close_devices()

There is another place that delete items lock_stripe_add(), but there we don't need safe version, because after deleting we exit from loop.

Signed-off-by Azat Khuzhin <[email protected]>
---
 fs/btrfs/volumes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/volumes.c +1 -1
index 78b8717..1d1b595 100644
@@ -616,13 +616,13 @@ static void free_device(struct rcu_head *head)
 
 static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
-       struct btrfs_device *device;
+       struct btrfs_device *device, *next;
 
        if (--fs_devices->opened > 0)
                return 0;
Quoted 9 lines
Quoted 9 lines

fs/btrfs/volumes.c static void free_device(struct rcu_head *head) · line 627

Quoted 5 lines
fs/btrfs/volumes.c +1 -1
        mutex_lock(&fs_devices->device_list_mutex);
-       list_for_each_entry(device, &fs_devices->devices, dev_list) {
+       list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
                struct btrfs_device *new_device;
                struct rcu_string *name;

There is "kfree(device);" at the end of loop, maybe there must "goto again;" after it?
(instead of this patch)

Ugh. I was looking into another function!

Quoted 10 lines

Your fix is right, we needn't search from the head once again.

The other fix way is:

        call_rcu(&device->rcu, free_device);

+ device = new_device;
}
but from the viewpoint of the readability, this way is not so good.

Reviewed-by Miao Xie <[email protected]>

Thanks!
Miao, should I resend patch with you reviewed-by?

Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
fs/btrfs/volumes.c +0 -1
--

1.7.10.4

Signature · 2 lines
Respectfully
Azat Khuzhin