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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Hi Vasily,

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

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

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

Unsure if your single patches should be labeled with:

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

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

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

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

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

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

If you send a v2 please add my:

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

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

Thanks.

Regards,
- Sedat -




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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux