On Fri, 11 Jan 2019 at 05:07, Jeff Layton <jlayton@xxxxxxxxxx>: > 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@xxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> > -- Best regards, Pavel Shilovsky