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@xxxxxxxxxx> Thanks. I'll go through and add that in, then push again.
