Re: [PATCH] mm: Add memcpy_from_file_folio()

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 26 Jan 2023 20:15:52 +0000 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:

> This is the equivalent of memcpy_from_page().  It differs in that it
> takes the position in a file instead of offset in a folio, it accepts
> the total number of bytes to be copied (instead of the number of bytes
> to be copied from this folio) and it returns how many bytes were copied
> from the folio, rather than making the caller calculate that and then
> checking if the caller got it right.
> 
> ...
>
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -413,6 +413,35 @@ static inline void memzero_page(struct page *page, size_t offset, size_t len)
>  	kunmap_local(addr);
>  }
>  
> +/**
> + * memcpy_from_file_folio - Copy some bytes from a file folio.
> + * @to: The destination buffer.
> + * @folio: The folio to copy from.
> + * @pos: The position in the file.
> + * @len: The maximum number of bytes to copy.
> + *
> + * Copy up to @len bytes from this folio.  This may be limited by PAGE_SIZE
> + * if the folio comes from HIGHMEM, and by the size of the folio.
> + *
> + * 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);
      |                       ^~~

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

--- 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;
 
-	memcpy(to, from, len);
+	memcpy(to, from, min(len, remaining));
 	kunmap_local(from);
 
 	return len;
_