> On Jul 8, 2024, at 6:36 AM, Greg KH <greg@xxxxxxxxx> wrote:
>
> On Sat, Jul 06, 2024 at 07:46:19AM +0000, Sherry Yang wrote:
>>
>>
>>> On Jul 6, 2024, at 12:11 AM, Greg KH <greg@xxxxxxxxx> wrote:
>>>
>>> On Fri, Jul 05, 2024 at 02:19:18PM +0000, Chuck Lever III wrote:
>>>>
>>>>
>>>>> On Jul 2, 2024, at 6:55 PM, Calum Mackay <calum.mackay@xxxxxxxxxx> wrote:
>>>>>
>>>>> To clarify…
>>>>>
>>>>> On 02/07/2024 5:54 pm, Calum Mackay wrote:
>>>>>> hi Petr,
>>>>>> I noticed your LTP patch [1][2] which adjusts the nfsstat01 test on v6.9 kernels, to account for Josef's changes [3], which restrict the NFS/RPC stats per-namespace.
>>>>>> I see that Josef's changes were backported, as far back as longterm v5.4,
>>>>>
>>>>> Sorry, that's not quite accurate.
>>>>>
>>>>> Josef's NFS client changes were all backported from v6.9, as far as longterm v5.4.y:
>>>>>
>>>>> 2057a48d0dd0 sunrpc: add a struct rpc_stats arg to rpc_create_args
>>>>> d47151b79e32 nfs: expose /proc/net/sunrpc/nfs in net namespaces
>>>>> 1548036ef120 nfs: make the rpc_stat per net namespace
>>>>>
>>>>>
>>>>> Of Josef's NFS server changes, four were backported from v6.9 to v6.8:
>>>>>
>>>>> 418b9687dece sunrpc: use the struct net as the svc proc private
>>>>> d98416cc2154 nfsd: rename NFSD_NET_* to NFSD_STATS_*
>>>>> 93483ac5fec6 nfsd: expose /proc/net/sunrpc/nfsd in net namespaces
>>>>> 4b14885411f7 nfsd: make all of the nfsd stats per-network namespace
>>>>>
>>>>> and the others remained only in v6.9:
>>>>>
>>>>> ab42f4d9a26f sunrpc: don't change ->sv_stats if it doesn't exist
>>>>> a2214ed588fb nfsd: stop setting ->pg_stats for unused stats
>>>>> f09432386766 sunrpc: pass in the sv_stats struct through svc_create_pooled
>>>>> 3f6ef182f144 sunrpc: remove ->pg_stats from svc_program
>>>>> e41ee44cc6a4 nfsd: remove nfsd_stats, make th_cnt a global counter
>>>>> 16fb9808ab2c nfsd: make svc_stat per-network namespace instead of global
>>>>>
>>>>>
>>>>>
>>>>> I'm wondering if this difference between NFS client, and NFS server, stat behaviour, across kernel versions, may perhaps cause some user confusion?
>>>>
>>>> As a refresher for the stable folken, Josef's changes make
>>>> nfsstats silo'd, so they no longer show counts from the whole
>>>> system, but only for NFS operations relating to the local net
>>>> namespace. That is a surprising change for some users, tools,
>>>> and testing.
>>>>
>>>> I'm not clear on whether there are any rules/guidelines around
>>>> LTS backports causing behavior changes that user tools, like
>>>> nfsstat, might be impacted by.
>>>
>>> The same rules that apply for Linus's tree (i.e. no userspace
>>> regressions.)
>>
>> Given the current data we have, LTP nfsstat01[1] failed on LTS v5.4.278 because of kernel commit 1548036ef1204 ("nfs:
>> make the rpc_stat per net namespace") [2]. Other LTS which backported the same commit are very likely troubled with the same LTP test failure.
>>
>> The following are the LTP nfsstat01 failure output
>>
>> ========
>> network 1 TINFO: initialize 'lhost' 'ltp_ns_veth2' interface
>> network 1 TINFO: add local addr 10.0.0.2/24
>> network 1 TINFO: add local addr fd00:1:1:1::2/64
>> network 1 TINFO: initialize 'rhost' 'ltp_ns_veth1' interface
>> network 1 TINFO: add remote addr 10.0.0.1/24
>> network 1 TINFO: add remote addr fd00:1:1:1::1/64
>> network 1 TINFO: Network config (local -- remote):
>> network 1 TINFO: ltp_ns_veth2 -- ltp_ns_veth1
>> network 1 TINFO: 10.0.0.2/24 -- 10.0.0.1/24
>> network 1 TINFO: fd00:1:1:1::2/64 -- fd00:1:1:1::1/64
>> <<<test_start>>>
>> tag=veth|nfsstat3_01 stime=1719943586
>> cmdline="nfsstat01"
>> contacts=""
>> analysis=exit
>> <<<test_output>>>
>> incrementing stop
>> nfsstat01 1 TINFO: timeout per run is 0h 20m 0s
>> nfsstat01 1 TINFO: setup NFSv3, socket type udp
>> nfsstat01 1 TINFO: Mounting NFS: mount -t nfs -o proto=udp,vers=3 10.0.0.2:/tmp/netpan-4577/LTP_nfsstat01.lz6zhgQHoV/3/udp /tmp/netpan-4577/LTP_nfsstat01.lz6zhgQHoV/3/0
>> nfsstat01 1 TINFO: checking RPC calls for server/client
>> nfsstat01 1 TINFO: calls 98/0
>> nfsstat01 1 TINFO: Checking for tracking of RPC calls for server/client
>> nfsstat01 1 TINFO: new calls 102/0
>> nfsstat01 1 TPASS: server RPC calls increased
>> nfsstat01 1 TFAIL: client RPC calls not increased
>> nfsstat01 1 TINFO: checking NFS calls for server/client
>> nfsstat01 1 TINFO: calls 2/2
>> nfsstat01 1 TINFO: Checking for tracking of NFS calls for server/client
>> nfsstat01 1 TINFO: new calls 3/3
>> nfsstat01 1 TPASS: server NFS calls increased
>> nfsstat01 1 TPASS: client NFS calls increased
>> nfsstat01 2 TINFO: Cleaning up testcase
>> nfsstat01 2 TINFO: SELinux enabled in enforcing mode, this may affect test results
>> nfsstat01 2 TINFO: it can be disabled with TST_DISABLE_SELINUX=1 (requires super/root)
>> nfsstat01 2 TINFO: install seinfo to find used SELinux profiles
>> nfsstat01 2 TINFO: loaded SELinux profiles: none
>>
>> Summary:
>> passed 3
>> failed 1
>> skipped 0
>> warnings 0
>> <<<execution_status>>>
>> initiation_status="ok"
>> duration=1 termination_type=exited termination_id=1 corefile=no
>> cutime=11 cstime=16
>> <<<test_end>>>
>> ltp-pan reported FAIL
>> ========
>>
>> We can observe the number of RPC client calls is 0, which is wired. And this happens from the kernel commit 57d1ce96d7655 ("nfs: make the rpc_stat per net namespace”). So now we’re not sure the kernel backport of nfs client changes is proper, or the LTP tests / userspace need to be modified.
>>
>> If no userspace regression, should we revert the Josef’s NFS client-side changes on LTS?
>
> This sounds like a regression in Linus's tree too, so why isn't it
> reverted there first?
There is a change in behavior in the upstream code, but Josef's
patches fix an information leak and make the statistics more
sensible in container environments. I'm not certain that
should be considered a regression, but confess I don't know
the regression rules to this fine a degree of detail.
If it is indeed a regression, how can we go about retaining
both behaviors (selectable by Kconfig or perhaps administrative
UI)?
--
Chuck Lever
