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
