Re: [PATCH] mm: Add memcpy_from_file_folio()

[Matthew Wilcox <willy@infradead.org>]

On Thu, Jan 26, 2023 at 03:41:52PM -0800, Andrew Morton wrote:
> > + * Return: The number of bytes copied from the folio.

> > +static inline size_t memcpy_from_file_folio(char *to, struct folio *folio,
> > +		loff_t pos, size_t len)
> > +{
> > +	size_t offset = offset_in_folio(folio, pos);
> > +	char *from = kmap_local_folio(folio, offset);
> > +
> > +	if (folio_test_highmem(folio))
> > +		len = min(len, PAGE_SIZE - offset);
> > +	else
> > +		len = min(len, folio_size(folio) - offset);
> 
> min() blows up on arm allnoconfig.
> 
> ./include/linux/highmem.h: In function 'memcpy_from_file_folio':
> ./include/linux/minmax.h:20:35: warning: comparison of distinct pointer types lacks a cast
>    20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>       |                                   ^~
> ./include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
>    26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
>       |                  ^~~~~~~~~~~
> ./include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
>    36 |         __builtin_choose_expr(__safe_cmp(x, y), \
>       |                               ^~~~~~~~~~
> ./include/linux/minmax.h:67:25: note: in expansion of macro '__careful_cmp'
>    67 | #define min(x, y)       __careful_cmp(x, y, <)
>       |                         ^~~~~~~~~~~~~
> ./include/linux/highmem.h:435:23: note: in expansion of macro 'min'
>   435 |                 len = min(len, PAGE_SIZE - offset);
>       |                       ^~~

Oh, right, PAGE_SIZE is size_t everywhere except on ARM.

> We could use min_t(), but perhaps and explanatorialy named variable is
> nicer?

But buggy because we return the number of bytes copied.

> --- a/include/linux/highmem.h~mm-add-memcpy_from_file_folio-fix
> +++ a/include/linux/highmem.h
> @@ -430,13 +430,14 @@ static inline size_t memcpy_from_file_fo
>  {
>  	size_t offset = offset_in_folio(folio, pos);
>  	char *from = kmap_local_folio(folio, offset);
> +	size_t remaining;
>  
>  	if (folio_test_highmem(folio))
> -		len = min(len, PAGE_SIZE - offset);
> +		remaining = PAGE_SIZE - offset;
>  	else
> -		len = min(len, folio_size(folio) - offset);
> +		remaining = folio_size(folio) - offset;

I don't think remaining is a great name for this.  The key thing is
that for any platform we care about folio_test_highmem() is constant false,
so this just optimises away the min() call.

You could salvage this approach by doing

	len = min(remaining, len);
	memcpy(to, from, len);
	return len;

but I think it's probably better to just do min_t on the PAGE_SIZE line.
Stupid ARM.

> -	memcpy(to, from, len);
> +	memcpy(to, from, min(len, remaining));
>  	kunmap_local(from);
>  
>  	return len;
> _
>