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.
