Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <154322517208.18737.3297786654135648324.stgit@localhost.localdomain>
 <154322557765.18737.14337090699283695815.stgit@localhost.localdomain>
 <CAJfpegvKxJ06hfXR5yxWkdz0GzTw7vR2x8E5bjdO1df1J1C54w@mail.gmail.com> <30c6d750-01b1-fb7e-eb80-0f60f242974a@virtuozzo.com>
In-Reply-To: <30c6d750-01b1-fb7e-eb80-0f60f242974a@virtuozzo.com>
From:   Miklos Szeredi <miklos@szeredi.hu>
Date:   Tue, 15 Jan 2019 17:36:18 +0100
Message-ID: <CAJfpeguXy_Eebok9z_FCGvjGM8_qHHyTqsTMFeaJbmrW12kqYA@mail.gmail.com>
Subject: Re: [PATCH 2/2] fuse: Replace page without copying in fuse_writepage_in_flight()
To:     Kirill Tkhai <ktkhai@virtuozzo.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 15, 2019 at 5:14 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> On 15.01.2019 18:39, Miklos Szeredi wrote:
> > On Mon, Nov 26, 2018 at 10:46 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> >>
> >> It looks like we can optimize old_req page replacement
> >> and avoid copying by simple updating the request's page.
> >>
> >> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> >> ---
> >>  fs/fuse/file.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> >> index c6650c68b31a..83b54b082c86 100644
> >> --- a/fs/fuse/file.c
> >> +++ b/fs/fuse/file.c
> >> @@ -1778,7 +1778,7 @@ static bool fuse_writepage_in_flight(struct fuse_req *new_req,
> >>         if (old_req->num_pages == 1 && old_req != first_req) {
> >>                 struct backing_dev_info *bdi = inode_to_bdi(page->mapping->host);
> >>
> >> -               copy_highpage(old_req->pages[0], page);
> >> +               swap(old_req->pages[0], page);
> >
> > This would mess up refcounting for all pages involved.   need to swap
> > with the temp page in new_req.    Fixed version in #for-next.
>
> You are sure, page is just a simple pointer, not struct **page.
> Then we would have had to change fuse_writepage_in_flight() to use ** pointer.

Using a struct page** would still have been broken, not because of
refcounting, but because of putting the wrong page into the request
(we do the temporary copy to avoid some issues with adding the page
cache page directly into the request)

Thanks,
Miklos