| Message ID | 55B76B9D.2090103@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Why are we removing these two test cases? The Ganesha NFS server at least passes them. Frank > -----Original Message----- > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > owner@vger.kernel.org] On Behalf Of Kinglong Mee > Sent: Tuesday, July 28, 2015 4:47 AM > To: J. Bruce Fields; linux-nfs@vger.kernel.org > Cc: tigran.mkrtchyan@desy.de; kinglongmee@gmail.com > Subject: [PATCH 4/4] 4.1 create_session: Skip test of CB_NULL for nfs4.1 > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > --- > nfs4.1/server41tests/st_create_session.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/nfs4.1/server41tests/st_create_session.py > b/nfs4.1/server41tests/st_create_session.py > index b42e0ab..4c56bb4 100644 > --- a/nfs4.1/server41tests/st_create_session.py > +++ b/nfs4.1/server41tests/st_create_session.py > @@ -333,7 +333,7 @@ def testManyClients(t, env): > def testCallbackProgram(t, env): > """Check server can handle random transient program number > > - FLAGS: create_session all > + FLAGS: > CODE: CSESS20 > """ > cb_occurred = threading.Event() > @@ -360,7 +360,7 @@ def testCallbackProgram(t, env): > def testCallbackVersion(t, env): > """Check server sends callback program with a version listed in > nfs4client.py > > - FLAGS: create_session all > + FLAGS: > CODE: CSESS21 > """ > cb_occurred = threading.Event() > -- > 2.4.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the > body of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 28, 2015 at 09:03:38AM -0700, Frank Filz wrote: > Why are we removing these two test cases? The Ganesha NFS server at least passes them. And there's nothing wrong with passing the tests, but unfortunately there's not necessarily anything wrong with failing either: a 4.1 server isn't required to do a NULL callback to probe the backchannel. There might be a better test (depend on delegation recalls instead of CB_NULL's?), till then I'm OK with turning them off by default. --b. > > Frank > > > -----Original Message----- > > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > > owner@vger.kernel.org] On Behalf Of Kinglong Mee > > Sent: Tuesday, July 28, 2015 4:47 AM > > To: J. Bruce Fields; linux-nfs@vger.kernel.org > > Cc: tigran.mkrtchyan@desy.de; kinglongmee@gmail.com > > Subject: [PATCH 4/4] 4.1 create_session: Skip test of CB_NULL for nfs4.1 > > > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > > --- > > nfs4.1/server41tests/st_create_session.py | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/nfs4.1/server41tests/st_create_session.py > > b/nfs4.1/server41tests/st_create_session.py > > index b42e0ab..4c56bb4 100644 > > --- a/nfs4.1/server41tests/st_create_session.py > > +++ b/nfs4.1/server41tests/st_create_session.py > > @@ -333,7 +333,7 @@ def testManyClients(t, env): > > def testCallbackProgram(t, env): > > """Check server can handle random transient program number > > > > - FLAGS: create_session all > > + FLAGS: > > CODE: CSESS20 > > """ > > cb_occurred = threading.Event() > > @@ -360,7 +360,7 @@ def testCallbackProgram(t, env): > > def testCallbackVersion(t, env): > > """Check server sends callback program with a version listed in > > nfs4client.py > > > > - FLAGS: create_session all > > + FLAGS: > > CODE: CSESS21 > > """ > > cb_occurred = threading.Event() > > -- > > 2.4.3 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the > > body of a message to majordomo@vger.kernel.org More majordomo info at > > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Jul 28, 2015 at 09:03:38AM -0700, Frank Filz wrote: > > Why are we removing these two test cases? The Ganesha NFS server at > least passes them. > > And there's nothing wrong with passing the tests, but unfortunately there's > not necessarily anything wrong with failing either: a 4.1 server isn't required > to do a NULL callback to probe the backchannel. > > There might be a better test (depend on delegation recalls instead of > CB_NULL's?), till then I'm OK with turning them off by default. I guess I'd at least like to see some explanation of why we're turning the tests off by default. I'm just resistant to removing testing for no reason. If the test doesn't actually test anything useful, that would be one explanation. If it's something that's not required and not all servers do it, then that's another explanation. There might be other explanations, just please give some insight. Thanks Frank > > > -----Original Message----- > > > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > > > owner@vger.kernel.org] On Behalf Of Kinglong Mee > > > Sent: Tuesday, July 28, 2015 4:47 AM > > > To: J. Bruce Fields; linux-nfs@vger.kernel.org > > > Cc: tigran.mkrtchyan@desy.de; kinglongmee@gmail.com > > > Subject: [PATCH 4/4] 4.1 create_session: Skip test of CB_NULL for > > > nfs4.1 > > > > > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > > > --- > > > nfs4.1/server41tests/st_create_session.py | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/nfs4.1/server41tests/st_create_session.py > > > b/nfs4.1/server41tests/st_create_session.py > > > index b42e0ab..4c56bb4 100644 > > > --- a/nfs4.1/server41tests/st_create_session.py > > > +++ b/nfs4.1/server41tests/st_create_session.py > > > @@ -333,7 +333,7 @@ def testManyClients(t, env): > > > def testCallbackProgram(t, env): > > > """Check server can handle random transient program number > > > > > > - FLAGS: create_session all > > > + FLAGS: > > > CODE: CSESS20 > > > """ > > > cb_occurred = threading.Event() @@ -360,7 +360,7 @@ def > > > testCallbackProgram(t, env): > > > def testCallbackVersion(t, env): > > > """Check server sends callback program with a version listed in > > > nfs4client.py > > > > > > - FLAGS: create_session all > > > + FLAGS: > > > CODE: CSESS21 > > > """ > > > cb_occurred = threading.Event() > > > -- > > > 2.4.3 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" > > > in the body of a message to majordomo@vger.kernel.org More > majordomo > > > info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the > body of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 28, 2015 at 11:16:21AM -0700, Frank Filz wrote: > > On Tue, Jul 28, 2015 at 09:03:38AM -0700, Frank Filz wrote: > > > Why are we removing these two test cases? The Ganesha NFS server at > > least passes them. > > > > And there's nothing wrong with passing the tests, but unfortunately > there's > > not necessarily anything wrong with failing either: a 4.1 server isn't > required > > to do a NULL callback to probe the backchannel. > > > > There might be a better test (depend on delegation recalls instead of > > CB_NULL's?), till then I'm OK with turning them off by default. > > I guess I'd at least like to see some explanation of why we're turning the > tests off by default. I'm just resistant to removing testing for no reason. The story is: we turned off the CB_NULL probe in knfsd in the 4.1 case; it's unnecessary, and it was causing some problems (due partly to other bugs, but why ask for trouble?). So these tests started failing. I'd fix the server if there was an actual bug, but it's the tests that are wrong in this case, so oh well. > > If the test doesn't actually test anything useful, that would be one > explanation. > > If it's something that's not required and not all servers do it, then that's > another explanation. Right, this is it, CB_NULL probes aren't required. We're testing something useful, but something you need a callback to test, so trying to get some other kind of callback (like a deleg recall) would be another option. --b. > > There might be other explanations, just please give some insight. > > Thanks > > Frank > > > > > -----Original Message----- > > > > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > > > > owner@vger.kernel.org] On Behalf Of Kinglong Mee > > > > Sent: Tuesday, July 28, 2015 4:47 AM > > > > To: J. Bruce Fields; linux-nfs@vger.kernel.org > > > > Cc: tigran.mkrtchyan@desy.de; kinglongmee@gmail.com > > > > Subject: [PATCH 4/4] 4.1 create_session: Skip test of CB_NULL for > > > > nfs4.1 > > > > > > > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > > > > --- > > > > nfs4.1/server41tests/st_create_session.py | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/nfs4.1/server41tests/st_create_session.py > > > > b/nfs4.1/server41tests/st_create_session.py > > > > index b42e0ab..4c56bb4 100644 > > > > --- a/nfs4.1/server41tests/st_create_session.py > > > > +++ b/nfs4.1/server41tests/st_create_session.py > > > > @@ -333,7 +333,7 @@ def testManyClients(t, env): > > > > def testCallbackProgram(t, env): > > > > """Check server can handle random transient program number > > > > > > > > - FLAGS: create_session all > > > > + FLAGS: > > > > CODE: CSESS20 > > > > """ > > > > cb_occurred = threading.Event() @@ -360,7 +360,7 @@ def > > > > testCallbackProgram(t, env): > > > > def testCallbackVersion(t, env): > > > > """Check server sends callback program with a version listed in > > > > nfs4client.py > > > > > > > > - FLAGS: create_session all > > > > + FLAGS: > > > > CODE: CSESS21 > > > > """ > > > > cb_occurred = threading.Event() > > > > -- > > > > 2.4.3 > > > > > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" > > > > in the body of a message to majordomo@vger.kernel.org More > > majordomo > > > > info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the > > body of a message to majordomo@vger.kernel.org More majordomo info at > > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Jul 28, 2015 at 11:16:21AM -0700, Frank Filz wrote: > > > On Tue, Jul 28, 2015 at 09:03:38AM -0700, Frank Filz wrote: > > > > Why are we removing these two test cases? The Ganesha NFS server > > > > at > > > least passes them. > > > > > > And there's nothing wrong with passing the tests, but unfortunately > > there's > > > not necessarily anything wrong with failing either: a 4.1 server > > > isn't > > required > > > to do a NULL callback to probe the backchannel. > > > > > > There might be a better test (depend on delegation recalls instead > > > of CB_NULL's?), till then I'm OK with turning them off by default. > > > > I guess I'd at least like to see some explanation of why we're turning > > the tests off by default. I'm just resistant to removing testing for no reason. > > The story is: we turned off the CB_NULL probe in knfsd in the 4.1 case; it's > unnecessary, and it was causing some problems (due partly to other bugs, > but why ask for trouble?). So these tests started failing. I'd fix the server if > there was an actual bug, but it's the tests that are wrong in this case, so oh > well. Ah, ok. I'd love to understand the issues there better, maybe Ganesha shouldn't be doing the CB_NULL either... > > If the test doesn't actually test anything useful, that would be one > > explanation. > > > > If it's something that's not required and not all servers do it, then > > that's another explanation. > > Right, this is it, CB_NULL probes aren't required. Ok, with this perspective, I'm cool with the test being removed from default. > We're testing something useful, but something you need a callback to test, > so trying to get some other kind of callback (like a deleg recall) would be > another option. Hmm, more to think about from testing Ganesha perspective. The "simple" thing (i.e. using FSAL_VFS) to test in Ganesha doesn't do delegations, so the CB_NULL might be the only callback it makes, which might make it somewhat useful to prove Ganesha's callback code isn't totally broken... Thanks for the additional explanation. Frank > > There might be other explanations, just please give some insight. > > > > Thanks > > > > Frank > > > > > > > -----Original Message----- > > > > > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > > > > > owner@vger.kernel.org] On Behalf Of Kinglong Mee > > > > > Sent: Tuesday, July 28, 2015 4:47 AM > > > > > To: J. Bruce Fields; linux-nfs@vger.kernel.org > > > > > Cc: tigran.mkrtchyan@desy.de; kinglongmee@gmail.com > > > > > Subject: [PATCH 4/4] 4.1 create_session: Skip test of CB_NULL > > > > > for > > > > > nfs4.1 > > > > > > > > > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > > > > > --- > > > > > nfs4.1/server41tests/st_create_session.py | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/nfs4.1/server41tests/st_create_session.py > > > > > b/nfs4.1/server41tests/st_create_session.py > > > > > index b42e0ab..4c56bb4 100644 > > > > > --- a/nfs4.1/server41tests/st_create_session.py > > > > > +++ b/nfs4.1/server41tests/st_create_session.py > > > > > @@ -333,7 +333,7 @@ def testManyClients(t, env): > > > > > def testCallbackProgram(t, env): > > > > > """Check server can handle random transient program number > > > > > > > > > > - FLAGS: create_session all > > > > > + FLAGS: > > > > > CODE: CSESS20 > > > > > """ > > > > > cb_occurred = threading.Event() @@ -360,7 +360,7 @@ def > > > > > testCallbackProgram(t, env): > > > > > def testCallbackVersion(t, env): > > > > > """Check server sends callback program with a version > > > > > listed in nfs4client.py > > > > > > > > > > - FLAGS: create_session all > > > > > + FLAGS: > > > > > CODE: CSESS21 > > > > > """ > > > > > cb_occurred = threading.Event() > > > > > -- > > > > > 2.4.3 > > > > > > > > > > -- > > > > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" > > > > > in the body of a message to majordomo@vger.kernel.org More > > > majordomo > > > > > info at http://vger.kernel.org/majordomo-info.html > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" > > > in > > the > > > body of a message to majordomo@vger.kernel.org More majordomo > info > > > at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 28, 2015 at 12:55:26PM -0700, Frank Filz wrote: > > On Tue, Jul 28, 2015 at 11:16:21AM -0700, Frank Filz wrote: > > > > On Tue, Jul 28, 2015 at 09:03:38AM -0700, Frank Filz wrote: > > > > > Why are we removing these two test cases? The Ganesha NFS server > > > > > at > > > > least passes them. > > > > > > > > And there's nothing wrong with passing the tests, but unfortunately > > > there's > > > > not necessarily anything wrong with failing either: a 4.1 server > > > > isn't > > > required > > > > to do a NULL callback to probe the backchannel. > > > > > > > > There might be a better test (depend on delegation recalls instead > > > > of CB_NULL's?), till then I'm OK with turning them off by default. > > > > > > I guess I'd at least like to see some explanation of why we're turning > > > the tests off by default. I'm just resistant to removing testing for no > reason. > > > > The story is: we turned off the CB_NULL probe in knfsd in the 4.1 case; > it's > > unnecessary, and it was causing some problems (due partly to other bugs, > > but why ask for trouble?). So these tests started failing. I'd fix the > server if > > there was an actual bug, but it's the tests that are wrong in this case, > so oh > > well. > > Ah, ok. I'd love to understand the issues there better, maybe Ganesha > shouldn't be doing the CB_NULL either... I've actually forgotten; the patches are from April so linux-nfs archives from around there might have some discussion. I think there was actually a bug in client callback handling that frequent unnecessary CB_NULL's were triggering. > > > If the test doesn't actually test anything useful, that would be one > > > explanation. > > > > > > If it's something that's not required and not all servers do it, then > > > that's another explanation. > > > > Right, this is it, CB_NULL probes aren't required. > > Ok, with this perspective, I'm cool with the test being removed from > default. > > > We're testing something useful, but something you need a callback to test, > > so trying to get some other kind of callback (like a deleg recall) would > be > > another option. > > Hmm, more to think about from testing Ganesha perspective. The "simple" > thing (i.e. using FSAL_VFS) to test in Ganesha doesn't do delegations, so > the CB_NULL might be the only callback it makes, which might make it > somewhat useful to prove Ganesha's callback code isn't totally broken... Yeah, for testing it's annoying that there's no reliable to provoke a callback. pynfs could probably be smarter about trying various methods, and also about reporting the difference between "X failed" and "I couldn't test X because I couldn't get server to optional thing Y". --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/nfs4.1/server41tests/st_create_session.py b/nfs4.1/server41tests/st_create_session.py index b42e0ab..4c56bb4 100644 --- a/nfs4.1/server41tests/st_create_session.py +++ b/nfs4.1/server41tests/st_create_session.py @@ -333,7 +333,7 @@ def testManyClients(t, env): def testCallbackProgram(t, env): """Check server can handle random transient program number - FLAGS: create_session all + FLAGS: CODE: CSESS20 """ cb_occurred = threading.Event() @@ -360,7 +360,7 @@ def testCallbackProgram(t, env): def testCallbackVersion(t, env): """Check server sends callback program with a version listed in nfs4client.py - FLAGS: create_session all + FLAGS: CODE: CSESS21 """ cb_occurred = threading.Event()
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- nfs4.1/server41tests/st_create_session.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)