Re: [PATCH 7/7] CIFS: Fix error paths in writeback code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux