| Message ID | 20230609174659.60327-3-sprasad@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] cifs: fix status checks in cifs_tree_connect | expand |
should this be a warn once? Could it get very noisy? On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > We've seen the in-flight count go into negative with some > internal stress testing in Microsoft. > > Adding a WARN when this happens, in hope of understanding > why this happens when it happens. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/smb/client/smb2ops.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > index 6e3be58cfe49..43162915e03c 100644 > --- a/fs/smb/client/smb2ops.c > +++ b/fs/smb/client/smb2ops.c > @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server, > server->conn_id, server->hostname, *val, > add, server->in_flight); > } > + WARN_ON(server->in_flight == 0); > server->in_flight--; > if (server->in_flight == 0 && > ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) && > -- > 2.34.1 >
On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote: > > should this be a warn once? Could it get very noisy? > > On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > We've seen the in-flight count go into negative with some > > internal stress testing in Microsoft. > > > > Adding a WARN when this happens, in hope of understanding > > why this happens when it happens. > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > > --- > > fs/smb/client/smb2ops.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > > index 6e3be58cfe49..43162915e03c 100644 > > --- a/fs/smb/client/smb2ops.c > > +++ b/fs/smb/client/smb2ops.c > > @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server, > > server->conn_id, server->hostname, *val, > > add, server->in_flight); > > } > > + WARN_ON(server->in_flight == 0); > > server->in_flight--; > > if (server->in_flight == 0 && > > ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) && > > -- > > 2.34.1 > > > > > -- > Thanks, > > Steve Makes sense. We can have a warn once.
On 6/11/2023 4:01 AM, Shyam Prasad N wrote: > On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote: >> >> should this be a warn once? Could it get very noisy? >> >> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: >>> >>> We've seen the in-flight count go into negative with some >>> internal stress testing in Microsoft. >>> >>> Adding a WARN when this happens, in hope of understanding >>> why this happens when it happens. >>> >>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> >>> --- >>> fs/smb/client/smb2ops.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c >>> index 6e3be58cfe49..43162915e03c 100644 >>> --- a/fs/smb/client/smb2ops.c >>> +++ b/fs/smb/client/smb2ops.c >>> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server, >>> server->conn_id, server->hostname, *val, >>> add, server->in_flight); >>> } >>> + WARN_ON(server->in_flight == 0); >>> server->in_flight--; >>> if (server->in_flight == 0 && >>> ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) && >>> -- >>> 2.34.1 >>> >> >> >> -- >> Thanks, >> >> Steve > > Makes sense. We can have a warn once. Which sounds great, but isn't this connection basically toast? It's not super helpful to just whine. Why not clamp it at zero? Tom.
On Fri, Jun 23, 2023 at 9:52 PM Tom Talpey <tom@talpey.com> wrote: > > On 6/11/2023 4:01 AM, Shyam Prasad N wrote: > > On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote: > >> > >> should this be a warn once? Could it get very noisy? > >> > >> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > >>> > >>> We've seen the in-flight count go into negative with some > >>> internal stress testing in Microsoft. > >>> > >>> Adding a WARN when this happens, in hope of understanding > >>> why this happens when it happens. > >>> > >>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > >>> --- > >>> fs/smb/client/smb2ops.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > >>> index 6e3be58cfe49..43162915e03c 100644 > >>> --- a/fs/smb/client/smb2ops.c > >>> +++ b/fs/smb/client/smb2ops.c > >>> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server, > >>> server->conn_id, server->hostname, *val, > >>> add, server->in_flight); > >>> } > >>> + WARN_ON(server->in_flight == 0); > >>> server->in_flight--; > >>> if (server->in_flight == 0 && > >>> ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) && > >>> -- > >>> 2.34.1 > >>> > >> > >> > >> -- > >> Thanks, > >> > >> Steve > > > > Makes sense. We can have a warn once. > > Which sounds great, but isn't this connection basically toast? > It's not super helpful to just whine. Why not clamp it at zero? > > Tom. So there's no "legal" way that this count can go negative. If it has, that's definitely because there's a bug. The WARN will hopefully help us catch and fix the bug. We could also have a clamp at 0. I'll send an updated patch.
On 6/26/2023 2:33 AM, Shyam Prasad N wrote: > On Fri, Jun 23, 2023 at 9:52 PM Tom Talpey <tom@talpey.com> wrote: >> >> On 6/11/2023 4:01 AM, Shyam Prasad N wrote: >>> On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote: >>>> >>>> should this be a warn once? Could it get very noisy? >>>> >>>> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: >>>>> >>>>> We've seen the in-flight count go into negative with some >>>>> internal stress testing in Microsoft. >>>>> >>>>> Adding a WARN when this happens, in hope of understanding >>>>> why this happens when it happens. >>>>> >>>>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> >>>>> --- >>>>> fs/smb/client/smb2ops.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c >>>>> index 6e3be58cfe49..43162915e03c 100644 >>>>> --- a/fs/smb/client/smb2ops.c >>>>> +++ b/fs/smb/client/smb2ops.c >>>>> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server, >>>>> server->conn_id, server->hostname, *val, >>>>> add, server->in_flight); >>>>> } >>>>> + WARN_ON(server->in_flight == 0); >>>>> server->in_flight--; >>>>> if (server->in_flight == 0 && >>>>> ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) && >>>>> -- >>>>> 2.34.1 >>>>> >>>> >>>> >>>> -- >>>> Thanks, >>>> >>>> Steve >>> >>> Makes sense. We can have a warn once. >> >> Which sounds great, but isn't this connection basically toast? >> It's not super helpful to just whine. Why not clamp it at zero? >> >> Tom. > > So there's no "legal" way that this count can go negative. > If it has, that's definitely because there's a bug. The WARN will > hopefully help us catch and fix the bug. To be clear, I'm ok with the warn, it's the fact that the code goes to all the trouble to say it but doesn't do anything to recover. > We could also have a clamp at 0. I'll send an updated patch. Sounds good. Tom.
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 6e3be58cfe49..43162915e03c 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server, server->conn_id, server->hostname, *val, add, server->in_flight); } + WARN_ON(server->in_flight == 0); server->in_flight--; if (server->in_flight == 0 && ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
We've seen the in-flight count go into negative with some internal stress testing in Microsoft. Adding a WARN when this happens, in hope of understanding why this happens when it happens. Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> --- fs/smb/client/smb2ops.c | 1 + 1 file changed, 1 insertion(+)