| Message ID | 20240814085721.518800-1-pvorel@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] nfsstat01: Update client RPC calls for kernel 6.9 | expand |
On Wed, Aug 14, 2024 at 10:57:21AM +0200, Petr Vorel wrote: > 6.9 moved client RPC calls to namespace in "Make nfs stats visible in > network NS" patchet. > > https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Changes v1->v2: > * Point out whole patchset, not just single commit > * Add a comment about the patchset > > Hi all, > > could you please ack this so that we have fixed mainline? > > FYI Some parts has been backported, e.g.: > d47151b79e322 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces") > to all stable/LTS: 5.4.276, 5.10.217, 5.15.159, 6.1.91, 6.6.31. > > But most of that is not yet (but planned to be backported), e.g. > 93483ac5fec62 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") > see Chuck's patchset for 6.6 > https://lore.kernel.org/linux-nfs/20240812223604.32592-1-cel@kernel.org/ > > Once all kernels up to 5.4 fixed we should update the version. > > Kind regards, > Petr > > testcases/network/nfs/nfsstat01/nfsstat01.sh | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh > index c2856eff1f..1beecbec43 100755 > --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh > +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh > @@ -15,7 +15,14 @@ get_calls() > local calls opt > > [ "$name" = "rpc" ] && opt="r" || opt="n" > - ! tst_net_use_netns && [ "$nfs_f" != "nfs" ] && type="rhost" > + > + if tst_net_use_netns; then > + # "Make nfs stats visible in network NS" patchet > + # https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > + tst_kvcmp -ge "6.9" && [ "$nfs_f" = "nfs" ] && type="rhost" Hello Petr- My concern with this fix is it targets v6.9 specifically, yet we know these fixes will appear in LTS/stable kernels as well. Neil Brown suggested an alternative approach that might not depend on knowing the specific kernel version: https://lore.kernel.org/linux-nfs/172078283934.15471.13377048166707693692@noble.neil.brown.name/ HTH > + else > + [ "$nfs_f" != "nfs" ] && type="rhost" > + fi > > if [ "$type" = "lhost" ]; then > calls="$(grep $name /proc/net/rpc/$nfs_f | cut -d' ' -f$field)" > -- > 2.45.2 > >
Hi Chuck, Neil, all, > On Wed, Aug 14, 2024 at 10:57:21AM +0200, Petr Vorel wrote: > > 6.9 moved client RPC calls to namespace in "Make nfs stats visible in > > network NS" patchet. > > https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > Changes v1->v2: > > * Point out whole patchset, not just single commit > > * Add a comment about the patchset > > Hi all, > > could you please ack this so that we have fixed mainline? > > FYI Some parts has been backported, e.g.: > > d47151b79e322 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces") > > to all stable/LTS: 5.4.276, 5.10.217, 5.15.159, 6.1.91, 6.6.31. > > But most of that is not yet (but planned to be backported), e.g. > > 93483ac5fec62 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") > > see Chuck's patchset for 6.6 > > https://lore.kernel.org/linux-nfs/20240812223604.32592-1-cel@kernel.org/ > > Once all kernels up to 5.4 fixed we should update the version. > > Kind regards, > > Petr > > testcases/network/nfs/nfsstat01/nfsstat01.sh | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh > > index c2856eff1f..1beecbec43 100755 > > --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh > > +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh > > @@ -15,7 +15,14 @@ get_calls() > > local calls opt > > [ "$name" = "rpc" ] && opt="r" || opt="n" > > - ! tst_net_use_netns && [ "$nfs_f" != "nfs" ] && type="rhost" > > + > > + if tst_net_use_netns; then > > + # "Make nfs stats visible in network NS" patchet > > + # https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > > + tst_kvcmp -ge "6.9" && [ "$nfs_f" = "nfs" ] && type="rhost" > Hello Petr- > My concern with this fix is it targets v6.9 specifically, yet we > know these fixes will appear in LTS/stable kernels as well. Great! I see you already fixed up to 5.15. I suppose the code is really backportable to the other still active branches (5.10, 5.4, 4.19). We discussed in v1 how to fix tests. Neil suggested to fix the test the way so that it works on all kernels. As I note [1] 1) either we give up on checking the new functionality still works (if we fallback to old behavior) 2) or we need to specify from which kernel we expect new functionality (so far it's 5.15, I suppose it will be older). I would prefer 2) to have new functionality always tested. Or am I missing something obvious? Kind regards, Petr [1] https://lore.kernel.org/linux-nfs/172367387549.6062.7078032983644586462@noble.neil.brown.name/ > Neil Brown suggested an alternative approach that might not depend > on knowing the specific kernel version: > https://lore.kernel.org/linux-nfs/172078283934.15471.13377048166707693692@noble.neil.brown.name/ > HTH > > + else > > + [ "$nfs_f" != "nfs" ] && type="rhost" > > + fi > > if [ "$type" = "lhost" ]; then > > calls="$(grep $name /proc/net/rpc/$nfs_f | cut -d' ' -f$field)" > > -- > > 2.45.2
> On Aug 23, 2024, at 2:46 AM, Petr Vorel <pvorel@suse.cz> wrote: > > Hi Chuck, Neil, all, > >> On Wed, Aug 14, 2024 at 10:57:21AM +0200, Petr Vorel wrote: >>> 6.9 moved client RPC calls to namespace in "Make nfs stats visible in >>> network NS" patchet. > >>> https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > >>> Signed-off-by: Petr Vorel <pvorel@suse.cz> >>> --- >>> Changes v1->v2: >>> * Point out whole patchset, not just single commit >>> * Add a comment about the patchset > >>> Hi all, > >>> could you please ack this so that we have fixed mainline? > >>> FYI Some parts has been backported, e.g.: >>> d47151b79e322 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces") >>> to all stable/LTS: 5.4.276, 5.10.217, 5.15.159, 6.1.91, 6.6.31. > >>> But most of that is not yet (but planned to be backported), e.g. >>> 93483ac5fec62 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") >>> see Chuck's patchset for 6.6 >>> https://lore.kernel.org/linux-nfs/20240812223604.32592-1-cel@kernel.org/ > >>> Once all kernels up to 5.4 fixed we should update the version. > >>> Kind regards, >>> Petr > >>> testcases/network/nfs/nfsstat01/nfsstat01.sh | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>> diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh >>> index c2856eff1f..1beecbec43 100755 >>> --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh >>> +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh >>> @@ -15,7 +15,14 @@ get_calls() >>> local calls opt > >>> [ "$name" = "rpc" ] && opt="r" || opt="n" >>> - ! tst_net_use_netns && [ "$nfs_f" != "nfs" ] && type="rhost" >>> + >>> + if tst_net_use_netns; then >>> + # "Make nfs stats visible in network NS" patchet >>> + # https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ >>> + tst_kvcmp -ge "6.9" && [ "$nfs_f" = "nfs" ] && type="rhost" > >> Hello Petr- > >> My concern with this fix is it targets v6.9 specifically, yet we >> know these fixes will appear in LTS/stable kernels as well. > > Great! I see you already fixed up to 5.15. I suppose the code is really > backportable to the other still active branches (5.10, 5.4, 4.19). I plan to work on backporting to v5.10 next week. I've been asked to look at v5.4, but I'm not sure how difficult that will be because it's missing a lot of NFSD patches. I will look into that in a couple of weeks. I'm very likely to punt on v4.19, though Oracle's stable backport team might try to tackle it at some point. (pun intended) > We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > that it works on all kernels. As I note [1] > > 1) either we give up on checking the new functionality still works (if we > fallback to old behavior) > 2) or we need to specify from which kernel we expect new functionality > (so far it's 5.15, I suppose it will be older). > > I would prefer 2) to have new functionality always tested. > Or am I missing something obvious? I don't quite understand the question. The "old functionality" of reporting these statistics globally is broken, but we're stuck with it in the older kernels. I guess you might want to confirm that, for a given recent kernel release, the stats are actually per-namespace -- that's what we expect in fixed kernels. Is that what you mean? > Kind regards, > Petr > > [1] https://lore.kernel.org/linux-nfs/172367387549.6062.7078032983644586462@noble.neil.brown.name/ > >> Neil Brown suggested an alternative approach that might not depend >> on knowing the specific kernel version: > >> https://lore.kernel.org/linux-nfs/172078283934.15471.13377048166707693692@noble.neil.brown.name/ > >> HTH > > >>> + else >>> + [ "$nfs_f" != "nfs" ] && type="rhost" >>> + fi > >>> if [ "$type" = "lhost" ]; then >>> calls="$(grep $name /proc/net/rpc/$nfs_f | cut -d' ' -f$field)" >>> -- >>> 2.45.2 -- Chuck Lever
> > On Aug 23, 2024, at 2:46 AM, Petr Vorel <pvorel@suse.cz> wrote: > > Hi Chuck, Neil, all, > >> On Wed, Aug 14, 2024 at 10:57:21AM +0200, Petr Vorel wrote: > >>> 6.9 moved client RPC calls to namespace in "Make nfs stats visible in > >>> network NS" patchet. > >>> https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > >>> Signed-off-by: Petr Vorel <pvorel@suse.cz> > >>> --- > >>> Changes v1->v2: > >>> * Point out whole patchset, not just single commit > >>> * Add a comment about the patchset > >>> Hi all, > >>> could you please ack this so that we have fixed mainline? > >>> FYI Some parts has been backported, e.g.: > >>> d47151b79e322 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces") > >>> to all stable/LTS: 5.4.276, 5.10.217, 5.15.159, 6.1.91, 6.6.31. > >>> But most of that is not yet (but planned to be backported), e.g. > >>> 93483ac5fec62 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") > >>> see Chuck's patchset for 6.6 > >>> https://lore.kernel.org/linux-nfs/20240812223604.32592-1-cel@kernel.org/ > >>> Once all kernels up to 5.4 fixed we should update the version. > >>> Kind regards, > >>> Petr > >>> testcases/network/nfs/nfsstat01/nfsstat01.sh | 9 ++++++++- > >>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>> diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh > >>> index c2856eff1f..1beecbec43 100755 > >>> --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh > >>> +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh > >>> @@ -15,7 +15,14 @@ get_calls() > >>> local calls opt > >>> [ "$name" = "rpc" ] && opt="r" || opt="n" > >>> - ! tst_net_use_netns && [ "$nfs_f" != "nfs" ] && type="rhost" > >>> + > >>> + if tst_net_use_netns; then > >>> + # "Make nfs stats visible in network NS" patchet > >>> + # https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > >>> + tst_kvcmp -ge "6.9" && [ "$nfs_f" = "nfs" ] && type="rhost" > >> Hello Petr- > >> My concern with this fix is it targets v6.9 specifically, yet we > >> know these fixes will appear in LTS/stable kernels as well. > > Great! I see you already fixed up to 5.15. I suppose the code is really > > backportable to the other still active branches (5.10, 5.4, 4.19). > I plan to work on backporting to v5.10 next week. > I've been asked to look at v5.4, but I'm not sure how difficult > that will be because it's missing a lot of NFSD patches. I will > look into that in a couple of weeks. > I'm very likely to punt on v4.19, though Oracle's stable backport > team might try to tackle it at some point. (pun intended) Thanks a lot for info, we'll see what you / your Oracle backport team will manage in the end. > > We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > > that it works on all kernels. As I note [1] > > 1) either we give up on checking the new functionality still works (if we > > fallback to old behavior) > > 2) or we need to specify from which kernel we expect new functionality > > (so far it's 5.15, I suppose it will be older). > > I would prefer 2) to have new functionality always tested. > > Or am I missing something obvious? > I don't quite understand the question. > The "old functionality" of reporting these statistics globally > is broken, but we're stuck with it in the older kernels. I guess > you might want to confirm that, for a given recent kernel > release, the stats are actually per-namespace -- that's what we > expect in fixed kernels. Is that what you mean? Yes. I'm just trying to say that Neil's proposal "work everywhere without checking kernel version" will not work. I would like next week, after you send 5.10 patches to expect anything >= 5.10 will have new functionality and update kernel version if more gets backported. Kind regards, Petr > > Kind regards, > > Petr > > [1] https://lore.kernel.org/linux-nfs/172367387549.6062.7078032983644586462@noble.neil.brown.name/ > >> Neil Brown suggested an alternative approach that might not depend > >> on knowing the specific kernel version: > >> https://lore.kernel.org/linux-nfs/172078283934.15471.13377048166707693692@noble.neil.brown.name/ > >> HTH > >>> + else > >>> + [ "$nfs_f" != "nfs" ] && type="rhost" > >>> + fi > >>> if [ "$type" = "lhost" ]; then > >>> calls="$(grep $name /proc/net/rpc/$nfs_f | cut -d' ' -f$field)" > >>> -- > >>> 2.45.2
On Fri, 23 Aug 2024, Petr Vorel wrote: > Hi Chuck, Neil, all, > > > On Wed, Aug 14, 2024 at 10:57:21AM +0200, Petr Vorel wrote: > > > 6.9 moved client RPC calls to namespace in "Make nfs stats visible in > > > network NS" patchet. > > > > https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > --- > > > Changes v1->v2: > > > * Point out whole patchset, not just single commit > > > * Add a comment about the patchset > > > > Hi all, > > > > could you please ack this so that we have fixed mainline? > > > > FYI Some parts has been backported, e.g.: > > > d47151b79e322 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces") > > > to all stable/LTS: 5.4.276, 5.10.217, 5.15.159, 6.1.91, 6.6.31. > > > > But most of that is not yet (but planned to be backported), e.g. > > > 93483ac5fec62 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") > > > see Chuck's patchset for 6.6 > > > https://lore.kernel.org/linux-nfs/20240812223604.32592-1-cel@kernel.org/ > > > > Once all kernels up to 5.4 fixed we should update the version. > > > > Kind regards, > > > Petr > > > > testcases/network/nfs/nfsstat01/nfsstat01.sh | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh > > > index c2856eff1f..1beecbec43 100755 > > > --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh > > > +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh > > > @@ -15,7 +15,14 @@ get_calls() > > > local calls opt > > > > [ "$name" = "rpc" ] && opt="r" || opt="n" > > > - ! tst_net_use_netns && [ "$nfs_f" != "nfs" ] && type="rhost" > > > + > > > + if tst_net_use_netns; then > > > + # "Make nfs stats visible in network NS" patchet > > > + # https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > > > + tst_kvcmp -ge "6.9" && [ "$nfs_f" = "nfs" ] && type="rhost" > > > Hello Petr- > > > My concern with this fix is it targets v6.9 specifically, yet we > > know these fixes will appear in LTS/stable kernels as well. > > Great! I see you already fixed up to 5.15. I suppose the code is really > backportable to the other still active branches (5.10, 5.4, 4.19). > > We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > that it works on all kernels. As I note [1] > > 1) either we give up on checking the new functionality still works (if we > fallback to old behavior) I don't understand. What exactly do you mean by "the new functionality". As I understand it there is no new functionality. All there was was and information leak between network namespaces, and we stopped the leak. Do you consider that to be new functionality? > 2) or we need to specify from which kernel we expect new functionality > (so far it's 5.15, I suppose it will be older). > > I would prefer 2) to have new functionality always tested. > Or am I missing something obvious? As I understand it the primary purpose of the test is to confirm that when an NFS request is made, the client sees an increase the the count of RPC requests in the client statistics. and the server sees an increase in the count of RPC requests in the server statistics. That test, if performed correctly, should always work. Is there something else you want to test? If you want to test that the server DOESN'T see and increase in the CLIENT statistics, then that is a valid thing to test and it won't work on early kernels. I think we only need to test that for kernels since the fix landed in mainline. I'm sure one of us is missing something obvious because I am CONFIDENT that a test for correct functionality can be written to work on all kernels. We didn't add any new functionality and didn't break any old functionality. We just fixed a bug. NeilBrown > > Kind regards, > Petr > > [1] https://lore.kernel.org/linux-nfs/172367387549.6062.7078032983644586462@noble.neil.brown.name/ > > > Neil Brown suggested an alternative approach that might not depend > > on knowing the specific kernel version: > > > https://lore.kernel.org/linux-nfs/172078283934.15471.13377048166707693692@noble.neil.brown.name/ > > > HTH > > > > > + else > > > + [ "$nfs_f" != "nfs" ] && type="rhost" > > > + fi > > > > if [ "$type" = "lhost" ]; then > > > calls="$(grep $name /proc/net/rpc/$nfs_f | cut -d' ' -f$field)" > > > -- > > > 2.45.2 >
On Fri, Aug 23, 2024 at 08:53:02PM +0200, Petr Vorel wrote: > > > > > On Aug 23, 2024, at 2:46 AM, Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Chuck, Neil, all, > > > >> On Wed, Aug 14, 2024 at 10:57:21AM +0200, Petr Vorel wrote: > > >>> 6.9 moved client RPC calls to namespace in "Make nfs stats visible in > > >>> network NS" patchet. > > > >>> https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > > > >>> Signed-off-by: Petr Vorel <pvorel@suse.cz> > > >>> --- > > >>> Changes v1->v2: > > >>> * Point out whole patchset, not just single commit > > >>> * Add a comment about the patchset > > > >>> Hi all, > > > >>> could you please ack this so that we have fixed mainline? > > > >>> FYI Some parts has been backported, e.g.: > > >>> d47151b79e322 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces") > > >>> to all stable/LTS: 5.4.276, 5.10.217, 5.15.159, 6.1.91, 6.6.31. > > > >>> But most of that is not yet (but planned to be backported), e.g. > > >>> 93483ac5fec62 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") > > >>> see Chuck's patchset for 6.6 > > >>> https://lore.kernel.org/linux-nfs/20240812223604.32592-1-cel@kernel.org/ > > > >>> Once all kernels up to 5.4 fixed we should update the version. > > > >>> Kind regards, > > >>> Petr > > > >>> testcases/network/nfs/nfsstat01/nfsstat01.sh | 9 ++++++++- > > >>> 1 file changed, 8 insertions(+), 1 deletion(-) > > > >>> diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh > > >>> index c2856eff1f..1beecbec43 100755 > > >>> --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh > > >>> +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh > > >>> @@ -15,7 +15,14 @@ get_calls() > > >>> local calls opt > > > >>> [ "$name" = "rpc" ] && opt="r" || opt="n" > > >>> - ! tst_net_use_netns && [ "$nfs_f" != "nfs" ] && type="rhost" > > >>> + > > >>> + if tst_net_use_netns; then > > >>> + # "Make nfs stats visible in network NS" patchet > > >>> + # https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ > > >>> + tst_kvcmp -ge "6.9" && [ "$nfs_f" = "nfs" ] && type="rhost" > > > >> Hello Petr- > > > >> My concern with this fix is it targets v6.9 specifically, yet we > > >> know these fixes will appear in LTS/stable kernels as well. > > > > Great! I see you already fixed up to 5.15. I suppose the code is really > > > backportable to the other still active branches (5.10, 5.4, 4.19). > > > I plan to work on backporting to v5.10 next week. > > > I've been asked to look at v5.4, but I'm not sure how difficult > > that will be because it's missing a lot of NFSD patches. I will > > look into that in a couple of weeks. > > > I'm very likely to punt on v4.19, though Oracle's stable backport > > team might try to tackle it at some point. (pun intended) > > Thanks a lot for info, we'll see what you / your Oracle backport team will > manage in the end. > > > > We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > > > that it works on all kernels. As I note [1] > > > > 1) either we give up on checking the new functionality still works (if we > > > fallback to old behavior) > > > 2) or we need to specify from which kernel we expect new functionality > > > (so far it's 5.15, I suppose it will be older). > > > > I would prefer 2) to have new functionality always tested. > > > Or am I missing something obvious? > > > I don't quite understand the question. > > > The "old functionality" of reporting these statistics globally > > is broken, but we're stuck with it in the older kernels. I guess > > you might want to confirm that, for a given recent kernel > > release, the stats are actually per-namespace -- that's what we > > expect in fixed kernels. Is that what you mean? > > Yes. I'm just trying to say that Neil's proposal "work everywhere without > checking kernel version" will not work. I would like next week, after you send > 5.10 patches to expect anything >= 5.10 will have new functionality > and update kernel version if more gets backported. I wanted to be sure you were aware of Neil's suggestion, and it sounds like it isn't workable for you. So, fair enough. I will get to work on v5.10.y ;-) > Kind regards, > Petr > > > > Kind regards, > > > Petr > > > > [1] https://lore.kernel.org/linux-nfs/172367387549.6062.7078032983644586462@noble.neil.brown.name/ > > > >> Neil Brown suggested an alternative approach that might not depend > > >> on knowing the specific kernel version: > > > >> https://lore.kernel.org/linux-nfs/172078283934.15471.13377048166707693692@noble.neil.brown.name/ > > > >> HTH > > > > >>> + else > > >>> + [ "$nfs_f" != "nfs" ] && type="rhost" > > >>> + fi > > > >>> if [ "$type" = "lhost" ]; then > > >>> calls="$(grep $name /proc/net/rpc/$nfs_f | cut -d' ' -f$field)" > > >>> -- > > >>> 2.45.2
On 23. 08. 24 23:59, NeilBrown wrote: > On Fri, 23 Aug 2024, Petr Vorel wrote: >> We discussed in v1 how to fix tests. Neil suggested to fix the test the way so >> that it works on all kernels. As I note [1] >> >> 1) either we give up on checking the new functionality still works (if we >> fallback to old behavior) > > I don't understand. What exactly do you mean by "the new > functionality". > As I understand it there is no new functionality. All there was was and > information leak between network namespaces, and we stopped the leak. > Do you consider that to be new functionality? The new functionality is that the patches add a new file to network namespaces: /proc/net/rpc/nfs. This file did not exist outside the root network namespace at least on some of the kernels where we still need to run this test. So the question is: How aggressively do we want to enforce backporting of these NFS patches into distros with older kernels? We have 3 options how to fix the test depending on the answer: 1) Don't enforce at all. We'll check whether /proc/net/rpc/nfs exists in the client namespace and read it only if it does. Otherwise we'll fall back on the global file. 2) Enforce aggressively. We'll hardcode a minimal kernel version into the test (e.g. v5.4) and if the procfile doesn't exist on any newer kernel, it's a bug. 3) Enforce on new kernels only. We'll set a hard requirement for kernel v6.9+ as in option 2) and check for existence of the procfile on any older kernels as in option 1).
Hi all, > On 23. 08. 24 23:59, NeilBrown wrote: > > On Fri, 23 Aug 2024, Petr Vorel wrote: > > > We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > > > that it works on all kernels. As I note [1] > > > 1) either we give up on checking the new functionality still works (if we > > > fallback to old behavior) > > I don't understand. What exactly do you mean by "the new > > functionality". > > As I understand it there is no new functionality. All there was was and > > information leak between network namespaces, and we stopped the leak. > > Do you consider that to be new functionality? Thanks Martin for jumping in. I hoped I was clear, but obviously not. Following are the questions for kernel maintainers and developers. I put my opinion, but it's really up to you what you want to have tested. > The new functionality is that the patches add a new file to network > namespaces: /proc/net/rpc/nfs. This file did not exist outside the root > network namespace at least on some of the kernels where we still need to run > this test. So the question is: How aggressively do we want to enforce > backporting of these NFS patches into distros with older kernels? > We have 3 options how to fix the test depending on the answer: > 1) Don't enforce at all. We'll check whether /proc/net/rpc/nfs exists in the > client namespace and read it only if it does. Otherwise we'll fall back on > the global file. 1) is IMHO the worst case because it's not testing patch gets reverted. > 2) Enforce aggressively. We'll hardcode a minimal kernel version into the > test (e.g. v5.4) and if the procfile doesn't exist on any newer kernel, it's > a bug. I would prefer 2), which is the usual LTP approach (do not hide bugs, we even fail on upstream kernel WONTFIX [1], why we should refuse the policy here?). Whichever older LTS upstream kernel gets fixed would drive the line where new functionality is requested (currently v5.14, I suppose at least 5.10 will also be fixed). LTP also has a way to specify enterprise distro kernel version if older enterprise kernel also gets fixed (this should not be needed, but it'd be possible). > 3) Enforce on new kernels only. We'll set a hard requirement for kernel > v6.9+ as in option 2) and check for existence of the procfile on any older > kernels as in option 1). Due way to specify enterprise distro kernel version and upstream kernel testing expecting people update to the latest stable/LTS we should not worry much about people with older kernels. Kind regards, Petr [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ustat/ustat01.c#L48-L49
> On Aug 27, 2024, at 9:22 AM, Petr Vorel <pvorel@suse.cz> wrote: > > Hi all, > >> On 23. 08. 24 23:59, NeilBrown wrote: >>> On Fri, 23 Aug 2024, Petr Vorel wrote: >>>> We discussed in v1 how to fix tests. Neil suggested to fix the test the way so >>>> that it works on all kernels. As I note [1] > >>>> 1) either we give up on checking the new functionality still works (if we >>>> fallback to old behavior) > >>> I don't understand. What exactly do you mean by "the new >>> functionality". >>> As I understand it there is no new functionality. All there was was and >>> information leak between network namespaces, and we stopped the leak. >>> Do you consider that to be new functionality? > > Thanks Martin for jumping in. I hoped I was clear, but obviously not. > > Following are the questions for kernel maintainers and developers. I put my > opinion, but it's really up to you what you want to have tested. > >> The new functionality is that the patches add a new file to network >> namespaces: /proc/net/rpc/nfs. This file did not exist outside the root >> network namespace at least on some of the kernels where we still need to run >> this test. So the question is: How aggressively do we want to enforce >> backporting of these NFS patches into distros with older kernels? > >> We have 3 options how to fix the test depending on the answer: >> 1) Don't enforce at all. We'll check whether /proc/net/rpc/nfs exists in the >> client namespace and read it only if it does. Otherwise we'll fall back on >> the global file. > > 1) is IMHO the worst case because it's not testing patch gets reverted. > >> 2) Enforce aggressively. We'll hardcode a minimal kernel version into the >> test (e.g. v5.4) and if the procfile doesn't exist on any newer kernel, it's >> a bug. > > I would prefer 2), which is the usual LTP approach (do not hide bugs, we even > fail on upstream kernel WONTFIX [1], why we should refuse the policy here?). 2) makes sense to me. > Whichever older LTS upstream kernel gets fixed would drive the line where new > functionality is requested (currently v5.14, I suppose at least 5.10 will also > be fixed). LTP also has a way to specify enterprise distro kernel version if > older enterprise kernel also gets fixed (this should not be needed, but it'd be > possible). > >> 3) Enforce on new kernels only. We'll set a hard requirement for kernel >> v6.9+ as in option 2) and check for existence of the procfile on any older >> kernels as in option 1). > > Due way to specify enterprise distro kernel version and upstream kernel testing > expecting people update to the latest stable/LTS we should not worry much about > people with older kernels. > > Kind regards, > Petr > > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ustat/ustat01.c#L48-L49 -- Chuck Lever
Hi all, > > On Aug 27, 2024, at 9:22 AM, Petr Vorel <pvorel@suse.cz> wrote: > > Hi all, > >> On 23. 08. 24 23:59, NeilBrown wrote: > >>> On Fri, 23 Aug 2024, Petr Vorel wrote: > >>>> We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > >>>> that it works on all kernels. As I note [1] > >>>> 1) either we give up on checking the new functionality still works (if we > >>>> fallback to old behavior) > >>> I don't understand. What exactly do you mean by "the new > >>> functionality". > >>> As I understand it there is no new functionality. All there was was and > >>> information leak between network namespaces, and we stopped the leak. > >>> Do you consider that to be new functionality? > > Thanks Martin for jumping in. I hoped I was clear, but obviously not. > > Following are the questions for kernel maintainers and developers. I put my > > opinion, but it's really up to you what you want to have tested. > >> The new functionality is that the patches add a new file to network > >> namespaces: /proc/net/rpc/nfs. This file did not exist outside the root > >> network namespace at least on some of the kernels where we still need to run > >> this test. So the question is: How aggressively do we want to enforce > >> backporting of these NFS patches into distros with older kernels? > >> We have 3 options how to fix the test depending on the answer: > >> 1) Don't enforce at all. We'll check whether /proc/net/rpc/nfs exists in the > >> client namespace and read it only if it does. Otherwise we'll fall back on > >> the global file. > > 1) is IMHO the worst case because it's not testing patch gets reverted. > >> 2) Enforce aggressively. We'll hardcode a minimal kernel version into the > >> test (e.g. v5.4) and if the procfile doesn't exist on any newer kernel, it's > >> a bug. > > I would prefer 2), which is the usual LTP approach (do not hide bugs, we even > > fail on upstream kernel WONTFIX [1], why we should refuse the policy here?). > 2) makes sense to me. Thanks for your opinion. I'll send another version (+ still wait for others input). Kind regards, Petr > > Whichever older LTS upstream kernel gets fixed would drive the line where new > > functionality is requested (currently v5.14, I suppose at least 5.10 will also > > be fixed). LTP also has a way to specify enterprise distro kernel version if > > older enterprise kernel also gets fixed (this should not be needed, but it'd be > > possible). > >> 3) Enforce on new kernels only. We'll set a hard requirement for kernel > >> v6.9+ as in option 2) and check for existence of the procfile on any older > >> kernels as in option 1). > > Due way to specify enterprise distro kernel version and upstream kernel testing > > expecting people update to the latest stable/LTS we should not worry much about > > people with older kernels. > > Kind regards, > > Petr > > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ustat/ustat01.c#L48-L49
On Tue, 27 Aug 2024, Martin Doucha wrote: > On 23. 08. 24 23:59, NeilBrown wrote: > > On Fri, 23 Aug 2024, Petr Vorel wrote: > >> We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > >> that it works on all kernels. As I note [1] > >> > >> 1) either we give up on checking the new functionality still works (if we > >> fallback to old behavior) > > > > I don't understand. What exactly do you mean by "the new > > functionality". > > As I understand it there is no new functionality. All there was was and > > information leak between network namespaces, and we stopped the leak. > > Do you consider that to be new functionality? > > The new functionality is that the patches add a new file to network > namespaces: /proc/net/rpc/nfs. This file did not exist outside the root > network namespace at least on some of the kernels where we still need to > run this test. So the question is: How aggressively do we want to > enforce backporting of these NFS patches into distros with older kernels? Thanks for explaining that. I had assumed that the the file was always visible from all name spaces, but before the fix every namespace saw the same file. Clearly I was wrong. > > We have 3 options how to fix the test depending on the answer: > 1) Don't enforce at all. We'll check whether /proc/net/rpc/nfs exists in > the client namespace and read it only if it does. Otherwise we'll fall > back on the global file. > 2) Enforce aggressively. We'll hardcode a minimal kernel version into > the test (e.g. v5.4) and if the procfile doesn't exist on any newer > kernel, it's a bug. > 3) Enforce on new kernels only. We'll set a hard requirement for kernel > v6.9+ as in option 2) and check for existence of the procfile on any > older kernels as in option 1). I think there are two totally separate questions here. 1/ How to fix the existing test to work on new and old kernels. The existing test checked that the rpc count increased when NFS traffic happened. I think 1 is the correct fix. I don't think the test should check kernel version. 2/ We have discovered a bug and want to add a test to guard against regression. This should be a new test. That test can simply check if the given file exist in a non-init namespace. I have no particular opinion about testing kernel versions. A credible approach would be to choose the oldest kernel which it still maintained at the time that that bug was discovered. Or maybe create a list of kernel versions where were maintained at that time and only run the test on versions in that list, or after the last in the list. I really think there is value in having two different tests because we are testing two different things. Thanks, NeilBrown > > -- > Martin Doucha mdoucha@suse.cz > SW Quality Engineer > SUSE LINUX, s.r.o. > CORSO IIa > Krizikova 148/34 > 186 00 Prague 8 > Czech Republic > >
Hi Neil, > On Tue, 27 Aug 2024, Martin Doucha wrote: > > On 23. 08. 24 23:59, NeilBrown wrote: > > > On Fri, 23 Aug 2024, Petr Vorel wrote: > > >> We discussed in v1 how to fix tests. Neil suggested to fix the test the way so > > >> that it works on all kernels. As I note [1] > > >> 1) either we give up on checking the new functionality still works (if we > > >> fallback to old behavior) > > > I don't understand. What exactly do you mean by "the new > > > functionality". > > > As I understand it there is no new functionality. All there was was and > > > information leak between network namespaces, and we stopped the leak. > > > Do you consider that to be new functionality? > > The new functionality is that the patches add a new file to network > > namespaces: /proc/net/rpc/nfs. This file did not exist outside the root > > network namespace at least on some of the kernels where we still need to > > run this test. So the question is: How aggressively do we want to > > enforce backporting of these NFS patches into distros with older kernels? > Thanks for explaining that. I had assumed that the the file was always > visible from all name spaces, but before the fix every namespace saw the > same file. Clearly I was wrong. > > We have 3 options how to fix the test depending on the answer: > > 1) Don't enforce at all. We'll check whether /proc/net/rpc/nfs exists in > > the client namespace and read it only if it does. Otherwise we'll fall > > back on the global file. > > 2) Enforce aggressively. We'll hardcode a minimal kernel version into > > the test (e.g. v5.4) and if the procfile doesn't exist on any newer > > kernel, it's a bug. > > 3) Enforce on new kernels only. We'll set a hard requirement for kernel > > v6.9+ as in option 2) and check for existence of the procfile on any > > older kernels as in option 1). > I think there are two totally separate questions here. > 1/ How to fix the existing test to work on new and old kernels. The > existing test checked that the rpc count increased when NFS traffic > happened. I think 1 is the correct fix. I don't think the test > should check kernel version. > 2/ We have discovered a bug and want to add a test to guard against > regression. This should be a new test. That test can simply check if > the given file exist in a non-init namespace. I have no particular > opinion about testing kernel versions. A credible approach would be > to choose the oldest kernel which it still maintained at the time that > that bug was discovered. Or maybe create a list of kernel versions > where were maintained at that time and only run the test on versions > in that list, or after the last in the list. > I really think there is value in having two different tests because we > are testing two different things. IMHO this is 3), just split in 2 tests (maybe more obvious for a reviewer). Sure, we can be explicit and split it into 2 tests. Regards the version, I would really either draw the line for new change for 6.9 or whatever stable will be the last which gets that. I mean, if it's e.g. 5.14, this test should fail on some old e.g. unsupported 5.15 some companies may test. When we in LTP test if fix is still working (no regression), usually the same tests is used to verify if the fix was applied. Kind regards, Petr > Thanks, > NeilBrown > > -- > > Martin Doucha mdoucha@suse.cz > > SW Quality Engineer > > SUSE LINUX, s.r.o. > > CORSO IIa > > Krizikova 148/34 > > 186 00 Prague 8 > > Czech Republic
diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh index c2856eff1f..1beecbec43 100755 --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh @@ -15,7 +15,14 @@ get_calls() local calls opt [ "$name" = "rpc" ] && opt="r" || opt="n" - ! tst_net_use_netns && [ "$nfs_f" != "nfs" ] && type="rhost" + + if tst_net_use_netns; then + # "Make nfs stats visible in network NS" patchet + # https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ + tst_kvcmp -ge "6.9" && [ "$nfs_f" = "nfs" ] && type="rhost" + else + [ "$nfs_f" != "nfs" ] && type="rhost" + fi if [ "$type" = "lhost" ]; then calls="$(grep $name /proc/net/rpc/$nfs_f | cut -d' ' -f$field)"
6.9 moved client RPC calls to namespace in "Make nfs stats visible in network NS" patchet. https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/ Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Changes v1->v2: * Point out whole patchset, not just single commit * Add a comment about the patchset Hi all, could you please ack this so that we have fixed mainline? FYI Some parts has been backported, e.g.: d47151b79e322 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces") to all stable/LTS: 5.4.276, 5.10.217, 5.15.159, 6.1.91, 6.6.31. But most of that is not yet (but planned to be backported), e.g. 93483ac5fec62 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") see Chuck's patchset for 6.6 https://lore.kernel.org/linux-nfs/20240812223604.32592-1-cel@kernel.org/ Once all kernels up to 5.4 fixed we should update the version. Kind regards, Petr testcases/network/nfs/nfsstat01/nfsstat01.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)