John Groves <[email protected]> writes:
>
> [...snip...]
>
>>
>> I'm implementing something similar for guest_memfd and was going to
>> reuse __split_folio_to_order(). Would you consider using the
>> __split_folio_to_order() function?
>>
>> I see that dax_folio_reset_order() needs to set f->share to 0 though,
>> which is a union with index, and __split_folio_to_order() sets non-0
>> indices.
>>
>> Also, __split_folio_to_order() doesn't handle f->pgmap (or f->lru).
>>
>> Could these two steps be added to a separate loop after
>> __split_folio_to_order()?
>>
>> Does dax_folio_reset_order() need to handle any of the folio flags that
>> __split_folio_to_order() handles?
>
> Sorry to reply slowly; this took some thought.
>
No worries, thanks for your consideration!
> I'm nervous about sharing folio initialization code between the page cache
> and dax. Might this be something we could unify after the fact - if it
> passes muster?
>
> Unifying paths like this could be regression-prone (page cache changes
> breaking dax or vice versa) unless it's really well conceived...
>
guest_memfd's (future) usage of __split_folio_to_order() is probably
closer in spirit to the original usage of __split_folio_to_order() that
dax's, feel free go ahead :)
For guest_memfd, I do want to use __split_folio_to_order() since I do
want to make sure that any updates to page flags are taken into account
for guest_memfd as well.
>>
>> > static inline unsigned long dax_folio_put(struct folio *folio)
>> > {
>> > unsigned long ref;
>> > @@ -391,28 +430,13 @@ static inline unsigned long dax_folio_put(struct
>> > folio *folio)
>> > if (ref)
>> > return ref;
>> >
>> > - folio->mapping = NULL;
>> > - order = folio_order(folio);
>> > - if (!order)
>> > - return 0;
>> > - folio_reset_order(folio);
>> > + order = dax_folio_reset_order(folio);
>> >
>> > + /* Debug check: verify refcounts are zero for all sub-folios */
>> > for (i = 0; i < (1UL << order); i++) {
>> > - struct dev_pagemap *pgmap = page_pgmap(&folio->page);
>> > struct page *page = folio_page(folio, i);
>> > - struct folio *new_folio = (struct folio *)page;
>> >
>> > - ClearPageHead(page);
>> > - clear_compound_head(page);
>> > -
>> > - new_folio->mapping = NULL;
>> > - /*
>> > - * Reset pgmap which was over-written by
>> > - * prep_compound_page().
>> > - */
>>
>> Actually, where's the call to prep_compound_page()? Was that in
>> dax_folio_init()? Is this comment still valid and does pgmap have to be
>> reset?
>
> Yep, in dax_folio_init()...
>
On another look, prep_compound_tail() in prep_compound_page() is the
one that overwrites folio->pgmap, by writing to page->compound_head,
which aliases with pgmap.
No issues here. I was just comparing the before/after of this
refactoring and saw that the comment was dropped, which led me to look
more at this part.
Reviewed-by: Ackerley Tng <[email protected]>
>
> Thanks,
> John
>
> [snip]
