Skip to contentSkip to conversation

Btrfs: fix bh leak on __btrfs_open_devices path

PATCH New Xiao Guangrong 1 participant 5 messages Apr 20, 2011 Btrfs file system

lore
Share & tools
lore

Xiao Guangrong

author
#0 lore raw

PATCH 1/5 Btrfs: fix bh leak on __btrfs_open_devices path

'bh' is forgot to release if no error is detected

Signed-off-by Xiao Guangrong <[email protected]>
---
 fs/btrfs/volumes.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
fs/btrfs/volumes.c +1 -0
index 8b9fb8c..69fc902 100644
@@ -631,6 +631,7 @@ static int __btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 			list_add(&device->dev_alloc_list,
 				 &fs_devices->alloc_list);
 		}
+		brelse(bh);
 		continue;
 
 error_brelse:
Signature · 1 line
1.7.4.4
author
#1 lore raw

PATCH 2/5 Btrfs: fix the race between reading and updating devices

On btrfs_congested_fn and __unplug_io_fn paths, we should hold device_list_mutex to avoid remove/add device path to update fs_devices->devices

On __btrfs_close_devices and btrfs_prepare_sprout paths, the devices in fs_devices->devices or fs_devices->devices is updated, so we should hold the mutex to avoid the reader side to reach them

Signed-off-by Xiao Guangrong <[email protected]>
---
 fs/btrfs/disk-io.c |    4 ++++
 fs/btrfs/volumes.c |    7 +++++++
 2 files changed, 11 insertions(+), 0 deletions(-)
fs/btrfs/disk-io.c +4 -0
index ef6865c..5a70096 100644
@@ -1412,6 +1412,7 @@ static int btrfs_congested_fn(void *congested_data, int bdi_bits)
 	struct btrfs_device *device;
 	struct backing_dev_info *bdi;
 
+	mutex_lock(&info->fs_devices->device_list_mutex);
 	list_for_each_entry(device, &info->fs_devices->devices, dev_list) {
 		if (!device->bdev)
 			continue;

@@ -1421,6 +1422,7 @@ static int btrfs_congested_fn(void *congested_data, int bdi_bits)
 			break;
 		}
 	}
+	mutex_unlock(&info->fs_devices->device_list_mutex);
 	return ret;
 }
 

@@ -1434,6 +1436,7 @@ static void __unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
 	struct btrfs_fs_info *info;
 
 	info = (struct btrfs_fs_info *)bdi->unplug_io_data;
+	mutex_lock(&info->fs_devices->device_list_mutex);
 	list_for_each_entry(device, &info->fs_devices->devices, dev_list) {
 		if (!device->bdev)
 			continue;

@@ -1442,6 +1445,7 @@ static void __unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
 		if (bdi->unplug_io_fn)
 			bdi->unplug_io_fn(bdi, page);
 	}
+	mutex_unlock(&info->fs_devices->device_list_mutex);
 }
 
 static void btrfs_unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
fs/btrfs/volumes.c +7 -0
index 69fc902..a672249 100644
@@ -515,6 +515,7 @@ static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 	if (--fs_devices->opened > 0)
 		return 0;
 
+	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry(device, &fs_devices->devices, dev_list) {
 		if (device->bdev) {
 			blkdev_put(device->bdev, device->mode);

@@ -529,6 +530,8 @@ static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 		device->writeable = 0;
 		device->in_fs_metadata = 0;
 	}
+	mutex_unlock(&fs_devices->device_list_mutex);
+
 	WARN_ON(fs_devices->open_devices);
 	WARN_ON(fs_devices->rw_devices);
 	fs_devices->opened = 0;

@@ -1449,7 +1452,11 @@ static int btrfs_prepare_sprout(struct btrfs_trans_handle *trans,
 	INIT_LIST_HEAD(&seed_devices->devices);
 	INIT_LIST_HEAD(&seed_devices->alloc_list);
 	mutex_init(&seed_devices->device_list_mutex);
+
+	mutex_lock(&root->fs_info->fs_devices->device_list_mutex);
 	list_splice_init(&fs_devices->devices, &seed_devices->devices);
+	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
+
 	list_splice_init(&fs_devices->alloc_list, &seed_devices->alloc_list);
 	list_for_each_entry(device, &seed_devices->devices, dev_list) {
 		device->fs_devices = seed_devices;
Signature · 1 line
1.7.4.4
author
#2 lore raw

PATCH 3/5 Btrfs: fix the race between remove dev and alloc chunk

In reply to Xiao Guangrong

On remove device path, it updates device->dev_alloc_list but does not hold chunk lock

Signed-off-by Xiao Guangrong <[email protected]>
---
 fs/btrfs/volumes.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
fs/btrfs/volumes.c +6 -0
index a672249..d20351f 100644
@@ -1325,7 +1325,9 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	}
 
 	if (device->writeable) {
+		lock_chunks(root);
 		list_del_init(&device->dev_alloc_list);
+		unlock_chunks(root);
 		root->fs_info->fs_devices->rw_devices--;
 	}
 

@@ -1379,7 +1381,9 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 		}
 		fs_devices->seed = device->fs_devices->seed;
 		device->fs_devices->seed = NULL;
+		lock_chunks(root);
 		__btrfs_close_devices(device->fs_devices);
+		unlock_chunks(root);
 		free_fs_devices(device->fs_devices);
 	}
 

