Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Naohiro Aota <naohiro.aota@wdc.com>
To: Josef Bacik <josef@toxicpanda.com>
Cc: linux-btrfs@vger.kernel.org, David Sterba <dsterba@suse.com>,
	Chris Mason <clm@fb.com>, Nikolay Borisov <nborisov@suse.com>,
	Damien Le Moal <damien.lemoal@wdc.com>,
	Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	Hannes Reinecke <hare@suse.com>,
	Anand Jain <anand.jain@oracle.com>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 09/21] btrfs: factor out create_chunk()
Date: Thu, 20 Feb 2020 19:17:23 +0900
Message-ID: <20200220101723.5wpidi3wd2a7wvgn@naota.dhcp.fujisawa.hgst.com> (raw)
In-Reply-To: <7514070d-b7a8-be1c-c23a-f01b9ee3c7ce@toxicpanda.com>

On Thu, Feb 13, 2020 at 11:24:57AM -0500, Josef Bacik wrote:
>On 2/12/20 2:20 AM, Naohiro Aota wrote:
>>Factor out create_chunk() from __btrfs_alloc_chunk(). This function finally
>>creates a chunk. There is no functional changes.
>>
>>Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>---
>>  fs/btrfs/volumes.c | 130 ++++++++++++++++++++++++---------------------
>>  1 file changed, 70 insertions(+), 60 deletions(-)
>>
>>diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>index 00085943e4dd..3e2e3896d72a 100644
>>--- a/fs/btrfs/volumes.c
>>+++ b/fs/btrfs/volumes.c
>>@@ -5052,90 +5052,53 @@ static int decide_stripe_size(struct btrfs_fs_devices *fs_devices,
>>  	}
>>  }
>>-static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>-			       u64 start, u64 type)
>>+static int create_chunk(struct btrfs_trans_handle *trans,
>>+			struct alloc_chunk_ctl *ctl,
>>+			struct btrfs_device_info *devices_info)
>>  {
>>  	struct btrfs_fs_info *info = trans->fs_info;
>>-	struct btrfs_fs_devices *fs_devices = info->fs_devices;
>>  	struct map_lookup *map = NULL;
>>  	struct extent_map_tree *em_tree;
>>  	struct extent_map *em;
>>-	struct btrfs_device_info *devices_info = NULL;
>>-	struct alloc_chunk_ctl ctl;
>>+	u64 start = ctl->start;
>>+	u64 type = ctl->type;
>>  	int ret;
>>  	int i;
>>  	int j;
>>-	if (!alloc_profile_is_valid(type, 0)) {
>>-		ASSERT(0);
>>-		return -EINVAL;
>>-	}
>>-
>>-	if (list_empty(&fs_devices->alloc_list)) {
>>-		if (btrfs_test_opt(info, ENOSPC_DEBUG))
>>-			btrfs_debug(info, "%s: no writable device", __func__);
>>-		return -ENOSPC;
>>-	}
>>-
>>-	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
>>-		btrfs_err(info, "invalid chunk type 0x%llx requested", type);
>>-		BUG();
>>-	}
>>-
>>-	ctl.start = start;
>>-	ctl.type = type;
>>-	init_alloc_chunk_ctl(fs_devices, &ctl);
>>-
>>-	devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info),
>>-			       GFP_NOFS);
>>-	if (!devices_info)
>>+	map = kmalloc(map_lookup_size(ctl->num_stripes), GFP_NOFS);
>>+	if (!map)
>>  		return -ENOMEM;
>>+	map->num_stripes = ctl->num_stripes;
>>-	ret = gather_device_info(fs_devices, &ctl, devices_info);
>>-	if (ret < 0)
>>-		goto error;
>>-
>>-	ret = decide_stripe_size(fs_devices, &ctl, devices_info);
>>-	if (ret < 0)
>>-		goto error;
>>-
>>-	map = kmalloc(map_lookup_size(ctl.num_stripes), GFP_NOFS);
>>-	if (!map) {
>>-		ret = -ENOMEM;
>>-		goto error;
>>-	}
>>-
>>-	map->num_stripes = ctl.num_stripes;
>>-
>>-	for (i = 0; i < ctl.ndevs; ++i) {
>>-		for (j = 0; j < ctl.dev_stripes; ++j) {
>>-			int s = i * ctl.dev_stripes + j;
>>+	for (i = 0; i < ctl->ndevs; ++i) {
>>+		for (j = 0; j < ctl->dev_stripes; ++j) {
>>+			int s = i * ctl->dev_stripes + j;
>>  			map->stripes[s].dev = devices_info[i].dev;
>>  			map->stripes[s].physical = devices_info[i].dev_offset +
>>-						   j * ctl.stripe_size;
>>+						   j * ctl->stripe_size;
>>  		}
>>  	}
>>  	map->stripe_len = BTRFS_STRIPE_LEN;
>>  	map->io_align = BTRFS_STRIPE_LEN;
>>  	map->io_width = BTRFS_STRIPE_LEN;
>>  	map->type = type;
>>-	map->sub_stripes = ctl.sub_stripes;
>>+	map->sub_stripes = ctl->sub_stripes;
>>-	trace_btrfs_chunk_alloc(info, map, start, ctl.chunk_size);
>>+	trace_btrfs_chunk_alloc(info, map, start, ctl->chunk_size);
>>  	em = alloc_extent_map();
>>  	if (!em) {
>>  		kfree(map);
>>-		ret = -ENOMEM;
>>-		goto error;
>>+		return -ENOMEM;
>>  	}
>>  	set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags);
>>  	em->map_lookup = map;
>>  	em->start = start;
>>-	em->len = ctl.chunk_size;
>>+	em->len = ctl->chunk_size;
>>  	em->block_start = 0;
>>  	em->block_len = em->len;
>>-	em->orig_block_len = ctl.stripe_size;
>>+	em->orig_block_len = ctl->stripe_size;
>>  	em_tree = &info->mapping_tree;
>>  	write_lock(&em_tree->lock);
>>@@ -5143,11 +5106,11 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>  	if (ret) {
>>  		write_unlock(&em_tree->lock);
>>  		free_extent_map(em);
>>-		goto error;
>>+		return ret;
>>  	}
>>  	write_unlock(&em_tree->lock);
>>-	ret = btrfs_make_block_group(trans, 0, type, start, ctl.chunk_size);
>>+	ret = btrfs_make_block_group(trans, 0, type, start, ctl->chunk_size);
>>  	if (ret)
>>  		goto error_del_extent;
>>@@ -5155,20 +5118,19 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>  		struct btrfs_device *dev = map->stripes[i].dev;
>>  		btrfs_device_set_bytes_used(dev,
>>-					    dev->bytes_used + ctl.stripe_size);
>>+					    dev->bytes_used + ctl->stripe_size);
>>  		if (list_empty(&dev->post_commit_list))
>>  			list_add_tail(&dev->post_commit_list,
>>  				      &trans->transaction->dev_update_list);
>>  	}
>>-	atomic64_sub(ctl.stripe_size * map->num_stripes,
>>+	atomic64_sub(ctl->stripe_size * map->num_stripes,
>>  		     &info->free_chunk_space);
>>  	free_extent_map(em);
>>  	check_raid56_incompat_flag(info, type);
>>  	check_raid1c34_incompat_flag(info, type);
>>-	kfree(devices_info);
>>  	return 0;
>>  error_del_extent:
>>@@ -5180,7 +5142,55 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>  	free_extent_map(em);
>>  	/* One for the tree reference */
>>  	free_extent_map(em);
>>-error:
>>+
>>+	return ret;
>>+}
>>+
>>+static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>+			       u64 start, u64 type)
>>+{
>>+	struct btrfs_fs_info *info = trans->fs_info;
>>+	struct btrfs_fs_devices *fs_devices = info->fs_devices;
>>+	struct btrfs_device_info *devices_info = NULL;
>>+	struct alloc_chunk_ctl ctl;
>>+	int ret;
>>+
>>+	if (!alloc_profile_is_valid(type, 0)) {
>>+		ASSERT(0);
>>+		return -EINVAL;
>>+	}
>>+
>>+	if (list_empty(&fs_devices->alloc_list)) {
>>+		if (btrfs_test_opt(info, ENOSPC_DEBUG))
>>+			btrfs_debug(info, "%s: no writable device", __func__);
>>+		return -ENOSPC;
>>+	}
>>+
>>+	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
>>+		btrfs_err(info, "invalid chunk type 0x%llx requested", type);
>>+		BUG();
>>+	}
>
>This is superfluous, alloc_profile_is_valid() handles this check.  Thanks,
>
>Josef

