18 марта 2012 г. 14:50 пользователь Jeff Layton <jlayton@xxxxxxxxxx> написал: > 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). >> > > By the way... > > What's the plan for blocking locks and echoes here? Do you plan to make > them just ignore the maxmpx limit? I think we shouldn't play with them for stable. We still don't know what exactly we should do with them - really risky to make experiments on stable kernels. So, as we are sure that exceeding negotiated maxmpx value with any request except blocking lock and echo is wrong, I suggest to fix this - it is what this patch (#1) does. When we get more information about blocking locks and echos we can fix this too. > > Another important case to consider is the (apparently common) case > where maxmpx is 1. We don't really want to make things worse against > such servers... > If server negotiates maxmpx=1 then we simply disable oplocks for now (that the patch does) - don't think it makes things worse for those servers. -- Best regards, Pavel Shilovsky. -- 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
