| Message ID | 154322557765.18737.14337090699283695815.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] fuse: Fix race in fuse_writepage_in_flight() | expand |
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. Thanks, Miklos
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. Updated version looks good for me. Thanks, Kirill
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
On 15.01.2019 19:36, Miklos Szeredi wrote: > 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) Ok, thanks for the explanation. Kirill
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); spin_unlock(&fc->lock); dec_wb_stat(&bdi->wb, WB_WRITEBACK);
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(-)