This checks if at least one block group type (data, metadata or
system) flag is set. OTOH, alloc_profile_is_valid() checks if profile
bits are valid.

Maybe, we can move this check into alloc_profile_is_valid()?

Thanks,

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-12  7:20 [PATCH v2 00/21] btrfs: refactor and generalize chunk/dev_extent/extent allocation Naohiro Aota
2020-02-12  7:20 ` [PATCH v2 01/21] btrfs: change type of full_search to bool Naohiro Aota
2020-02-12  7:20 ` [PATCH v2 02/21] btrfs: do not BUG_ON with invalid profile Naohiro Aota
2020-02-12  7:58   ` Johannes Thumshirn
2020-02-12 14:14   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 03/21] btrfs: introduce chunk allocation policy Naohiro Aota
2020-02-12  7:20 ` [PATCH v2 04/21] btrfs: refactor find_free_dev_extent_start() Naohiro Aota
2020-02-12 14:16   ` Josef Bacik
2020-02-13 11:32   ` Johannes Thumshirn
2020-02-12  7:20 ` [PATCH v2 05/21] btrfs: introduce alloc_chunk_ctl Naohiro Aota
2020-02-12  8:00   ` Johannes Thumshirn
2020-02-13 16:17   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 06/21] btrfs: factor out init_alloc_chunk_ctl Naohiro Aota
2020-02-13 16:20   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 07/21] btrfs: factor out gather_device_info() Naohiro Aota
2020-02-12  8:04   ` Johannes Thumshirn
2020-02-12  7:20 ` [PATCH v2 08/21] btrfs: factor out decide_stripe_size() Naohiro Aota
2020-02-12  7:20 ` [PATCH v2 09/21] btrfs: factor out create_chunk() Naohiro Aota
2020-02-12  8:07   ` Johannes Thumshirn
2020-02-13 16:24   ` Josef Bacik
2020-02-20 10:17     ` Naohiro Aota [this message]
2020-02-12  7:20 ` [PATCH v2 10/21] btrfs: parameterize dev_extent_min Naohiro Aota
2020-02-13 16:27   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 11/21] btrfs: introduce extent allocation policy Naohiro Aota
2020-02-13 19:55   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 12/21] btrfs: move hint_byte into find_free_extent_ctl Naohiro Aota
2020-02-12 14:11   ` Johannes Thumshirn
2020-02-13 19:56   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 13/21] btrfs: move vairalbes for clustered allocation " Naohiro Aota
2020-02-12 14:14   ` Johannes Thumshirn
2020-02-13 19:57   ` Josef Bacik
2020-02-20 14:27   ` Christoph Hellwig
2020-02-12  7:20 ` [PATCH v2 14/21] btrfs: factor out do_allocation() Naohiro Aota
2020-02-12 14:45   ` Johannes Thumshirn
2020-02-13 19:57   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 15/21] btrfs: drop unnecessary arguments from clustered allocation functions Naohiro Aota
2020-02-12 14:47   ` Johannes Thumshirn
2020-02-13 19:58   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 16/21] btrfs: factor out release_block_group() Naohiro Aota
2020-02-12 14:48   ` Johannes Thumshirn
2020-02-13 19:58   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 17/21] btrfs: factor out found_extent() Naohiro Aota
2020-02-12 14:50   ` Johannes Thumshirn
2020-02-13 19:58   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 18/21] btrfs: drop unnecessary arguments from find_free_extent_update_loop() Naohiro Aota
2020-02-12 14:51   ` Johannes Thumshirn
2020-02-13 19:58   ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 19/21] btrfs: factor out chunk_allocation_failed() Naohiro Aota
2020-02-12 14:56   ` Johannes Thumshirn
2020-02-13 19:52   ` Josef Bacik
2020-02-20  9:57     ` Naohiro Aota
2020-02-12  7:20 ` [PATCH v2 20/21] btrfs: skip LOOP_NO_EMPTY_SIZE if not clustered allocation Naohiro Aota
2020-02-13 19:55   ` Josef Bacik
2020-02-20  9:56     ` Naohiro Aota
2020-02-20 15:40       ` Josef Bacik
2020-02-12  7:20 ` [PATCH v2 21/21] btrfs: factor out prepare_allocation() Naohiro Aota
2020-02-13 19:59   ` Josef Bacik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200220101723.5wpidi3wd2a7wvgn@naota.dhcp.fujisawa.hgst.com \
    --to=naohiro.aota@wdc.com \
    --cc=Johannes.Thumshirn@wdc.com \
    --cc=anand.jain@oracle.com \
    --cc=clm@fb.com \
    --cc=damien.lemoal@wdc.com \
    --cc=dsterba@suse.com \
    --cc=hare@suse.com \
    --cc=josef@toxicpanda.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=nborisov@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git