Re: [PATCH 1/4] btrfs: fix 64bit division in btrfs_insert_striped_mirrored_raid_extents

[David Sterba]

On Tue, Sep 19, 2023 at 10:07:00AM +0930, Qu Wenruo wrote:
> On 2023/9/19 01:54, David Sterba wrote:
> > On Mon, Sep 18, 2023 at 03:03:10PM +0000, Johannes Thumshirn wrote:
> >> On 18.09.23 16:19, Geert Uytterhoeven wrote:
> >>> Hi Johannes,
> >>>
> >>> On Mon, Sep 18, 2023 at 4:14 PM Johannes Thumshirn
> >>> <johannes.thumshirn@xxxxxxx> wrote:
> >>>> Fix modpost error due to 64bit division on 32bit systems in
> >>>> btrfs_insert_striped_mirrored_raid_extents.
> >>>>
> >>>> Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> >>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> >>>
> >>> Thanks for your patch!
> >>>
> >>>> --- a/fs/btrfs/raid-stripe-tree.c
> >>>> +++ b/fs/btrfs/raid-stripe-tree.c
> >>>> @@ -148,10 +148,10 @@ static int btrfs_insert_striped_mirrored_raid_extents(
> >>>>    {
> >>>>           struct btrfs_io_context *bioc;
> >>>>           struct btrfs_io_context *rbioc;
> >>>> -       const int nstripes = list_count_nodes(&ordered->bioc_list);
> >>>> -       const int index = btrfs_bg_flags_to_raid_index(map_type);
> >>>> -       const int substripes = btrfs_raid_array[index].sub_stripes;
> >>>> -       const int max_stripes = trans->fs_info->fs_devices->rw_devices / substripes;
> >>>> +       const size_t nstripes = list_count_nodes(&ordered->bioc_list);
> >>>> +       const enum btrfs_raid_types index = btrfs_bg_flags_to_raid_index(map_type);
> >>>> +       const u8 substripes = btrfs_raid_array[index].sub_stripes;
> >>>> +       const int max_stripes = div_u64(trans->fs_info->fs_devices->rw_devices, substripes);
> >>>
> >>> What if the quotient does not fit in a signed 32-bit value?
> >>
> >> Then you've bought a lot of HDDs ;-)
> >>
> >> Jokes aside, yes this is theoretically correct. Dave can you fix
> >> max_stripes up to be u64 when applying?
> >
> > I think we can keep it int, or unsigned int if needed, we can't hit such
> > huge values for rw_devices. The 'theoretically' would fit for a machine
> > with infinite resources, otherwise the maximum number of devices I'd
> > expect is a few thousand.
> 
> In fact, we already have an check in btrfs_validate_super(), if the
> num_devices is over 1<<31, we would reject the fs.

No, it's just a warning in that case.

> I think we should be safe to further reduce the threshold.
> 
> U16_MAX sounds a valid and sane value to me.
> If no rejection I can send out a patch for this.
> 
> And later change internal rw_devices/num_devices to u16.

U16 does not make sense here, it's not a native int type on many
architectures and generates awkward assembly code. We use it in
justified cases where it's saving space in structures that are allocated
thousand times. The arbitrary limit 65536 is probably sane but not
much different than 1<<31, practically not hit and was useful to
note fuzzed superblocks.