On Sat, 17 Mar 2012 10:20:59 -0500 Steve French <smfrench@xxxxxxxxx> wrote: > On Sat, Mar 17, 2012 at 9:53 AM, Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote: > > 17 марта 2012 г. 15:12 пользователь Jeff Layton <jlayton@xxxxxxxxxx> написал: > >> On Fri, 16 Mar 2012 18:09:24 +0300 > >> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote: > >> > >>> Some servers sets this value less than 50 that was hardcoded and > >>> we lost the connection if when we exceed this limit. Fix this by > >>> respecting this value - not sending more than the server allows. > >>> > >>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx> > >>> --- > >>> fs/cifs/cifsfs.c | 8 ++++---- > >>> fs/cifs/cifsglob.h | 10 +++------- > >>> fs/cifs/cifssmb.c | 9 +++++++-- > >>> fs/cifs/connect.c | 11 ++++------- > >>> fs/cifs/dir.c | 6 ++++-- > >>> fs/cifs/file.c | 4 ++-- > >>> fs/cifs/transport.c | 4 ++-- > >> > >> Introducing a behavior change like this at the beginning of the series > >> is probably a mistake. You'll have no reasonable way to bisect down > >> regressions, so you won't know if a problem is due to the change to a > >> credit-based model or due to changing the client to respect the maxmpx. > >> > >> Instead of doing this, we should instead reorganize the code around a > >> credit based model first, while attempting to mimic the existing > >> behavior as closely as possible. Once that framework is in place, then > >> change the behavior and start respecting the maxmpx. > >> > >> If you do that, then someone can reasonably bisect between those two > >> changes and we can determine the source of a regression. > >> -- > >> Jeff Layton <jlayton@xxxxxxxxxx> > > > > This was done to push this patch to 3.3 and stable tree as well. > > That's why it is in the start of series. > > > > If we decide not to push it for now, I agree that this patch should be > > in the modified version (according to changes from other patches) at > > the end of the series. > > > > Steve, your thoughts? > > > > -- > > Best regards, > > Pavel Shilovsky. > > We want to fix the reported (cifs) problem first with a simple patch > (that would be suitable for stable too). > > The most important reported problems are mounts to servers > running some versions of Windows Vista and some versions of Windows 7 > which set maxmpx below 50 and more importantly have maxworkitems > (in the windows registry on the server) set at too small a value to handle > the 20 or 30 async reads or writes which are now often in flight to them. > > The fix we need first is to honor the maxmpx that the server sets (and > probably remove cifs_max_pending which is obsolete as > a global module parm or simply do the minimum of cifs_max_pending > and maxmpx as the earlier patch did). If desired (to workaround buggy > servers) in a second patch we could add a mount parm if we think there are > servers such as windows 7 for which we will need to set the > negotiated value lower (due to their maxworkitems registry > problem), or Samba for which maxmpx could be ignored > (and as JRA and others indicated - we could send > a thousand requests in parallel). > Ok, that sounds like a reasonable approach. If you want to do that though would it make more sense to do a patch or series that just fixes the problem with smb1 without introducing credits? That series could be targeted for stable and you could then do another series on top of it that converts to a credit based model and adds in the smb2 code. The spec is so ambiguous here that we can be reasonably sure that *something* will break. We ought to try to proceed in such a way that we can deal with those problems quickly and efficiently as they arise. The thing we don't want to do is conflate the credit based changes with the change to respect maxmpx too much since that'll make it tough to know what caused a particular problem... -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
