Message ID | 148703056420.16583.12871710781552274514.stgit@maxim-thinkpad |
---|---|
State | New |
Series | "fuse: fuse_prepare_write() cannot handle page from killed request" |
Headers | show |
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index a514748..41ed6f0 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1008,7 +1008,7 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode, static int __fuse_readpage(struct file *file, struct page *page, size_t count, int *err, struct fuse_req **req_pp, u64 *attr_ver_p, - bool *killed_p) + bool page_needs_release, bool *killed_p) { struct fuse_io_priv io = { .async = 0, .file = file }; struct inode *inode = page->mapping->host; @@ -1040,6 +1040,7 @@ static int __fuse_readpage(struct file *file, struct page *page, size_t count, req->pages[0] = page; req->page_descs[0].length = count; req->page_cache = 1; + req->page_needs_release = page_needs_release; num_read = fuse_send_read(req, &io, page_offset(page), count, NULL); killed = req->killed; @@ -1071,7 +1072,7 @@ static int fuse_readpage(struct file *file, struct page *page) goto out; num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver, - &killed); + false, &killed); if (!err) { /* * Short read means EOF. If file size is larger, truncate it @@ -1153,6 +1154,7 @@ static void fuse_send_readpages(struct fuse_req *req, struct file *file) req->out.page_zeroing = 1; req->out.page_replace = 1; req->page_cache = 1; + req->page_needs_release = false; fuse_read_fill(req, file, pos, count, FUSE_READ); fuse_account_request(fc, count); req->misc.read.attr_ver = fuse_get_attr_version(fc); @@ -2368,6 +2370,7 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file, unsigned num_read; unsigned page_len; int err; + bool killed = false; if (fuse_file_fail_immediately(file)) { unlock_page(page); @@ -2385,12 +2388,14 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file, } num_read = __fuse_readpage(file, page, page_len, &err, &req, NULL, - NULL); + true, &killed); if (req) fuse_put_request(fc, req); if (err) { - unlock_page(page); - page_cache_release(page); + if (!killed) { + unlock_page(page); + page_cache_release(page); + } } else if (num_read != PAGE_CACHE_SIZE) { zero_user_segment(page, num_read, PAGE_CACHE_SIZE); } diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 22eb9c9..fefa8ff 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -330,6 +330,9 @@ struct fuse_req { /** Request contains pages from page-cache */ unsigned page_cache:1; + /** Request pages need page_cache_release() */ + unsigned page_needs_release:1; + /** Request was killed -- pages were released */ unsigned killed:1; diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index b63aae2..ddd858c 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -378,6 +378,8 @@ static void fuse_kill_requests(struct fuse_conn *fc, struct inode *inode, struct page *page = req->pages[i]; SetPageError(page); unlock_page(page); + if (req->page_needs_release) + page_cache_release(page); req->pages[i] = NULL; }
Dima, please review the patch. -- Best regards, Konstantin Khorenko, Virtuozzo Linux Kernel Team On 02/14/2017 03:03 AM, Maxim Patlasov wrote: > After fuse_prepare_write() called __fuse_readpage(file, page, ...), > the page might be already unlocked by fuse_kill_requests(): > >> for (i = 0; i < req->num_pages; i++) { >> struct page *page = req->pages[i]; >> SetPageError(page); >> unlock_page(page); > > so it is incorrect to touch it at all. The problem can be easily > fixed the same way is it was done in fuse_readpage() checking "killed" > flag. > > Another minor complication is that there are three different use-cases > for that snippet from fuse_kill_requests() above: fuse_readpages(), > fuse_readpage() and fuse_prepare_write(). Among them only the latter > needs explicit page_cache_release() call. That's why the patch introduces > ad-hoc request flag "page_needs_release". > > https://jira.sw.ru/browse/PSBM-54547 > Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com> > --- > fs/fuse/file.c | 15 ++++++++++----- > fs/fuse/fuse_i.h | 3 +++ > fs/fuse/inode.c | 2 ++ > 3 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index a514748..41ed6f0 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1008,7 +1008,7 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode, > > static int __fuse_readpage(struct file *file, struct page *page, size_t count, > int *err, struct fuse_req **req_pp, u64 *attr_ver_p, > - bool *killed_p) > + bool page_needs_release, bool *killed_p) > { > struct fuse_io_priv io = { .async = 0, .file = file }; > struct inode *inode = page->mapping->host; > @@ -1040,6 +1040,7 @@ static int __fuse_readpage(struct file *file, struct page *page, size_t count, > req->pages[0] = page; > req->page_descs[0].length = count; > req->page_cache = 1; > + req->page_needs_release = page_needs_release; > > num_read = fuse_send_read(req, &io, page_offset(page), count, NULL); > killed = req->killed; > @@ -1071,7 +1072,7 @@ static int fuse_readpage(struct file *file, struct page *page) > goto out; > > num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver, > - &killed); > + false, &killed); > if (!err) { > /* > * Short read means EOF. If file size is larger, truncate it > @@ -1153,6 +1154,7 @@ static void fuse_send_readpages(struct fuse_req *req, struct file *file) > req->out.page_zeroing = 1; > req->out.page_replace = 1; > req->page_cache = 1; > + req->page_needs_release = false; > fuse_read_fill(req, file, pos, count, FUSE_READ); > fuse_account_request(fc, count); > req->misc.read.attr_ver = fuse_get_attr_version(fc); > @@ -2368,6 +2370,7 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file, > unsigned num_read; > unsigned page_len; > int err; > + bool killed = false; > > if (fuse_file_fail_immediately(file)) { > unlock_page(page); > @@ -2385,12 +2388,14 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file, > } > > num_read = __fuse_readpage(file, page, page_len, &err, &req, NULL, > - NULL); > + true, &killed); > if (req) > fuse_put_request(fc, req); > if (err) { > - unlock_page(page); > - page_cache_release(page); > + if (!killed) { > + unlock_page(page); > + page_cache_release(page); > + } > } else if (num_read != PAGE_CACHE_SIZE) { > zero_user_segment(page, num_read, PAGE_CACHE_SIZE); > } > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index 22eb9c9..fefa8ff 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -330,6 +330,9 @@ struct fuse_req { > /** Request contains pages from page-cache */ > unsigned page_cache:1; > > + /** Request pages need page_cache_release() */ > + unsigned page_needs_release:1; > + > /** Request was killed -- pages were released */ > unsigned killed:1; > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index b63aae2..ddd858c 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -378,6 +378,8 @@ static void fuse_kill_requests(struct fuse_conn *fc, struct inode *inode, > struct page *page = req->pages[i]; > SetPageError(page); > unlock_page(page); > + if (req->page_needs_release) > + page_cache_release(page); > req->pages[i] = NULL; > } > > > . >
Maxim Patlasov <mpatlasov@virtuozzo.com> writes: > After fuse_prepare_write() called __fuse_readpage(file, page, ...), > the page might be already unlocked by fuse_kill_requests(): > >> for (i = 0; i < req->num_pages; i++) { >> struct page *page = req->pages[i]; >> SetPageError(page); >> unlock_page(page); ACK. > > so it is incorrect to touch it at all. The problem can be easily > fixed the same way is it was done in fuse_readpage() checking "killed" > flag. > > Another minor complication is that there are three different use-cases > for that snippet from fuse_kill_requests() above: fuse_readpages(), > fuse_readpage() and fuse_prepare_write(). Among them only the latter > needs explicit page_cache_release() call. That's why the patch introduces > ad-hoc request flag "page_needs_release". > > https://jira.sw.ru/browse/PSBM-54547 > Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com> > --- > fs/fuse/file.c | 15 ++++++++++----- > fs/fuse/fuse_i.h | 3 +++ > fs/fuse/inode.c | 2 ++ > 3 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index a514748..41ed6f0 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1008,7 +1008,7 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode, > > static int __fuse_readpage(struct file *file, struct page *page, size_t count, > int *err, struct fuse_req **req_pp, u64 *attr_ver_p, > - bool *killed_p) > + bool page_needs_release, bool *killed_p) > { > struct fuse_io_priv io = { .async = 0, .file = file }; > struct inode *inode = page->mapping->host; > @@ -1040,6 +1040,7 @@ static int __fuse_readpage(struct file *file, struct page *page, size_t count, > req->pages[0] = page; > req->page_descs[0].length = count; > req->page_cache = 1; > + req->page_needs_release = page_needs_release; > > num_read = fuse_send_read(req, &io, page_offset(page), count, NULL); > killed = req->killed; > @@ -1071,7 +1072,7 @@ static int fuse_readpage(struct file *file, struct page *page) > goto out; > > num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver, > - &killed); > + false, &killed); > if (!err) { > /* > * Short read means EOF. If file size is larger, truncate it > @@ -1153,6 +1154,7 @@ static void fuse_send_readpages(struct fuse_req *req, struct file *file) > req->out.page_zeroing = 1; > req->out.page_replace = 1; > req->page_cache = 1; > + req->page_needs_release = false; > fuse_read_fill(req, file, pos, count, FUSE_READ); > fuse_account_request(fc, count); > req->misc.read.attr_ver = fuse_get_attr_version(fc); > @@ -2368,6 +2370,7 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file, > unsigned num_read; > unsigned page_len; > int err; > + bool killed = false; > > if (fuse_file_fail_immediately(file)) { > unlock_page(page); > @@ -2385,12 +2388,14 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file, > } > > num_read = __fuse_readpage(file, page, page_len, &err, &req, NULL, > - NULL); > + true, &killed); > if (req) > fuse_put_request(fc, req); > if (err) { > - unlock_page(page); > - page_cache_release(page); > + if (!killed) { > + unlock_page(page); > + page_cache_release(page); > + } > } else if (num_read != PAGE_CACHE_SIZE) { > zero_user_segment(page, num_read, PAGE_CACHE_SIZE); > } > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index 22eb9c9..fefa8ff 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -330,6 +330,9 @@ struct fuse_req { > /** Request contains pages from page-cache */ > unsigned page_cache:1; > > + /** Request pages need page_cache_release() */ > + unsigned page_needs_release:1; > + > /** Request was killed -- pages were released */ > unsigned killed:1; > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index b63aae2..ddd858c 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -378,6 +378,8 @@ static void fuse_kill_requests(struct fuse_conn *fc, struct inode *inode, > struct page *page = req->pages[i]; > SetPageError(page); > unlock_page(page); > + if (req->page_needs_release) > + page_cache_release(page); > req->pages[i] = NULL; > } >