Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Fri, 15 Sep 2023 12:03:23 +0200 (CEST)
From:   Geert Uytterhoeven <geert@linux-m68k.org>
To:     David Sterba <dsterba@suse.cz>
cc:     Johannes Thumshirn <johannes.thumshirn@wdc.com>,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>,
        Christoph Hellwig <hch@lst.de>,
        Naohiro Aota <naohiro.aota@wdc.com>, Qu Wenruo <wqu@suse.com>,
        Damien Le Moal <dlemoal@kernel.org>,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 03/11] btrfs: add support for inserting raid stripe
 extents
In-Reply-To: <20230914180701.GB20408@twin.jikos.cz>
Message-ID: <dd759994-678f-1d2c-a33e-6320b8ac4c6c@linux-m68k.org>
References: <20230914-raid-stripe-tree-v9-0-15d423829637@wdc.com> <20230914-raid-stripe-tree-v9-3-15d423829637@wdc.com> <20230914180701.GB20408@twin.jikos.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Precedence: bulk

 	Hi David,

On Thu, 14 Sep 2023, David Sterba wrote:
> On Thu, Sep 14, 2023 at 09:06:58AM -0700, Johannes Thumshirn wrote:
>> Add support for inserting stripe extents into the raid stripe tree on
>> completion of every write that needs an extra logical-to-physical
>> translation when using RAID.
>>
>> Inserting the stripe extents happens after the data I/O has completed,
>> this is done to a) support zone-append and b) rule out the possibility of
>> a RAID-write-hole.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

>> --- /dev/null
>> +++ b/fs/btrfs/raid-stripe-tree.c
>> +static int btrfs_insert_striped_mirrored_raid_extents(
>> +				      struct btrfs_trans_handle *trans,
>> +				      struct btrfs_ordered_extent *ordered,
>> +				      u64 map_type)
>> +{
>> +	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;
>
> This will probably warn due to u64/u32 division.

Worse, it causes link failures in linux-next, as e.g. reported by
noreply@ellerman.id.au:

     ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!

So despite being aware of the issue, you still queued it?

The use of "int" for almost all variables is also a red flag:
   - list_count_nodes() returns size_t,
   - btrfs_bg_flags_to_raid_index() returns an enum.
   - btrfs_raid_array[index].sub_stripes is u8,
   - The result of the division may not fit in 32-bit.

Thanks for fixing, soon! ;-)

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds