Thanks. I like your changes and will defer to you on the period and Oxford comma. :-) > On Jan 2, 2022, at 18:27, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > 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. >
