mbox series

[0/1] Enable inter server to server copies on a export

Message ID 20211028144851.644018-1-steved@redhat.com (mailing list archive)
Headers show
Series Enable inter server to server copies on a export | expand

Message

Steve Dickson Oct. 28, 2021, 2:48 p.m. UTC
This kernel patch and an upcoming nfs-utils patch
adds a new export option 's2sc' which will allow
inter server to server copies.

The option needs to be set on the destination server.

The idea behind the option is to allow admins the 
ability to control which export can do these 
types of copies.

Steve Dickson (1):
  nfsd4_copy: Adds the ability to do inter server to server on an export

 fs/nfsd/nfs4proc.c               | 3 ++-
 include/uapi/linux/nfsd/export.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

J. Bruce Fields Oct. 29, 2021, 1:45 p.m. UTC | #1
On Thu, Oct 28, 2021 at 10:48:50AM -0400, Steve Dickson wrote:
> This kernel patch and an upcoming nfs-utils patch
> adds a new export option 's2sc' which will allow
> inter server to server copies.

They're already allowed by a module option.  Why is an export option
better?  And why should it be set on the destination server and not the
source server?

Really, first I think we should try to identify what the problem is that
we're trying to solve.

--b.

> 
> The option needs to be set on the destination server.
> 
> The idea behind the option is to allow admins the 
> ability to control which export can do these 
> types of copies.
> 
> Steve Dickson (1):
>   nfsd4_copy: Adds the ability to do inter server to server on an export
> 
>  fs/nfsd/nfs4proc.c               | 3 ++-
>  include/uapi/linux/nfsd/export.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> -- 
> 2.31.1
J. Bruce Fields Oct. 29, 2021, 2:26 p.m. UTC | #2
On Fri, Oct 29, 2021 at 09:45:34AM -0400, bfields wrote:
> On Thu, Oct 28, 2021 at 10:48:50AM -0400, Steve Dickson wrote:
> > This kernel patch and an upcoming nfs-utils patch
> > adds a new export option 's2sc' which will allow
> > inter server to server copies.
> 
> They're already allowed by a module option.  Why is an export option
> better?  And why should it be set on the destination server and not the
> source server?
> 
> Really, first I think we should try to identify what the problem is that
> we're trying to solve.

I guess we're thinking: we expect server-to-server copy to be a win in
some cases, but not others.

What would those cases look like?

Say you've got a client "C" and two servers, "S" and "T", and C is
copying a file from S to T.

I'd expect bandwidth of the traditional read-write-loop copy to be the
minimum of the network bandwidth between S and C, and the network
bandwidth between C and T.  Are there common cases were the S-to-T
bandwidth would be significantly less than both of those?

My guess would've been that that's relatively unusual.  So as a first
pass, just turning on COPY unconditionally doesn't seem so bad.

I know you're thinking about in cases where S and T are connected by a
high-performance network not necessarily available to C.

In that case, we know we want to use server-to-server copy for copies
between S and T.  But is it necessarily a problem to also use it for
copies between some other server and T?  Also, does knowing the export
containing the destination file on T really tell you whether or not the
copy will be coming from S and not from some other server?

I'd think the bigger issue in that case is figuring out how to configure
S so that it returns the right IP address in the cnr_source_server field
of the COPY_NOTIFY reply.  Currently we return address that the client
sent the COPY_NOTIFY, and I don't know if that's correct for that case.

--b.
Steve Dickson Oct. 29, 2021, 2:56 p.m. UTC | #3
Hey!

On 10/29/21 09:45, J. Bruce Fields wrote:
> On Thu, Oct 28, 2021 at 10:48:50AM -0400, Steve Dickson wrote:
>> This kernel patch and an upcoming nfs-utils patch
>> adds a new export option 's2sc' which will allow
>> inter server to server copies.
> 
> They're already allowed by a module option.  Why is an export option
> better?  
I was talking with an old admin... that you might know :-)
and he was suggestion he would what to control which
export used these types of copies... And I agreed with him.

> And why should it be set on the destination server and not the
> source server?
Maybe I'm missing something... but I believe nfsd4_copy() is only
called on the destination server since it is the only
function that looks at the inter_copy_offload_enable param.

> 
> Really, first I think we should try to identify what the problem is that
> we're trying to solve.
I'm not trying to solve any problem... I'm just trying to enable a
feature in a sane and safe way. By only have one module param it
is on all the time on all copies... With an export opt, admins
can pick and choose which exports use it. I'm thinking that
is a bit less risky.

The option of setting the inter_copy_offload_enable is still
there... if admins want to go that route.

