[7/7] CIFS: Fix error paths in writeback code
diff mbox series

Message ID 1547159098-19011-8-git-send-email-pshilov@microsoft.com
State New
Headers show
Series
  • SMB3 credit flow control handling and writeback fixes
Related show

Commit Message

Pavel Shilovsky Jan. 10, 2019, 10:24 p.m. UTC
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(-)

Comments

Jeff Layton Jan. 11, 2019, 1:07 p.m. UTC | #1
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>
Pavel Shilovsky Jan. 11, 2019, 6:21 p.m. UTC | #2
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
Steve French Jan. 11, 2019, 6:29 p.m. UTC | #3
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

Patch
diff mbox series

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;