mbox series

[0/5] nfs-utils: Improving NFS re-exports

Message ID 20220502085045.13038-1-richard@nod.at (mailing list archive)
Headers show
Series nfs-utils: Improving NFS re-exports | expand

Message

Richard Weinberger May 2, 2022, 8:50 a.m. UTC
This is the first non-RFC iteration of the NFS re-export
improvement series for nfs-utils.
While the kernel side[0] didn't change at all and is still small,
the userspace side saw much more changes.

The core idea is adding new export option: reeport=
Using reexport= it is possible to mark an export entry in the exports
file explicitly as NFS re-export and select a strategy how unique
identifiers should be provided.
Currently two strategies are supported, "auto-fsidnum" and
"predefined-fsidnum", both use a SQLite database as backend to keep
track of generated ids.
For a more detailed description see patch "exports: Implement new export option reexport=".
I choose SQLite because nfs-utils already uses it and using SQL ids can nicely
generated and maintained. It will also scale for large setups where the amount
of subvolumes is high.

Beside of id generation this series also addresses the reboot problem.
If the re-exporting NFS server reboots, uncovered NFS subvolumes are not yet
mounted and file handles become stale.
Now mountd/exportd keeps track of uncovered subvolumes and makes sure they get
uncovered while nfsd starts.

The whole set of features is currently opt-in via --enable-reexport.
I'm also not sure about the rearrangement of the reexport code,
currently it is a helper library.

A typical export entry on a re-exporting server looks like:
	/nfs *(rw,no_root_squash,no_subtree_check,crossmnt,reexport=auto-fsidnum)
reexport=auto-fsidnum will automatically assign an fsid= to /nfs and all
uncovered subvolumes.

Richard Weinberger (5):
  Implement reexport helper library
  exports: Implement new export option reexport=
  export: Implement logic behind reexport=
  export: Avoid fsid= conflicts
  reexport: Make state database location configurable

[0] https://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git/log/?h=nfs_reexport_clean

 configure.ac                   |  12 ++
 nfs.conf                       |   3 +
 support/Makefile.am            |   4 +
 support/export/Makefile.am     |   2 +
 support/export/cache.c         |  71 ++++++-
 support/export/export.c        |  27 ++-
 support/include/nfslib.h       |   1 +
 support/nfs/Makefile.am        |   1 +
 support/nfs/exports.c          |  68 +++++++
 support/reexport/Makefile.am   |   6 +
 support/reexport/reexport.c    | 354 +++++++++++++++++++++++++++++++++
 support/reexport/reexport.h    |  39 ++++
 systemd/Makefile.am            |   4 +
 systemd/nfs-server-generator.c |  14 +-
 systemd/nfs.conf.man           |   6 +
 utils/exportd/Makefile.am      |   8 +-
 utils/exportd/exportd.c        |   5 +
 utils/exportfs/Makefile.am     |   6 +
 utils/exportfs/exportfs.c      |  21 +-
 utils/exportfs/exports.man     |  31 +++
 utils/mount/Makefile.am        |   7 +
 utils/mountd/Makefile.am       |   6 +
 utils/mountd/mountd.c          |   1 +
 utils/mountd/svc_run.c         |   6 +
 24 files changed, 690 insertions(+), 13 deletions(-)
 create mode 100644 support/reexport/Makefile.am
 create mode 100644 support/reexport/reexport.c
 create mode 100644 support/reexport/reexport.h

Comments

J. Bruce Fields May 2, 2022, 4:17 p.m. UTC | #1
On Mon, May 02, 2022 at 10:50:40AM +0200, Richard Weinberger wrote:
> This is the first non-RFC iteration of the NFS re-export
> improvement series for nfs-utils.
> While the kernel side[0] didn't change at all and is still small,
> the userspace side saw much more changes.
> 
> The core idea is adding new export option: reeport=
> Using reexport= it is possible to mark an export entry in the exports
> file explicitly as NFS re-export and select a strategy how unique
> identifiers should be provided.
> Currently two strategies are supported, "auto-fsidnum" and
> "predefined-fsidnum", both use a SQLite database as backend to keep
> track of generated ids.
> For a more detailed description see patch "exports: Implement new export option reexport=".
> I choose SQLite because nfs-utils already uses it and using SQL ids can nicely
> generated and maintained. It will also scale for large setups where the amount
> of subvolumes is high.
> 
> Beside of id generation this series also addresses the reboot problem.
> If the re-exporting NFS server reboots, uncovered NFS subvolumes are not yet
> mounted and file handles become stale.
> Now mountd/exportd keeps track of uncovered subvolumes and makes sure they get
> uncovered while nfsd starts.
> 
> The whole set of features is currently opt-in via --enable-reexport.

