| Message ID | 20201128185706.8968-1-pc@cjr.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: allow syscalls to be restarted in __smb_send_rqst() | expand |
Steve French <smfrench@gmail.com> writes: > Did it fix the customer application? Yes, it did. > Thoughts on cc:stable? Looks like a great candidate, yes.
Paulo Alcantara <pc@cjr.nz> writes: > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index e27e255d40dd..55853b9ed13d 100644 > --- a/fs/cifs/transport.c > +++ b/fs/cifs/transport.c > @@ -338,10 +338,8 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst, > if (ssocket == NULL) > return -EAGAIN; > > - if (signal_pending(current)) { > - cifs_dbg(FYI, "signal is pending before sending any data\n"); > - return -EINTR; > - } > + if (signal_pending(current)) > + return -ERESTARTSYS; Can we keep the debug message call? Cheers,
Aurélien Aptel <aaptel@suse.com> writes: > Paulo Alcantara <pc@cjr.nz> writes: >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >> index e27e255d40dd..55853b9ed13d 100644 >> --- a/fs/cifs/transport.c >> +++ b/fs/cifs/transport.c >> @@ -338,10 +338,8 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst, >> if (ssocket == NULL) >> return -EAGAIN; >> >> - if (signal_pending(current)) { >> - cifs_dbg(FYI, "signal is pending before sending any data\n"); >> - return -EINTR; >> - } >> + if (signal_pending(current)) >> + return -ERESTARTSYS; > > Can we keep the debug message call? Yes. Steve, could you please update for-next with above debug msg?
On Mon, Nov 30, 2020 at 7:02 AM Paulo Alcantara <pc@cjr.nz> wrote: > > Aurélien Aptel <aaptel@suse.com> writes: > > > Paulo Alcantara <pc@cjr.nz> writes: > >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > >> index e27e255d40dd..55853b9ed13d 100644 > >> --- a/fs/cifs/transport.c > >> +++ b/fs/cifs/transport.c > >> @@ -338,10 +338,8 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst, > >> if (ssocket == NULL) > >> return -EAGAIN; > >> > >> - if (signal_pending(current)) { > >> - cifs_dbg(FYI, "signal is pending before sending any data\n"); > >> - return -EINTR; > >> - } > >> + if (signal_pending(current)) > >> + return -ERESTARTSYS; > > > > Can we keep the debug message call? > > Yes. > > Steve, could you please update for-next with above debug msg? Done. Please check to see if my lightly modified debug message text is ok.
Steve French <smfrench@gmail.com> writes: > On Mon, Nov 30, 2020 at 7:02 AM Paulo Alcantara <pc@cjr.nz> wrote: >> >> Aurélien Aptel <aaptel@suse.com> writes: >> >> > Paulo Alcantara <pc@cjr.nz> writes: >> >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >> >> index e27e255d40dd..55853b9ed13d 100644 >> >> --- a/fs/cifs/transport.c >> >> +++ b/fs/cifs/transport.c >> >> @@ -338,10 +338,8 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst, >> >> if (ssocket == NULL) >> >> return -EAGAIN; >> >> >> >> - if (signal_pending(current)) { >> >> - cifs_dbg(FYI, "signal is pending before sending any data\n"); >> >> - return -EINTR; >> >> - } >> >> + if (signal_pending(current)) >> >> + return -ERESTARTSYS; >> > >> > Can we keep the debug message call? >> >> Yes. >> >> Steve, could you please update for-next with above debug msg? > > Done. Please check to see if my lightly modified debug message text is ok. OK for me. Thanks!
Agree with the patch. The original change was inspired by the existing code returning -EINTR potentially after partially sending an SMB packet. Returning -ERESTARTSYS seems like a right thing to do here. Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> -- Best regards, Pavel Shilovsky пн, 30 нояб. 2020 г. в 08:35, Paulo Alcantara <pc@cjr.nz>: > > Steve French <smfrench@gmail.com> writes: > > > On Mon, Nov 30, 2020 at 7:02 AM Paulo Alcantara <pc@cjr.nz> wrote: > >> > >> Aurélien Aptel <aaptel@suse.com> writes: > >> > >> > Paulo Alcantara <pc@cjr.nz> writes: > >> >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > >> >> index e27e255d40dd..55853b9ed13d 100644 > >> >> --- a/fs/cifs/transport.c > >> >> +++ b/fs/cifs/transport.c > >> >> @@ -338,10 +338,8 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst, > >> >> if (ssocket == NULL) > >> >> return -EAGAIN; > >> >> > >> >> - if (signal_pending(current)) { > >> >> - cifs_dbg(FYI, "signal is pending before sending any data\n"); > >> >> - return -EINTR; > >> >> - } > >> >> + if (signal_pending(current)) > >> >> + return -ERESTARTSYS; > >> > > >> > Can we keep the debug message call? > >> > >> Yes. > >> > >> Steve, could you please update for-next with above debug msg? > > > > Done. Please check to see if my lightly modified debug message text is ok. > > OK for me. Thanks!
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index e27e255d40dd..55853b9ed13d 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -338,10 +338,8 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst, if (ssocket == NULL) return -EAGAIN; - if (signal_pending(current)) { - cifs_dbg(FYI, "signal is pending before sending any data\n"); - return -EINTR; - } + if (signal_pending(current)) + return -ERESTARTSYS; /* cork the socket */ tcp_sock_set_cork(ssocket->sk, true);
A customer has reported that several files in their multi-threaded app were left with size of 0 because most of the read(2) calls returned -EINTR and they assumed no bytes were read. Obviously, they could have fixed it by simply retrying on -EINTR. We noticed that most of the -EINTR on read(2) were due to real-time signals sent by glibc to process wide credential changes (SIGRT_1), and its signal handler had been established with SA_RESTART, in which case those calls could have been automatically restarted by the kernel. Let me the kernel decide to whether or not restart the syscalls when there is a signal pending in __smb_send_rqst() by returing -ERESTARTSYS. If it can't, it will return -EINTR anyway. Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> --- fs/cifs/transport.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)