linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Fwd: [PATCH 0/7] SMB3 credit flow control handling and writeback fixes
       [not found] <1547159098-19011-1-git-send-email-pshilov@microsoft.com>
@ 2019-01-11  6:22 ` Steve French
  2019-01-11  6:22   ` Steve French
       [not found] ` <1547159098-19011-8-git-send-email-pshilov@microsoft.com>
       [not found] ` <1547159098-19011-5-git-send-email-pshilov@microsoft.com>
  2 siblings, 1 reply; 6+ messages in thread
From: Steve French @ 2019-01-11  6:22 UTC (permalink / raw)
  To: bfoster, Jeff Layton, linux-fsdevel; +Cc: LKML, Pavel Shilovsky, CIFS

This is an important series, addressing various problems with
crediting (flow control) but more importantly patches 4 and 7 deal
with error cases in writeback, in writepages and writepage which may
be of more general interest.     Brian and Jeff have changed loosely
related code in the VFS and may have insight into whether additional
changes would be helpful beyond what Pavel has done in patches 4 and 7
in this series.

---------- Forwarded message ---------
From: Pavel Shilovsky <piastryyy@gmail.com>
Date: Thu, Jan 10, 2019 at 4:25 PM
Subject: [PATCH 0/7] SMB3 credit flow control handling and writeback fixes
To: <linux-cifs@vger.kernel.org>
Cc: Steve French <smfrench@gmail.com>, Ronnie Sahlberg <lsahlber@redhat.com>


This is the series of patches that address few problems with the current code.

Patch #1 adjusts credits properly for MTU credits if we didn't use all the
credits taken for an operation. This prevents loosing credits in error paths.

Patch #2 removes resetting credit number to 1 if server didn't grant us
any credits for the compounding chain. The current code didn't follow
the protocol correctly.

Patch #3 fixes credits handling for compounded requests by taking
one credit per part of the chain. All error paths that return credits
back are fixed accordingly.

Patch #4 fixes hiding EINTR error code returned after an interrupted attemp
to send SMB packet through the network.

Patch #5 removes wrong assumption that the server grants us one credit
for a cancelled request and parse the actual response to get the credit
number.

Patch #6 moves process credits granted by the server in the demultiplex
thread in order to avoid races with reconnects.

Patch #7 addresses few problems in writeback code, in particular related
to interrupted error code returned by the transport layer.

Patches #1-4 as marked for stable. Reviews are welcome.

Pavel Shilovsky (7):
  CIFS: Fix adjustment of credits for MTU requests
  CIFS: Do not set credits to 1 if the server didn't grant anything
  CIFS: Fix credit computation for compounded requests
  CIFS: Do not hide EINTR while sending network packets
  CIFS: Fix credits calculation for cancelled requests
  CIFS: Move credit processing to mid callbacks for SMB3
  CIFS: Fix error paths in writeback code

 fs/cifs/cifsglob.h  |  20 +++++++++
 fs/cifs/cifssmb.c   |   7 +--
 fs/cifs/file.c      |  29 ++++++++++---
 fs/cifs/inode.c     |  10 +++++
 fs/cifs/smb2pdu.c   |   8 +++-
 fs/cifs/transport.c | 122 +++++++++++++++++++++++++++++++++++++++-------------
 6 files changed, 155 insertions(+), 41 deletions(-)

--
2.7.4



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Fwd: [PATCH 0/7] SMB3 credit flow control handling and writeback fixes
  2019-01-11  6:22 ` Fwd: [PATCH 0/7] SMB3 credit flow control handling and writeback fixes Steve French
@ 2019-01-11  6:22   ` Steve French
  0 siblings, 0 replies; 6+ messages in thread
From: Steve French @ 2019-01-11  6:22 UTC (permalink / raw)
  To: bfoster, Jeff Layton, linux-fsdevel; +Cc: LKML, Pavel Shilovsky, CIFS

This is an important series, addressing various problems with
crediting (flow control) but more importantly patches 4 and 7 deal
with error cases in writeback, in writepages and writepage which may
be of more general interest.     Brian and Jeff have changed loosely
related code in the VFS and may have insight into whether additional
changes would be helpful beyond what Pavel has done in patches 4 and 7
in this series.

---------- Forwarded message ---------
From: Pavel Shilovsky <piastryyy@gmail.com>
Date: Thu, Jan 10, 2019 at 4:25 PM
Subject: [PATCH 0/7] SMB3 credit flow control handling and writeback fixes
To: <linux-cifs@vger.kernel.org>
Cc: Steve French <smfrench@gmail.com>, Ronnie Sahlberg <lsahlber@redhat.com>


