| Message ID | 20231229111618.38887-3-sprasad@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held | expand |
On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote: > > From: Shyam Prasad N <sprasad@microsoft.com> > > cifs_pick_channel does not take into account the current > state of the channel today. As a result, if some channels > are unhealthy, they could still get picked, resulting > in unnecessary errors. > > This change checks the channel transport status before > making the choice. If all channels are unhealthy, the > primary channel will be returned anyway. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/smb/client/transport.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c > index 4f717ad7c21b..f8e6636e90a3 100644 > --- a/fs/smb/client/transport.c > +++ b/fs/smb/client/transport.c > @@ -1026,6 +1026,19 @@ struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses) > if (!server || server->terminate) > continue; > > + /* > + * do not pick a channel that's not healthy. > + * if all channels are unhealthy, we'll use > + * the primary channel > + */ > + spin_lock(&server->srv_lock); > + if (server->tcpStatus != CifsNew && > + server->tcpStatus != CifsGood) { > + spin_unlock(&server->srv_lock); > + continue; > + } > + spin_unlock(&server->srv_lock); > + > /* > * strictly speaking, we should pick up req_lock to read > * server->in_flight. But it shouldn't matter much here if we > -- > 2.34.1 > Please skip this patch. I'll submit a revised patch next week.
for-next updated to remove that patch On Fri, Dec 29, 2023 at 11:02 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote: > > > > From: Shyam Prasad N <sprasad@microsoft.com> > > > > cifs_pick_channel does not take into account the current > > state of the channel today. As a result, if some channels > > are unhealthy, they could still get picked, resulting > > in unnecessary errors. > > > > This change checks the channel transport status before > > making the choice. If all channels are unhealthy, the > > primary channel will be returned anyway. > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > > --- > > fs/smb/client/transport.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c > > index 4f717ad7c21b..f8e6636e90a3 100644 > > --- a/fs/smb/client/transport.c > > +++ b/fs/smb/client/transport.c > > @@ -1026,6 +1026,19 @@ struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses) > > if (!server || server->terminate) > > continue; > > > > + /* > > + * do not pick a channel that's not healthy. > > + * if all channels are unhealthy, we'll use > > + * the primary channel > > + */ > > + spin_lock(&server->srv_lock); > > + if (server->tcpStatus != CifsNew && > > + server->tcpStatus != CifsGood) { > > + spin_unlock(&server->srv_lock); > > + continue; > > + } > > + spin_unlock(&server->srv_lock); > > + > > /* > > * strictly speaking, we should pick up req_lock to read > > * server->in_flight. But it shouldn't matter much here if we > > -- > > 2.34.1 > > > Please skip this patch. I'll submit a revised patch next week. > > -- > Regards, > Shyam
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c index 4f717ad7c21b..f8e6636e90a3 100644 --- a/fs/smb/client/transport.c +++ b/fs/smb/client/transport.c @@ -1026,6 +1026,19 @@ struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses) if (!server || server->terminate) continue; + /* + * do not pick a channel that's not healthy. + * if all channels are unhealthy, we'll use + * the primary channel + */ + spin_lock(&server->srv_lock); + if (server->tcpStatus != CifsNew && + server->tcpStatus != CifsGood) { + spin_unlock(&server->srv_lock); + continue; + } + spin_unlock(&server->srv_lock); + /* * strictly speaking, we should pick up req_lock to read * server->in_flight. But it shouldn't matter much here if we