@@ -1411,8 +1415,10 @@ out:
 	return ret;
 error_undo:
 	if (device->writeable) {
+		lock_chunks(root);
 		list_add(&device->dev_alloc_list,
 			 &root->fs_info->fs_devices->alloc_list);
+		unlock_chunks(root);
 		root->fs_info->fs_devices->rw_devices++;
 	}
 	goto error_brelse;
Signature · 1 line
1.7.4.4
author
#3 lore raw

PATCH 4/5 Btrfs: drop unnecessary device lock

In reply to Xiao Guangrong

Drop device_list_mutex for the reader side on clone_fs_devices and btrfs_rm_device pathes since the fs_info->volume_mutex can ensure the device list is not updated

btrfs_close_extra_devices is the initialized path, we can not add or remove device at this time, so we can simply drop the mutex safely, like other initialized function does(add_missing_dev, __find_device, __btrfs_open_devices ...).

Signed-off-by Xiao Guangrong <[email protected]>
---
 fs/btrfs/volumes.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
fs/btrfs/volumes.c +6 -7
index d20351f..f43b946 100644
@@ -440,7 +440,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 	fs_devices->latest_trans = orig->latest_trans;
 	memcpy(fs_devices->fsid, orig->fsid, sizeof(fs_devices->fsid));
 
-	mutex_lock(&orig->device_list_mutex);
+	/* We have held the volume lock, it is safe to get the devices. */
 	list_for_each_entry(orig_dev, &orig->devices, dev_list) {
 		device = kzalloc(sizeof(*device), GFP_NOFS);
 		if (!device)

@@ -463,10 +463,8 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 		device->fs_devices = fs_devices;
 		fs_devices->num_devices++;
 	}
-	mutex_unlock(&orig->device_list_mutex);
 	return fs_devices;
 error:
-	mutex_unlock(&orig->device_list_mutex);
 	free_fs_devices(fs_devices);
 	return ERR_PTR(-ENOMEM);
 }

@@ -477,7 +475,7 @@ int btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices)
 
 	mutex_lock(&uuid_mutex);
 again:
-	mutex_lock(&fs_devices->device_list_mutex);
+	/* This is the initialized path, it is safe to release the devices. */
 	list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
 		if (device->in_fs_metadata)
 			continue;

@@ -497,7 +495,6 @@ again:
 		kfree(device->name);
 		kfree(device);
 	}
-	mutex_unlock(&fs_devices->device_list_mutex);
 
 	if (fs_devices->seed) {
 		fs_devices = fs_devices->seed;

@@ -1276,14 +1273,16 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 
 		device = NULL;
 		devices = &root->fs_info->fs_devices->devices;
-		mutex_lock(&root->fs_info->fs_devices->device_list_mutex);
+		/*
+		 * It is safe to read the devices since the volume_mutex
+		 * is held.
+		 */
 		list_for_each_entry(tmp, devices, dev_list) {
 			if (tmp->in_fs_metadata && !tmp->bdev) {
 				device = tmp;
 				break;
 			}
 		}
-		mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
 		bdev = NULL;
 		bh = NULL;
 		disk_super = NULL;
Signature · 1 line
1.7.4.4
author
#4 lore raw

PATCH 5/5 Btrfs: using rcu lock in the reader side of devices list

In reply to Xiao Guangrong

fs_devices->devices is only updated on remove and add device paths, so we can use rcu to protect it in the reader side

Signed-off-by Xiao Guangrong <[email protected]>
---
 fs/btrfs/disk-io.c |   21 +++++++------
 fs/btrfs/ioctl.c   |    7 ++--
 fs/btrfs/volumes.c |   85 ++++++++++++++++++++++++++++++++++++----------------
 fs/btrfs/volumes.h |    2 +
 4 files changed, 76 insertions(+), 39 deletions(-)
fs/btrfs/disk-io.c +11 -10
index 5a70096..ea13c9f 100644
@@ -1412,8 +1412,8 @@ static int btrfs_congested_fn(void *congested_data, int bdi_bits)
 	struct btrfs_device *device;
 	struct backing_dev_info *bdi;
 
-	mutex_lock(&info->fs_devices->device_list_mutex);
-	list_for_each_entry(device, &info->fs_devices->devices, dev_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(device, &info->fs_devices->devices, dev_list) {
 		if (!device->bdev)
 			continue;
 		bdi = blk_get_backing_dev_info(device->bdev);

@@ -1422,7 +1422,7 @@ static int btrfs_congested_fn(void *congested_data, int bdi_bits)
 			break;
 		}
 	}
-	mutex_unlock(&info->fs_devices->device_list_mutex);
+	rcu_read_unlock();
 	return ret;
 }
 

