| Message ID | 1547159098-19011-8-git-send-email-pshilov@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | SMB3 credit flow control handling and writeback fixes | expand |
On Thu, 2019-01-10 at 14:24 -0800, Pavel Shilovsky wrote: > This patch aims to address writeback code problems related to error > paths. In particular it respects EINTR and related error codes and > stores the first error occured during writeback in order to return it > to a caller. > The current semantic with respect to fsync is to return the last error code. > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > --- > fs/cifs/cifsglob.h | 19 +++++++++++++++++++ > fs/cifs/cifssmb.c | 7 ++++--- > fs/cifs/file.c | 29 +++++++++++++++++++++++------ > fs/cifs/inode.c | 10 ++++++++++ > 4 files changed, 56 insertions(+), 9 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 7709268..94dbdbe 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1575,6 +1575,25 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, > kfree(param); > } > > +static inline bool is_interrupt_error(int error) > +{ > + switch (error) { > + case -EINTR: > + case -ERESTARTSYS: > + case -ERESTARTNOHAND: > + case -ERESTARTNOINTR: > + return true; > + } > + return false; > +} > + > +static inline bool is_retryable_error(int error) > +{ > + if (is_interrupt_error(error) || error == -EAGAIN) > + return true; > + return false; > +} > + > #define MID_FREE 0 > #define MID_REQUEST_ALLOCATED 1 > #define MID_REQUEST_SUBMITTED 2 > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index b1f49c1..6930cdb 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -2114,7 +2114,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > for (j = 0; j < nr_pages; j++) { > unlock_page(wdata2->pages[j]); > - if (rc != 0 && rc != -EAGAIN) { > + if (rc != 0 && !is_retryable_error(rc)) { > SetPageError(wdata2->pages[j]); > end_page_writeback(wdata2->pages[j]); > put_page(wdata2->pages[j]); > @@ -2123,7 +2123,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > if (rc) { > kref_put(&wdata2->refcount, cifs_writedata_release); > - if (rc == -EAGAIN) > + if (is_retryable_error(rc)) > continue; > break; > } > @@ -2132,7 +2132,8 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > i += nr_pages; > } while (i < wdata->nr_pages); > > - mapping_set_error(inode->i_mapping, rc); > + if (rc != 0 && !is_retryable_error(rc)) > + mapping_set_error(inode->i_mapping, rc); > kref_put(&wdata->refcount, cifs_writedata_release); > } > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index c23bf9d..109b1ef 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -732,7 +732,8 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush) > > if (can_flush) { > rc = filemap_write_and_wait(inode->i_mapping); > - mapping_set_error(inode->i_mapping, rc); > + if (!is_interrupt_error(rc)) > + mapping_set_error(inode->i_mapping, rc); > > if (tcon->unix_ext) > rc = cifs_get_inode_info_unix(&inode, full_path, > @@ -2109,6 +2110,7 @@ static int cifs_writepages(struct address_space *mapping, > pgoff_t end, index; > struct cifs_writedata *wdata; > int rc = 0; > + int saved_rc = 0; > unsigned int xid; > > /* > @@ -2137,8 +2139,10 @@ static int cifs_writepages(struct address_space *mapping, > > rc = server->ops->wait_mtu_credits(server, cifs_sb->wsize, > &wsize, &credits); > - if (rc) > + if (rc != 0) { > + done = true; > break; > + } > > tofind = min((wsize / PAGE_SIZE) - 1, end - index) + 1; > > @@ -2146,6 +2150,7 @@ static int cifs_writepages(struct address_space *mapping, > &found_pages); > if (!wdata) { > rc = -ENOMEM; > + done = true; > add_credits_and_wake_if(server, credits, 0); > break; > } > @@ -2174,7 +2179,7 @@ static int cifs_writepages(struct address_space *mapping, > if (rc != 0) { > add_credits_and_wake_if(server, wdata->credits, 0); > for (i = 0; i < nr_pages; ++i) { > - if (rc == -EAGAIN) > + if (is_retryable_error(rc)) > redirty_page_for_writepage(wbc, > wdata->pages[i]); > else > @@ -2182,7 +2187,7 @@ static int cifs_writepages(struct address_space *mapping, > end_page_writeback(wdata->pages[i]); > put_page(wdata->pages[i]); > } > - if (rc != -EAGAIN) > + if (!is_retryable_error(rc)) > mapping_set_error(mapping, rc); > } > kref_put(&wdata->refcount, cifs_writedata_release); > @@ -2192,6 +2197,15 @@ static int cifs_writepages(struct address_space *mapping, > continue; > } > > + /* Return immediately if we received a signal during writing */ > + if (is_interrupt_error(rc)) { > + done = true; > + break; > + } > + > + if (rc != 0 && saved_rc == 0) > + saved_rc = rc; > + > wbc->nr_to_write -= nr_pages; > if (wbc->nr_to_write <= 0) > done = true; > @@ -2209,6 +2223,9 @@ static int cifs_writepages(struct address_space *mapping, > goto retry; > } > > + if (saved_rc != 0) > + rc = saved_rc; > + > if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0)) > mapping->writeback_index = index; > > @@ -2241,8 +2258,8 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc) > set_page_writeback(page); > retry_write: > rc = cifs_partialpagewrite(page, 0, PAGE_SIZE); > - if (rc == -EAGAIN) { > - if (wbc->sync_mode == WB_SYNC_ALL) > + if (is_retryable_error(rc)) { > + if (wbc->sync_mode == WB_SYNC_ALL && rc == -EAGAIN) > goto retry_write; > redirty_page_for_writepage(wbc, page); > } else if (rc != 0) { > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 5b883a0..ba1152b 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -2260,6 +2260,11 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs) > * the flush returns error? > */ > rc = filemap_write_and_wait(inode->i_mapping); > + if (is_interrupt_error(rc)) { > + rc = -ERESTARTSYS; > + goto out; > + } > + > mapping_set_error(inode->i_mapping, rc); > rc = 0; > > @@ -2403,6 +2408,11 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs) > * the flush returns error? > */ > rc = filemap_write_and_wait(inode->i_mapping); > + if (is_interrupt_error(rc)) { > + rc = -ERESTARTSYS; > + goto cifs_setattr_exit; > + } > + > mapping_set_error(inode->i_mapping, rc); > rc = 0; > Took me a minute to look over this code again, but I think this is OK. Acked-by: Jeff Layton <jlayton@kernel.org>
On Fri, 11 Jan 2019 at 05:07, Jeff Layton <jlayton@kernel.org>: > Thanks for taking a look. > On Thu, 2019-01-10 at 14:24 -0800, Pavel Shilovsky wrote: > > This patch aims to address writeback code problems related to error > > paths. In particular it respects EINTR and related error codes and > > stores the first error occured during writeback in order to return it > > to a caller. > > > > The current semantic with respect to fsync is to return the last error > code. It seem that write_cache_pages (which is being called from generic_writepages) returns the first error occurred during writeback in WB_SYNC_ALL cases: 2231 error = (*writepage)(page, wbc, data); 2232 if (unlikely(error)) { 2233 /* 2234 * Handle errors according to the type of 2235 * writeback. There's no need to continue for 2236 * background writeback. Just push done_index 2237 * past this page so media errors won't choke 2238 * writeout for the entire file. For integrity 2239 * writeback, we must process the entire dirty 2240 * set regardless of errors because the fs may 2241 * still have state to clear for each page. In 2242 * that case we continue processing and return 2243 * the first error. 2244 */ 2245 if (error == AOP_WRITEPAGE_ACTIVATE) { 2246 unlock_page(page); 2247 error = 0; 2248 } else if (wbc->sync_mode != WB_SYNC_ALL) { 2249 ret = error; 2250 done_index = page->index + 1; 2251 done = 1; 2252 break; 2253 } 2254 if (!ret) 2255 ret = error; ... 2284 return ret; (https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=blob;f=mm/page-writeback.c;h=7d1010453fb95a26c13e9004999d028659815987;hb=48d2ba6257013676e57ff69444d5212031aee763#l2254) The idea of the patch was to follow the same semantic. > > > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > > --- > > fs/cifs/cifsglob.h | 19 +++++++++++++++++++ > > fs/cifs/cifssmb.c | 7 ++++--- > > fs/cifs/file.c | 29 +++++++++++++++++++++++------ > > fs/cifs/inode.c | 10 ++++++++++ > > 4 files changed, 56 insertions(+), 9 deletions(-) > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index 7709268..94dbdbe 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -1575,6 +1575,25 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, > > kfree(param); > > } > > > > +static inline bool is_interrupt_error(int error) > > +{ > > + switch (error) { > > + case -EINTR: > > + case -ERESTARTSYS: > > + case -ERESTARTNOHAND: > > + case -ERESTARTNOINTR: > > + return true; > > + } > > + return false; > > +} > > + > > +static inline bool is_retryable_error(int error) > > +{ > > + if (is_interrupt_error(error) || error == -EAGAIN) > > + return true; > > + return false; > > +} > > + > > #define MID_FREE 0 > > #define MID_REQUEST_ALLOCATED 1 > > #define MID_REQUEST_SUBMITTED 2 > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > > index b1f49c1..6930cdb 100644 > > --- a/fs/cifs/cifssmb.c > > +++ b/fs/cifs/cifssmb.c > > @@ -2114,7 +2114,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > > > for (j = 0; j < nr_pages; j++) { > > unlock_page(wdata2->pages[j]); > > - if (rc != 0 && rc != -EAGAIN) { > > + if (rc != 0 && !is_retryable_error(rc)) { > > SetPageError(wdata2->pages[j]); > > end_page_writeback(wdata2->pages[j]); > > put_page(wdata2->pages[j]); > > @@ -2123,7 +2123,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > > > if (rc) { > > kref_put(&wdata2->refcount, cifs_writedata_release); > > - if (rc == -EAGAIN) > > + if (is_retryable_error(rc)) > > continue; > > break; > > } > > @@ -2132,7 +2132,8 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > i += nr_pages; > > } while (i < wdata->nr_pages); > > > > - mapping_set_error(inode->i_mapping, rc); > > + if (rc != 0 && !is_retryable_error(rc)) > > + mapping_set_error(inode->i_mapping, rc); > > kref_put(&wdata->refcount, cifs_writedata_release); > > } > > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > > index c23bf9d..109b1ef 100644 > > --- a/fs/cifs/file.c > > +++ b/fs/cifs/file.c > > @@ -732,7 +732,8 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush) > > > > if (can_flush) { > > rc = filemap_write_and_wait(inode->i_mapping); > > - mapping_set_error(inode->i_mapping, rc); > > + if (!is_interrupt_error(rc)) > > + mapping_set_error(inode->i_mapping, rc); > > > > if (tcon->unix_ext) > > rc = cifs_get_inode_info_unix(&inode, full_path, > > @@ -2109,6 +2110,7 @@ static int cifs_writepages(struct address_space *mapping, > > pgoff_t end, index; > > struct cifs_writedata *wdata; > > int rc = 0; > > + int saved_rc = 0; > > unsigned int xid; > > > > /* > > @@ -2137,8 +2139,10 @@ static int cifs_writepages(struct address_space *mapping, > > > > rc = server->ops->wait_mtu_credits(server, cifs_sb->wsize, > > &wsize, &credits); > > - if (rc) > > + if (rc != 0) { > > + done = true; > > break; > > + } > > > > tofind = min((wsize / PAGE_SIZE) - 1, end - index) + 1; > > > > @@ -2146,6 +2150,7 @@ static int cifs_writepages(struct address_space *mapping, > > &found_pages); > > if (!wdata) { > > rc = -ENOMEM; > > + done = true; > > add_credits_and_wake_if(server, credits, 0); > > break; > > } > > @@ -2174,7 +2179,7 @@ static int cifs_writepages(struct address_space *mapping, > > if (rc != 0) { > > add_credits_and_wake_if(server, wdata->credits, 0); > > for (i = 0; i < nr_pages; ++i) { > > - if (rc == -EAGAIN) > > + if (is_retryable_error(rc)) > > redirty_page_for_writepage(wbc, > > wdata->pages[i]); > > else > > @@ -2182,7 +2187,7 @@ static int cifs_writepages(struct address_space *mapping, > > end_page_writeback(wdata->pages[i]); > > put_page(wdata->pages[i]); > > } > > - if (rc != -EAGAIN) > > + if (!is_retryable_error(rc)) > > mapping_set_error(mapping, rc); > > } > > kref_put(&wdata->refcount, cifs_writedata_release); > > @@ -2192,6 +2197,15 @@ static int cifs_writepages(struct address_space *mapping, > > continue; > > } > > > > + /* Return immediately if we received a signal during writing */ > > + if (is_interrupt_error(rc)) { > > + done = true; > > + break; > > + } > > + > > + if (rc != 0 && saved_rc == 0) > > + saved_rc = rc; > > + > > wbc->nr_to_write -= nr_pages; > > if (wbc->nr_to_write <= 0) > > done = true; > > @@ -2209,6 +2223,9 @@ static int cifs_writepages(struct address_space *mapping, > > goto retry; > > } > > > > + if (saved_rc != 0) > > + rc = saved_rc; > > + > > if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0)) > > mapping->writeback_index = index; > > > > @@ -2241,8 +2258,8 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc) > > set_page_writeback(page); > > retry_write: > > rc = cifs_partialpagewrite(page, 0, PAGE_SIZE); > > - if (rc == -EAGAIN) { > > - if (wbc->sync_mode == WB_SYNC_ALL) > > + if (is_retryable_error(rc)) { > > + if (wbc->sync_mode == WB_SYNC_ALL && rc == -EAGAIN) > > goto retry_write; > > redirty_page_for_writepage(wbc, page); > > } else if (rc != 0) { > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > > index 5b883a0..ba1152b 100644 > > --- a/fs/cifs/inode.c > > +++ b/fs/cifs/inode.c > > @@ -2260,6 +2260,11 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs) > > * the flush returns error? > > */ > > rc = filemap_write_and_wait(inode->i_mapping); > > + if (is_interrupt_error(rc)) { > > + rc = -ERESTARTSYS; > > + goto out; > > + } > > + > > mapping_set_error(inode->i_mapping, rc); > > rc = 0; > > > > @@ -2403,6 +2408,11 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs) > > * the flush returns error? > > */ > > rc = filemap_write_and_wait(inode->i_mapping); > > + if (is_interrupt_error(rc)) { > > + rc = -ERESTARTSYS; > > + goto cifs_setattr_exit; > > + } > > + > > mapping_set_error(inode->i_mapping, rc); > > rc = 0; > > > > Took me a minute to look over this code again, but I think this is OK. > > Acked-by: Jeff Layton <jlayton@kernel.org> > -- Best regards, Pavel Shilovsky
I doubt that first vs. last is going to matter much in this case - and first error seems intuitive to me. On Fri, Jan 11, 2019 at 12:21 PM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > On Fri, 11 Jan 2019 at 05:07, Jeff Layton <jlayton@kernel.org>: > > > > Thanks for taking a look. > > > On Thu, 2019-01-10 at 14:24 -0800, Pavel Shilovsky wrote: > > > This patch aims to address writeback code problems related to error > > > paths. In particular it respects EINTR and related error codes and > > > stores the first error occured during writeback in order to return it > > > to a caller. > > > > > > > The current semantic with respect to fsync is to return the last error > > code. > > It seem that write_cache_pages (which is being called from > generic_writepages) returns the first error occurred during writeback > in WB_SYNC_ALL cases: > > 2231 error = (*writepage)(page, wbc, data); > 2232 if (unlikely(error)) { > 2233 /* > 2234 * Handle errors according to the type of > 2235 * writeback. There's no need to > continue for > 2236 * background writeback. Just > push done_index > 2237 * past this page so media errors > won't choke > 2238 * writeout for the entire file. > For integrity > 2239 * writeback, we must process the > entire dirty > 2240 * set regardless of errors > because the fs may > 2241 * still have state to clear for > each page. In > 2242 * that case we continue > processing and return > 2243 * the first error. > 2244 */ > 2245 if (error == AOP_WRITEPAGE_ACTIVATE) { > 2246 unlock_page(page); > 2247 error = 0; > 2248 } else if (wbc->sync_mode != WB_SYNC_ALL) { > 2249 ret = error; > 2250 done_index = page->index + 1; > 2251 done = 1; > 2252 break; > 2253 } > 2254 if (!ret) > 2255 ret = error; > ... > 2284 return ret; > > (https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=blob;f=mm/page-writeback.c;h=7d1010453fb95a26c13e9004999d028659815987;hb=48d2ba6257013676e57ff69444d5212031aee763#l2254) > > The idea of the patch was to follow the same semantic. > > > > > > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > > > --- > > > fs/cifs/cifsglob.h | 19 +++++++++++++++++++ > > > fs/cifs/cifssmb.c | 7 ++++--- > > > fs/cifs/file.c | 29 +++++++++++++++++++++++------ > > > fs/cifs/inode.c | 10 ++++++++++ > > > 4 files changed, 56 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > > index 7709268..94dbdbe 100644 > > > --- a/fs/cifs/cifsglob.h > > > +++ b/fs/cifs/cifsglob.h > > > @@ -1575,6 +1575,25 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, > > > kfree(param); > > > } > > > > > > +static inline bool is_interrupt_error(int error) > > > +{ > > > + switch (error) { > > > + case -EINTR: > > > + case -ERESTARTSYS: > > > + case -ERESTARTNOHAND: > > > + case -ERESTARTNOINTR: > > > + return true; > > > + } > > > + return false; > > > +} > > > + > > > +static inline bool is_retryable_error(int error) > > > +{ > > > + if (is_interrupt_error(error) || error == -EAGAIN) > > > + return true; > > > + return false; > > > +} > > > + > > > #define MID_FREE 0 > > > #define MID_REQUEST_ALLOCATED 1 > > > #define MID_REQUEST_SUBMITTED 2 > > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > > > index b1f49c1..6930cdb 100644 > > > --- a/fs/cifs/cifssmb.c > > > +++ b/fs/cifs/cifssmb.c > > > @@ -2114,7 +2114,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > > > > > for (j = 0; j < nr_pages; j++) { > > > unlock_page(wdata2->pages[j]); > > > - if (rc != 0 && rc != -EAGAIN) { > > > + if (rc != 0 && !is_retryable_error(rc)) { > > > SetPageError(wdata2->pages[j]); > > > end_page_writeback(wdata2->pages[j]); > > > put_page(wdata2->pages[j]); > > > @@ -2123,7 +2123,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > > > > > if (rc) { > > > kref_put(&wdata2->refcount, cifs_writedata_release); > > > - if (rc == -EAGAIN) > > > + if (is_retryable_error(rc)) > > > continue; > > > break; > > > } > > > @@ -2132,7 +2132,8 @@ cifs_writev_requeue(struct cifs_writedata *wdata) > > > i += nr_pages; > > > } while (i < wdata->nr_pages); > > > > > > - mapping_set_error(inode->i_mapping, rc); > > > + if (rc != 0 && !is_retryable_error(rc)) > > > + mapping_set_error(inode->i_mapping, rc); > > > kref_put(&wdata->refcount, cifs_writedata_release); > > > } > > > > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > > > index c23bf9d..109b1ef 100644 > > > --- a/fs/cifs/file.c > > > +++ b/fs/cifs/file.c > > > @@ -732,7 +732,8 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush) > > > > > > if (can_flush) { > > > rc = filemap_write_and_wait(inode->i_mapping); > > > - mapping_set_error(inode->i_mapping, rc); > > > + if (!is_interrupt_error(rc)) > > > + mapping_set_error(inode->i_mapping, rc); > > > > > > if (tcon->unix_ext) > > > rc = cifs_get_inode_info_unix(&inode, full_path, > > > @@ -2109,6 +2110,7 @@ static int cifs_writepages(struct address_space *mapping, > > > pgoff_t end, index; > > > struct cifs_writedata *wdata; > > > int rc = 0; > > > + int saved_rc = 0; > > > unsigned int xid; > > > > > > /* > > > @@ -2137,8 +2139,10 @@ static int cifs_writepages(struct address_space *mapping, > > > > > > rc = server->ops->wait_mtu_credits(server, cifs_sb->wsize, > > > &wsize, &credits); > > > - if (rc) > > > + if (rc != 0) { > > > + done = true; > > > break; > > > + } > > > > > > tofind = min((wsize / PAGE_SIZE) - 1, end - index) + 1; > > > > > > @@ -2146,6 +2150,7 @@ static int cifs_writepages(struct address_space *mapping, > > > &found_pages); > > > if (!wdata) { > > > rc = -ENOMEM; > > > + done = true; > > > add_credits_and_wake_if(server, credits, 0); > > > break; > > > } > > > @@ -2174,7 +2179,7 @@ static int cifs_writepages(struct address_space *mapping, > > > if (rc != 0) { > > > add_credits_and_wake_if(server, wdata->credits, 0); > > > for (i = 0; i < nr_pages; ++i) { > > > - if (rc == -EAGAIN) > > > + if (is_retryable_error(rc)) > > > redirty_page_for_writepage(wbc, > > > wdata->pages[i]); > > > else > > > @@ -2182,7 +2187,7 @@ static int cifs_writepages(struct address_space *mapping, > > > end_page_writeback(wdata->pages[i]); > > > put_page(wdata->pages[i]); > > > } > > > - if (rc != -EAGAIN) > > > + if (!is_retryable_error(rc)) > > > mapping_set_error(mapping, rc); > > > } > > > kref_put(&wdata->refcount, cifs_writedata_release); > > > @@ -2192,6 +2197,15 @@ static int cifs_writepages(struct address_space *mapping, > > > continue; > > > } > > > > > > + /* Return immediately if we received a signal during writing */ > > > + if (is_interrupt_error(rc)) { > > > + done = true; > > > + break; > > > + } > > > + > > > + if (rc != 0 && saved_rc == 0) > > > + saved_rc = rc; > > > + > > > wbc->nr_to_write -= nr_pages; > > > if (wbc->nr_to_write <= 0) > > > done = true; > > > @@ -2209,6 +2223,9 @@ static int cifs_writepages(struct address_space *mapping, > > > goto retry; > > > } > > > > > > + if (saved_rc != 0) > > > + rc = saved_rc; > > > + > > > if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0)) > > > mapping->writeback_index = index; > > > > > > @@ -2241,8 +2258,8 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc) > > > set_page_writeback(page); > > > retry_write: > > > rc = cifs_partialpagewrite(page, 0, PAGE_SIZE); > > > - if (rc == -EAGAIN) { > > > - if (wbc->sync_mode == WB_SYNC_ALL) > > > + if (is_retryable_error(rc)) { > > > + if (wbc->sync_mode == WB_SYNC_ALL && rc == -EAGAIN) > > > goto retry_write; > > > redirty_page_for_writepage(wbc, page); > > > } else if (rc != 0) { > > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > > > index 5b883a0..ba1152b 100644 > > > --- a/fs/cifs/inode.c > > > +++ b/fs/cifs/inode.c > > > @@ -2260,6 +2260,11 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs) > > > * the flush returns error? > > > */ > > > rc = filemap_write_and_wait(inode->i_mapping); > > > + if (is_interrupt_error(rc)) { > > > + rc = -ERESTARTSYS; > > > + goto out; > > > + } > > > + > > > mapping_set_error(inode->i_mapping, rc); > > > rc = 0; > > > > > > @@ -2403,6 +2408,11 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs) > > > * the flush returns error? > > > */ > > > rc = filemap_write_and_wait(inode->i_mapping); > > > + if (is_interrupt_error(rc)) { > > > + rc = -ERESTARTSYS; > > > + goto cifs_setattr_exit; > > > + } > > > + > > > mapping_set_error(inode->i_mapping, rc); > > > rc = 0; > > > > > > > Took me a minute to look over this code again, but I think this is OK. > > > > Acked-by: Jeff Layton <jlayton@kernel.org> > > > > -- > Best regards, > Pavel Shilovsky
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 7709268..94dbdbe 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1575,6 +1575,25 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, kfree(param); } +static inline bool is_interrupt_error(int error) +{ + switch (error) { + case -EINTR: + case -ERESTARTSYS: + case -ERESTARTNOHAND: + case -ERESTARTNOINTR: + return true; + } + return false; +} + +static inline bool is_retryable_error(int error) +{ + if (is_interrupt_error(error) || error == -EAGAIN) + return true; + return false; +} + #define MID_FREE 0 #define MID_REQUEST_ALLOCATED 1 #define MID_REQUEST_SUBMITTED 2 diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index b1f49c1..6930cdb 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -2114,7 +2114,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) for (j = 0; j < nr_pages; j++) { unlock_page(wdata2->pages[j]); - if (rc != 0 && rc != -EAGAIN) { + if (rc != 0 && !is_retryable_error(rc)) { SetPageError(wdata2->pages[j]); end_page_writeback(wdata2->pages[j]); put_page(wdata2->pages[j]); @@ -2123,7 +2123,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata) if (rc) { kref_put(&wdata2->refcount, cifs_writedata_release); - if (rc == -EAGAIN) + if (is_retryable_error(rc)) continue; break; } @@ -2132,7 +2132,8 @@ cifs_writev_requeue(struct cifs_writedata *wdata) i += nr_pages; } while (i < wdata->nr_pages); - mapping_set_error(inode->i_mapping, rc); + if (rc != 0 && !is_retryable_error(rc)) + mapping_set_error(inode->i_mapping, rc); kref_put(&wdata->refcount, cifs_writedata_release); } diff --git a/fs/cifs/file.c b/fs/cifs/file.c index c23bf9d..109b1ef 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -732,7 +732,8 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush) if (can_flush) { rc = filemap_write_and_wait(inode->i_mapping); - mapping_set_error(inode->i_mapping, rc); + if (!is_interrupt_error(rc)) + mapping_set_error(inode->i_mapping, rc); if (tcon->unix_ext) rc = cifs_get_inode_info_unix(&inode, full_path, @@ -2109,6 +2110,7 @@ static int cifs_writepages(struct address_space *mapping, pgoff_t end, index; struct cifs_writedata *wdata; int rc = 0; + int saved_rc = 0; unsigned int xid; /* @@ -2137,8 +2139,10 @@ static int cifs_writepages(struct address_space *mapping, rc = server->ops->wait_mtu_credits(server, cifs_sb->wsize, &wsize, &credits); - if (rc) + if (rc != 0) { + done = true; break; + } tofind = min((wsize / PAGE_SIZE) - 1, end - index) + 1; @@ -2146,6 +2150,7 @@ static int cifs_writepages(struct address_space *mapping, &found_pages); if (!wdata) { rc = -ENOMEM; + done = true; add_credits_and_wake_if(server, credits, 0); break; } @@ -2174,7 +2179,7 @@ static int cifs_writepages(struct address_space *mapping, if (rc != 0) { add_credits_and_wake_if(server, wdata->credits, 0); for (i = 0; i < nr_pages; ++i) { - if (rc == -EAGAIN) + if (is_retryable_error(rc)) redirty_page_for_writepage(wbc, wdata->pages[i]); else @@ -2182,7 +2187,7 @@ static int cifs_writepages(struct address_space *mapping, end_page_writeback(wdata->pages[i]); put_page(wdata->pages[i]); } - if (rc != -EAGAIN) + if (!is_retryable_error(rc)) mapping_set_error(mapping, rc); } kref_put(&wdata->refcount, cifs_writedata_release); @@ -2192,6 +2197,15 @@ static int cifs_writepages(struct address_space *mapping, continue; } + /* Return immediately if we received a signal during writing */ + if (is_interrupt_error(rc)) { + done = true; + break; + } + + if (rc != 0 && saved_rc == 0) + saved_rc = rc; + wbc->nr_to_write -= nr_pages; if (wbc->nr_to_write <= 0) done = true; @@ -2209,6 +2223,9 @@ static int cifs_writepages(struct address_space *mapping, goto retry; } + if (saved_rc != 0) + rc = saved_rc; + if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0)) mapping->writeback_index = index; @@ -2241,8 +2258,8 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc) set_page_writeback(page); retry_write: rc = cifs_partialpagewrite(page, 0, PAGE_SIZE); - if (rc == -EAGAIN) { - if (wbc->sync_mode == WB_SYNC_ALL) + if (is_retryable_error(rc)) { + if (wbc->sync_mode == WB_SYNC_ALL && rc == -EAGAIN) goto retry_write; redirty_page_for_writepage(wbc, page); } else if (rc != 0) { diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index 5b883a0..ba1152b 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -2260,6 +2260,11 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs) * the flush returns error? */ rc = filemap_write_and_wait(inode->i_mapping); + if (is_interrupt_error(rc)) { + rc = -ERESTARTSYS; + goto out; + } + mapping_set_error(inode->i_mapping, rc); rc = 0; @@ -2403,6 +2408,11 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs) * the flush returns error? */ rc = filemap_write_and_wait(inode->i_mapping); + if (is_interrupt_error(rc)) { + rc = -ERESTARTSYS; + goto cifs_setattr_exit; + } + mapping_set_error(inode->i_mapping, rc); rc = 0;
This patch aims to address writeback code problems related to error paths. In particular it respects EINTR and related error codes and stores the first error occured during writeback in order to return it to a caller. Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> --- fs/cifs/cifsglob.h | 19 +++++++++++++++++++ fs/cifs/cifssmb.c | 7 ++++--- fs/cifs/file.c | 29 +++++++++++++++++++++++------ fs/cifs/inode.c | 10 ++++++++++ 4 files changed, 56 insertions(+), 9 deletions(-)