Can we remove that option before upstreaming?

For testing purposes it may makes sense to be able to turn the new code
on and off quickly.  But for something we're really going to support,
it's just another hurdle for users to jump through, and another case we
probably won't remember to test.  The export options themselves should
be enough configuration.

Anyway, basically sounds reasonable to me.  I'll try to give it a proper
review sometime, feel free to bug me if I don't get to it in a week or
so.

--b.


> I'm also not sure about the rearrangement of the reexport code,
> currently it is a helper library.
> 
> A typical export entry on a re-exporting server looks like:
> 	/nfs *(rw,no_root_squash,no_subtree_check,crossmnt,reexport=auto-fsidnum)
> reexport=auto-fsidnum will automatically assign an fsid= to /nfs and all
> uncovered subvolumes.
> 
> Richard Weinberger (5):
>   Implement reexport helper library
>   exports: Implement new export option reexport=
>   export: Implement logic behind reexport=
>   export: Avoid fsid= conflicts
>   reexport: Make state database location configurable
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git/log/?h=nfs_reexport_clean
> 
>  configure.ac                   |  12 ++
>  nfs.conf                       |   3 +
>  support/Makefile.am            |   4 +
>  support/export/Makefile.am     |   2 +
>  support/export/cache.c         |  71 ++++++-
>  support/export/export.c        |  27 ++-
>  support/include/nfslib.h       |   1 +
>  support/nfs/Makefile.am        |   1 +
>  support/nfs/exports.c          |  68 +++++++
>  support/reexport/Makefile.am   |   6 +
>  support/reexport/reexport.c    | 354 +++++++++++++++++++++++++++++++++
>  support/reexport/reexport.h    |  39 ++++
>  systemd/Makefile.am            |   4 +
>  systemd/nfs-server-generator.c |  14 +-
>  systemd/nfs.conf.man           |   6 +
>  utils/exportd/Makefile.am      |   8 +-
>  utils/exportd/exportd.c        |   5 +
>  utils/exportfs/Makefile.am     |   6 +
>  utils/exportfs/exportfs.c      |  21 +-
>  utils/exportfs/exports.man     |  31 +++
>  utils/mount/Makefile.am        |   7 +
>  utils/mountd/Makefile.am       |   6 +
>  utils/mountd/mountd.c          |   1 +
>  utils/mountd/svc_run.c         |   6 +
>  24 files changed, 690 insertions(+), 13 deletions(-)
>  create mode 100644 support/reexport/Makefile.am
>  create mode 100644 support/reexport/reexport.c
>  create mode 100644 support/reexport/reexport.h
> 
> -- 
> 2.31.1
Steve Dickson May 2, 2022, 10:46 p.m. UTC | #2
On 5/2/22 12:17 PM, J. Bruce Fields wrote:
> On Mon, May 02, 2022 at 10:50:40AM +0200, Richard Weinberger wrote:
>> This is the first non-RFC iteration of the NFS re-export
>> improvement series for nfs-utils.
>> While the kernel side[0] didn't change at all and is still small,
>> the userspace side saw much more changes.
>>
>> The core idea is adding new export option: reeport=
>> Using reexport= it is possible to mark an export entry in the exports
>> file explicitly as NFS re-export and select a strategy how unique
>> identifiers should be provided.
>> Currently two strategies are supported, "auto-fsidnum" and
>> "predefined-fsidnum", both use a SQLite database as backend to keep
>> track of generated ids.
>> For a more detailed description see patch "exports: Implement new export option reexport=".
>> I choose SQLite because nfs-utils already uses it and using SQL ids can nicely
>> generated and maintained. It will also scale for large setups where the amount
>> of subvolumes is high.
>>
>> Beside of id generation this series also addresses the reboot problem.
>> If the re-exporting NFS server reboots, uncovered NFS subvolumes are not yet
>> mounted and file handles become stale.
>> Now mountd/exportd keeps track of uncovered subvolumes and makes sure they get
>> uncovered while nfsd starts.
>>
>> The whole set of features is currently opt-in via --enable-reexport.
> 
> Can we remove that option before upstreaming?
> 
> For testing purposes it may makes sense to be able to turn the new code
> on and off quickly.  But for something we're really going to support,
> it's just another hurdle for users to jump through, and another case we
> probably won't remember to test.  The export options themselves should
> be enough configuration.
> 
> Anyway, basically sounds reasonable to me.  I'll try to give it a proper
> review sometime, feel free to bug me if I don't get to it in a week or
> so.
How about --disable-reexport? So it is on by default to help testing
but if there are issues we can turn it off...

