On Mon, Feb 23, 2026 at 7:41 AM David Hildenbrand (Arm) <david@kernel.org>
wrote:

> On 2/23/26 15:24, David Hildenbrand (Arm) wrote:
> > On 2/12/26 21:26, Nico Pache wrote:
> >> On Thu, Feb 12, 2026 at 1:04 PM David Hildenbrand (Arm)
> >> <david@kernel.org> wrote:
> >>>
> >>>
> >>> Having triggered_wb set where writeback is not actually triggered is
> >>> suboptimal.
> >>
> >> It took me a second to figure out what you were referring to, but I
> >> see it now. if we return SCAN_PAGE_D_OR_WB but the can_writeback fails
> >> it still retries.
> >>
> >> Would be an appropriate solution if can_writeback fails to modify the
> >> return value.
> >> ie)
> >>
> >> if (!cc->is_khugepaged && result == SCAN_PAGE_DIRTY_OR_WRITEBACK) {
> >>      if (mapping_can_writeback(file->f_mapping)) {
> >>         const loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
> >>         const loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
> >>
> >>         filemap_write_and_wait_range(file->f_mapping, lstart, lend);
> >>      } else {
> >>         result = SCAN_(SOMETHING?)
> >>      }
> >> }
> >> fput(file);
> >>
> >> we dont have a enum that fits this description but we want one that
> >> will continue.
> >
> > I stared at the patch and possible ways to change it, but I wondered
> > whether this refactoring is really the right approach.
> >
> > The whole mmap locking just makes this all very weird.
> >
> > Let me think about it some more.
> >
>
> As we don't really need to re-grab the MM lock, I think we can just do:
>

Yes this looks much better thank you!

Cheers,
-- Nico


>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 844a8a27845e..d546ff173833 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2395,6 +2395,7 @@ static enum scan_result collapse_single_pmd(unsigned
> long addr,
>                  struct collapse_control *cc)
>   {
>          struct mm_struct *mm = vma->vm_mm;
> +       bool triggered_wb = false;
>          enum scan_result result;
>          struct file *file;
>          pgoff_t pgoff;
> @@ -2409,14 +2410,22 @@ static enum scan_result
> collapse_single_pmd(unsigned long addr,
>
>          mmap_read_unlock(mm);
>          *mmap_locked = false;
> +
> +retry:
>          result = collapse_scan_file(mm, addr, file, pgoff, cc);
>
> +       /*
> +        * For MADV_COLLAPSE, try to writeback once to retry the collapse
> +        * immediately.
> +        */
>          if (!cc->is_khugepaged && result == SCAN_PAGE_DIRTY_OR_WRITEBACK
> &&
> -           mapping_can_writeback(file->f_mapping)) {
> +           !triggered_wb && mapping_can_writeback(file->f_mapping)) {
>                  const loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
>                  const loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
>
>                  filemap_write_and_wait_range(file->f_mapping, lstart,
> lend);
> +               triggered_wb = true;
> +               goto retry;
>          }
>          fput(file);
>
> @@ -2814,9 +2823,7 @@ int madvise_collapse(struct vm_area_struct *vma,
> unsigned long start,
>
>          for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) {
>                  enum scan_result result = SCAN_FAIL;
> -               bool triggered_wb = false;
>
> -retry:
>                  if (!mmap_locked) {
>                          cond_resched();
>                          mmap_read_lock(mm);
> @@ -2838,11 +2845,6 @@ int madvise_collapse(struct vm_area_struct *vma,
> unsigned long start,
>                  if (!mmap_locked)
>                          *lock_dropped = true;
>
> -               if (result == SCAN_PAGE_DIRTY_OR_WRITEBACK &&
> !triggered_wb) {
> -                       triggered_wb = true;
> -                       goto retry;
> -               }
> -
>                  switch (result) {
>                  case SCAN_SUCCEED:
>                  case SCAN_PMD_MAPPED:
>
>
> --
> Cheers,
>
> David
>
>