Re: [PATCH 00/48] Folios for 5.17

[Matthew Wilcox <willy@infradead.org>]

Sorry I missed this while travelling.

On Sun, Dec 26, 2021 at 10:26:23PM +0000, William Kucharski wrote:
> Consolidated multiple review comments into one email, the majority are nits at
> best:
> 
> [PATCH 04/48]:
> 
> An obnoxiously pendantic English grammar nit:
> 
> + * lock).  This can also be called from mark_buffer_dirty(), which I
> 
> The period should be inside the paren, e.g.: "lock.)"

That's at least debatable.  The full context here is:

 [...] A few have the folio blocked from truncation through other
+ * means (eg zap_page_range() has it mapped and is holding the page table
+ * lock).

According to AP Style, the period goes outside the paren in this case:
https://blog.apastyle.org/apastyle/2013/03/punctuation-junction-periods-and-parentheses.html

I'm sure you can find an authority to support always placing a period
inside a paren, but we don't have a controlling authority for how to
punctuate our documentation.  I'm great fun at parties when I get going
on the subject of the Oxford comma.

> [PATCH 05/48]:
> 
> +       unsigned char aux[3];
> 
> I'd like to see an explanation of why this is "3."

I got rid of it ... for now ;-)

> +static inline void folio_batch_init(struct folio_batch *fbatch)
> +{
> +       fbatch->nr = 0;
> +}
> +
> +static inline unsigned int folio_batch_count(struct folio_batch *fbatch)
> +{
> +       return fbatch->nr;
> +}
> +
> +static inline unsigned int fbatch_space(struct folio_batch *fbatch)
> +{
> +       return PAGEVEC_SIZE - fbatch->nr;
> +}
> +
> +/**
> + * folio_batch_add() - Add a folio to a batch.
> + * @fbatch: The folio batch.
> + * @folio: The folio to add.
> + *
> + * The folio is added to the end of the batch.
> + * The batch must have previously been initialised using folio_batch_init().
> + *
> + * Return: The number of slots still available.
> + */
> +static inline unsigned folio_batch_add(struct folio_batch *fbatch,
> +               struct folio *folio)
> +{
> +       fbatch->folios[fbatch->nr++] = folio;
> 
> Is there any need to validate fbatch in these inlines?

I don't think so?  At least, there's no validation for the pagevec
equivalents.  I'd be open to adding something cheap if it's likely to
catch a bug someone's likely to introduce.

> [PATCH 07/48]:
> 
> +       xas_for_each(&xas, folio, ULONG_MAX) {                  \
>                 unsigned left;                                  \
> -               if (xas_retry(&xas, head))                      \
> +               size_t offset = offset_in_folio(folio, start + __off);  \
> +               if (xas_retry(&xas, folio))                     \
>                         continue;                               \
> -               if (WARN_ON(xa_is_value(head)))                 \
> +               if (WARN_ON(xa_is_value(folio)))                \
>                         break;                                  \
> -               if (WARN_ON(PageHuge(head)))                    \
> +               if (WARN_ON(folio_test_hugetlb(folio)))         \
>                         break;                                  \
> -               for (j = (head->index < index) ? index - head->index : 0; \
> -                    j < thp_nr_pages(head); j++) {             \
> -                       void *kaddr = kmap_local_page(head + j);        \
> -                       base = kaddr + offset;                  \
> -                       len = PAGE_SIZE - offset;               \
> +               while (offset < folio_size(folio)) {            \
> 
> Since offset is not actually used until after a bunch of error conditions
> may exit or restart the loop, and isn't used at all in between, defer
> its calculation until just before its first use in the "while."

Hmm.  Those conditions aren't likely to occur, but ... now that you
mention it, checking xa_is_value() after using folio as if it's not a
value is Wrong.  So I'm going to fold in this:

@@ -78,13 +78,14 @@
        rcu_read_lock();                                        \
        xas_for_each(&xas, folio, ULONG_MAX) {                  \
                unsigned left;                                  \
-               size_t offset = offset_in_folio(folio, start + __off);  \
+               size_t offset;                                  \
                if (xas_retry(&xas, folio))                     \
                        continue;                               \
                if (WARN_ON(xa_is_value(folio)))                \
                        break;                                  \
                if (WARN_ON(folio_test_hugetlb(folio)))         \
                        break;                                  \
+               offset = offset_in_folio(folio, start + __off); \
                while (offset < folio_size(folio)) {            \
                        base = kmap_local_folio(folio, offset); \
                        len = min(n, len);                      \

> Reviewed-by: William Kucharski <william.kucharski@oracle.com>

Thanks.  I'll go through and add that in, then push again.