steved.

> 
> --b.
> 
> 
>> I'm also not sure about the rearrangement of the reexport code,
>> currently it is a helper library.
>>
>> A typical export entry on a re-exporting server looks like:
>> 	/nfs *(rw,no_root_squash,no_subtree_check,crossmnt,reexport=auto-fsidnum)
>> reexport=auto-fsidnum will automatically assign an fsid= to /nfs and all
>> uncovered subvolumes.
>>
>> Richard Weinberger (5):
>>    Implement reexport helper library
>>    exports: Implement new export option reexport=
>>    export: Implement logic behind reexport=
>>    export: Avoid fsid= conflicts
>>    reexport: Make state database location configurable
>>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git/log/?h=nfs_reexport_clean
>>
>>   configure.ac                   |  12 ++
>>   nfs.conf                       |   3 +
>>   support/Makefile.am            |   4 +
>>   support/export/Makefile.am     |   2 +
>>   support/export/cache.c         |  71 ++++++-
>>   support/export/export.c        |  27 ++-
>>   support/include/nfslib.h       |   1 +
>>   support/nfs/Makefile.am        |   1 +
>>   support/nfs/exports.c          |  68 +++++++
>>   support/reexport/Makefile.am   |   6 +
>>   support/reexport/reexport.c    | 354 +++++++++++++++++++++++++++++++++
>>   support/reexport/reexport.h    |  39 ++++
>>   systemd/Makefile.am            |   4 +
>>   systemd/nfs-server-generator.c |  14 +-
>>   systemd/nfs.conf.man           |   6 +
>>   utils/exportd/Makefile.am      |   8 +-
>>   utils/exportd/exportd.c        |   5 +
>>   utils/exportfs/Makefile.am     |   6 +
>>   utils/exportfs/exportfs.c      |  21 +-
>>   utils/exportfs/exports.man     |  31 +++
>>   utils/mount/Makefile.am        |   7 +
>>   utils/mountd/Makefile.am       |   6 +
>>   utils/mountd/mountd.c          |   1 +
>>   utils/mountd/svc_run.c         |   6 +
>>   24 files changed, 690 insertions(+), 13 deletions(-)
>>   create mode 100644 support/reexport/Makefile.am
>>   create mode 100644 support/reexport/reexport.c
>>   create mode 100644 support/reexport/reexport.h
>>
>> -- 
>> 2.31.1
>
Chuck Lever III May 3, 2022, midnight UTC | #3
> On May 2, 2022, at 6:46 PM, Steve Dickson <steved@redhat.com> wrote:
> 
> 
> 
> On 5/2/22 12:17 PM, J. Bruce Fields wrote:
>> On Mon, May 02, 2022 at 10:50:40AM +0200, Richard Weinberger wrote:
>>> This is the first non-RFC iteration of the NFS re-export
>>> improvement series for nfs-utils.
>>> While the kernel side[0] didn't change at all and is still small,
>>> the userspace side saw much more changes.
>>> 
>>> The core idea is adding new export option: reeport=
>>> Using reexport= it is possible to mark an export entry in the exports
>>> file explicitly as NFS re-export and select a strategy how unique
>>> identifiers should be provided.
>>> Currently two strategies are supported, "auto-fsidnum" and
>>> "predefined-fsidnum", both use a SQLite database as backend to keep
>>> track of generated ids.
>>> For a more detailed description see patch "exports: Implement new export option reexport=".
>>> I choose SQLite because nfs-utils already uses it and using SQL ids can nicely
>>> generated and maintained. It will also scale for large setups where the amount
>>> of subvolumes is high.
>>> 
>>> Beside of id generation this series also addresses the reboot problem.
>>> If the re-exporting NFS server reboots, uncovered NFS subvolumes are not yet
>>> mounted and file handles become stale.
>>> Now mountd/exportd keeps track of uncovered subvolumes and makes sure they get
>>> uncovered while nfsd starts.
>>> 
>>> The whole set of features is currently opt-in via --enable-reexport.
>> Can we remove that option before upstreaming?

