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

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

[Vasily Averin]

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")
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 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

[PATCH] fuse_writepages_fill: simplified "if-else if" constuction

[Vasily Averin]

fuse_writepages_fill uses following construction:
if (wpa && ap->num_pages &&
    (A || B || C)) {
	action;
} else if (wpa && D) {
	if (E) {
		the same action;
	}
}

- ap->num_pages check is always true and can be removed
- "if" and "else if" calls the same action and can be merged.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/fuse/file.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index cf267bd..c023f7f0 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2035,17 +2035,14 @@ static int fuse_writepages_fill(struct page *page,
 	 */
 	is_writeback = fuse_page_is_writeback(inode, page->index);
 
-	if (wpa && ap->num_pages &&
+	if (wpa &&
 	    (is_writeback || ap->num_pages == fc->max_pages ||
 	     (ap->num_pages + 1) * PAGE_SIZE > fc->max_write ||
-	     data->orig_pages[ap->num_pages - 1]->index + 1 != page->index)) {
+	     (data->orig_pages[ap->num_pages - 1]->index + 1 != page->index) ||
+	     ((ap->num_pages == data->max_pages) &&
+				(!fuse_pages_realloc(data))))) {
 		fuse_writepages_send(data);
 		data->wpa = NULL;
-	} else if (wpa && ap->num_pages == data->max_pages) {
-		if (!fuse_pages_realloc(data)) {
-			fuse_writepages_send(data);
-			data->wpa = NULL;
-		}
 	}
 
 	err = -ENOMEM;
-- 
1.8.3.1

[PATCH] fuse_writepages ignores errors from fuse_writepages_fill

[Vasily Averin]

fuse_writepages() ignores some errors taken from fuse_writepages_fill()
I believe it is a bug: if .writepages is called with WB_SYNC_ALL
it should either guarantee that all data was successfully saved
or return error.

Fixes: 26d614df1da9 ("fuse: Implement writepages callback")
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/fuse/file.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index c023f7f0..5986739 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2148,10 +2148,8 @@ static int fuse_writepages(struct address_space *mapping,
 
 	err = write_cache_pages(mapping, wbc, fuse_writepages_fill, &data);
 	if (data.wpa) {
-		/* Ignore errors if we can write at least one page */
 		WARN_ON(!data.wpa->ia.ap.num_pages);
 		fuse_writepages_send(&data);
-		err = 0;
 	}
 	if (data.ff)
 		fuse_file_put(data.ff, false, false);
-- 
1.8.3.1

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

[Sedat Dilek]

On Thu, Jun 25, 2020 at 11:52 AM Vasily Averin <vvs@virtuozzo.com> 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@gmail.com> # 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@intel.com>
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  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
>

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

[Vivek Goyal]

On Thu, Jun 25, 2020 at 12:02:53PM +0300, Vasily Averin 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")
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

I think I am facing this issue with virtiofs. I am running podman which
mounts overlayfs over virtiofs (virtiofs uses fuse). While running podman
I am seeing tons of following warnings no console. So this needs to
be fixed in 5.8-rc.

[24908.795483] ------------[ cut here ]------------
[24908.795484] WARNING: CPU: 6 PID: 11376 at fs/fuse/file.c:1684 tree_insert+0xaf/0xc0
[24908.795484] Modules linked in: ip6table_nat ip6_tables xt_conntrack xt_MASQUERADE xt_comment iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 veth overlay dax_pmem_compat virtio_net device_dax dax_pmem_core net_failover joydev failover br_netfilter bridge drm stp llc virtiofs virtio_blk serio_raw nd_pmem nd_btt qemu_fw_cfg
[24908.795495] CPU: 6 PID: 11376 Comm: dnf Tainted: G        W         5.8.0-rc2+ #18
[24908.795496] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
[24908.795496] RIP: 0010:tree_insert+0xaf/0xc0
[24908.795497] Code: 80 c8 00 00 00 49 c7 80 d0 00 00 00 00 00 00 00 49 c7 80 d8 00 00 00 00 00 00 00 48 89 39 e9 a8 9a 1b 00 0f 0b eb a5 0f 0b c3 <0f> 0b e9 71 ff ff ff 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00
[24908.795497] RSP: 0018:ffffb17840717bc0 EFLAGS: 00010246
[24908.795498] RAX: 0000000000000000 RBX: ffffb17840717d20 RCX: 8c6318c6318c6319
[24908.795499] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9f6cc1a72ee0
[24908.795499] RBP: ffffe0dedfd6e640 R08: ffff9f6d1261c800 R09: ffffffffffffffff
[24908.795500] R10: ffff9f6cc1a72ee0 R11: 0000000000000000 R12: ffffe0dee05725c0
[24908.795500] R13: ffff9f6cc1a72a00 R14: ffff9f6cc1a72f90 R15: ffff9f6d1261c800
[24908.795501] FS:  00007f377b267740(0000) GS:ffff9f6d1fa00000(0000) knlGS:0000000000000000
[24908.795501] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[24908.795502] CR2: 00007f37777588e8 CR3: 0000000828a0e000 CR4: 00000000000006e0
[24908.795502] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[24908.795503] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[24908.795503] Call Trace:
[24908.795504]  fuse_writepages_fill+0x5b0/0x670
[24908.795504]  write_cache_pages+0x1c2/0x4b0
[24908.795504]  ? fuse_writepages+0x110/0x110
[24908.795505]  fuse_writepages+0x8d/0x110
[24908.795505]  do_writepages+0x34/0xc0
[24908.795506]  __filemap_fdatawrite_range+0xc5/0x100
[24908.795506]  filemap_write_and_wait_range+0x40/0xa0
[24908.795507]  remove_vma+0x31/0x70
[24908.795507]  __do_munmap+0x2d9/0x4a0
[24908.795507]  __vm_munmap+0x6a/0xc0
[24908.795508]  __x64_sys_munmap+0x28/0x30
[24908.795508]  do_syscall_64+0x52/0xb0
[24908.795509]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[24908.795509] RIP: 0033:0x7f377b81067b
[24908.795510] Code: Bad RIP value.
[24908.795510] RSP: 002b:00007ffd88f96fd8 EFLAGS: 00000246 ORIG_RAX: 000000000000000b
[24908.795511] RAX: ffffffffffffffda RBX: 0000559c662c79d0 RCX: 00007f377b81067b
[24908.795511] RDX: 0000559c66349720 RSI: 0000000000104000 RDI: 00007f37778da000
[24908.795512] RBP: 00007f37778da1e0 R08: 00007f3777894308 R09: 0000000000000001
[24908.795512] R10: 00000000000001a4 R11: 0000000000000246 R12: 0000000000000000
[24908.795513] R13: 0000559c65d843a0 R14: 00007f37778da000 R15: 0000000000000017
[24908.795515] irq event stamp: 3775373
[24908.795515] hardirqs last  enabled at (3775373): [<ffffffffa72f6a21>] page_outside_zone_boundaries+0xd1/0x100
[24908.795516] hardirqs last disabled at (3775372): [<ffffffffa72f698e>] page_outside_zone_boundaries+0x3e/0x100
[24908.795517] softirqs last  enabled at (3775348): [<ffffffffa7e0035d>] __do_softirq+0x35d/0x400
[24908.795517] softirqs last disabled at (3775341): [<ffffffffa7c01052>] asm_call_on_stack+0x12/0x20
[24908.795518] ---[ end trace f23c513c015212d2 ]---

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

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <vvs@virtuozzo.com> 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.

Looks good.  However, I don't like the way fuse_writepage_in_flight()
is silently changed to insert page into the rb_tree.  Also the
insertion can be merged with the search for in-flight and be done
unconditionally to simplify the logic.  See attached patch.

Thanks,
Miklos

[-- Attachment #2: fuse-fix-warning-in-tree_insert.patch --]
[-- Type: application/x-patch, Size: 4142 bytes --]

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

[Vasily Averin]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

On 7/11/20 7:01 AM, Miklos Szeredi wrote:
> On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <vvs@virtuozzo.com> 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.
> 
> Looks good.  However, I don't like the way fuse_writepage_in_flight()
> is silently changed to insert page into the rb_tree.  Also the
> insertion can be merged with the search for in-flight and be done
> unconditionally to simplify the logic.  See attached patch.

Your patch looks correct for me except 2 things:
1) you have lost "data->wpa = NULL;" when fuse_writepage_add() returns false.
2) in the same case old code did not set data->orig_pages[ap->num_pages] = page;

I've lightly updated your patch to fix noticed problems, please see attached patch.

Thank you,
	Vasily Averin

[-- Attachment #2: vvs.fuse-fix-warning-in-tree_insert.patch --]
[-- Type: text/x-patch, Size: 4057 bytes --]

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index e573b0cd2737..57721570c005 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1674,7 +1674,8 @@ __acquires(fi->lock)
 	}
 }
 
