On Mon, 19 Mar 2012 14:32:17 -0500 Steve French <smfrench@xxxxxxxxx> wrote: > On Mon, Mar 19, 2012 at 2:04 PM, Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote: > > 19 марта 2012 г. 19:04 пользователь Jeff Layton <jlayton@xxxxxxxxxx> написал: > >> On Sun, 18 Mar 2012 22:23:47 +0400 > >> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote: > >> > >>> 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. > >>> > >> > >> Then I'm confused, since you said this in your earlier email: > >> > >>> 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 you're not worrying about stable (and I agree that it's premature to > >> do that here), then you should focus on converting everything to use > >> the new credit-based scheme first and only then introduce the behavior > >> change of respecting maxmpx. > >> > >>> 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. > >>> > >> > >> Well, according to what MS wrote back to Steve when he asked, exceeding > >> the maxmpx is wrong _period_, regardless of the call. Echoes are fairly > >> easy to deal with, but blocking locks are another matter entirely. I > >> believe we need a fundamental rethink there. > > > > I meant that playing with things like blocking locks and echos is too > > risky for stable. But if we can fix the existing stable problem (not > > respecting MaxMpxCount value by other commands) with a strait-forward > > patch like this - we should do it. > > yes. agreed > > Fair enough. Given that the existing code just ignores this, that's probably a reasonable first step. One question though -- has anyone actually done any testing against servers with a maxmpx of 1 with this patch? It would be good to have some idea of how this behaves in the pessimal case... -- 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
