[5/6] btrfs: use raid_attr for minimum stripe count in btrfs_calc_avail_data_space
diff mbox series

Message ID ffdc7e77015c2f5ad45de7acc2336fe2901bb605.1560880630.git.dsterba@suse.com
State New
Headers show
Series
  • Minor cleanups
Related show

Commit Message

David Sterba June 18, 2019, 6 p.m. UTC
Minimum stripe count matches the minimum devices required for a given
profile. The open coded assignments match the raid_attr table.

What's changed here is the meaning for RAID5/6. Previously their
min_stripes would be 1, while newly it's devs_min. This however shold be
the same as before because it's not possible to create filesystem on
fewer devices than the raid_attr table allows.

There's no adjustment regarding the parity stripes (like
calc_data_stripes does), because we're interested in overall space that
would fit on the devices.

Missing devices make no difference for the whole calculation, we have
the size stored in the structures.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Nikolay Borisov June 19, 2019, 7:51 a.m. UTC | #1
On 18.06.19 г. 21:00 ч., David Sterba wrote:
> Minimum stripe count matches the minimum devices required for a given
> profile. The open coded assignments match the raid_attr table.
> 
> What's changed here is the meaning for RAID5/6. Previously their
> min_stripes would be 1, while newly it's devs_min. This however shold be
> the same as before because it's not possible to create filesystem on
> fewer devices than the raid_attr table allows.
> 
> There's no adjustment regarding the parity stripes (like
> calc_data_stripes does), because we're interested in overall space that
> would fit on the devices.
> 
> Missing devices make no difference for the whole calculation, we have
> the size stored in the structures.

I think the clean up in this function should include more here's list of
things which I think will make it more readable. Something like the
attached diff on top of your patch:
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6e196b8a0820..230aef8314da 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1898,11 +1898,10 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 	struct btrfs_device_info *devices_info;
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	struct btrfs_device *device;
-	u64 skip_space;
 	u64 type;
 	u64 avail_space;
 	u64 min_stripe_size;
-	int min_stripes = 1, num_stripes = 1;
+	int num_stripes = 1;
 	int i = 0, nr_devices;
 
 	/*
@@ -1957,28 +1956,21 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 		avail_space = device->total_bytes - device->bytes_used;
 
 		/* align with stripe_len */
-		avail_space = div_u64(avail_space, BTRFS_STRIPE_LEN);
-		avail_space *= BTRFS_STRIPE_LEN;
+		avail_space = rounddown(avail_space, BTRFS_STRIPE_LEN);
 
 		/*
 		 * In order to avoid overwriting the superblock on the drive,
 		 * btrfs starts at an offset of at least 1MB when doing chunk
 		 * allocation.
+		 *
+		 * This ensures we have at least min_stripe_size free space
+		 * after excluding 1mb.
 		 */
-		skip_space = SZ_1M;
-
-		/*
-		 * we can use the free space in [0, skip_space - 1], subtract
-		 * it from the total.
-		 */
-		if (avail_space && avail_space >= skip_space)
-			avail_space -= skip_space;
-		else
-			avail_space = 0;
-
-		if (avail_space < min_stripe_size)
+		if (avail_space <= SZ_1M + min_stripe_size)
 			continue;
 
+		avail_space -= SZ_1M;
+
 		devices_info[i].dev = device;
 		devices_info[i].max_avail = avail_space;
 
@@ -1992,9 +1984,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 
 	i = nr_devices - 1;
 	avail_space = 0;
-	while (nr_devices >= min_stripes) {
-		if (num_stripes > nr_devices)
-			num_stripes = nr_devices;
+	while (nr_devices >= rattr->devs_min) {
+		num_stripes = min(num_stripes, nr_devices);
 
 		if (devices_info[i].max_avail >= min_stripe_size) {
 			int j;
David Sterba June 19, 2019, 12:09 p.m. UTC | #2
On Wed, Jun 19, 2019 at 10:51:14AM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.06.19 г. 21:00 ч., David Sterba wrote:
> > Minimum stripe count matches the minimum devices required for a given
> > profile. The open coded assignments match the raid_attr table.
> > 
> > What's changed here is the meaning for RAID5/6. Previously their
> > min_stripes would be 1, while newly it's devs_min. This however shold be
> > the same as before because it's not possible to create filesystem on
> > fewer devices than the raid_attr table allows.
> > 
> > There's no adjustment regarding the parity stripes (like
> > calc_data_stripes does), because we're interested in overall space that
> > would fit on the devices.
> > 
> > Missing devices make no difference for the whole calculation, we have
> > the size stored in the structures.
> 
> I think the clean up in this function should include more here's list of
> things which I think will make it more readable.

I did not intend to clean up the whole function in this patch, only whre
the raid table could be used.

> Something like the
> attached diff on top of your patch:
> 
> 

> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 6e196b8a0820..230aef8314da 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1898,11 +1898,10 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  	struct btrfs_device_info *devices_info;
>  	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>  	struct btrfs_device *device;
> -	u64 skip_space;
>  	u64 type;
>  	u64 avail_space;
>  	u64 min_stripe_size;
> -	int min_stripes = 1, num_stripes = 1;
> +	int num_stripes = 1;
>  	int i = 0, nr_devices;
>  
>  	/*
> @@ -1957,28 +1956,21 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  		avail_space = device->total_bytes - device->bytes_used;
>  
>  		/* align with stripe_len */
> -		avail_space = div_u64(avail_space, BTRFS_STRIPE_LEN);
> -		avail_space *= BTRFS_STRIPE_LEN;
> +		avail_space = rounddown(avail_space, BTRFS_STRIPE_LEN);

As long as the stripe length is a constant, this is fine. rounddown uses
% (modulo) so this can be come division that will not work for u64
types.

>  
>  		/*
>  		 * In order to avoid overwriting the superblock on the drive,
>  		 * btrfs starts at an offset of at least 1MB when doing chunk
>  		 * allocation.
> +		 *
> +		 * This ensures we have at least min_stripe_size free space
> +		 * after excluding 1mb.
>  		 */
> -		skip_space = SZ_1M;
> -
> -		/*
> -		 * we can use the free space in [0, skip_space - 1], subtract
> -		 * it from the total.
> -		 */
> -		if (avail_space && avail_space >= skip_space)
> -			avail_space -= skip_space;
> -		else
> -			avail_space = 0;
> -
> -		if (avail_space < min_stripe_size)
> +		if (avail_space <= SZ_1M + min_stripe_size)
>  			continue;
>  
> +		avail_space -= SZ_1M;
> +
>  		devices_info[i].dev = device;
>  		devices_info[i].max_avail = avail_space;
>  
> @@ -1992,9 +1984,8 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
>  
>  	i = nr_devices - 1;
>  	avail_space = 0;
> -	while (nr_devices >= min_stripes) {
> -		if (num_stripes > nr_devices)
> -			num_stripes = nr_devices;
> +	while (nr_devices >= rattr->devs_min) {
> +		num_stripes = min(num_stripes, nr_devices);
>  
>  		if (devices_info[i].max_avail >= min_stripe_size) {
>  			int j;

All of the above look good to me, feel free to send them as proper
patches.

Patch
diff mbox series

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index a813b582fa72..9286f9e49c0c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1902,7 +1902,7 @@  static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 	u64 type;
 	u64 avail_space;
 	u64 min_stripe_size;
-	int min_stripes = 1, num_stripes = 1;
+	int min_stripes, num_stripes = 1;
 	int i = 0, nr_devices;
 	const struct btrfs_raid_attr *rattr;
 
@@ -1930,14 +1930,12 @@  static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 	type = btrfs_data_alloc_profile(fs_info);
 	rattr = &btrfs_raid_array[btrfs_bg_flags_to_raid_index(type)];
 	ASSERT(rattr);
+	min_stripes = rattr->devs_min;
 	if (type & BTRFS_BLOCK_GROUP_RAID0) {
-		min_stripes = 2;
 		num_stripes = nr_devices;
 	} else if (type & BTRFS_BLOCK_GROUP_RAID1) {
-		min_stripes = 2;
 		num_stripes = 2;
 	} else if (type & BTRFS_BLOCK_GROUP_RAID10) {
-		min_stripes = 4;
 		num_stripes = 4;
 	}