@@ -1436,8 +1436,9 @@ static void __unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
 	struct btrfs_fs_info *info;
 
 	info = (struct btrfs_fs_info *)bdi->unplug_io_data;
-	mutex_lock(&info->fs_devices->device_list_mutex);
-	list_for_each_entry(device, &info->fs_devices->devices, dev_list) {
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(device, &info->fs_devices->devices, dev_list) {
 		if (!device->bdev)
 			continue;
 

@@ -1445,7 +1446,7 @@ static void __unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
 		if (bdi->unplug_io_fn)
 			bdi->unplug_io_fn(bdi, page);
 	}
-	mutex_unlock(&info->fs_devices->device_list_mutex);
+	rcu_read_unlock();
 }
 
 static void btrfs_unplug_io_fn(struct backing_dev_info *bdi, struct page *page)

@@ -2414,9 +2415,9 @@ int write_all_supers(struct btrfs_root *root, int max_mirrors)
 	sb = &root->fs_info->super_for_commit;
 	dev_item = &sb->dev_item;
 
-	mutex_lock(&root->fs_info->fs_devices->device_list_mutex);
+	rcu_read_lock();
 	head = &root->fs_info->fs_devices->devices;
-	list_for_each_entry(dev, head, dev_list) {
+	list_for_each_entry_rcu(dev, head, dev_list) {
 		if (!dev->bdev) {
 			total_errors++;
 			continue;

@@ -2449,7 +2450,7 @@ int write_all_supers(struct btrfs_root *root, int max_mirrors)
 	}
 
 	total_errors = 0;
-	list_for_each_entry(dev, head, dev_list) {
+	list_for_each_entry_rcu(dev, head, dev_list) {
 		if (!dev->bdev)
 			continue;
 		if (!dev->in_fs_metadata || !dev->writeable)

@@ -2459,7 +2460,7 @@ int write_all_supers(struct btrfs_root *root, int max_mirrors)
 		if (ret)
 			total_errors++;
 	}
-	mutex_unlock(&root->fs_info->fs_devices->device_list_mutex);
+	rcu_read_unlock();
 	if (total_errors > max_errors) {
 		printk(KERN_ERR "btrfs: %d errors while writing supers\n",
 		       total_errors);
fs/btrfs/ioctl.c +4 -3
index f580a3a..c2f8920 100644
@@ -275,8 +275,9 @@ static noinline int btrfs_ioctl_fitrim(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	list_for_each_entry(device, &fs_info->fs_devices->devices, dev_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(device, &fs_info->fs_devices->devices,
+				dev_list) {
 		if (!device->bdev)
 			continue;
 		q = bdev_get_queue(device->bdev);

@@ -286,7 +287,7 @@ static noinline int btrfs_ioctl_fitrim(struct file *file, void __user *arg)
 				     minlen);
 		}
 	}
-	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
+	rcu_read_unlock();
 	if (!num_devices)
 		return -EOPNOTSUPP;
fs/btrfs/volumes.c +59 -26

181 lines not on this page · raw message

fs/btrfs/volumes.h +2 -0
index cc2eada..f1b2e4f 100644
@@ -86,6 +86,8 @@ struct btrfs_device {
 	u8 uuid[BTRFS_UUID_SIZE];
 
 	struct btrfs_work work;
+	struct rcu_head rcu;
+	struct work_struct rcu_work;
 };
 
 struct btrfs_fs_devices {
Signature · 1 line
1.7.4.4