Re: RFC: Patch for mountd to handle a database for exports

On Fri, Jul 21, 2023 at 1:33 PM Peter Eriksson <pen@lysator.liu.se> wrote:
>
>
> I did try to fix that but the code quickly got hairy so I didn’t like it. If we really want/need that I’m thinking of creating a special case for the V4: handling, sort of like prefixing the database key with a NUL byte or something (so that it will be sorted first).
>
> Does the ZFS share property generate a V4: line?
> I doubt anyone will convert a non-ZFS /etc/exports file to a DB file,
> so support of that does not seem to be needed.
>
> I see this as useful for the ZFS share property case,
> so if that works correctly, I do not see the above as a concern.
>
>
> No, the ZFS share property stuff never generates the V4: line(s) so it’s not a problem for the /etc/zfs/exports.db case.
>
> And converting /etc/exports to /etc/exports.db is not really necessary from a performance point since I doubt it ever will be very long.
> However I bet some enthusiastic fellow is bound to try to do it sometime in the future just because it can be done :-)
>
Yep. Capturing this in a man page update when the time comes needs
to be done, I think? (That the V4: line doesn't work in the DB.)

>
>
> Multiline options - well, the current ZFS code doesn’t support it either so no change from the current setup but it would be nice to have.
>  Ie, support for things like:
>     /export -sec=krb5
>     /export -sec=sys ro
>
> Yes. There is at least one PR that requests that the ZFS share property
> be enhanced to do this. Part of the reason for getting this patch in main
> is so that this can be pursued.
>
> Again, I only see this as a ZFS share property issue because I do not
> see any reason to convert /etc/exports flat files to db files?
>
>
> I agree that the need probably isn’t there.
>
> The current DB code will generate a syslog(LOG_ERR) warning if it detects anything not starting with / in the keys (and ignores it).
>
> Perhaps there should be a warning in the manual for mountd about not trying to convert /etc/exports into a DB (if it uses NFSv4 / the “V4:” line(s)).
> Or should we just special-case /etc/exports and forbid the check for /etc/exports.db?
>
> - Peter
>
> rick
>
>
>
>
>
>
> (I also agree that the USE_SHAREDB probably should be removed, I just have that here for now so that I can quickly disable the code)
>
> Regarding the patch to the zfs part - I’m not sure which way to go there - the current patch switches to always use the DB. But one could argue that the code could check for an existing /etc/zfs/exports.db and only use the DB-writing code if that already exists. That way it will support both the old way and the new way, but it requires an empty /etc/zfs/exports.db to be pre-created at initial boot time for it to start using it.

That's up to the ZFS folk. Hopefully kevans@ can figure it out.

rick

>
> - Peter
>
>
> On 21 Jul 2023, at 00:50, Rick Macklem <rick.macklem@gmail.com> wrote:
>
> Hi,
>
> Peter Eriksson has submitted an interesting patch that adds support
> for a database file for exports.  My understanding is that this improves
> performance when used with the ZFS share property for exporting a
> large number of file systems.
>
> There are a couple of user visible issues that I'd like feedback from
> others. (Hopefully Peter can correct me if I get any of these wrong.)
>
> - The patch uses a single database file and a new "-D" option to
> specify it on the command line.
> --> I think it might be less confusing to just put the database file(s)
>       in the exports list and identify them via a ".db" filename suffix.
> What do others think?
>
> The changes are #ifdef'd on USE_SHAREDB. I'm thinking that this
> change will be always built, so maybe USE_SHAREDB is not needed?
> (Obviously mountd(8)'s semantics will only changed if/when database
> file(s) are provided.)
>
> Once I have feedback on the above, I will put a patch up on
> phabricator.
>
> rick
> ps: I believe kevans@ has volunteered to shepperd the ZFS share
>     property changes.
>
>