fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert
diff mbox series

Message ID d6e8ef46-c311-b993-909c-4ae2823e2237@virtuozzo.com
State New
Headers show
Series
  • fuse_writepages_fill() optimization to avoid WARN_ON in tree_insert
Related show

Commit Message

Vasily Averin June 25, 2020, 9:02 a.m. UTC
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(-)

Comments

Sedat Dilek June 27, 2020, 10:31 a.m. UTC | #1
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
>
Vivek Goyal June 29, 2020, 9:11 p.m. UTC | #2
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 ]---
Miklos Szeredi July 11, 2020, 4:01 a.m. UTC | #3
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
Vasily Averin July 13, 2020, 8:02 a.m. UTC | #4
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
Miklos Szeredi July 13, 2020, 4:14 p.m. UTC | #5
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
Vasily Averin July 14, 2020, 6:18 a.m. UTC | #6
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
Sedat Dilek July 14, 2020, 12:40 p.m. UTC | #7
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 -
Miklos Szeredi July 14, 2020, 12:52 p.m. UTC | #8
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
Sedat Dilek July 14, 2020, 12:57 p.m. UTC | #9
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 -
Sedat Dilek July 15, 2020, 7:48 a.m. UTC | #10
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 -

Patch
diff mbox series

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;
 }