From nobody Fri Jul 21 20:33:17 2023 X-Original-To: freebsd-fs@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4R71V54vRVz4nnbK for ; Fri, 21 Jul 2023 20:33:33 +0000 (UTC) (envelope-from pen@lysator.liu.se) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4R71V43ttxz3jkv; Fri, 21 Jul 2023 20:33:32 +0000 (UTC) (envelope-from pen@lysator.liu.se) Authentication-Results: mx1.freebsd.org; dkim=none; spf=pass (mx1.freebsd.org: domain of pen@lysator.liu.se designates 130.236.254.3 as permitted sender) smtp.mailfrom=pen@lysator.liu.se; dmarc=pass (policy=none) header.from=lysator.liu.se Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 1453F1EB67; Fri, 21 Jul 2023 22:33:30 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id F2EEA1EBF2; Fri, 21 Jul 2023 22:33:29 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,AWL,HTML_MESSAGE, T_SCC_BODY_TEXT_LINE autolearn=disabled version=3.4.6 X-Spam-Score: -1.0 Received: from smtpclient.apple (unknown [IPv6:2001:9b1:28fa:7900:1db5:1a62:7f0f:4ba]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id EE43C1EBF1; Fri, 21 Jul 2023 22:33:28 +0200 (CEST) From: Peter Eriksson Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_F528DA9E-C185-410F-9675-2F557F67BE84" List-Id: Filesystems List-Archive: https://lists.freebsd.org/archives/freebsd-fs List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-fs@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.600.7\)) Subject: Re: RFC: Patch for mountd to handle a database for exports Date: Fri, 21 Jul 2023 22:33:17 +0200 In-Reply-To: Cc: kevans@freebsd.org, Rick Macklem To: FreeBSD FS References: <2D8B626E-82BB-410C-B7C7-35B4D3834A44@lysator.liu.se> X-Mailer: Apple Mail (2.3731.600.7) X-Virus-Scanned: ClamAV using ClamSMTP X-Spamd-Result: default: False [-3.49 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.99)[-0.985]; MV_CASE(0.50)[]; DMARC_POLICY_ALLOW(-0.50)[lysator.liu.se,none]; RCVD_IN_DNSWL_MED(-0.20)[130.236.254.3:from]; R_SPF_ALLOW(-0.20)[+a:mail.lysator.liu.se]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; ASN(0.00)[asn:2843, ipnet:130.236.0.0/16, country:SE]; MLMMJ_DEST(0.00)[freebsd-fs@freebsd.org]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; R_DKIM_NA(0.00)[]; RCVD_TLS_LAST(0.00)[]; RCVD_COUNT_THREE(0.00)[4]; FREEMAIL_CC(0.00)[freebsd.org,gmail.com]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; TAGGED_RCPT(0.00)[]; RCPT_COUNT_THREE(0.00)[3]; TO_DN_SOME(0.00)[]; MID_RHS_MATCH_FROM(0.00)[] X-Rspamd-Queue-Id: 4R71V43ttxz3jkv X-Spamd-Bar: --- --Apple-Mail=_F528DA9E-C185-410F-9675-2F557F67BE84 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 >> I did try to fix that but the code quickly got hairy so I didn=E2=80=99= t like it. If we really want/need that I=E2=80=99m 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). >>=20 > 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. >=20 > 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=E2=80=99s 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.=20 However I bet some enthusiastic fellow is bound to try to do it sometime = in the future just because it can be done :-) >>=20 >> Multiline options - well, the current ZFS code doesn=E2=80=99t = support it either so no change from the current setup but it would be = nice to have. >> Ie, support for things like: >> /export -sec=3Dkrb5 >> /export -sec=3Dsys ro >>=20 > 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. >=20 > 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? >=20 I agree that the need probably isn=E2=80=99t 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 = =E2=80=9CV4:=E2=80=9D line(s)).=20 Or should we just special-case /etc/exports and forbid the check for = /etc/exports.db?=20 - Peter > rick >=20 >>=20 >>=20 >>=20 >>=20 >>=20 >> (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) >>=20 >> Regarding the patch to the zfs part - I=E2=80=99m 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. >>=20 >> - Peter >>=20 >>=20 >>> On 21 Jul 2023, at 00:50, Rick Macklem = wrote: >>>=20 >>> Hi, >>>=20 >>> 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. >>>=20 >>> 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.) >>>=20 >>> - 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? >>>=20 >>> 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.) >>>=20 >>> Once I have feedback on the above, I will put a patch up on >>> phabricator. >>>=20 >>> rick >>> ps: I believe kevans@ has volunteered to shepperd the ZFS share >>> property changes. --Apple-Mail=_F528DA9E-C185-410F-9675-2F557F67BE84 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
I did try to fix that but the code quickly got hairy so I = didn=E2=80=99t like it. If we really want/need that I=E2=80=99m 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=E2=80=99s 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 = :-)



Multiline options - well, the current ZFS code doesn=E2=80=99t = support it either so no change from the current setup but it would be = nice to have.
 Ie, support for things = like:
    /export = -sec=3Dkrb5
    /export -sec=3Dsys = 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=E2=80=99t 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 =E2=80=9CV4:=E2=80=9D= 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=E2=80=99m 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.

- 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.

= --Apple-Mail=_F528DA9E-C185-410F-9675-2F557F67BE84--