LinuxLists.cc - Re: [fuse] 6b2fb79963: WARNING:at_fs/fuse/file.c:#tree

[permalink] [raw]

Subject: Re: [fuse] 6b2fb79963: WARNING:at_fs/fuse/file.c:#tree_insert[fuse]

On Mon, Jun 15, 2020 at 7:59 AM Vasily Averin <[email protected]> wrote:
>
> On 6/15/20 3:50 AM, kernel test robot wrote:
> > FYI, we noticed the following commit (built with gcc-9):
> >
> > commit: 6b2fb79963fbed7db3ef850926d913518fd5c62f ("fuse: optimize writepages search")
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>
> > [ 1030.995703] ------------[ cut here ]------------
> > [ 1030.997563] WARNING: CPU: 1 PID: 17211 at fs/fuse/file.c:1728 tree_insert+0xab/0xc0 [fuse]
> > [ 1031.021943] RIP: 0010:tree_insert+0xab/0xc0 [fuse]
> > [ 1031.057802] Call Trace:
> > [ 1031.060015] fuse_writepages_fill+0x5da/0x6a0 [fuse]
> > [ 1031.062536] write_cache_pages+0x171/0x470
> > [ 1031.064933] ? fuse_writepages+0x100/0x100 [fuse]
> > [ 1031.067419] ? terminate_walk+0xd3/0xf0
> > [ 1031.069707] ? _cond_resched+0x19/0x30
> > [ 1031.072140] ? __kmalloc+0x274/0x280
> > [ 1031.074407] fuse_writepages+0x8a/0x100 [fuse]
> > [ 1031.076599] do_writepages+0x43/0xe0
>
> It is WARN_ON(!wpa->ia.ap.num_pages);
> however tree_insert() was called from fuse_writepages_fill() with ap->num_pages = 0;
> In submitted PATCH RFC we have used
>
> +static int tree_insert(struct rb_root *root, struct fuse_req *ins_req)
> ...
> + pgoff_t idx_to = idx_from + (ins_req->num_pages ?
> + ins_req->num_pages - 1 : 0);
>
> Though committed patch version have
>
> +static void tree_insert(struct rb_root *root, struct fuse_writepage_args *wpa)
> ...
> + pgoff_t idx_to = idx_from + wpa->ia.ap.num_pages - 1;
> ...
> + WARN_ON(!wpa->ia.ap.num_pages);
>
> Miklos,
> do you have any objections if I return to our initial proposal?
> Am I missed something and it is not allowed now?

No objections, but you need to explain with a comment why that special
casing of num_pages == 0 is needed. I don't understand it yet.

Thanks,
Miklos

2020-06-18 10:34:05

by Kirill Tkhai

[permalink] [raw]

Subject: Re: Re: [fuse] 6b2fb79963: WARNING:at_fs/fuse/file.c:#tree_insert[fuse]

Hi,

On 16.06.2020 15:35, Miklos Szeredi wrote:
> On Mon, Jun 15, 2020 at 7:59 AM Vasily Averin <[email protected]> wrote:
>>
>> On 6/15/20 3:50 AM, kernel test robot wrote:
>>> FYI, we noticed the following commit (built with gcc-9):
>>>
>>> commit: 6b2fb79963fbed7db3ef850926d913518fd5c62f ("fuse: optimize writepages search")
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>>
>>> [ 1030.995703] ------------[ cut here ]------------
>>> [ 1030.997563] WARNING: CPU: 1 PID: 17211 at fs/fuse/file.c:1728 tree_insert+0xab/0xc0 [fuse]
>>> [ 1031.021943] RIP: 0010:tree_insert+0xab/0xc0 [fuse]
>>> [ 1031.057802] Call Trace:
>>> [ 1031.060015] fuse_writepages_fill+0x5da/0x6a0 [fuse]
>>> [ 1031.062536] write_cache_pages+0x171/0x470
>>> [ 1031.064933] ? fuse_writepages+0x100/0x100 [fuse]
>>> [ 1031.067419] ? terminate_walk+0xd3/0xf0
>>> [ 1031.069707] ? _cond_resched+0x19/0x30
>>> [ 1031.072140] ? __kmalloc+0x274/0x280
>>> [ 1031.074407] fuse_writepages+0x8a/0x100 [fuse]
>>> [ 1031.076599] do_writepages+0x43/0xe0
>>
>> It is WARN_ON(!wpa->ia.ap.num_pages);
>> however tree_insert() was called from fuse_writepages_fill() with ap->num_pages = 0;
>> In submitted PATCH RFC we have used
>>
>> +static int tree_insert(struct rb_root *root, struct fuse_req *ins_req)
>> ...
>> + pgoff_t idx_to = idx_from + (ins_req->num_pages ?
>> + ins_req->num_pages - 1 : 0);
>>
>> Though committed patch version have
>>
>> +static void tree_insert(struct rb_root *root, struct fuse_writepage_args *wpa)
>> ...
>> + pgoff_t idx_to = idx_from + wpa->ia.ap.num_pages - 1;
>> ...
>> + WARN_ON(!wpa->ia.ap.num_pages);
>>
>> Miklos,
>> do you have any objections if I return to our initial proposal?
>> Am I missed something and it is not allowed now?
>
> No objections, but you need to explain with a comment why that special
> casing of num_pages == 0 is needed. I don't understand it yet.