steved.
> 
> --b.
> 
>>
>> The option needs to be set on the destination server.
>>
>> The idea behind the option is to allow admins the
>> ability to control which export can do these
>> types of copies.
>>
>> Steve Dickson (1):
>>    nfsd4_copy: Adds the ability to do inter server to server on an export
>>
>>   fs/nfsd/nfs4proc.c               | 3 ++-
>>   include/uapi/linux/nfsd/export.h | 1 +
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> -- 
>> 2.31.1
>
Steve Dickson Oct. 29, 2021, 3:24 p.m. UTC | #4
On 10/29/21 10:26, J. Bruce Fields wrote:
> On Fri, Oct 29, 2021 at 09:45:34AM -0400, bfields wrote:
>> On Thu, Oct 28, 2021 at 10:48:50AM -0400, Steve Dickson wrote:
>>> This kernel patch and an upcoming nfs-utils patch
>>> adds a new export option 's2sc' which will allow
>>> inter server to server copies.
>>
>> They're already allowed by a module option.  Why is an export option
>> better?  And why should it be set on the destination server and not the
>> source server?
>>
>> Really, first I think we should try to identify what the problem is that
>> we're trying to solve.
> 
> I guess we're thinking: we expect server-to-server copy to be a win in
> some cases, but not others.
Right!

> 
> What would those cases look like?
> 
> Say you've got a client "C" and two servers, "S" and "T", and C is
> copying a file from S to T.
> 
> I'd expect bandwidth of the traditional read-write-loop copy to be the
> minimum of the network bandwidth between S and C, and the network
> bandwidth between C and T.  Are there common cases were the S-to-T
> bandwidth would be significantly less than both of those?
I was thinking the connection between S-to-T would be much
faster (come type of cluster interconnect) that the connection
between C-S or C-T.

> 
> My guess would've been that that's relatively unusual.  So as a first
> pass, just turning on COPY unconditionally doesn't seem so bad.
> 
> I know you're thinking about in cases where S and T are connected by a
> high-performance network not necessarily available to C.
Exactly... well said.

> 
> In that case, we know we want to use server-to-server copy for copies
> between S and T.  But is it necessarily a problem to also use it for
> copies between some other server and T?  Also, does knowing the export
> containing the destination file on T really tell you whether or not the
> copy will be coming from S and not from some other server?
No... All the export is doing is allowing the COPY request
to succeed. I don't think T has any idea where the
COPY is coming from... but you would know better that I :-)

> 
> I'd think the bigger issue in that case is figuring out how to configure
> S so that it returns the right IP address in the cnr_source_server field
> of the COPY_NOTIFY reply.  Currently we return address that the client
> sent the COPY_NOTIFY, and I don't know if that's correct for that case.
I didn't look at this part of the transaction since it was
not controlled by the module param... But How would S know its the
"right IP address" since it is C that did the mount to T and
is giving S the address?

Keeping in context of what I'm trying to do... All I'm
trying to do is enable the feature, and with your points,
I think you are making my case there should be an
alternate of enabling these copies.

steved.
J. Bruce Fields Oct. 29, 2021, 4:40 p.m. UTC | #5
On Fri, Oct 29, 2021 at 10:56:24AM -0400, Steve Dickson wrote:
> On 10/29/21 09:45, J. Bruce Fields wrote:
> >Really, first I think we should try to identify what the problem is that
> >we're trying to solve.
> I'm not trying to solve any problem... I'm just trying to enable a
> feature in a sane and safe way. By only have one module param it
> is on all the time on all copies... With an export opt, admins
> can pick and choose which exports use it. I'm thinking that
> is a bit less risky.

So, I'm not sure which risk you're thinking about.

If it's the risk of the client telling the destination server to copy
from a rogue server--I'm kind of regretting bringing that up.  If there
are bugs in that area, I'd rather they be fixed, than that we introduce
new configuration to work around bugs that may or may not exist.  They'd
need to be fixed anyway, for other reasons.  I think Dai has volunteered
to look through the code that handles replies to the small number of
operations concerned, and I think that's good enough due diligence.  And
I'm not getting the impression Trond is particularly worried about the
client code here either.

If the risk is performance--like I say, I'm not sure exactly what those
cases look like.

I've been thinking about it in terms of bandwidth, but maybe that's
wrong--the bigger problem may be when the source server is only
accessible to the client for some reason (like, you're copying from a
local server to a destination server that's outside a firewall).  Maybe
the destination server will end up waiting a long time trying to reach
the source.

But, again, I'm not sure an export option helps, because I don't see why
that problem would necessarily be per-destination-server-export.

Instead, we may just need to make sure the destination server is quick
to timeout, or has some mechanism to blacklist source servers so it's
not repeatedly timing out trying to copy from the same server.  I don't
know, I'm just thinking out loud.

> The option of setting the inter_copy_offload_enable is still
> there... if admins want to go that route.

Let's just stick with that for now, and leave it off by default until
we're sure it's mature enough.  Let's not introduce new configuration to
work around problems that we haven't really analyzed yet.

It's not as though turning on a module option is any more difficult than
setting an export option.

--b.
Steve Dickson Oct. 29, 2021, 5:30 p.m. UTC | #6
On 10/29/21 12:40, J. Bruce Fields wrote:
> On Fri, Oct 29, 2021 at 10:56:24AM -0400, Steve Dickson wrote:
>> On 10/29/21 09:45, J. Bruce Fields wrote:
>>> Really, first I think we should try to identify what the problem is that
>>> we're trying to solve.
>> I'm not trying to solve any problem... I'm just trying to enable a
>> feature in a sane and safe way. By only have one module param it
>> is on all the time on all copies... With an export opt, admins
>> can pick and choose which exports use it. I'm thinking that
>> is a bit less risky.
> 
> So, I'm not sure which risk you're thinking about.
Now, I don't know how often a client copies files
from one server to another... but change the path on
all those copies... without making the admins aware
that is happening... is the risk I'm thinking about.

> 
> If it's the risk of the client telling the destination server to copy
> from a rogue server--I'm kind of regretting bringing that up.  If there
> are bugs in that area, I'd rather they be fixed, than that we introduce
> new configuration to work around bugs that may or may not exist.  They'd
> need to be fixed anyway, for other reasons.  I think Dai has volunteered
> to look through the code that handles replies to the small number of
> operations concerned, and I think that's good enough due diligence.  And
> I'm not getting the impression Trond is particularly worried about the
> client code here either.
I agree we should fix the bugs is better than working around them.
But having the bug effect all these types of copies verses
a few of them on a select number of exports... We still
would see the bugs.

> 
> If the risk is performance--like I say, I'm not sure exactly what those
> cases look like.
Well Jorge seem to show there was a was a big performance gain [1]

> 
> I've been thinking about it in terms of bandwidth, but maybe that's
> wrong--the bigger problem may be when the source server is only
> accessible to the client for some reason (like, you're copying from a
> local server to a destination server that's outside a firewall).  Maybe
> the destination server will end up waiting a long time trying to reach
> the source.
> 
> But, again, I'm not sure an export option helps, because I don't see why
> that problem would necessarily be per-destination-server-export.
It helps enabling the technology without out the big hammer approach.
Allowing these type of copies on a couple exports verses all
exports would still show any problem.

> 
> Instead, we may just need to make sure the destination server is quick
> to timeout, or has some mechanism to blacklist source servers so it's
> not repeatedly timing out trying to copy from the same server.  I don't
> know, I'm just thinking out loud.
This sounds like you not happy with the design... And it appears
where is there today is what the RPC specified...
Again... I'm just trying to enable it...

> 
>> The option of setting the inter_copy_offload_enable is still
>> there... if admins want to go that route.
> 
> Let's just stick with that for now, and leave it off by default until
> we're sure it's mature enough.  Let's not introduce new configuration to
> work around problems that we haven't really analyzed yet.
How is this going to find problems? At least with the export option
it is documented and it more if a stick you toe in the pool verses
jumping in...

steved.
> 
> It's not as though turning on a module option is any more difficult than
> setting an export option.
> 


[1] https://marc.info/?l=linux-nfs&m=149201305018467&w=2
J. Bruce Fields Oct. 29, 2021, 7:14 p.m. UTC | #7
On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
> On 10/29/21 12:40, J. Bruce Fields wrote:
> >Let's just stick with that for now, and leave it off by default until
> >we're sure it's mature enough.  Let's not introduce new configuration to
> >work around problems that we haven't really analyzed yet.
> How is this going to find problems? At least with the export option
> it is documented

That sounds fixable.  We need documentation of module parameters anyway.

> and it more if a stick you toe in the pool verses
> jumping in...

If we want more fine-grained control, I'm not yet seeing the argument
that an export option on the destination server side is the way to do
it.

Let's document the module parameter and go with that for now.

--b.
Steve Dickson Nov. 1, 2021, 3:30 p.m. UTC | #8
Hey!

On 10/29/21 15:14, J. Bruce Fields wrote:
> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
>> On 10/29/21 12:40, J. Bruce Fields wrote:
>>> Let's just stick with that for now, and leave it off by default until
>>> we're sure it's mature enough.  Let's not introduce new configuration to
>>> work around problems that we haven't really analyzed yet.
>> How is this going to find problems? At least with the export option
>> it is documented
> 
> That sounds fixable.  We need documentation of module parameters anyway.
Yeah I just took I don't see any documentation of module
parameters anywhere for any of the modules. But by documentation
I meant having the feature in the exports(5) manpage.

> 
>> and it more if a stick you toe in the pool verses
>> jumping in...
> 
> If we want more fine-grained control, I'm not yet seeing the argument
> that an export option on the destination server side is the way to do
> it.
> 
> Let's document the module parameter and go with that for now.
Now that cp will use copy_file_range() when available,
what are the steps needed to enable these fast copies?

steved.
J. Bruce Fields Nov. 1, 2021, 3:40 p.m. UTC | #9
On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
> Hey!
> 
> On 10/29/21 15:14, J. Bruce Fields wrote:
> >On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
> >>On 10/29/21 12:40, J. Bruce Fields wrote:
> >>>Let's just stick with that for now, and leave it off by default until
> >>>we're sure it's mature enough.  Let's not introduce new configuration to
> >>>work around problems that we haven't really analyzed yet.
> >>How is this going to find problems? At least with the export option
> >>it is documented
> >
> >That sounds fixable.  We need documentation of module parameters anyway.
> Yeah I just took I don't see any documentation of module
> parameters anywhere for any of the modules. But by documentation
> I meant having the feature in the exports(5) manpage.

I think I'd probably create a new page for sysctls (this isn't the only
one needing documentation), and make sure it's listed in the "SEE ALSO"
section of the other man pages.

> >>and it more if a stick you toe in the pool verses
> >>jumping in...
> >
> >If we want more fine-grained control, I'm not yet seeing the argument
> >that an export option on the destination server side is the way to do
> >it.
> >
> >Let's document the module parameter and go with that for now.
> Now that cp will use copy_file_range() when available,
> what are the steps needed to enable these fast copies?

1) Make sure client and both servers support NFSv4.2 and
server-to-server copy.

