Re: [f2fs-dev] [PATCH v3 14/23] f2fs: Convert f2fs_write_cache_pages() to use filemap_get_folios_tag()

[Chao Yu]

Hi Vishal,

Sorry for the delay reply.

On 2022/12/6 4:34, Vishal Moola wrote:
> On Tue, Nov 22, 2022 at 6:26 PM Vishal Moola <vishal.moola@xxxxxxxxx> wrote:
> > On Mon, Nov 14, 2022 at 1:38 PM Vishal Moola <vishal.moola@xxxxxxxxx> wrote:
> > > On Sun, Nov 13, 2022 at 11:02 PM Chao Yu <chao@xxxxxxxxxx> wrote:
> > > > On 2022/10/18 4:24, Vishal Moola (Oracle) wrote:
> > > > > Converted the function to use a folio_batch instead of pagevec. This is in
> > > > > preparation for the removal of find_get_pages_range_tag().
> > > > >
> > > > > Also modified f2fs_all_cluster_page_ready to take in a folio_batch instead
> > > > > of pagevec. This does NOT support large folios. The function currently
> > > >
> > > > Vishal,
> > > >
> > > > It looks this patch tries to revert Fengnan's change:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=01fc4b9a6ed8eacb64e5609bab7ac963e1c7e486
> > > >
> > > > How about doing some tests to evaluate its performance effect?
> > >
> > > Yeah I'll play around with it to see how much of a difference it makes.
> >
> > I did some testing. Looks like reverting Fengnan's change allows for
> > occasional, but significant, spikes in write latency. I'll work on a variation
> > of the patch that maintains the use of F2FS_ONSTACK_PAGES and send
> > that in the next version of the patch series. Thanks for pointing that out!
>
> Following Matthew's comment, I'm thinking we should go with this patch
> as is. The numbers between both variations did not have substantial
> differences with regard to latency.
>
> While the new variant would maintain the use of F2FS_ONSTACK_PAGES,
> the code becomes messier and would end up limiting the number of
> folios written back once large folio support is added. This means it would
> have to be converted down to this version later anyways.
>
> Does leaving this patch as is sound good to you?
>
> For reference, here's what the version continuing to use a page
> array of size F2FS_ONSTACK_PAGES would change:
>
> +               nr_pages = 0;
> +again:
> +               nr_folios = filemap_get_folios_tag(mapping, &index, end,
> +                               tag, &fbatch);
> +               if (nr_folios == 0) {
> +                       if (nr_pages)
> +                               goto write;
> +                               goto write;

Duplicated code.

>                          break;
> +               }
>
> +               for (i = 0; i < nr_folios; i++) {
> +                       struct folio* folio = fbatch.folios[i];
> +
> +                       idx = 0;
> +                       p = folio_nr_pages(folio);
> +add_more:
> +                       pages[nr_pages] = folio_page(folio,idx);
> +                       folio_ref_inc(folio);
> +                       if (++nr_pages == F2FS_ONSTACK_PAGES) {
> +                               index = folio->index + idx + 1;
> +                               folio_batch_release(&fbatch);
> +                               goto write;
> +                       }
> +                       if (++idx < p)
> +                               goto add_more;
> +               }
> +               folio_batch_release(&fbatch);
> +               goto again;
> +write:

Looks fine to me, can you please send a formal patch?

Thanks,

> > How do the remaining f2fs patches in the series look to you?
> > Patch 16/23 f2fs_sync_meta_pages() in particular seems like it may
> > be prone to problems. If there are any changes that need to be made to
> > it I can include those in the next version as well.
>
> Thanks for reviewing the patches so far. I wanted to follow up on asking
> for review of the last couple of patches.