[PATCH] nfsd: Fix build error

[PATCH] nfsd: Fix build error

[YueHaibing]

fs/nfsd/nfs4proc.o: In function `nfsd4_do_copy':
nfs4proc.c:(.text+0x23b7): undefined reference to `nfs42_ssc_close'
fs/nfsd/nfs4proc.o: In function `nfsd4_copy':
nfs4proc.c:(.text+0x5d2a): undefined reference to `nfs_sb_deactive'
fs/nfsd/nfs4proc.o: In function `nfsd4_do_async_copy':
nfs4proc.c:(.text+0x61d5): undefined reference to `nfs42_ssc_open'
nfs4proc.c:(.text+0x6389): undefined reference to `nfs_sb_deactive'

Add dependency to NFSD_V4_2_INTER_SSC to fix this.

Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 fs/nfsd/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
index f368f32..fc587a5 100644
--- a/fs/nfsd/Kconfig
+++ b/fs/nfsd/Kconfig
@@ -136,6 +136,7 @@ config NFSD_FLEXFILELAYOUT
 
 config NFSD_V4_2_INTER_SSC
 	bool "NFSv4.2 inter server to server COPY"
+	depends on !(NFSD=y && NFS_FS=m)
 	depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
 	help
 	  This option enables support for NFSv4.2 inter server to
-- 
2.7.4

Re: [PATCH] nfsd: Fix build error

[Chuck Lever]

Hi-

> On Mar 4, 2020, at 8:18 AM, YueHaibing <yuehaibing@huawei.com> wrote:
> 
> fs/nfsd/nfs4proc.o: In function `nfsd4_do_copy':
> nfs4proc.c:(.text+0x23b7): undefined reference to `nfs42_ssc_close'
> fs/nfsd/nfs4proc.o: In function `nfsd4_copy':
> nfs4proc.c:(.text+0x5d2a): undefined reference to `nfs_sb_deactive'
> fs/nfsd/nfs4proc.o: In function `nfsd4_do_async_copy':
> nfs4proc.c:(.text+0x61d5): undefined reference to `nfs42_ssc_open'
> nfs4proc.c:(.text+0x6389): undefined reference to `nfs_sb_deactive'
> 
> Add dependency to NFSD_V4_2_INTER_SSC to fix this.
> 
> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> fs/nfsd/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index f368f32..fc587a5 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -136,6 +136,7 @@ config NFSD_FLEXFILELAYOUT
> 
> config NFSD_V4_2_INTER_SSC
> 	bool "NFSv4.2 inter server to server COPY"
> +	depends on !(NFSD=y && NFS_FS=m)

The new dependency is not especially clear to me; more explanation
in the patch description about the cause of the build failure
would definitely be helpful.

NFSD_V4 can't be set unless NFSD is also set.

NFS_V4_2 can't be set unless NFS_V4_1 is also set, and that cannot
be set unless NFS_FS is also set.

So what's really going on here?


> 	depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
> 	help
> 	  This option enables support for NFSv4.2 inter server to
> -- 
> 2.7.4
> 
> 

--
Chuck Lever

Re: [PATCH] nfsd: Fix build error

[Bruce Fields]