2) Make sure destination server can access (at least for read) any
exports on the source that you want to be able to copy from.

3) echo 1 >/sys/module/nfsd/parameters/inter_copy_offload_enable on the
destination server.

--b.
Chuck Lever Nov. 1, 2021, 4:55 p.m. UTC | #10
> On Nov 1, 2021, at 11:40 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
>> Hey!
>> 
>> On 10/29/21 15:14, J. Bruce Fields wrote:
>>> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
>>>> On 10/29/21 12:40, J. Bruce Fields wrote:
>>>>> Let's just stick with that for now, and leave it off by default until
>>>>> we're sure it's mature enough.  Let's not introduce new configuration to
>>>>> work around problems that we haven't really analyzed yet.
>>>> How is this going to find problems? At least with the export option
>>>> it is documented
>>> 
>>> That sounds fixable.  We need documentation of module parameters anyway.
>> Yeah I just took I don't see any documentation of module
>> parameters anywhere for any of the modules. But by documentation
>> I meant having the feature in the exports(5) manpage.
> 
> I think I'd probably create a new page for sysctls (this isn't the only
> one needing documentation), and make sure it's listed in the "SEE ALSO"
> section of the other man pages.

Aren't sysctls documented under Documentation/ ?


>>>> and it more if a stick you toe in the pool verses
>>>> jumping in...
>>> 
>>> If we want more fine-grained control, I'm not yet seeing the argument
>>> that an export option on the destination server side is the way to do
>>> it.
>>> 
>>> Let's document the module parameter and go with that for now.
>> Now that cp will use copy_file_range() when available,
>> what are the steps needed to enable these fast copies?
> 
> 1) Make sure client and both servers support NFSv4.2 and
> server-to-server copy.
> 
> 2) Make sure destination server can access (at least for read) any
> exports on the source that you want to be able to copy from.
> 
> 3) echo 1 >/sys/module/nfsd/parameters/inter_copy_offload_enable on the
> destination server.
> 
> --b.

--
Chuck Lever
Steve Dickson Nov. 1, 2021, 6:24 p.m. UTC | #11
On 11/1/21 12:55, Chuck Lever III wrote:
> 
>> On Nov 1, 2021, at 11:40 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>
>> On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
>>> Hey!
>>>
>>> On 10/29/21 15:14, J. Bruce Fields wrote:
>>>> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
>>>>> On 10/29/21 12:40, J. Bruce Fields wrote:
>>>>>> Let's just stick with that for now, and leave it off by default until
>>>>>> we're sure it's mature enough.  Let's not introduce new configuration to
>>>>>> work around problems that we haven't really analyzed yet.
>>>>> How is this going to find problems? At least with the export option
>>>>> it is documented
>>>>
>>>> That sounds fixable.  We need documentation of module parameters anyway.
>>> Yeah I just took I don't see any documentation of module
>>> parameters anywhere for any of the modules. But by documentation
>>> I meant having the feature in the exports(5) manpage.
>>
>> I think I'd probably create a new page for sysctls (this isn't the only
>> one needing documentation), and make sure it's listed in the "SEE ALSO"
>> section of the other man pages.
> 
> Aren't sysctls documented under Documentation/ ?No... there is a Documentation/admin-guide/sysctl/sunrpc.rst
but it talks about rpcdebug commands and there is nothing
that documents the module parameters under
Documentation/filesystems/nfs

steved.

> 
> 
>>>>> and it more if a stick you toe in the pool verses
>>>>> jumping in...
>>>>
>>>> If we want more fine-grained control, I'm not yet seeing the argument
>>>> that an export option on the destination server side is the way to do
>>>> it.
>>>>
>>>> Let's document the module parameter and go with that for now.
>>> Now that cp will use copy_file_range() when available,
>>> what are the steps needed to enable these fast copies?
>>
>> 1) Make sure client and both servers support NFSv4.2 and
>> server-to-server copy.
>>
>> 2) Make sure destination server can access (at least for read) any
>> exports on the source that you want to be able to copy from.
>>
>> 3) echo 1 >/sys/module/nfsd/parameters/inter_copy_offload_enable on the
>> destination server.
>>
>> --b.
> 
> --
> Chuck Lever
> 
> 
>
J. Bruce Fields Nov. 1, 2021, 6:39 p.m. UTC | #12
On Mon, Nov 01, 2021 at 04:55:45PM +0000, Chuck Lever III wrote:
> 
> > On Nov 1, 2021, at 11:40 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
> >> Hey!
> >> 
> >> On 10/29/21 15:14, J. Bruce Fields wrote:
> >>> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
> >>>> On 10/29/21 12:40, J. Bruce Fields wrote:
> >>>>> Let's just stick with that for now, and leave it off by default until
> >>>>> we're sure it's mature enough.  Let's not introduce new configuration to
> >>>>> work around problems that we haven't really analyzed yet.
> >>>> How is this going to find problems? At least with the export option
> >>>> it is documented
> >>> 
> >>> That sounds fixable.  We need documentation of module parameters anyway.
> >> Yeah I just took I don't see any documentation of module
> >> parameters anywhere for any of the modules. But by documentation
> >> I meant having the feature in the exports(5) manpage.
> > 
> > I think I'd probably create a new page for sysctls (this isn't the only
> > one needing documentation), and make sure it's listed in the "SEE ALSO"
> > section of the other man pages.
> 
> Aren't sysctls documented under Documentation/ ?