This is the series of patches that address few problems with the current code.

Patch #1 adjusts credits properly for MTU credits if we didn't use all the
credits taken for an operation. This prevents loosing credits in error paths.

Patch #2 removes resetting credit number to 1 if server didn't grant us
any credits for the compounding chain. The current code didn't follow
the protocol correctly.

Patch #3 fixes credits handling for compounded requests by taking
one credit per part of the chain. All error paths that return credits
back are fixed accordingly.

Patch #4 fixes hiding EINTR error code returned after an interrupted attemp
to send SMB packet through the network.

Patch #5 removes wrong assumption that the server grants us one credit
for a cancelled request and parse the actual response to get the credit
number.

Patch #6 moves process credits granted by the server in the demultiplex
thread in order to avoid races with reconnects.

Patch #7 addresses few problems in writeback code, in particular related
to interrupted error code returned by the transport layer.

Patches #1-4 as marked for stable. Reviews are welcome.

Pavel Shilovsky (7):
  CIFS: Fix adjustment of credits for MTU requests
  CIFS: Do not set credits to 1 if the server didn't grant anything
  CIFS: Fix credit computation for compounded requests
  CIFS: Do not hide EINTR while sending network packets
  CIFS: Fix credits calculation for cancelled requests
  CIFS: Move credit processing to mid callbacks for SMB3
  CIFS: Fix error paths in writeback code

 fs/cifs/cifsglob.h  |  20 +++++++++
 fs/cifs/cifssmb.c   |   7 +--
 fs/cifs/file.c      |  29 ++++++++++---
 fs/cifs/inode.c     |  10 +++++
 fs/cifs/smb2pdu.c   |   8 +++-
 fs/cifs/transport.c | 122 +++++++++++++++++++++++++++++++++++++++-------------
 6 files changed, 155 insertions(+), 41 deletions(-)

--
2.7.4



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Fwd: [PATCH 7/7] CIFS: Fix error paths in writeback code
       [not found] ` <1547159098-19011-8-git-send-email-pshilov@microsoft.com>
@ 2019-01-11  6:24   ` Steve French
  2019-01-11  6:24     ` Steve French
  0 siblings, 1 reply; 6+ messages in thread
From: Steve French @ 2019-01-11  6:24 UTC (permalink / raw)
  To: linux-fsdevel, bfoster, Jeff Layton; +Cc: Pavel Shilovskiy, CIFS, LKML

These changes in writepage and writepages for cifs remind me of
loosely related writeback changes that Jeff and Brian have done in the
VFS/MM layer, so was hopeful that others might have comments if they
see anything missing from Pavel's patch.

---------- Forwarded message ---------
From: Pavel Shilovsky <piastryyy@gmail.com>
Date: Thu, Jan 10, 2019 at 4:25 PM
Subject: [PATCH 7/7] CIFS: Fix error paths in writeback code
To: <linux-cifs@vger.kernel.org>
Cc: Steve French <smfrench@gmail.com>, Ronnie Sahlberg <lsahlber@redhat.com>


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(-)

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;

--
2.7.4



-- 
Thanks,

Steve

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Fwd: [PATCH 7/7] CIFS: Fix error paths in writeback code
  2019-01-11  6:24   ` Fwd: [PATCH 7/7] CIFS: Fix error paths in writeback code Steve French
@ 2019-01-11  6:24     ` Steve French
  0 siblings, 0 replies; 6+ messages in thread
From: Steve French @ 2019-01-11  6:24 UTC (permalink / raw)
  To: linux-fsdevel, bfoster, Jeff Layton; +Cc: Pavel Shilovskiy, CIFS, LKML

These changes in writepage and writepages for cifs remind me of
loosely related writeback changes that Jeff and Brian have done in the
VFS/MM layer, so was hopeful that others might have comments if they
see anything missing from Pavel's patch.

---------- Forwarded message ---------
From: Pavel Shilovsky <piastryyy@gmail.com>
Date: Thu, Jan 10, 2019 at 4:25 PM
Subject: [PATCH 7/7] CIFS: Fix error paths in writeback code
To: <linux-cifs@vger.kernel.org>
Cc: Steve French <smfrench@gmail.com>, Ronnie Sahlberg <lsahlber@redhat.com>


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(-)

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;

--
2.7.4



-- 
Thanks,

Steve

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Fwd: [PATCH 4/7] CIFS: Do not hide EINTR after sending network packets
       [not found] ` <1547159098-19011-5-git-send-email-pshilov@microsoft.com>
