* [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert [not found] <2733b41a-b4c6-be94-0118-a1a8d6f26eec@virtuozzo.com> @ 2020-06-25 9:02 ` Vasily Averin 2020-06-27 10:31 ` Sedat Dilek ` (2 more replies) 2020-06-25 9:30 ` [PATCH] fuse_writepages_fill: simplified "if-else if" constuction Vasily Averin 2020-06-25 9:39 ` [PATCH] fuse_writepages ignores errors from fuse_writepages_fill Vasily Averin 2 siblings, 3 replies; 16+ messages in thread From: Vasily Averin @ 2020-06-25 9:02 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-06-25 9:02 ` [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert Vasily Averin @ 2020-06-27 10:31 ` Sedat Dilek 2020-06-29 21:11 ` Vivek Goyal 2020-07-11 4:01 ` Miklos Szeredi 2 siblings, 0 replies; 16+ messages in thread From: Sedat Dilek @ 2020-06-27 10:31 UTC (permalink / raw) To: Vasily Averin Cc: Miklos Szeredi, linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-06-25 9:02 ` [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert Vasily Averin 2020-06-27 10:31 ` Sedat Dilek @ 2020-06-29 21:11 ` Vivek Goyal 2020-07-11 4:01 ` Miklos Szeredi 2 siblings, 0 replies; 16+ messages in thread From: Vivek Goyal @ 2020-06-29 21:11 UTC (permalink / raw) To: Vasily Averin Cc: Miklos Szeredi, linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ]--- ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-06-25 9:02 ` [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert Vasily Averin 2020-06-27 10:31 ` Sedat Dilek 2020-06-29 21:11 ` Vivek Goyal @ 2020-07-11 4:01 ` Miklos Szeredi 2020-07-13 8:02 ` Vasily Averin 2 siblings, 1 reply; 16+ messages in thread From: Miklos Szeredi @ 2020-07-11 4:01 UTC (permalink / raw) To: Vasily Averin; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-07-11 4:01 ` Miklos Szeredi @ 2020-07-13 8:02 ` Vasily Averin 2020-07-13 16:14 ` Miklos Szeredi 0 siblings, 1 reply; 16+ messages in thread From: Vasily Averin @ 2020-07-13 8:02 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML [-- 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); ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-07-13 8:02 ` Vasily Averin @ 2020-07-13 16:14 ` Miklos Szeredi 2020-07-14 6:18 ` Vasily Averin 2020-07-14 12:40 ` Sedat Dilek 0 siblings, 2 replies; 16+ messages in thread From: Miklos Szeredi @ 2020-07-13 16:14 UTC (permalink / raw) To: Vasily Averin; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-07-13 16:14 ` Miklos Szeredi @ 2020-07-14 6:18 ` Vasily Averin 2020-07-14 12:40 ` Sedat Dilek 1 sibling, 0 replies; 16+ messages in thread From: Vasily Averin @ 2020-07-14 6:18 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-07-13 16:14 ` Miklos Szeredi 2020-07-14 6:18 ` Vasily Averin @ 2020-07-14 12:40 ` Sedat Dilek 2020-07-14 12:52 ` Miklos Szeredi 1 sibling, 1 reply; 16+ messages in thread From: Sedat Dilek @ 2020-07-14 12:40 UTC (permalink / raw) To: Miklos Szeredi Cc: Vasily Averin, linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 - ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-07-14 12:40 ` Sedat Dilek @ 2020-07-14 12:52 ` Miklos Szeredi 2020-07-14 12:57 ` Sedat Dilek 0 siblings, 1 reply; 16+ messages in thread From: Miklos Szeredi @ 2020-07-14 12:52 UTC (permalink / raw) To: Sedat Dilek Cc: Vasily Averin, linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-07-14 12:52 ` Miklos Szeredi @ 2020-07-14 12:57 ` Sedat Dilek 2020-07-15 7:48 ` Sedat Dilek 0 siblings, 1 reply; 16+ messages in thread From: Sedat Dilek @ 2020-07-14 12:57 UTC (permalink / raw) To: Miklos Szeredi Cc: Vasily Averin, linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 - ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert 2020-07-14 12:57 ` Sedat Dilek @ 2020-07-15 7:48 ` Sedat Dilek 0 siblings, 0 replies; 16+ messages in thread From: Sedat Dilek @ 2020-07-15 7:48 UTC (permalink / raw) To: Miklos Szeredi Cc: Vasily Averin, linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 - ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] fuse_writepages_fill: simplified "if-else if" constuction [not found] <2733b41a-b4c6-be94-0118-a1a8d6f26eec@virtuozzo.com> 2020-06-25 9:02 ` [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert Vasily Averin @ 2020-06-25 9:30 ` Vasily Averin 2020-07-14 12:24 ` Miklos Szeredi 2020-06-25 9:39 ` [PATCH] fuse_writepages ignores errors from fuse_writepages_fill Vasily Averin 2 siblings, 1 reply; 16+ messages in thread From: Vasily Averin @ 2020-06-25 9:30 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill: simplified "if-else if" constuction 2020-06-25 9:30 ` [PATCH] fuse_writepages_fill: simplified "if-else if" constuction Vasily Averin @ 2020-07-14 12:24 ` Miklos Szeredi 2020-07-14 18:53 ` Vasily Averin 0 siblings, 1 reply; 16+ messages in thread From: Miklos Szeredi @ 2020-07-14 12:24 UTC (permalink / raw) To: Vasily Averin; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML [-- 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; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages_fill: simplified "if-else if" constuction 2020-07-14 12:24 ` Miklos Szeredi @ 2020-07-14 18:53 ` Vasily Averin 0 siblings, 0 replies; 16+ messages in thread From: Vasily Averin @ 2020-07-14 18:53 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] fuse_writepages ignores errors from fuse_writepages_fill [not found] <2733b41a-b4c6-be94-0118-a1a8d6f26eec@virtuozzo.com> 2020-06-25 9:02 ` [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert Vasily Averin 2020-06-25 9:30 ` [PATCH] fuse_writepages_fill: simplified "if-else if" constuction Vasily Averin @ 2020-06-25 9:39 ` Vasily Averin 2020-07-14 12:44 ` Miklos Szeredi 2 siblings, 1 reply; 16+ messages in thread From: Vasily Averin @ 2020-06-25 9:39 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] fuse_writepages ignores errors from fuse_writepages_fill 2020-06-25 9:39 ` [PATCH] fuse_writepages ignores errors from fuse_writepages_fill Vasily Averin @ 2020-07-14 12:44 ` Miklos Szeredi 0 siblings, 0 replies; 16+ messages in thread From: Miklos Szeredi @ 2020-07-14 12:44 UTC (permalink / raw) To: Vasily Averin; +Cc: linux-fsdevel, Maxim Patlasov, Kirill Tkhai, LKML 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-07-15 7:48 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <2733b41a-b4c6-be94-0118-a1a8d6f26eec@virtuozzo.com> 2020-06-25 9:02 ` [PATCH] fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert Vasily Averin 2020-06-27 10:31 ` Sedat Dilek 2020-06-29 21:11 ` Vivek Goyal 2020-07-11 4:01 ` Miklos Szeredi 2020-07-13 8:02 ` Vasily Averin 2020-07-13 16:14 ` Miklos Szeredi 2020-07-14 6:18 ` Vasily Averin 2020-07-14 12:40 ` Sedat Dilek 2020-07-14 12:52 ` Miklos Szeredi 2020-07-14 12:57 ` Sedat Dilek 2020-07-15 7:48 ` Sedat Dilek 2020-06-25 9:30 ` [PATCH] fuse_writepages_fill: simplified "if-else if" constuction Vasily Averin 2020-07-14 12:24 ` Miklos Szeredi 2020-07-14 18:53 ` Vasily Averin 2020-06-25 9:39 ` [PATCH] fuse_writepages ignores errors from fuse_writepages_fill Vasily Averin 2020-07-14 12:44 ` Miklos Szeredi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).