Thanks for making the changes. Please avoid unnecessary changes like:
if (server->tcpStatus == CifsNeedReconnect
- || server->tcpStatus == CifsExiting)
+ || server->tcpStatus == CifsExiting)
return;
as they don't bring any logic but complicate further backporting.
Other than that the patch and the whole series look good:
Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
--
Best regards,
Pavel Shilovsky
вт, 9 февр. 2021 г. в 22:09, Shyam Prasad N <nspmangalore@xxxxxxxxx>:
>
> Hi Pavel,
>
> Thanks for the review. Attaching updated patch.
>
> Regards,
> Shyam
>
> On Tue, Feb 9, 2021 at 12:05 PM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> >
> > 108 if (server->tcpStatus == CifsNeedReconnect
> > 109 || server->tcpStatus == CifsExiting)
> > 110 return;
> >
> > If you remove this check then the dmesg log may be populated by below
> > VFS logs. THe intent of this check was to avoid printing them because
> > they don't make sense in reconnect/umount situations and may confuse
> > users.
> >
> > 111
> > 112 switch (rc) {
> > 113 case -1:
> > 114 /* change_conf hasn't been executed */
> > 115 break;
> > 116 case 0:
> > 117 cifs_server_dbg(VFS, "Possible client or server
> > bug - zero credits\n");
> > 118 break;
> > 119 case 1:
> > 120 cifs_server_dbg(VFS, "disabling echoes and oplocks\n");
> > 121 break;
> > 122 case 2:
> > 123 cifs_dbg(FYI, "disabling oplocks\n");
> > 124 break;
> > 125 default:
> > 126 trace_smb3_add_credits(server->CurrentMid,
> > 127 server->hostname, rc, add);
> > 128 cifs_dbg(FYI, "%s: added %u credits total=%d\n",
> > __func__, add, rc);
> > 129 }
> >
> >
> > I would also suggest to rename sin_flight to just "in_flight" :)
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > сб, 6 февр. 2021 г. в 20:18, Shyam Prasad N <nspmangalore@xxxxxxxxx>:
> > >
> > > Forgot to attach again. :)
> > >
> > > On Sat, Feb 6, 2021 at 8:18 PM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
> > > >
> > > > Here's an updated version with some formatting changes per checkpatch.pl.
> > > > @Pavel Shilovsky @ronnie sahlberg Hoping that one of you can review
> > > > this. Particularly the above point.
> > > >
> > > > Regards,
> > > > Shyam
> > > >
> > > > On Thu, Feb 4, 2021 at 12:13 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
> > > > >
> > > > > @@ -97,17 +99,25 @@ smb2_add_credits(struct TCP_Server_Info *server,
> > > > > - if (server->tcpStatus == CifsNeedReconnect
> > > > > - || server->tcpStatus == CifsExiting)
> > > > > - return;
> > > > >
> > > > > @Pavel Shilovsky This check prevented a tracepoint from getting
> > > > > printed. I do not see much value in these lines, since all we do is
> > > > > print the tracepoint and exit. Hence removing it. Please let me know
> > > > > if that is not okay.
> > > > >
> > > > > On Thu, Feb 4, 2021 at 12:09 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > > Shyam
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Shyam
> > > >
> > > >
> > > >
> > > > --
> > > > Regards,
> > > > Shyam
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Shyam
>
>
>
> --
> Regards,
> Shyam
