Commit bb21e302 authored by Anand Jain's avatar Anand Jain Committed by David Sterba
Browse files

btrfs: move device->name RCU allocation and assign to btrfs_alloc_device() 



There is a repeating code section in the parent function after calling
btrfs_alloc_device(), as below:

      name = rcu_string_strdup(path, GFP_...);
      if (!name) {
              btrfs_free_device(device);
              return ERR_PTR(-ENOMEM);
      }
      rcu_assign_pointer(device->name, name);

Except in add_missing_dev() for obvious reasons.

This patch consolidates that repeating code into the btrfs_alloc_device()
itself so that the parent function doesn't have to duplicate code.
This consolidation also helps to review issues regarding RCU lock
violation with device->name.

Parent function device_list_add() and add_missing_dev() use GFP_NOFS for
the allocation, whereas the rest of the parent functions use GFP_KERNEL,
so bring the NOFS allocation context using memalloc_nofs_save() in the
function device_list_add() and add_missing_dev() is already doing it.

Signed-off-by: default avatarAnand Jain <anand.jain@oracle.com>
Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
parent 3e09b5b2

fs/btrfs/dev-replace.c

+1 −9
Original line number Diff line number Diff line
@@ -249,7 +249,6 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, 
	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;

	struct btrfs_device *device;

	struct block_device *bdev;

	struct rcu_string *name;

	u64 devid = BTRFS_DEV_REPLACE_DEVID;

	int ret = 0;
@@ -293,19 +292,12 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, 
	}





	device = btrfs_alloc_device(NULL, &devid, NULL);

	device = btrfs_alloc_device(NULL, &devid, NULL, device_path);

	if (IS_ERR(device)) {

		ret = PTR_ERR(device);

		goto error;

	}



	name = rcu_string_strdup(device_path, GFP_KERNEL);

	if (!name) {

		btrfs_free_device(device);

		ret = -ENOMEM;

		goto error;

	}

	rcu_assign_pointer(device->name, name);

	ret = lookup_bdev(device_path, &device->devt);

	if (ret)

		goto error;

fs/btrfs/volumes.c

+31 −37
Original line number Diff line number Diff line
@@ -845,26 +845,23 @@ static noinline struct btrfs_device *device_list_add(const char *path, 
	}



	if (!device) {

		unsigned int nofs_flag;



		if (fs_devices->opened) {

			mutex_unlock(&fs_devices->device_list_mutex);

			return ERR_PTR(-EBUSY);

		}



		nofs_flag = memalloc_nofs_save();

		device = btrfs_alloc_device(NULL, &devid,

					    disk_super->dev_item.uuid);

					    disk_super->dev_item.uuid, path);

		memalloc_nofs_restore(nofs_flag);

		if (IS_ERR(device)) {

			mutex_unlock(&fs_devices->device_list_mutex);

			/* we can safely leave the fs_devices entry around */

			return device;

		}



		name = rcu_string_strdup(path, GFP_NOFS);

		if (!name) {

			btrfs_free_device(device);

			mutex_unlock(&fs_devices->device_list_mutex);

			return ERR_PTR(-ENOMEM);

		}

		rcu_assign_pointer(device->name, name);

		device->devt = path_devt;



		list_add_rcu(&device->dev_list, &fs_devices->devices);