Sorry, I meant "module parameter".  Anyway, yes, looks like both are
listed in Documentation/admin-guide/kernel-parameters.txt.

Might be nice if these were in a man page too somewhere, but maybe
that's not the most important thing these days.

--b.

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 91ba391f9b32..14bc3f0b0149 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3243,6 +3243,19 @@
 			driver. A non-zero value sets the minimum interval
 			in seconds between layoutstats transmissions.
 
+	nfsd.inter_copy_offload_enable =
+			[NFSv4.2] When set to 1, the server will support
+			server-to-server copies for which this server is
+			the destination of the copy.
+
+	nfsd.nfsd4_ssc_umount_timeout =
+			[NFSv4.2] When used as the destination of a
+			server-to-server copy, knfsd temporarily mounts
+			the source server.  It caches the mount in case
+			it will be needed again, and discards it if not
+			used for the number of milliseconds specified by
+			this parameter.
+
 	nfsd.nfs4_disable_idmapping=
 			[NFSv4] When set to the default of '1', the NFSv4
 			server will return only numeric uids and gids to
@@ -3250,6 +3263,7 @@
 			and gids from such clients.  This is intended to ease
 			migration from NFSv2/v3.
 
+
 	nmi_backtrace.backtrace_idle [KNL]
 			Dump stacks even of idle CPUs in response to an
 			NMI stack-backtrace request.
Chuck Lever Nov. 1, 2021, 6:44 p.m. UTC | #13
> On Nov 1, 2021, at 2:39 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Mon, Nov 01, 2021 at 04:55:45PM +0000, Chuck Lever III wrote:
>> 
>>> On Nov 1, 2021, at 11:40 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>> 
>>> On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
>>>> Hey!
>>>> 
>>>> On 10/29/21 15:14, J. Bruce Fields wrote:
>>>>> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
>>>>>> On 10/29/21 12:40, J. Bruce Fields wrote:
>>>>>>> Let's just stick with that for now, and leave it off by default until
>>>>>>> we're sure it's mature enough.  Let's not introduce new configuration to
>>>>>>> work around problems that we haven't really analyzed yet.
>>>>>> How is this going to find problems? At least with the export option
>>>>>> it is documented
>>>>> 
>>>>> That sounds fixable.  We need documentation of module parameters anyway.
>>>> Yeah I just took I don't see any documentation of module
>>>> parameters anywhere for any of the modules. But by documentation
>>>> I meant having the feature in the exports(5) manpage.
>>> 
>>> I think I'd probably create a new page for sysctls (this isn't the only
>>> one needing documentation), and make sure it's listed in the "SEE ALSO"
>>> section of the other man pages.
>> 
>> Aren't sysctls documented under Documentation/ ?
> 
> Sorry, I meant "module parameter".  Anyway, yes, looks like both are
> listed in Documentation/admin-guide/kernel-parameters.txt.
> 
> Might be nice if these were in a man page too somewhere, but maybe
> that's not the most important thing these days.
> 
> --b.

This looks like a step forward to me.


> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 91ba391f9b32..14bc3f0b0149 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3243,6 +3243,19 @@
> 			driver. A non-zero value sets the minimum interval
> 			in seconds between layoutstats transmissions.
> 
> +	nfsd.inter_copy_offload_enable =
> +			[NFSv4.2] When set to 1, the server will support
> +			server-to-server copies for which this server is
> +			the destination of the copy.
> +
> +	nfsd.nfsd4_ssc_umount_timeout =
> +			[NFSv4.2] When used as the destination of a
> +			server-to-server copy, knfsd temporarily mounts
> +			the source server.  It caches the mount in case
> +			it will be needed again, and discards it if not
> +			used for the number of milliseconds specified by
> +			this parameter.
> +
> 	nfsd.nfs4_disable_idmapping=
> 			[NFSv4] When set to the default of '1', the NFSv4
> 			server will return only numeric uids and gids to
> @@ -3250,6 +3263,7 @@
> 			and gids from such clients.  This is intended to ease
> 			migration from NFSv2/v3.
> 
> +
> 	nmi_backtrace.backtrace_idle [KNL]
> 			Dump stacks even of idle CPUs in response to an
> 			NMI stack-backtrace request.

