Re: [RFC PATCH v1 04/10] lib/freader: support reading more than 2 folios

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 10/3/25 19:16, Andrii Nakryiko wrote:
> On Fri, Oct 3, 2025 at 9:04 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> freader_fetch currently reads from at most two folios. When a read spans
>> into a third folio, the overflow bytes are copied adjacent to the second
>> folio’s data instead of being handled as a separate folio.
>> This patch modifies fetch algorithm to support reading from many folios.
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>>   lib/freader.c | 27 ++++++++++++++++-----------
>>   1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/freader.c b/lib/freader.c
>> index 32a17d137b32..f73b594a137d 100644
>> --- a/lib/freader.c
>> +++ b/lib/freader.c
>> @@ -105,17 +105,22 @@ const void *freader_fetch(struct freader *r, loff_t file_off, size_t sz)
>>          folio_sz = folio_size(r->folio);
>>          if (file_off + sz > r->folio_off + folio_sz) {
>>                  int part_sz = r->folio_off + folio_sz - file_off;
> AI suggests this should be size_t or u64, it's not strictly necessary,
> probably better to have all the offsets and sizes of the same bitness
>
>> -
>> -               /* copy the part that resides in the current folio */
>> -               memcpy(r->buf, r->addr + (file_off - r->folio_off), part_sz);
>> -
>> -               /* fetch next folio */
>> -               r->err = freader_get_folio(r, r->folio_off + folio_sz);
>> -               if (r->err)
>> -                       return NULL;
>> -
>> -               /* copy the rest of requested data */
>> -               memcpy(r->buf + part_sz, r->addr, sz - part_sz);
>> +               size_t dst_off = 0, src_off = file_off - r->folio_off;
>> +
>> +               do {
>> +                       memcpy(r->buf + dst_off, r->addr + src_off, part_sz);
>> +                       sz -= part_sz;
>> +                       if (sz == 0)
>> +                               break;
>> +                       /* fetch next folio */
>> +                       r->err = freader_get_folio(r, r->folio_off + folio_sz);
>> +                       if (r->err)
>> +                               return NULL;
>> +                       folio_sz = folio_size(r->folio);
>> +                       src_off = 0; /* read from the beginning, starting second folio */
>> +                       dst_off += part_sz;
>> +                       part_sz = min_t(u64, sz, folio_sz);
>> +               } while (sz);
> it's a bit sloppy that we have sz check twice, what if we rewrite it a bit
>
> u64 part_sz = r->folio_off + folio_size(r->folio) - file_off, off;
>
> /* copy the part that resides in the first folio */
> memcpy(r->buf, r->addr + (file_off - r->folio_off), part_sz);
> off = part_sz;
>
> while (off < sz) {
>      /* fetch next folio */
>      r->err = freader_get_folio(r, file_off + off);
>      if (r->err)
>          return NULL;
>
>      part_sz = min(u64, file_off + sz - r->folio_off, folio_ssize(r->folio));
>      memcpy(r->buf + off, r->addr, part_sz);
>
>      off += part_sz;
> }
That'll do, the choice is to check size twice or memcpy twice.
Let's do as you suggest, also helps dropping src_off variable.
>
>>                  return r->buf;
>>          }
>> --
>> 2.51.0
>>