Somehow I missed Bruce's comment, but I had the same thought.
The cover letter does not explain why someone would want to
build nfs-utils without re-export support. If there isn't a
good reason for needing this switch, IMO it can be removed.


>> For testing purposes it may makes sense to be able to turn the new code
>> on and off quickly.  But for something we're really going to support,
>> it's just another hurdle for users to jump through, and another case we
>> probably won't remember to test.  The export options themselves should
>> be enough configuration.
>> Anyway, basically sounds reasonable to me.  I'll try to give it a proper
>> review sometime, feel free to bug me if I don't get to it in a week or
>> so.
> How about --disable-reexport? So it is on by default to help testing
> but if there are issues we can turn it off...
> 
> steved.
> 
>> --b.
>>> I'm also not sure about the rearrangement of the reexport code,
>>> currently it is a helper library.
>>> 
>>> A typical export entry on a re-exporting server looks like:
>>> 	/nfs *(rw,no_root_squash,no_subtree_check,crossmnt,reexport=auto-fsidnum)
>>> reexport=auto-fsidnum will automatically assign an fsid= to /nfs and all
>>> uncovered subvolumes.
>>> 
>>> Richard Weinberger (5):
>>>   Implement reexport helper library
>>>   exports: Implement new export option reexport=
>>>   export: Implement logic behind reexport=
>>>   export: Avoid fsid= conflicts
>>>   reexport: Make state database location configurable
>>> 
>>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git/log/?h=nfs_reexport_clean
>>> 
>>>  configure.ac                   |  12 ++
>>>  nfs.conf                       |   3 +
>>>  support/Makefile.am            |   4 +
>>>  support/export/Makefile.am     |   2 +
>>>  support/export/cache.c         |  71 ++++++-
>>>  support/export/export.c        |  27 ++-
>>>  support/include/nfslib.h       |   1 +
>>>  support/nfs/Makefile.am        |   1 +
>>>  support/nfs/exports.c          |  68 +++++++
>>>  support/reexport/Makefile.am   |   6 +
>>>  support/reexport/reexport.c    | 354 +++++++++++++++++++++++++++++++++
>>>  support/reexport/reexport.h    |  39 ++++
>>>  systemd/Makefile.am            |   4 +
>>>  systemd/nfs-server-generator.c |  14 +-
>>>  systemd/nfs.conf.man           |   6 +
>>>  utils/exportd/Makefile.am      |   8 +-
>>>  utils/exportd/exportd.c        |   5 +
>>>  utils/exportfs/Makefile.am     |   6 +
>>>  utils/exportfs/exportfs.c      |  21 +-
>>>  utils/exportfs/exports.man     |  31 +++
>>>  utils/mount/Makefile.am        |   7 +
>>>  utils/mountd/Makefile.am       |   6 +
>>>  utils/mountd/mountd.c          |   1 +
>>>  utils/mountd/svc_run.c         |   6 +
>>>  24 files changed, 690 insertions(+), 13 deletions(-)
>>>  create mode 100644 support/reexport/Makefile.am
>>>  create mode 100644 support/reexport/reexport.c
>>>  create mode 100644 support/reexport/reexport.h
>>> 
>>> -- 
>>> 2.31.1
> 

--
Chuck Lever
Richard Weinberger May 23, 2022, 7:53 a.m. UTC | #4
Bruce,

----- Urspr√ľngliche Mail -----
> Von: "bfields" <bfields@fieldses.org>
>> The whole set of features is currently opt-in via --enable-reexport.
> 
> Can we remove that option before upstreaming?

What is the final resolution regarding this option?
I can think of embedded/memory constraint systems where the dependency on SQLite
is not wanted.