@ 2019-01-11  6:29   ` Steve French
  2019-01-11  6:29     ` Steve French
  0 siblings, 1 reply; 6+ messages in thread
From: Steve French @ 2019-01-11  6:29 UTC (permalink / raw)
  To: linux-fsdevel, LKML

Pavel's patch looks correct to me, but since this may be a common
issue, how to better handle unexpected EINTR on sock_sendmsg, others
outside cifs/smb3 world may have run into this before and have
additional comments.

---------- Forwarded message ---------
From: Pavel Shilovsky <piastryyy@gmail.com>
Date: Thu, Jan 10, 2019 at 4:25 PM
Subject: [PATCH 4/7] CIFS: Do not hide EINTR after sending network packets
To: <linux-cifs@vger.kernel.org>
Cc: Steve French <smfrench@gmail.com>, Ronnie Sahlberg <lsahlber@redhat.com>


Currently we hide EINTR code returned from sock_sendmsg()
and return 0 instead. This makes a caller think that we
have successfully completed the network operation which is
not true. Fix this by properly returning EINTR to callers.

Cc: <stable@vger.kernel.org>
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
---
 fs/cifs/transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index e047f06..aaff9c5 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -387,7 +387,7 @@ __smb_send_rqst(struct TCP_Server_Info *server,
int num_rqst,
        if (rc < 0 && rc != -EINTR)
                cifs_dbg(VFS, "Error %d sending data on socket to server\n",
                         rc);
-       else
+       else if (rc > 0)
                rc = 0;

        return rc;
--
2.7.4



-- 
Thanks,

Steve

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Fwd: [PATCH 4/7] CIFS: Do not hide EINTR after sending network packets
  2019-01-11  6:29   ` Fwd: [PATCH 4/7] CIFS: Do not hide EINTR after sending network packets Steve French
@ 2019-01-11  6:29     ` Steve French
  0 siblings, 0 replies; 6+ messages in thread
From: Steve French @ 2019-01-11  6:29 UTC (permalink / raw)
  To: linux-fsdevel, LKML

Pavel's patch looks correct to me, but since this may be a common
issue, how to better handle unexpected EINTR on sock_sendmsg, others
outside cifs/smb3 world may have run into this before and have
additional comments.

---------- Forwarded message ---------
From: Pavel Shilovsky <piastryyy@gmail.com>
Date: Thu, Jan 10, 2019 at 4:25 PM
Subject: [PATCH 4/7] CIFS: Do not hide EINTR after sending network packets
To: <linux-cifs@vger.kernel.org>
Cc: Steve French <smfrench@gmail.com>, Ronnie Sahlberg <lsahlber@redhat.com>


Currently we hide EINTR code returned from sock_sendmsg()
and return 0 instead. This makes a caller think that we
have successfully completed the network operation which is
not true. Fix this by properly returning EINTR to callers.

Cc: <stable@vger.kernel.org>
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
---
 fs/cifs/transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index e047f06..aaff9c5 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -387,7 +387,7 @@ __smb_send_rqst(struct TCP_Server_Info *server,
int num_rqst,
        if (rc < 0 && rc != -EINTR)
                cifs_dbg(VFS, "Error %d sending data on socket to server\n",
                         rc);
-       else
+       else if (rc > 0)
                rc = 0;

        return rc;
--
2.7.4



-- 
Thanks,

Steve

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-01-11  6:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1547159098-19011-1-git-send-email-pshilov@microsoft.com>
2019-01-11  6:22 ` Fwd: [PATCH 0/7] SMB3 credit flow control handling and writeback fixes Steve French
2019-01-11  6:22   ` Steve French
     [not found] ` <1547159098-19011-8-git-send-email-pshilov@microsoft.com>
2019-01-11  6:24   ` Fwd: [PATCH 7/7] CIFS: Fix error paths in writeback code Steve French
2019-01-11  6:24     ` Steve French
     [not found] ` <1547159098-19011-5-git-send-email-pshilov@microsoft.com>
2019-01-11  6:29   ` Fwd: [PATCH 4/7] CIFS: Do not hide EINTR after sending network packets Steve French
2019-01-11  6:29     ` Steve French

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).