-static void tree_insert(struct rb_root *root, struct fuse_writepage_args *wpa)
+static struct fuse_writepage_args *fuse_insert_writeback(struct rb_root *root,
+						struct fuse_writepage_args *wpa)
 {
 	pgoff_t idx_from = wpa->ia.write.in.offset >> PAGE_SHIFT;
 	pgoff_t idx_to = idx_from + wpa->ia.ap.num_pages - 1;
@@ -1697,11 +1698,17 @@ static void tree_insert(struct rb_root *root, struct fuse_writepage_args *wpa)
 		else if (idx_to < curr_index)
 			p = &(*p)->rb_left;
 		else
-			return (void) WARN_ON(true);
+			return curr;
 	}
 
 	rb_link_node(&wpa->writepages_entry, parent, p);
 	rb_insert_color(&wpa->writepages_entry, root);
+	return NULL;
+}
+
+static void tree_insert(struct rb_root *root, struct fuse_writepage_args *wpa)
+{
+	WARN_ON(fuse_insert_writeback(root, wpa));
 }
 
 static void fuse_writepage_end(struct fuse_conn *fc, struct fuse_args *args,
@@ -1952,14 +1959,14 @@ static void fuse_writepages_send(struct fuse_fill_wb_data *data)
 }
 
 /*
- * First recheck under fi->lock if the offending offset is still under
- * writeback.  If yes, then iterate auxiliary write requests, to see if there's
+ * Check under fi->lock if the page is under writeback, and insert it onto the
+ * rb_tree if not. Otherwise iterate auxiliary write requests, to see if there's
  * one already added for a page at this offset.  If there's none, then insert
  * this new request onto the auxiliary list, otherwise reuse the existing one by
- * copying the new page contents over to the old temporary page.
+ * swapping the new temp page with the old one.
  */
-static bool fuse_writepage_in_flight(struct fuse_writepage_args *new_wpa,
-				     struct page *page)
+static bool fuse_writepage_add(struct fuse_writepage_args *new_wpa,
+			       struct page *page)
 {
 	struct fuse_inode *fi = get_fuse_inode(new_wpa->inode);
 	struct fuse_writepage_args *tmp;
@@ -1967,17 +1974,15 @@ static bool fuse_writepage_in_flight(struct fuse_writepage_args *new_wpa,
 	struct fuse_args_pages *new_ap = &new_wpa->ia.ap;
 
 	WARN_ON(new_ap->num_pages != 0);
+	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);
+	old_wpa = fuse_insert_writeback(&fi->writepages, new_wpa);
 	if (!old_wpa) {
-		tree_insert(&fi->writepages, new_wpa);
 		spin_unlock(&fi->lock);
-		return false;
+		return true;
 	}
 
-	new_ap->num_pages = 1;
 	for (tmp = old_wpa->next; tmp; tmp = tmp->next) {
 		pgoff_t curr_index;
 
@@ -2006,7 +2011,7 @@ static bool fuse_writepage_in_flight(struct fuse_writepage_args *new_wpa,
 		fuse_writepage_free(new_wpa);
 	}
 
-	return true;
+	return false;
 }
 
 static int fuse_writepages_fill(struct page *page,
@@ -2085,12 +2090,6 @@ static int fuse_writepages_fill(struct page *page,
 		ap->args.end = fuse_writepage_end;
 		ap->num_pages = 0;
 		wpa->inode = inode;
-
-		spin_lock(&fi->lock);
-		tree_insert(&fi->writepages, wpa);
-		spin_unlock(&fi->lock);
-
-		data->wpa = wpa;
 	}
 	set_page_writeback(page);
 
@@ -2103,20 +2102,22 @@ static int fuse_writepages_fill(struct page *page,
 	inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP);
 
 	err = 0;
-	if (is_writeback && fuse_writepage_in_flight(wpa, page)) {
+	if (data->wpa) {
+		/*
+		 * Protected by fi->lock against concurrent access by
+		 * fuse_page_is_writeback().
+		 */
+		spin_lock(&fi->lock);
+		ap->num_pages++;
+		spin_unlock(&fi->lock);
+	} else if (fuse_writepage_add(wpa, page)) {
+		data->wpa = wpa;
+	} else {
 		end_page_writeback(page);
 		data->wpa = NULL;
 		goto out_unlock;
 	}
-	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[ap->num_pages-1] = page;
 
 out_unlock:
 	unlock_page(page);

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

[Miklos Szeredi]

On Mon, Jul 13, 2020 at 10:02 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> On 7/11/20 7:01 AM, Miklos Szeredi wrote:
> > On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <vvs@virtuozzo.com> 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.
> >
> > Looks good.  However, I don't like the way fuse_writepage_in_flight()
> > is silently changed to insert page into the rb_tree.  Also the
> > insertion can be merged with the search for in-flight and be done
> > unconditionally to simplify the logic.  See attached patch.
>
> Your patch looks correct for me except 2 things:

Thanks for reviewing.

> 1) you have lost "data->wpa = NULL;" when fuse_writepage_add() returns false.

This is intentional, because this is in the !data->wpa branch.

> 2) in the same case old code did not set data->orig_pages[ap->num_pages] = page;

That is also intentional, in this case the origi_pages[0] is either
overwritten with the next page or discarded due to data->wpa being
NULL.

I'll write these up in the patch header.

Thanks,
Miklos

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

[Vasily Averin]

On 7/13/20 7:14 PM, Miklos Szeredi wrote:
> On Mon, Jul 13, 2020 at 10:02 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>>
>> On 7/11/20 7:01 AM, Miklos Szeredi wrote:
>>> On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <vvs@virtuozzo.com> 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.
>>>
>>> Looks good.  However, I don't like the way fuse_writepage_in_flight()
>>> is silently changed to insert page into the rb_tree.  Also the
>>> insertion can be merged with the search for in-flight and be done
>>> unconditionally to simplify the logic.  See attached patch.
>>
>> Your patch looks correct for me except 2 things:
> 
> Thanks for reviewing.
> 
>> 1) you have lost "data->wpa = NULL;" when fuse_writepage_add() returns false.
> 
> This is intentional, because this is in the !data->wpa branch.