--
Chuck Lever
Steve Dickson Nov. 1, 2021, 7:02 p.m. UTC | #14
On 11/1/21 11:40, J. Bruce Fields wrote:
> On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
>> Hey!
>>
>> On 10/29/21 15:14, J. Bruce Fields wrote:
>>> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
>>>> On 10/29/21 12:40, J. Bruce Fields wrote:
>>>>> Let's just stick with that for now, and leave it off by default until
>>>>> we're sure it's mature enough.  Let's not introduce new configuration to
>>>>> work around problems that we haven't really analyzed yet.
>>>> How is this going to find problems? At least with the export option
>>>> it is documented
>>>
>>> That sounds fixable.  We need documentation of module parameters anyway.
>> Yeah I just took I don't see any documentation of module
>> parameters anywhere for any of the modules. But by documentation
>> I meant having the feature in the exports(5) manpage.
> 
> I think I'd probably create a new page for sysctls (this isn't the only
> one needing documentation), and make sure it's listed in the "SEE ALSO"
> section of the other man pages.
> 
>>>> and it more if a stick you toe in the pool verses
>>>> jumping in...
>>>
>>> If we want more fine-grained control, I'm not yet seeing the argument
>>> that an export option on the destination server side is the way to do
>>> it.
>>>
>>> Let's document the module parameter and go with that for now.
>> Now that cp will use copy_file_range() when available,
>> what are the steps needed to enable these fast copies?
> 
> 1) Make sure client and both servers support NFSv4.2 and
> server-to-server copy.
Something is already figuring this out... The only time
the client sends a COPY_NOTIFY and COPY is when both
mounts are 4.2. I have not looked into but that is what
the network traces are showing.

> 
> 2) Make sure destination server can access (at least for read) any
> exports on the source that you want to be able to copy from.
How can one server know what the other server has exported
or access to??

> 
> 3) echo 1 >/sys/module/nfsd/parameters/inter_copy_offload_enable on the
> destination server.
Who would be doing this? Plus this would not survive over a reboot.
An export would as well a /etc/modprobe.d/ file.

I can see the admin setting up the export but I really
don't see the admin doing the echo or creating the file
esp since the neither would is documented.

steved.
Steve Dickson Nov. 1, 2021, 7:10 p.m. UTC | #15
On 11/1/21 14:39, Bruce Fields wrote:
> On Mon, Nov 01, 2021 at 04:55:45PM +0000, Chuck Lever III wrote:
>>
>>> On Nov 1, 2021, at 11:40 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>>
>>> On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
>>>> Hey!
>>>>
>>>> On 10/29/21 15:14, J. Bruce Fields wrote:
>>>>> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
>>>>>> On 10/29/21 12:40, J. Bruce Fields wrote:
>>>>>>> Let's just stick with that for now, and leave it off by default until
>>>>>>> we're sure it's mature enough.  Let's not introduce new configuration to
>>>>>>> work around problems that we haven't really analyzed yet.
>>>>>> How is this going to find problems? At least with the export option
>>>>>> it is documented
>>>>>
>>>>> That sounds fixable.  We need documentation of module parameters anyway.
>>>> Yeah I just took I don't see any documentation of module
>>>> parameters anywhere for any of the modules. But by documentation
>>>> I meant having the feature in the exports(5) manpage.
>>>
>>> I think I'd probably create a new page for sysctls (this isn't the only
>>> one needing documentation), and make sure it's listed in the "SEE ALSO"
>>> section of the other man pages.
>>
>> Aren't sysctls documented under Documentation/ ?
> 
> Sorry, I meant "module parameter".  Anyway, yes, looks like both are
> listed in Documentation/admin-guide/kernel-parameters.txt.
> 
> Might be nice if these were in a man page too somewhere, but maybe
> that's not the most important thing these days.
> 
> --b.
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 91ba391f9b32..14bc3f0b0149 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3243,6 +3243,19 @@
>   			driver. A non-zero value sets the minimum interval
>   			in seconds between layoutstats transmissions.
>   
> +	nfsd.inter_copy_offload_enable =
> +			[NFSv4.2] When set to 1, the server will support
> +			server-to-server copies for which this server is
> +			the destination of the copy.
> +
> +	nfsd.nfsd4_ssc_umount_timeout =
> +			[NFSv4.2] When used as the destination of a
> +			server-to-server copy, knfsd temporarily mounts
> +			the source server.  It caches the mount in case
> +			it will be needed again, and discards it if not
> +			used for the number of milliseconds specified by
> +			this parameter.
> +
>   	nfsd.nfs4_disable_idmapping=
>   			[NFSv4] When set to the default of '1', the NFSv4
>   			server will return only numeric uids and gids to
> @@ -3250,6 +3263,7 @@
>   			and gids from such clients.  This is intended to ease
>   			migration from NFSv2/v3.
>   
> +
>   	nmi_backtrace.backtrace_idle [KNL]
>   			Dump stacks even of idle CPUs in response to an
>   			NMI stack-backtrace request.
> 
Interesting... I don't see these in the Linus tree I'm looking at [1]
find Documentation/ -type f  | xargs grep -i inter_copy_offload_enable

steved

