чт, 21 янв. 2021 г. в 04:16, Paulo Alcantara <pc@xxxxxx>:
>
> Aurélien Aptel <aaptel@xxxxxxxx> writes:
>
> > Pavel Shilovsky <piastryyy@xxxxxxxxx> writes:
> >
> >> вт, 19 янв. 2021 г. в 22:38, Steve French <smfrench@xxxxxxxxx>:
> >>>
> >>> The patch won't merge (also has some text corruptions in it). This
> >>> line of code is different due to commit 6988a619f5b79
> >>>
> >>> 6988a619f5b79 (Paulo Alcantara 2020-11-28 15:57:06 -0300 342)
> >>> cifs_dbg(FYI, "signal pending before send request\n");
> >>> 6988a619f5b79 (Paulo Alcantara 2020-11-28 15:57:06 -0300 343)
> >>> return -ERESTARTSYS;
> >>>
> >>> if (signal_pending(current)) {
> >>> cifs_dbg(FYI, "signal pending before send request\n");
> >>> return -ERESTARTSYS;
> >>> }
> >>>
> >>> See:
> >>>
> >>> Author: Paulo Alcantara <pc@xxxxxx>
> >>> Date: Sat Nov 28 15:57:06 2020 -0300
> >>>
> >>> cifs: allow syscalls to be restarted in __smb_send_rqst()
> >>>
> >>> 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 the kernel decide to whether or not restart the syscalls when
> >>> there is a signal pending in __smb_send_rqst() by returning
> >>> -ERESTARTSYS. If it can't, it will return -EINTR anyway.
> >>>
> >>> Signed-off-by: Paulo Alcantara (SUSE) <pc@xxxxxx>
> >>> CC: Stable <stable@xxxxxxxxxxxxxxx>
> >>> Reviewed-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> >>> Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
> >>>
> >>> On Tue, Jan 19, 2021 at 10:32 PM Ronnie Sahlberg <lsahlber@xxxxxxxxxx> wrote:
> >>> >
> >>> > RHBZ 1848178
> >>> >
> >>> > There is no need to fail this function if non-fatal signals are
> >>> > pending when we enter it.
> >>> >
> >>> > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> >>> > ---
> >>> > 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 c42bda5a5008..98752f7d2cd2 100644
> >>> > --- a/fs/cifs/transport.c
> >>> > +++ b/fs/cifs/transport.c
> >>> > @@ -339,7 +339,7 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
> >>> > if (ssocket == NULL)
> >>> > return -EAGAIN;
> >>> >
> >>> > - if (signal_pending(current)) {
> >>> > + if (fatal_signal_pending(current)) {
> >>> > cifs_dbg(FYI, "signal is pending before sending any data\n");
> >>> > return -EINTR;
> >>> > }
> >
> > I've looked up the difference
> >
> > static inline int __fatal_signal_pending(struct task_struct *p)
> > {
> > return unlikely(sigismember(&p->pending.signal, SIGKILL));
> > }
> >
> >
> >> I have been thinking around the same lines. The original intent of
> >> failing the function here was to avoid interrupting packet send in the
> >> middle of the packet and not breaking an SMB connection.
> >> That's also why signals are blocked around smb_send_kvec() calls. I
> >> guess most of the time a socket buffer is not full, so, those
> >> functions immediately return success without waiting internally and
> >> checking for pending signals. With this change the code may break SMB
> >
> > Ah, interesting.
> >
> > I looked up the difference between fatal/non-fatal and it seems
> > fatal_signal_pending() really only checks for SIGKILL, but I would
> > expect ^C (SIGINT) to return quickly as well.
> >
> > I thought the point of checking for pending signal early was to return
> > quickly to userspace and not be stuck in some unkillable state.
> >
> > After reading your explanation, you're saying the kernel funcs to send
> > on socket will check for any signal and err early in any case.
> >
> > some_syscall() {
> >
> > if (pending_fatal_signal) <===== if we ignore non-fatal here
> > fail_early();
> >
> > block_signals();
> > r = kernel_socket_send {
> > if (pending_signal) <==== they will be caught here
> > return error;
As far as I understood, pending signal checking only happens if TCP
buffers are full and the kernel is waiting for the free space there to
put SMB packet. Otherwise TCP send returns immediately without waiting
and checking for signals. See:
https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp.c#L1404
https://elixir.bootlin.com/linux/latest/source/net/core/stream.c#L137
> >
> > ...
> > }
> > unblock_signals();
> > if (r)
> > fail();
> > ...
> > }
> >
> > So this patch will (potentially) trigger more reconnect (because we
> > actually send the packet as a vector in a loop) but I'm not sure I
> > understand why it returns less errors to userspace?
There are many situations when there is a non-fatal signal pending
before entering __smb_send_rqst() which won't result in being stuck
waiting for a free space in TCP buffers (most times there is a space).
So, with Ronnie's patch we allow signal to still be pending while we
are sending an SMB packet. If TCP buffers become full in the middle,
then yes, we will return the interrupt failure to the caller.
> >
> > Also, shouldn't we move the pending_fatal_signal check *inside* the blocked
> > signal section?
We most likely should, but I would avoid doing this for stable kernels
to play safe.
> >
> > In any case I think we should try to test some of those changes given
> > how we have 3,4 patches trying to tweak it on top of each other.
Agree. I also think we should amend Ronnies' patch with the change to
the following place:
-----------
/*
* If signal is pending but we have already sent the whole packet to
* the server we need to return success status to allow a corresponding
* mid entry to be kept in the pending requests queue thus allowing
* to handle responses from the server by the client.
*
* If only part of the packet has been sent there is no need to hide
* interrupt because the session will be reconnected anyway, so there
* won't be any response from the server to handle.
*/
if (signal_pending(current) && (total_len != send_length)) {
cifs_dbg(FYI, "signal is pending after attempt to send\n");
rc = -EINTR;
^^^
This should be changed to -ERESTARTSYS to allow kernel to
restart a syscall.
}
-----------
> I think it would make sense to have something like
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index e9abb41aa89b..f7292c14863e 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -340,7 +340,7 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
>
> if (signal_pending(current)) {
> cifs_dbg(FYI, "signal pending before send request\n");
> - return -ERESTARTSYS;
> + return __fatal_signal_pending(current) ? -EINTR : -ERESTARTSYS;
> }
>
> /* cork the socket */
>
> so that we allow signal handlers to be executed before restarting
> syscalls when receiving non-fatal signals, otherwise -EINTR.
Even if this is fatal signal, what harm is to return -ERESTARTSYS
unconditionally? It seems that the signal will be handled and the
process will be terminated anyway.
--
Best regards,
Pavel Shilovsky