Currently here is already a bomb in mainline kernel.

This fuse_writepages_fill()->tree_insert() with num_pages == 0 corrupts fi->writepages tree.
Normally we have half-closed intervals in the tree:

[5, 6)
/ \
[1, 3) [7, 9)

But tree_insert(num_pages == 0) adds an abnormal interval with both ends included:

[5, 5]
/ \
[1, 3) [7, 9)

This may seriously confuse a reader iterating a tree, which does not know about special case (num_pages == 0).
As I see, nobody in the code cares about num_pages == 0, and there is no comment about that case.

This will fire soon or later. I think we should stop insert abnormal intervals (num_pages == 0) there.

Reasons fuse_writepages_fill() does use this (num_pages == 0)?!

2020-06-25 07:50:06

[permalink] [raw]

Subject: Re: [fuse] 6b2fb79963: WARNING:at_fs/fuse/file.c:#tree_insert[fuse]

On 6/16/20 3:35 PM, Miklos Szeredi wrote:
> On Mon, Jun 15, 2020 at 7:59 AM Vasily Averin <[email protected]> wrote:
>>
>> On 6/15/20 3:50 AM, kernel test robot wrote:
>>> FYI, we noticed the following commit (built with gcc-9):
>>>
>>> commit: 6b2fb79963fbed7db3ef850926d913518fd5c62f ("fuse: optimize writepages search")
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>>
>>> [ 1030.995703] ------------[ cut here ]------------
>>> [ 1030.997563] WARNING: CPU: 1 PID: 17211 at fs/fuse/file.c:1728 tree_insert+0xab/0xc0 [fuse]
>>> [ 1031.021943] RIP: 0010:tree_insert+0xab/0xc0 [fuse]
>>> [ 1031.057802] Call Trace:
>>> [ 1031.060015] fuse_writepages_fill+0x5da/0x6a0 [fuse]
>>> [ 1031.062536] write_cache_pages+0x171/0x470
>>> [ 1031.064933] ? fuse_writepages+0x100/0x100 [fuse]
>>> [ 1031.067419] ? terminate_walk+0xd3/0xf0
>>> [ 1031.069707] ? _cond_resched+0x19/0x30
>>> [ 1031.072140] ? __kmalloc+0x274/0x280
>>> [ 1031.074407] fuse_writepages+0x8a/0x100 [fuse]
>>> [ 1031.076599] do_writepages+0x43/0xe0
>>
>> It is WARN_ON(!wpa->ia.ap.num_pages);
>> however tree_insert() was called from fuse_writepages_fill() with ap->num_pages = 0;
>> In submitted PATCH RFC we have used
>>
>> +static int tree_insert(struct rb_root *root, struct fuse_req *ins_req)
>> ...
>> + pgoff_t idx_to = idx_from + (ins_req->num_pages ?
>> + ins_req->num_pages - 1 : 0);
>>
>> Though committed patch version have
>>
>> +static void tree_insert(struct rb_root *root, struct fuse_writepage_args *wpa)
>> ...
>> + pgoff_t idx_to = idx_from + wpa->ia.ap.num_pages - 1;
>> ...
>> + WARN_ON(!wpa->ia.ap.num_pages);
>>
>> Miklos,
>> do you have any objections if I return to our initial proposal?
>> Am I missed something and it is not allowed now?
>
> No objections, but you need to explain with a comment why that special
> casing of num_pages == 0 is needed. I don't understand it yet.