On Wed, Mar 04, 2020 at 01:00:12PM -0500, Chuck Lever wrote:
> Hi-
> 
> > On Mar 4, 2020, at 8:18 AM, YueHaibing <yuehaibing@huawei.com> wrote:
> > 
> > fs/nfsd/nfs4proc.o: In function `nfsd4_do_copy':
> > nfs4proc.c:(.text+0x23b7): undefined reference to `nfs42_ssc_close'
> > fs/nfsd/nfs4proc.o: In function `nfsd4_copy':
> > nfs4proc.c:(.text+0x5d2a): undefined reference to `nfs_sb_deactive'
> > fs/nfsd/nfs4proc.o: In function `nfsd4_do_async_copy':
> > nfs4proc.c:(.text+0x61d5): undefined reference to `nfs42_ssc_open'
> > nfs4proc.c:(.text+0x6389): undefined reference to `nfs_sb_deactive'
> > 
> > Add dependency to NFSD_V4_2_INTER_SSC to fix this.
> > 
> > Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
> > Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > ---
> > fs/nfsd/Kconfig | 1 +
> > 1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > index f368f32..fc587a5 100644
> > --- a/fs/nfsd/Kconfig
> > +++ b/fs/nfsd/Kconfig
> > @@ -136,6 +136,7 @@ config NFSD_FLEXFILELAYOUT
> > 
> > config NFSD_V4_2_INTER_SSC
> > 	bool "NFSv4.2 inter server to server COPY"
> > +	depends on !(NFSD=y && NFS_FS=m)
> 
> The new dependency is not especially clear to me; more explanation
> in the patch description about the cause of the build failure
> would definitely be helpful.
> 
> NFSD_V4 can't be set unless NFSD is also set.
> 
> NFS_V4_2 can't be set unless NFS_V4_1 is also set, and that cannot
> be set unless NFS_FS is also set.
> 
> So what's really going on here?

I don't understand that "depends" either.

The fundamental problem, though, is that nfsd is calling nfs code
directly.

Which I noticed in earlier review and then forgot to follow up on,
sorry.

So either we:

	- let nfsd depend on nfs, fix up Kconfig to reflect the fact, or
	- write some code so nfsd can load nfs and find those symbols at
	  runtime if it needs to do a copy.

The latter's certainly doable, but it'd be simplest to do the former.
Are there actually a lot of people who want nfsd but not nfs?  Does that
cause a real problem for anyone?

--b.

Re: [PATCH] nfsd: Fix build error

[Chuck Lever]

> On Mar 4, 2020, at 3:06 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Wed, Mar 04, 2020 at 01:00:12PM -0500, Chuck Lever wrote:
>> Hi-
>> 
>>> On Mar 4, 2020, at 8:18 AM, YueHaibing <yuehaibing@huawei.com> wrote:
>>> 
>>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_copy':
>>> nfs4proc.c:(.text+0x23b7): undefined reference to `nfs42_ssc_close'
>>> fs/nfsd/nfs4proc.o: In function `nfsd4_copy':
>>> nfs4proc.c:(.text+0x5d2a): undefined reference to `nfs_sb_deactive'
>>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_async_copy':
>>> nfs4proc.c:(.text+0x61d5): undefined reference to `nfs42_ssc_open'
>>> nfs4proc.c:(.text+0x6389): undefined reference to `nfs_sb_deactive'
>>> 
>>> Add dependency to NFSD_V4_2_INTER_SSC to fix this.
>>> 
>>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>> ---
>>> fs/nfsd/Kconfig | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
>>> index f368f32..fc587a5 100644
>>> --- a/fs/nfsd/Kconfig
>>> +++ b/fs/nfsd/Kconfig
>>> @@ -136,6 +136,7 @@ config NFSD_FLEXFILELAYOUT
>>> 
>>> config NFSD_V4_2_INTER_SSC
>>> 	bool "NFSv4.2 inter server to server COPY"
>>> +	depends on !(NFSD=y && NFS_FS=m)
>> 
>> The new dependency is not especially clear to me; more explanation
>> in the patch description about the cause of the build failure
>> would definitely be helpful.
>> 
>> NFSD_V4 can't be set unless NFSD is also set.
>> 
>> NFS_V4_2 can't be set unless NFS_V4_1 is also set, and that cannot
>> be set unless NFS_FS is also set.
>> 
>> So what's really going on here?
> 
> I don't understand that "depends" either.
> 
> The fundamental problem, though, is that nfsd is calling nfs code
> directly.
> 
> Which I noticed in earlier review and then forgot to follow up on,
> sorry.
> 
> So either we:
> 
> 	- let nfsd depend on nfs, fix up Kconfig to reflect the fact, or
> 	- write some code so nfsd can load nfs and find those symbols at
> 	  runtime if it needs to do a copy.

Another practical option would be to create a copy of these functions in
the server's SSC code. At least nfs_sb_deactive() is not large.

Not clear if nfs42_ssc_open/close are even used on the client. Maybe they
could be moved to the server?

> The latter's certainly doable, but it'd be simplest to do the former.
> Are there actually a lot of people who want nfsd but not nfs?  Does that
> cause a real problem for anyone?
> 
> --b.

--
Chuck Lever

Re: [PATCH] nfsd: Fix build error

[Kornievskaia, Olga]