[1] git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
Dai Ngo Nov. 1, 2021, 7:22 p.m. UTC | #16
On 11/1/21 12:02 PM, Steve Dickson wrote:
>
>
> On 11/1/21 11:40, J. Bruce Fields wrote:
>> On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
>>> Hey!
>>>
>>> On 10/29/21 15:14, J. Bruce Fields wrote:
>>>> On Fri, Oct 29, 2021 at 01:30:36PM -0400, Steve Dickson wrote:
>>>>> On 10/29/21 12:40, J. Bruce Fields wrote:
>>>>>> Let's just stick with that for now, and leave it off by default 
>>>>>> until
>>>>>> we're sure it's mature enough.  Let's not introduce new 
>>>>>> configuration to
>>>>>> work around problems that we haven't really analyzed yet.
>>>>> How is this going to find problems? At least with the export option
>>>>> it is documented
>>>>
>>>> That sounds fixable.  We need documentation of module parameters 
>>>> anyway.
>>> Yeah I just took I don't see any documentation of module
>>> parameters anywhere for any of the modules. But by documentation
>>> I meant having the feature in the exports(5) manpage.
>>
>> I think I'd probably create a new page for sysctls (this isn't the only
>> one needing documentation), and make sure it's listed in the "SEE ALSO"
>> section of the other man pages.
>>
>>>>> and it more if a stick you toe in the pool verses
>>>>> jumping in...
>>>>
>>>> If we want more fine-grained control, I'm not yet seeing the argument
>>>> that an export option on the destination server side is the way to do
>>>> it.
>>>>
>>>> Let's document the module parameter and go with that for now.
>>> Now that cp will use copy_file_range() when available,
>>> what are the steps needed to enable these fast copies?
>>
>> 1) Make sure client and both servers support NFSv4.2 and
>> server-to-server copy.
> Something is already figuring this out... The only time
> the client sends a COPY_NOTIFY and COPY is when both
> mounts are 4.2. I have not looked into but that is what
> the network traces are showing.
>
>>
>> 2) Make sure destination server can access (at least for read) any
>> exports on the source that you want to be able to copy from.
> How can one server know what the other server has exported
> or access to??
>
>>
>> 3) echo 1 >/sys/module/nfsd/parameters/inter_copy_offload_enable on the
>> destination server.
> Who would be doing this? Plus this would not survive over a reboot.
> An export would as well a /etc/modprobe.d/ file.

You can add a line in /etc/modprobe.d/nfsd.conf:

options nfsd inter_copy_offload_enable=Y

to enable the option.

-Dai


>
> I can see the admin setting up the export but I really
> don't see the admin doing the echo or creating the file
> esp since the neither would is documented.
>
> steved.
>
J. Bruce Fields Nov. 1, 2021, 7:25 p.m. UTC | #17
On Mon, Nov 01, 2021 at 12:22:15PM -0700, dai.ngo@oracle.com wrote:
> 
> On 11/1/21 12:02 PM, Steve Dickson wrote:
> >
> >
> >On 11/1/21 11:40, J. Bruce Fields wrote:
> >>On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
> >>>Now that cp will use copy_file_range() when available,
> >>>what are the steps needed to enable these fast copies?
> >>
> >>1) Make sure client and both servers support NFSv4.2 and
> >>server-to-server copy.
> >Something is already figuring this out... The only time
> >the client sends a COPY_NOTIFY and COPY is when both
> >mounts are 4.2. I have not looked into but that is what
> >the network traces are showing.

Right.  I was thinking what I'd tell an admin who wanted to set up
server-to-server copy.  The first thing they'd need to do was check that
their clients and servers were new enough.

> >>2) Make sure destination server can access (at least for read) any
> >>exports on the source that you want to be able to copy from.
> >How can one server know what the other server has exported
> >or access to??

And the second is to make sure that the destination server is able to
read from the source.

> >>3) echo 1 >/sys/module/nfsd/parameters/inter_copy_offload_enable on the
> >>destination server.
> >Who would be doing this? Plus this would not survive over a reboot.
> >An export would as well a /etc/modprobe.d/ file.
> 
> You can add a line in /etc/modprobe.d/nfsd.conf:
> 
> options nfsd inter_copy_offload_enable=Y
> 
> to enable the option.

Yep, it would be better to document it that way, thanks.

--b.
J. Bruce Fields Nov. 1, 2021, 7:26 p.m. UTC | #18
On Mon, Nov 01, 2021 at 03:10:31PM -0400, Steve Dickson wrote:
> 
> 
> On 11/1/21 14:39, Bruce Fields wrote:
> >diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >index 91ba391f9b32..14bc3f0b0149 100644
> >--- a/Documentation/admin-guide/kernel-parameters.txt
> >+++ b/Documentation/admin-guide/kernel-parameters.txt
> >@@ -3243,6 +3243,19 @@
> >  			driver. A non-zero value sets the minimum interval
> >  			in seconds between layoutstats transmissions.
> >+	nfsd.inter_copy_offload_enable =
> >+			[NFSv4.2] When set to 1, the server will support
> >+			server-to-server copies for which this server is
> >+			the destination of the copy.
> >+
> >+	nfsd.nfsd4_ssc_umount_timeout =
> >+			[NFSv4.2] When used as the destination of a
> >+			server-to-server copy, knfsd temporarily mounts
> >+			the source server.  It caches the mount in case
> >+			it will be needed again, and discards it if not
> >+			used for the number of milliseconds specified by
> >+			this parameter.
> >+
> >  	nfsd.nfs4_disable_idmapping=
> >  			[NFSv4] When set to the default of '1', the NFSv4
> >  			server will return only numeric uids and gids to
> >@@ -3250,6 +3263,7 @@
> >  			and gids from such clients.  This is intended to ease
> >  			migration from NFSv2/v3.
> >+
> >  	nmi_backtrace.backtrace_idle [KNL]
> >  			Dump stacks even of idle CPUs in response to an
> >  			NMI stack-backtrace request.
> >
> Interesting... I don't see these in the Linus tree I'm looking at [1]
> find Documentation/ -type f  | xargs grep -i inter_copy_offload_enable