fuse_writepages_fill() handles few different cases,
in some of them data->wpa should be re-allocated and re-initialized,
another cases requires just add new page into existing wpa and increments num_pages.

In current implementation fuse_writepages_fill() tries to share the code,
by this way 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.

I've prepared patch that calls tree_insert() with num_pages = 1,
It have few other minor optimizations therefore I think it is better
than remove incorrect WARN_ON from tree_insert proposed earlier.

During investigations I've noticed few other stranges in neighborhood:
1) 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.

2) fuse_writepages_fill uses following construction:
if (wpa && ap->num_pages &&
(A || B || C)) {
action;
} else if (wpa && ap->num_pages == data->max_pages) {
if (D) {
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.

I'm going to sent separate patches to resolve it.

Thank you,
Vasily Averin

2020-06-25 09:51:09

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Vasily Averin <[email protected]>
---
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

2020-06-25 09:58:59

[permalink] [raw]

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

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 <[email protected]>
---
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

2020-06-25 10:02:00

[permalink] [raw]

Subject: [PATCH] fuse_writepages ignores errors from fuse_writepages_fill

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 <[email protected]>
---
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

2020-06-27 10:32:41

[permalink] [raw]

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

On Thu, Jun 25, 2020 at 11:52 AM Vasily Averin <[email protected]> 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 <[email protected]> # 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 <[email protected]>
> Signed-off-by: Vasily Averin <[email protected]>
> ---
> 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
>

2020-06-29 21:13:34

by Vivek Goyal

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Vasily Averin <[email protected]>

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 ]---

2020-07-11 04:01:53

[permalink] [raw]

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

On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <[email protected]> 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

Attachments:

fuse-fix-warning-in-tree_insert.patch (4.04 kB)

2020-07-13 08:03:41

[permalink] [raw]

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

On 7/11/20 7:01 AM, Miklos Szeredi wrote:
> On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <[email protected]> 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

Attachments:

vvs.fuse-fix-warning-in-tree_insert.patch (4.09 kB)

2020-07-13 16:15:16

[permalink] [raw]

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

On Mon, Jul 13, 2020 at 10:02 AM Vasily Averin <[email protected]> wrote:
>
> On 7/11/20 7:01 AM, Miklos Szeredi wrote:
> > On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <[email protected]> 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

2020-07-14 06:19:37

[permalink] [raw]

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

On 7/13/20 7:14 PM, Miklos Szeredi wrote:
> On Mon, Jul 13, 2020 at 10:02 AM Vasily Averin <[email protected]> wrote:
>>
>> On 7/11/20 7:01 AM, Miklos Szeredi wrote:
>>> On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <[email protected]> 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

2020-07-14 12:28:02

[permalink] [raw]

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

On Thu, Jun 25, 2020 at 11:30 AM Vasily Averin <[email protected]> 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

Attachments:

fuse-clean-up-writepage-sending.patch (2.99 kB)

2020-07-14 12:42:21

[permalink] [raw]

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

On Mon, Jul 13, 2020 at 6:16 PM Miklos Szeredi <[email protected]> wrote:
>
> On Mon, Jul 13, 2020 at 10:02 AM Vasily Averin <[email protected]> wrote:
> >
> > On 7/11/20 7:01 AM, Miklos Szeredi wrote:
> > > On Thu, Jun 25, 2020 at 11:02 AM Vasily Averin <[email protected]> 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 -

2020-07-14 12:47:38

[permalink] [raw]

Subject: Re: [PATCH] fuse_writepages ignores errors from fuse_writepages_fill

On Thu, Jun 25, 2020 at 11:39 AM Vasily Averin <[email protected]> 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 <[email protected]>

Applied. Thanks.

Miklos

2020-07-14 12:55:50

[permalink] [raw]

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

On Tue, Jul 14, 2020 at 2:40 PM Sedat Dilek <[email protected]> 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

2020-07-14 12:58:28

[permalink] [raw]

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

On Tue, Jul 14, 2020 at 2:53 PM Miklos Szeredi <[email protected]> wrote:
>
> On Tue, Jul 14, 2020 at 2:40 PM Sedat Dilek <[email protected]> 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 -

2020-07-14 18:57:15

[permalink] [raw]

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

On 7/14/20 3:24 PM, Miklos Szeredi wrote:
> On Thu, Jun 25, 2020 at 11:30 AM Vasily Averin <[email protected]> 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

2020-07-15 07:49:41