Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert

From: Sedat Dilek
Date: Sat Jun 27 2020 - 06:31:48 EST

On Thu, Jun 25, 2020 at 11:52 AM Vasily Averin <vvs@xxxxxxxxxxxxx> wrote:
>
> In current implementation fuse_writepages_fill() tries to share the code:
> for new wpa it calls tree_insert() with num_pages = 0
> then switches to common code used non-modified num_pages
> and increments it at the very end.
>
> Though it triggers WARN_ON(!wpa->ia.ap.num_pages) in tree_insert()
> WARNING: CPU: 1 PID: 17211 at fs/fuse/file.c:1728 tree_insert+0xab/0xc0 [fuse]
> RIP: 0010:tree_insert+0xab/0xc0 [fuse]
> Call Trace:
> fuse_writepages_fill+0x5da/0x6a0 [fuse]
> write_cache_pages+0x171/0x470
> fuse_writepages+0x8a/0x100 [fuse]
> do_writepages+0x43/0xe0
>
> This patch re-works fuse_writepages_fill() to call tree_insert()
> with num_pages = 1 and avoids its subsequent increment and
> an extra spin_lock(&fi->lock) for newly added wpa.
>
> Fixes: 6b2fb79963fb ("fuse: optimize writepages search")

Hi Vasily,

I have cherry-picked commit 6b2fb79963fb ("fuse: optimize writepages
search") on top of Linux v5.7.

Tested against Linux v5.7.6 with your triple patchset together (I
guess the triple belongs together?):

$ git log --oneline v5.7..
0b26115de7aa (HEAD -> for-5.7/fuse-writepages-optimization-vvs)
fuse_writepages ignores errors from fuse_writepages_fill
687be6184c30 fuse_writepages_fill: simplified "if-else if" constuction
8d8e2e5d90c0 fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert
cd4e568ca924 (for-5.7/fuse-writepages-optimization) fuse: optimize
writepages search

Unsure if your single patches should be labeled with:

"fuse:" or "fuse: writepages:" or "fuse: writepages_fill:"

It is common to use present tense not past tense in the subject line.
I found one typo in one subject line.

Example (understand this as suggestions):
1/3: fuse: writepages: Avoid WARN_ON in tree_insert in fuse_writepages_fill
2/3: fuse: writepages: Simplif*y* "if-else if" const*r*uction
3/3: fuse: writepages: Ignore errors from fuse_writepages_fill

Unsure how to test your patchset.
My usecase with fuse is to mount and read from the root.disk (loop,
ext4) of a WUBI-installation of Ubuntu/precise 12.04-LTS.

root@iniza# mount -r -t auto /dev/sda2 /mnt/win7
root@iniza# cd /path/to/root.disk
root@iniza# mount -r -t ext4 -o loop ./root.disk /mnt/ubuntu

BTW, your patchset is bullet-proof with Clang version 11.0.0-git IAS
(Integrated Assembler).

If you send a v2 please add my:

Tested-by: Sedat Dilek <sedat.dilek@xxxxxxxxx> # build+boot; Linux
v5.7.6 with clang-11 (IAS)

Can you send a (git) cover-letter if this is a patchset - next time?

Thanks.

Regards,
- Sedat -




> Reported-by: kernel test robot <rong.a.chen@xxxxxxxxx>
> Signed-off-by: Vasily Averin <vvs@xxxxxxxxxxxxx>
> ---
> fs/fuse/file.c | 56 +++++++++++++++++++++++++++++---------------------------
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index e573b0c..cf267bd 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1966,10 +1966,9 @@ static bool fuse_writepage_in_flight(struct fuse_writepage_args *new_wpa,
> struct fuse_writepage_args *old_wpa;
> struct fuse_args_pages *new_ap = &new_wpa->ia.ap;
>
> - WARN_ON(new_ap->num_pages != 0);
> + WARN_ON(new_ap->num_pages != 1);
>
> spin_lock(&fi->lock);
> - rb_erase(&new_wpa->writepages_entry, &fi->writepages);
> old_wpa = fuse_find_writeback(fi, page->index, page->index);
> if (!old_wpa) {
> tree_insert(&fi->writepages, new_wpa);
> @@ -1977,7 +1976,6 @@ static bool fuse_writepage_in_flight(struct fuse_writepage_args *new_wpa,
> return false;
> }
>
> - new_ap->num_pages = 1;
> for (tmp = old_wpa->next; tmp; tmp = tmp->next) {
> pgoff_t curr_index;
>
> @@ -2020,7 +2018,7 @@ static int fuse_writepages_fill(struct page *page,
> struct fuse_conn *fc = get_fuse_conn(inode);
> struct page *tmp_page;
> bool is_writeback;
> - int err;
> + int index, err;
>
> if (!data->ff) {
> err = -EIO;
> @@ -2083,44 +2081,48 @@ static int fuse_writepages_fill(struct page *page,
> wpa->next = NULL;
> ap->args.in_pages = true;
> ap->args.end = fuse_writepage_end;
> - ap->num_pages = 0;
> + ap->num_pages = 1;
> wpa->inode = inode;
> -
> - spin_lock(&fi->lock);
> - tree_insert(&fi->writepages, wpa);
> - spin_unlock(&fi->lock);
> -
> + index = 0;
> data->wpa = wpa;
> + } else {
> + index = ap->num_pages;
> }
> set_page_writeback(page);
>
> copy_highpage(tmp_page, page);
> - ap->pages[ap->num_pages] = tmp_page;
> - ap->descs[ap->num_pages].offset = 0;
> - ap->descs[ap->num_pages].length = PAGE_SIZE;
> + ap->pages[index] = tmp_page;
> + ap->descs[index].offset = 0;
> + ap->descs[index].length = PAGE_SIZE;
>
> inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
> inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP);
>
> err = 0;
> - if (is_writeback && fuse_writepage_in_flight(wpa, page)) {
> - end_page_writeback(page);
> - data->wpa = NULL;
> - goto out_unlock;
> + if (is_writeback) {
> + if (fuse_writepage_in_flight(wpa, page)) {
> + end_page_writeback(page);
> + data->wpa = NULL;
> + goto out_unlock;
> + }
> + } else if (!index) {
> + spin_lock(&fi->lock);
> + tree_insert(&fi->writepages, wpa);
> + spin_unlock(&fi->lock);
> }
> - data->orig_pages[ap->num_pages] = page;
> -
> - /*
> - * Protected by fi->lock against concurrent access by
> - * fuse_page_is_writeback().
> - */
> - spin_lock(&fi->lock);
> - ap->num_pages++;
> - spin_unlock(&fi->lock);
> + data->orig_pages[index] = page;
>
> + if (index) {
> + /*
> + * Protected by fi->lock against concurrent access by
> + * fuse_page_is_writeback().
> + */
> + spin_lock(&fi->lock);
> + ap->num_pages++;
> + spin_unlock(&fi->lock);
> + }
> out_unlock:
> unlock_page(page);
> -
> return err;
> }
>
> --
> 1.8.3.1
>