Agree, I was wrong here.

>> 2) in the same case old code did not set data->orig_pages[ap->num_pages] = page;
> 
> That is also intentional, in this case the origi_pages[0] is either
> overwritten with the next page or discarded due to data->wpa being
> NULL.

Got it, agree, it should not be a problem.

> I'll write these up in the patch header.

Thank you,
	Vasily Averin

Re: [PATCH] fuse_writepages_fill: simplified "if-else if" constuction

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On Thu, Jun 25, 2020 at 11:30 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> fuse_writepages_fill uses following construction:
> if (wpa && ap->num_pages &&
>     (A || B || C)) {
>         action;
> } else if (wpa && D) {
>         if (E) {
>                 the same action;
>         }
> }
>
> - ap->num_pages check is always true and can be removed
> - "if" and "else if" calls the same action and can be merged.

Makes sense.  Attached patch goes further and moves checking the
conditions to a separate helper for clarity.

Thanks,
Miklos

[-- Attachment #2: fuse-clean-up-writepage-sending.patch --]
[-- Type: text/x-patch, Size: 3058 bytes --]

From: Miklos Szeredi <mszeredi@redhat.com>
Subject: fuse: clean up condition for writepage sending

fuse_writepages_fill uses following construction:

if (wpa && ap->num_pages &&
    (A || B || C)) {
        action;
} else if (wpa && D) {
        if (E) {
                the same action;
        }
}

 - ap->num_pages check is always true and can be removed

 - "if" and "else if" calls the same action and can be merged.

Move checking A, B, C, D, E conditions to a helper, add comments.

Original-patch-by: Vasily Averin <vvs@virtuozzo.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/file.c |   51 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 18 deletions(-)

--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2015,6 +2015,38 @@ static bool fuse_writepage_add(struct fu
 	return false;
 }
 
+static bool fuse_writepage_need_send(struct fuse_conn *fc, struct page *page,
+				     struct fuse_args_pages *ap,
+				     struct fuse_fill_wb_data *data)
+{
+	/*
+	 * Being under writeback is unlikely but possible.  For example direct
+	 * read to an mmaped fuse file will set the page dirty twice; once when
+	 * the pages are faulted with get_user_pages(), and then after the read
+	 * completed.
+	 */
+	if (fuse_page_is_writeback(data->inode, page->index))
+		return true;
+
+	/* Reached max pages */
+	if (ap->num_pages == fc->max_pages)
+		return true;
+
+	/* Reached max write bytes */
+	if ((ap->num_pages + 1) * PAGE_SIZE > fc->max_write)
+		return true;
+
+	/* Discontinuity */
+	if (data->orig_pages[ap->num_pages - 1]->index + 1 != page->index)
+		return true;
+
+	/* Need to grow the pages array?  If so, did the expansion fail? */
+	if (ap->num_pages == data->max_pages && !fuse_pages_realloc(data))
+		return true;
+
+	return false;
+}
+
 static int fuse_writepages_fill(struct page *page,
 		struct writeback_control *wbc, void *_data)
 {
@@ -2025,7 +2057,6 @@ static int fuse_writepages_fill(struct p
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	struct page *tmp_page;
-	bool is_writeback;
 	int err;
 
 	if (!data->ff) {
@@ -2035,25 +2066,9 @@ static int fuse_writepages_fill(struct p
 			goto out_unlock;
 	}
 
-	/*
-	 * Being under writeback is unlikely but possible.  For example direct
-	 * read to an mmaped fuse file will set the page dirty twice; once when
-	 * the pages are faulted with get_user_pages(), and then after the read
-	 * completed.
-	 */
-	is_writeback = fuse_page_is_writeback(inode, page->index);
-
-	if (wpa && ap->num_pages &&
-	    (is_writeback || ap->num_pages == fc->max_pages ||
-	     (ap->num_pages + 1) * PAGE_SIZE > fc->max_write ||
-	     data->orig_pages[ap->num_pages - 1]->index + 1 != page->index)) {
+	if (wpa && fuse_writepage_need_send(fc, page, ap, data)) {
 		fuse_writepages_send(data);
 		data->wpa = NULL;
-	} else if (wpa && ap->num_pages == data->max_pages) {
-		if (!fuse_pages_realloc(data)) {
-			fuse_writepages_send(data);
-			data->wpa = NULL;
-		}
 	}
 
 	err = -ENOMEM;

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

[Sedat Dilek]

On Mon, Jul 13, 2020 at 6:16 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Jul 13, 2020 at 10:02 AM Vasily Averin <vvs@virtuozzo.com> wrote:
> >
> > On 7/11/20 7:01 AM, Miklos Szeredi wrote:
> > > On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <vvs@virtuozzo.com> 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.
> > >
> > > Looks good.  However, I don't like the way fuse_writepage_in_flight()
> > > is silently changed to insert page into the rb_tree.  Also the
> > > insertion can be merged with the search for in-flight and be done
> > > unconditionally to simplify the logic.  See attached patch.
> >
> > Your patch looks correct for me except 2 things:
>
> Thanks for reviewing.
>
> > 1) you have lost "data->wpa = NULL;" when fuse_writepage_add() returns false.
>
> This is intentional, because this is in the !data->wpa branch.
>
> > 2) in the same case old code did not set data->orig_pages[ap->num_pages] = page;
>
> That is also intentional, in this case the origi_pages[0] is either
> overwritten with the next page or discarded due to data->wpa being
> NULL.
>
> I'll write these up in the patch header.
>

Did you sent out a new version of your patch?
If yes, where can I get it from?

- Sedat -

Re: [PATCH] fuse_writepages ignores errors from fuse_writepages_fill

[Miklos Szeredi]

On Thu, Jun 25, 2020 at 11:39 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> fuse_writepages() ignores some errors taken from fuse_writepages_fill()
> I believe it is a bug: if .writepages is called with WB_SYNC_ALL
> it should either guarantee that all data was successfully saved
> or return error.
>
> Fixes: 26d614df1da9 ("fuse: Implement writepages callback")
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Applied.  Thanks.

Miklos

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

[Miklos Szeredi]

On Tue, Jul 14, 2020 at 2:40 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:

> Did you sent out a new version of your patch?
> If yes, where can I get it from?

Just pushed a bunch of fixes including this one to

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next

Thanks,
Miklos

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

[Sedat Dilek]

On Tue, Jul 14, 2020 at 2:53 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Jul 14, 2020 at 2:40 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> > Did you sent out a new version of your patch?
> > If yes, where can I get it from?
>
> Just pushed a bunch of fixes including this one to
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
>

Just-In-Time... I stopped my kernel-build and applied from <fuse.git#for-next>.

Thanks.

- Sedat -

Re: [PATCH] fuse_writepages_fill: simplified "if-else if" constuction

[Vasily Averin]

On 7/14/20 3:24 PM, Miklos Szeredi wrote:
> On Thu, Jun 25, 2020 at 11:30 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>>
>> fuse_writepages_fill uses following construction:
>> if (wpa && ap->num_pages &&
>>     (A || B || C)) {
>>         action;
>> } else if (wpa && D) {
>>         if (E) {
>>                 the same action;
>>         }
>> }
>>
>> - ap->num_pages check is always true and can be removed
>> - "if" and "else if" calls the same action and can be merged.
> 
> Makes sense.  Attached patch goes further and moves checking the
> conditions to a separate helper for clarity.

This looks perfect for me, thank you
	Vasily Averin

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

[Sedat Dilek]

On Tue, Jul 14, 2020 at 2:57 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 2:53 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, Jul 14, 2020 at 2:40 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> > > Did you sent out a new version of your patch?
> > > If yes, where can I get it from?
> >
> > Just pushed a bunch of fixes including this one to
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
> >
>
> Just-In-Time... I stopped my kernel-build and applied from <fuse.git#for-next>.
>
> Thanks.
>

Tested with my usual testcase "mount Ubuntu/precise 12.04 LTS
(WUBI-installation):

fdisk -l /dev/sdb

mount -r -t auto /dev/sdb2 /mnt/win7

cd /path/to/disks/
mount -r -t ext4 -o loop ./root.disk /mnt/ubuntu

df -hT

cd /mnt/ubuntu/
ls -alhR

dmesg -T | tail

Looks good.

- Sedat -