Re: [PATCH v2 01/11] CIFS: Respect negotiated MaxMpxCount
From: Steve French <hidden>
Date: 2012-03-17 15:20:59
On Sat, Mar 17, 2012 at 9:53 AM, Pavel Shilovsky [off-list ref] wrote:
17 марта 2012 г. 15:12 пользователь Jeff Layton [off-list ref] написал:quoted
On Fri, 16 Mar 2012 18:09:24 +0300 Pavel Shilovsky [off-list ref] wrote:quoted
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 <redacted> --- 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 [off-list ref]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). -- Thanks, Steve