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@xxxxxxxxx> wrote:
>
> 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
--
Thanks,
Steve
