Re: [PATCH 2/2] block: share more code for bio addition helpers

[Kundan Kumar <kundan.kumar@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

On 07/11/24 08:20AM, Christoph Hellwig wrote:
>On Wed, Nov 06, 2024 at 03:33:38PM +0530, Kundan Kumar wrote:
>>> -int bio_add_page(struct bio *bio, struct page *page,
>>> -		 unsigned int len, unsigned int offset)
>>> +static int bio_do_add_page(struct bio *bio, struct page *page,
>>> +		 unsigned int len, unsigned int offset, bool *same_page)
>>
>> As we are passing length within a folio, values will reach near UINT_MAX.
>> It will be better to make len and offset as size_t, also to add a check like :
>> if (len > UINT_MAX || offset > UINT_MAX)
>> 		return 0;
>
>Not sure what the point is.  IFF we get folio sizes overflowing an
>unsigned int we'll have a massive problem as it will overflow the
>bv_offset and bv_len fields.  So we'd need to address that first before
>doing anything else.

In my opinion, there isn't a current use case where it overflows, but
its better to include a check for the future. WARN_ON is also an option
as used in bio_add_folio_nofail()

>
>>> {
>>> -	bool same_page = false;
>>
>> nit: extra line got added
>
>Yes, the previous code was missing the empty line after the variable
>declaration, so this got fixed.
>

Actually the extra line got introduced at the beginning of function
definition of bio_do_add_page().

Otherwise the code looks good. 

Reviewed-by: Kundan Kumar <kundan.kumar@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]