On 3/4/20, 4:05 PM, "Chuck Lever" <chuck.lever@oracle.com> wrote:

    NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.
    
    
    
    
    > On Mar 4, 2020, at 3:06 PM, Bruce Fields <bfields@fieldses.org> wrote:
    >
    > On Wed, Mar 04, 2020 at 01:00:12PM -0500, Chuck Lever wrote:
    >> Hi-
    >>
    >>> On Mar 4, 2020, at 8:18 AM, YueHaibing <yuehaibing@huawei.com> wrote:
    >>>
    >>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_copy':
    >>> nfs4proc.c:(.text+0x23b7): undefined reference to `nfs42_ssc_close'
    >>> fs/nfsd/nfs4proc.o: In function `nfsd4_copy':
    >>> nfs4proc.c:(.text+0x5d2a): undefined reference to `nfs_sb_deactive'
    >>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_async_copy':
    >>> nfs4proc.c:(.text+0x61d5): undefined reference to `nfs42_ssc_open'
    >>> nfs4proc.c:(.text+0x6389): undefined reference to `nfs_sb_deactive'
    >>>
    >>> Add dependency to NFSD_V4_2_INTER_SSC to fix this.
    >>>
    >>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
    >>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
    >>> ---
    >>> fs/nfsd/Kconfig | 1 +
    >>> 1 file changed, 1 insertion(+)
    >>>
    >>> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
    >>> index f368f32..fc587a5 100644
    >>> --- a/fs/nfsd/Kconfig
    >>> +++ b/fs/nfsd/Kconfig
    >>> @@ -136,6 +136,7 @@ config NFSD_FLEXFILELAYOUT
    >>>
    >>> config NFSD_V4_2_INTER_SSC
    >>>     bool "NFSv4.2 inter server to server COPY"
    >>> +   depends on !(NFSD=y && NFS_FS=m)
    >>
    >> The new dependency is not especially clear to me; more explanation
    >> in the patch description about the cause of the build failure
    >> would definitely be helpful.
    >>
    >> NFSD_V4 can't be set unless NFSD is also set.
    >>
    >> NFS_V4_2 can't be set unless NFS_V4_1 is also set, and that cannot
    >> be set unless NFS_FS is also set.
    >>
    >> So what's really going on here?
    >
    > I don't understand that "depends" either.
    >
    > The fundamental problem, though, is that nfsd is calling nfs code
    > directly.
    >
    > Which I noticed in earlier review and then forgot to follow up on,
    > sorry.
    >
    > So either we:
    >
    >       - let nfsd depend on nfs, fix up Kconfig to reflect the fact, or
    >       - write some code so nfsd can load nfs and find those symbols at
    >         runtime if it needs to do a copy.
    
    Another practical option would be to create a copy of these functions in
    the server's SSC code. At least nfs_sb_deactive() is not large.
    
    Not clear if nfs42_ssc_open/close are even used on the client. Maybe they
    could be moved to the server?

Nfs42_ssc_open/close are not used on the client but their insides use client's structures (like nfs_fh, nfs4_stateid, etc) and calls nfs4_proc_getattr, nfs_fhget and others and VFS functions. It'll be a lot to copy. 
    
    > The latter's certainly doable, but it'd be simplest to do the former.
    > Are there actually a lot of people who want nfsd but not nfs?  Does that
    > cause a real problem for anyone?
    >
    > --b.
    
    --
    Chuck Lever
    
    
    
    

Re: [PATCH] nfsd: Fix build error

[Yuehaibing]

On 2020/3/5 4:06, Bruce Fields wrote:
> On Wed, Mar 04, 2020 at 01:00:12PM -0500, Chuck Lever wrote:
>> Hi-
>>
>>> On Mar 4, 2020, at 8:18 AM, YueHaibing <yuehaibing@huawei.com> wrote:
>>>
>>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_copy':
>>> nfs4proc.c:(.text+0x23b7): undefined reference to `nfs42_ssc_close'
>>> fs/nfsd/nfs4proc.o: In function `nfsd4_copy':
>>> nfs4proc.c:(.text+0x5d2a): undefined reference to `nfs_sb_deactive'
>>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_async_copy':
>>> nfs4proc.c:(.text+0x61d5): undefined reference to `nfs42_ssc_open'
>>> nfs4proc.c:(.text+0x6389): undefined reference to `nfs_sb_deactive'
>>>
>>> Add dependency to NFSD_V4_2_INTER_SSC to fix this.
>>>
>>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>> ---
>>> fs/nfsd/Kconfig | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
>>> index f368f32..fc587a5 100644
>>> --- a/fs/nfsd/Kconfig
>>> +++ b/fs/nfsd/Kconfig
>>> @@ -136,6 +136,7 @@ config NFSD_FLEXFILELAYOUT
>>>
>>> config NFSD_V4_2_INTER_SSC
>>> 	bool "NFSv4.2 inter server to server COPY"
>>> +	depends on !(NFSD=y && NFS_FS=m)
>>
>> The new dependency is not especially clear to me; more explanation
>> in the patch description about the cause of the build failure
>> would definitely be helpful.
>>
>> NFSD_V4 can't be set unless NFSD is also set.
>>
>> NFS_V4_2 can't be set unless NFS_V4_1 is also set, and that cannot
>> be set unless NFS_FS is also set.
>>
>> So what's really going on here?
> 
> I don't understand that "depends" either.
> 
> The fundamental problem, though, is that nfsd is calling nfs code
> directly.

Yes

> 
> Which I noticed in earlier review and then forgot to follow up on,
> sorry.
> 
> So either we:
> 
> 	- let nfsd depend on nfs, fix up Kconfig to reflect the fact, or

It only fails while NFSD=y && NFS_FS=m, other cases works fine as Chuck Lever pointed.

> 	- write some code so nfsd can load nfs and find those symbols at
> 	  runtime if it needs to do a copy.
> 
> The latter's certainly doable, but it'd be simplest to do the former.
> Are there actually a lot of people who want nfsd but not nfs?  Does that
> cause a real problem for anyone?
> 
> --b.
> 
> .
> 

Re: [PATCH] nfsd: Fix build error

[Chuck Lever]

> On Mar 4, 2020, at 10:46 PM, Yuehaibing <yuehaibing@huawei.com> wrote:
> 
> On 2020/3/5 4:06, Bruce Fields wrote:
>> On Wed, Mar 04, 2020 at 01:00:12PM -0500, Chuck Lever wrote:
>>> Hi-
>>> 
>>>> On Mar 4, 2020, at 8:18 AM, YueHaibing <yuehaibing@huawei.com> wrote:
>>>> 
>>>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_copy':
>>>> nfs4proc.c:(.text+0x23b7): undefined reference to `nfs42_ssc_close'
>>>> fs/nfsd/nfs4proc.o: In function `nfsd4_copy':
>>>> nfs4proc.c:(.text+0x5d2a): undefined reference to `nfs_sb_deactive'
>>>> fs/nfsd/nfs4proc.o: In function `nfsd4_do_async_copy':
>>>> nfs4proc.c:(.text+0x61d5): undefined reference to `nfs42_ssc_open'
>>>> nfs4proc.c:(.text+0x6389): undefined reference to `nfs_sb_deactive'
>>>> 
>>>> Add dependency to NFSD_V4_2_INTER_SSC to fix this.
>>>> 
>>>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>>> ---
>>>> fs/nfsd/Kconfig | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>> 
>>>> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
>>>> index f368f32..fc587a5 100644
>>>> --- a/fs/nfsd/Kconfig
>>>> +++ b/fs/nfsd/Kconfig
>>>> @@ -136,6 +136,7 @@ config NFSD_FLEXFILELAYOUT
>>>> 
>>>> config NFSD_V4_2_INTER_SSC
>>>> 	bool "NFSv4.2 inter server to server COPY"
>>>> +	depends on !(NFSD=y && NFS_FS=m)
>>> 
>>> The new dependency is not especially clear to me; more explanation
>>> in the patch description about the cause of the build failure
>>> would definitely be helpful.
>>> 
>>> NFSD_V4 can't be set unless NFSD is also set.
>>> 
>>> NFS_V4_2 can't be set unless NFS_V4_1 is also set, and that cannot
>>> be set unless NFS_FS is also set.
>>> 
>>> So what's really going on here?
>> 
>> I don't understand that "depends" either.
>> 
>> The fundamental problem, though, is that nfsd is calling nfs code
>> directly.
> 
> Yes
> 
>> 
>> Which I noticed in earlier review and then forgot to follow up on,
>> sorry.
>> 
>> So either we:
>> 
>> 	- let nfsd depend on nfs, fix up Kconfig to reflect the fact, or
> 
> It only fails while NFSD=y && NFS_FS=m, other cases works fine as Chuck Lever pointed.

TL;DR - we probably need both of Bruce's options, IMO.

IIUC current distributions have the NFS client and server built as
separate modules. On an NFS server, most of the time there would be
no NFS mounts, thus the NFS client module is unlikely to be in memory
when an inter-SSC type request arrives.

Stated differently, enabling SSC cannot break the kernel build when
the client is built as a module. So we need NFS_FS=m to work because
it is the most common configuration.

If creating that fix will take longer than the next merge window, it
would be good practice IMO if some kind of Kconfig dependency were
introduced now to prevent SSC from being enabled when NFS_FS=m. Someone
with better Kconfig fu than me should have a look at Yue's proposed
patch. Once the long term solution is ready, this temporary fix can
be reverted.


>> 	- write some code so nfsd can load nfs and find those symbols at
>> 	  runtime if it needs to do a copy.
>> 
>> The latter's certainly doable, but it'd be simplest to do the former.
>> Are there actually a lot of people who want nfsd but not nfs?  Does that
>> cause a real problem for anyone?
>> 
>> --b.

--
Chuck Lever