@@ -997,29 +994,21 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig) 
	fs_devices->total_devices = orig->total_devices;



	list_for_each_entry(orig_dev, &orig->devices, dev_list) {

		struct rcu_string *name;



		device = btrfs_alloc_device(NULL, &orig_dev->devid,

					    orig_dev->uuid);

		if (IS_ERR(device)) {

			ret = PTR_ERR(device);

			goto error;

		}

		const char *dev_path = NULL;



		/*

		 * This is ok to do without rcu read locked because we hold the

		 * This is ok to do without RCU read locked because we hold the

		 * uuid mutex so nothing we touch in here is going to disappear.

		 */

		if (orig_dev->name) {

			name = rcu_string_strdup(orig_dev->name->str,

					GFP_KERNEL);

			if (!name) {

				btrfs_free_device(device);

				ret = -ENOMEM;

		if (orig_dev->name)

			dev_path = orig_dev->name->str;



		device = btrfs_alloc_device(NULL, &orig_dev->devid,

					    orig_dev->uuid, dev_path);

		if (IS_ERR(device)) {

			ret = PTR_ERR(device);

			goto error;

		}

			rcu_assign_pointer(device->name, name);

		}



		if (orig_dev->zone_info) {

			struct btrfs_zoned_device_info *zone_info;
@@ -2604,7 +2593,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path 
	struct btrfs_device *device;

	struct block_device *bdev;

	struct super_block *sb = fs_info->sb;

	struct rcu_string *name;

	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;

	struct btrfs_fs_devices *seed_devices;

	u64 orig_super_total_bytes;
@@ -2645,20 +2633,13 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path 
	}

	rcu_read_unlock();



	device = btrfs_alloc_device(fs_info, NULL, NULL);

	device = btrfs_alloc_device(fs_info, NULL, NULL, device_path);

	if (IS_ERR(device)) {

		/* we can safely leave the fs_devices entry around */

		ret = PTR_ERR(device);

		goto error;

	}



	name = rcu_string_strdup(device_path, GFP_KERNEL);

	if (!name) {

		ret = -ENOMEM;

		goto error_free_device;

	}

	rcu_assign_pointer(device->name, name);



	device->fs_info = fs_info;

	device->bdev = bdev;

	ret = lookup_bdev(device_path, &device->devt);
@@ -6998,8 +6979,9 @@ static struct btrfs_device *add_missing_dev(struct btrfs_fs_devices *fs_devices, 
	 * always do NOFS because we use it in a lot of other GFP_KERNEL safe

	 * places.

	 */



	nofs_flag = memalloc_nofs_save();

	device = btrfs_alloc_device(NULL, &devid, dev_uuid);

	device = btrfs_alloc_device(NULL, &devid, dev_uuid, NULL);

	memalloc_nofs_restore(nofs_flag);

	if (IS_ERR(device))

		return device;
@@ -7023,14 +7005,15 @@ static struct btrfs_device *add_missing_dev(struct btrfs_fs_devices *fs_devices, 
 *		is generated.

 * @uuid:	a pointer to UUID for this device.  If NULL a new UUID

 *		is generated.

 * @path:	a pointer to device path if available, NULL otherwise.

 *

 * Return: a pointer to a new &struct btrfs_device on success; ERR_PTR()

 * on error.  Returned struct is not linked onto any lists and must be

 * destroyed with btrfs_free_device.

 */

struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,

					const u64 *devid,

					const u8 *uuid)

					const u64 *devid, const u8 *uuid,

					const char *path)

{

	struct btrfs_device *dev;

	u64 tmp;
@@ -7068,6 +7051,17 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info, 
	else

		generate_random_uuid(dev->uuid);



	if (path) {

		struct rcu_string *name;



		name = rcu_string_strdup(path, GFP_KERNEL);

		if (!name) {

			btrfs_free_device(dev);

			return ERR_PTR(-ENOMEM);

		}

		rcu_assign_pointer(dev->name, name);

	}



	return dev;

}

fs/btrfs/volumes.h

+2 −2
Original line number Diff line number Diff line
@@ -649,8 +649,8 @@ int btrfs_get_dev_args_from_path(struct btrfs_fs_info *fs_info, 
				 struct btrfs_dev_lookup_args *args,

				 const char *path);

struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,

					const u64 *devid,

					const u8 *uuid);

					const u64 *devid, const u8 *uuid,

					const char *path);

void btrfs_put_dev_args_from_path(struct btrfs_dev_lookup_args *args);

void btrfs_free_device(struct btrfs_device *device);

int btrfs_rm_device(struct btrfs_fs_info *fs_info,