I was suggesting that as a patch.  It's queued up for 5.16 now.

--b.
Steve Dickson Nov. 1, 2021, 8:25 p.m. UTC | #19
On 11/1/21 15:25, J. Bruce Fields wrote:
> On Mon, Nov 01, 2021 at 12:22:15PM -0700, dai.ngo@oracle.com wrote:
>>
>> On 11/1/21 12:02 PM, Steve Dickson wrote:
>>>
>>>
>>> On 11/1/21 11:40, J. Bruce Fields wrote:
>>>> On Mon, Nov 01, 2021 at 11:30:48AM -0400, Steve Dickson wrote:
>>>>> Now that cp will use copy_file_range() when available,
>>>>> what are the steps needed to enable these fast copies?
>>>>
>>>> 1) Make sure client and both servers support NFSv4.2 and
>>>> server-to-server copy.
>>> Something is already figuring this out... The only time
>>> the client sends a COPY_NOTIFY and COPY is when both
>>> mounts are 4.2. I have not looked into but that is what
>>> the network traces are showing.
> 
> Right.  I was thinking what I'd tell an admin who wanted to set up
> server-to-server copy.  The first thing they'd need to do was check that
> their clients and servers were new enough.
Well the code went in over two years ago
ce0887ac96d35 (Olga Kornievskaia 2019-10-09 11:50:48 -0400 1663)
so most modern kernel will have the feature.

The real question is does the cp command have the
copy_file_range() support which didn't go in until
Mar of this year
* Wed Mar 24 2021 Kamil Dudka <kdudka@redhat.com> - 8.32-20
- cp: use copy_file_range if available

> 
>>>> 2) Make sure destination server can access (at least for read) any
>>>> exports on the source that you want to be able to copy from.
>>> How can one server know what the other server has exported
>>> or access to??
> 
> And the second is to make sure that the destination server is able to
> read from the source.
> 
>>>> 3) echo 1 >/sys/module/nfsd/parameters/inter_copy_offload_enable on the
>>>> destination server.
>>> Who would be doing this? Plus this would not survive over a reboot.
>>> An export would as well a /etc/modprobe.d/ file.
>>
>> You can add a line in /etc/modprobe.d/nfsd.conf:
>>
>> options nfsd inter_copy_offload_enable=Y
>>
>> to enable the option.
Yes... This is what I meant by "a /etc/modprobe.d/ file"

> 
> Yep, it would be better to document it that way, thanks.
But the question is who would create this file?
nfs-utils or the admin?

If it is nfs-utils, it is a global switch unbeknown by
the admin (but documented). If by the admin, the person
would know about but it still will not documented.

With a export option, it would be per export switch
and documented. Just saying... :-)

steved.
Steve Dickson Nov. 1, 2021, 8:28 p.m. UTC | #20
On 11/1/21 15:26, Bruce Fields wrote:
> On Mon, Nov 01, 2021 at 03:10:31PM -0400, Steve Dickson wrote:
>>
>>
>> On 11/1/21 14:39, Bruce Fields wrote:
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index 91ba391f9b32..14bc3f0b0149 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -3243,6 +3243,19 @@
>>>   			driver. A non-zero value sets the minimum interval
>>>   			in seconds between layoutstats transmissions.
>>> +	nfsd.inter_copy_offload_enable =
>>> +			[NFSv4.2] When set to 1, the server will support
>>> +			server-to-server copies for which this server is
>>> +			the destination of the copy.
>>> +
>>> +	nfsd.nfsd4_ssc_umount_timeout =
>>> +			[NFSv4.2] When used as the destination of a
>>> +			server-to-server copy, knfsd temporarily mounts
>>> +			the source server.  It caches the mount in case
>>> +			it will be needed again, and discards it if not
>>> +			used for the number of milliseconds specified by
>>> +			this parameter.
>>> +
>>>   	nfsd.nfs4_disable_idmapping=
>>>   			[NFSv4] When set to the default of '1', the NFSv4
>>>   			server will return only numeric uids and gids to
>>> @@ -3250,6 +3263,7 @@
>>>   			and gids from such clients.  This is intended to ease
>>>   			migration from NFSv2/v3.
>>> +
>>>   	nmi_backtrace.backtrace_idle [KNL]
>>>   			Dump stacks even of idle CPUs in response to an
>>>   			NMI stack-backtrace request.
>>>
>> Interesting... I don't see these in the Linus tree I'm looking at [1]
>> find Documentation/ -type f  | xargs grep -i inter_copy_offload_enable
> 
> I was suggesting that as a patch.  It's queued up for 5.16 now.
Sorry I did miss the patch part of the email...

Does it make sense to document both nfs and nfsd module
parameters in a couple man pages?

steved.