On the other hand, with my latest patch, the plugin interface, the could be solved
too.

> For testing purposes it may makes sense to be able to turn the new code
> on and off quickly.  But for something we're really going to support,
> it's just another hurdle for users to jump through, and another case we
> probably won't remember to test.  The export options themselves should
> be enough configuration.
> 
> Anyway, basically sounds reasonable to me.  I'll try to give it a proper
> review sometime, feel free to bug me if I don't get to it in a week or
> so.

*kind ping* :-)

Please also don't forget the kernel side of this work. It is small but still needed.
See: https://lore.kernel.org/linux-nfs/20220110184419.27665-1-richard@nod.at/
or
https://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git/log/?h=nfs_reexport_clean

Since the kernel changes don't change the ABI, it should not really matter which part
is merged first. Kernel or userspace. The only important part is stating the right
kernel version in the nfs-utils manpages.

Thanks,
//richard
Chuck Lever III May 23, 2022, 2:25 p.m. UTC | #5
> On May 23, 2022, at 3:53 AM, Richard Weinberger <richard@nod.at> wrote:
> 
> Bruce,
> 
> ----- Urspr√ľngliche Mail -----
>> Von: "bfields" <bfields@fieldses.org>
>>> The whole set of features is currently opt-in via --enable-reexport.
>> 
>> Can we remove that option before upstreaming?
> 
> What is the final resolution regarding this option?
> I can think of embedded/memory constraint systems where the dependency on SQLite
> is not wanted.
> 
> On the other hand, with my latest patch, the plugin interface, the could be solved
> too.
> 
>> For testing purposes it may makes sense to be able to turn the new code
>> on and off quickly.  But for something we're really going to support,
>> it's just another hurdle for users to jump through, and another case we
>> probably won't remember to test.  The export options themselves should
>> be enough configuration.
>> 
>> Anyway, basically sounds reasonable to me.  I'll try to give it a proper
>> review sometime, feel free to bug me if I don't get to it in a week or
>> so.
> 
> *kind ping* :-)
> 
> Please also don't forget the kernel side of this work. It is small but still needed.
> See: https://lore.kernel.org/linux-nfs/20220110184419.27665-1-richard@nod.at/
> or
> https://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git/log/?h=nfs_reexport_clean

In his review comments, Bruce asked if testing with NFSv4 has been done.

Also, can an idle client allow the re-export server to unmount an
automounted export?

Once you have NFSv4 results, the kernel patches need to be posted again
with Cc: linux-fsdevel@vger.kernel.org and autofs@vger.kernel.org


> Since the kernel changes don't change the ABI, it should not really matter which part
> is merged first. Kernel or userspace. The only important part is stating the right
> kernel version in the nfs-utils manpages.
> 
> Thanks,
> //richard

--
Chuck Lever
J. Bruce Fields May 23, 2022, 2:29 p.m. UTC | #6
On Mon, May 23, 2022 at 09:53:45AM +0200, Richard Weinberger wrote:
> Please also don't forget the kernel side of this work. It is small but
> still needed.  See:
> https://lore.kernel.org/linux-nfs/20220110184419.27665-1-richard@nod.at/
> or
> https://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git/log/?h=nfs_reexport_clean

The follow_down() code, at least, needs vfs review, maybe resend with a
cc to Al Viro and linux-fsdevel.

I'm not really familiar with how the vfs automounting code works.

> Since the kernel changes don't change the ABI, it should not really
> matter which part is merged first. Kernel or userspace. The only
> important part is stating the right kernel version in the nfs-utils
> manpages.

OK, as long as we make sure nothing annoying happens in the
old-kernel/new-nfs-utils case.  (Looks to me like it should be OK.)

--b.
J. Bruce Fields May 23, 2022, 2:31 p.m. UTC | #7
On Mon, May 23, 2022 at 09:53:45AM +0200, Richard Weinberger wrote:
> > Von: "bfields" <bfields@fieldses.org>
> > Anyway, basically sounds reasonable to me.  I'll try to give it a proper
> > review sometime, feel free to bug me if I don't get to it in a week or
> > so.
> 
> *kind ping* :-)

Anyway, yeah, still planning to, but I have a few other things I wanted
to get out of the way first.

--b.