[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 12 Dec 2022 22:41:52 +0800
From: Chao Yu <chao@...nel.org>
To: Vishal Moola <vishal.moola@...il.com>
Cc: linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net, linux-mm@...ck.org,
fengnan chang <fengnanchang@...il.com>,
linux-fsdevel@...r.kernel.org
Subject: Re: [f2fs-dev] [PATCH v3 14/23] f2fs: Convert
f2fs_write_cache_pages() to use filemap_get_folios_tag()
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@...il.com> wrote:
>>
>> On Mon, Nov 14, 2022 at 1:38 PM Vishal Moola <vishal.moola@...il.com> wrote:
>>>
>>> On Sun, Nov 13, 2022 at 11:02 PM Chao Yu <chao@...nel.org> 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.
Powered by blists - more mailing lists