On 7/11/20 7:01 AM, Miklos Szeredi wrote:
> On Thu, Jun 25, 2020 at 11:02 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.
>
> 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
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);
