On Thu, Feb 06, 2020 at 12:01:28PM -0500, Josef Bacik wrote:
On 2/6/20 5:42 AM, Naohiro Aota wrote:
Introduce struct clustered_alloc_info to manage parameters related to
clustered allocation. By separating clustered_alloc_info and
find_free_extent_ctl, we can introduce other allocation policy. One can
access per-allocation policy private information from "alloc_info" of
struct find_free_extent_ctl.
Signed-off-by: Naohiro Aota <naohiro.aota@xxxxxxx>
---
fs/btrfs/extent-tree.c | 99 +++++++++++++++++++++++++-----------------
1 file changed, 59 insertions(+), 40 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b1f52eee24fe..8124a6461043 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3456,9 +3456,6 @@ struct find_free_extent_ctl {
/* Where to start the search inside the bg */
u64 search_start;
- /* For clustered allocation */
- u64 empty_cluster;
-
bool have_caching_bg;
bool orig_have_caching_bg;
@@ -3470,18 +3467,6 @@ struct find_free_extent_ctl {
*/
int loop;
- /*
- * Whether we're refilling a cluster, if true we need to re-search
- * current block group but don't try to refill the cluster again.
- */
- bool retry_clustered;
-
- /*
- * Whether we're updating free space cache, if true we need to re-search
- * current block group but don't try updating free space cache again.
- */
- bool retry_unclustered;
-
/* If current block group is cached */
int cached;
@@ -3499,8 +3484,28 @@ struct find_free_extent_ctl {
/* Allocation policy */
enum btrfs_extent_allocation_policy policy;
+ void *alloc_info;
};
+struct clustered_alloc_info {
+ /* For clustered allocation */
+ u64 empty_cluster;
+
+ /*
+ * Whether we're refilling a cluster, if true we need to re-search
+ * current block group but don't try to refill the cluster again.
+ */
+ bool retry_clustered;
+
+ /*
+ * Whether we're updating free space cache, if true we need to re-search
+ * current block group but don't try updating free space cache again.
+ */
+ bool retry_unclustered;
+
+ struct btrfs_free_cluster *last_ptr;
+ bool use_cluster;
This isn't the right place for this, rather I'd put it in the
find_free_extent_ctl if you want it at all.
And in fact I question the whole need for this in the first place. I
assume your goal is to just disable clustered allocation for shingle
drives, so why don't you just handle that with your extent allocation
policy flag? If it's set to shingled then use_cluster = false and you
are good to go, no need to add all this complication of the cluster
ctl.
Not really. The clustered allocator (= the current allocator) first
try allocation using a cluster by calling find_free_extent_clustered()
and, if it failed, try allocation without cluster by calling
find_free_extent_unclustered(). This "use_cluster" indicates to skip
the first find_free_extent_clustered().
When not in BTRFS_EXTENT_ALLOC_CLUSTERED, then both of these functions
are skipped (actually, another function for the policy is called). So
the policy controls higher-level function call and the "use_cluster"
controls lower level (only in BTRFS_EXTENT_ALLOC_CLUSTERED level)
function call.
If you are looking to save space in the ctl, then I would just union
{} the cluster stuff inside of the find_free_extent_ctl so the right
flags are used for the correction allocation policy.
This whole last set of 10 patches needs to be reworked. Thanks,
I'm fine with keeping these variable in find_free_extent_ctl. We can
delay this separation of clustered_alloc_info until we really want to
e.g. when some other policy want to use many per-policy
variables. (and, actually, zoned allocator is not using any per-policy
variable)
Thanks,
Naohiro
