Consistently use vop_stdpathconf() for default pathconf values.

[jhb]

Summary

Update filesystems not currently using vop_stdpathconf() in pathconf
VOPs to use vop_stdpathconf() for any configuration variables that do
not have filesystem-specific values. vop_stdpathconf() is used for
variables that have system-wide settings as well as providing default
values for some values based on system limits. Filesystems can still
explicitly override individual settings.

Note that there was some back and forth on vop_stdpathconf() between
phk and bde when it was first introduced. phk added vop_stdpathconf()
and then replaced various filesystem pathconf VOPs with the standard
version. bde reverted the replacements because he felt that it was
not reporting some filesystem-specific variables. However, I think
that the approach in this patch (fall through to vop_stdpathconf()
except for filesystem-specific overrides) is probably the better approach
than requiring each fs to reimplement the values from the standard one.

PR: 219851

[kib]

Curiously devfs already does this.

Also, I believe that zfs would also benefit from the same threatment, at least it start correctly handling more conf queries after the default case is added to zfs_pathconf().

[ngie]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

This doesn't necessarily seem correct -- this pathconf handler is whitelisting supported pathconf* use for FIFOs.

The smbfs in juxtaposition seems like it should support all of *pathconf since it's not a pseudo filesystem.

[pho]

Ran all of the stress2 tests on amd64 + a buildworld. No problems seen.

[jhb]

In D11541#238712, @kib wrote:

Curiously devfs already does this.

bde@ probably didn't revert devfs but only ufs?

Also, I believe that zfs would also benefit from the same threatment, at least it start correctly handling more conf queries after the default case is added to zfs_pathconf().

ZFS already does this, but in a non-obvious way. zfs_pathconf() returns EOPNOTSUPP in it's default case, but the actual VOP for FreeBSD is zfs_freebsd_pathconf() which calls vop_stdpathconf() if zfs_pathconf() returns EOPNOTSUPP.

[jhb]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

Are there values in vop_stdpathconf() that are not valid for FIFOs? They all seem applicable to me. To my mind we shouldn't put things in vop_stdpathconf() unless they are always valid for all filesystems. Some filesystems might want to override certain standard values if they have a smaller limit (e.g. NAME_MAX or LINK_MAX as smbfs does), but any setting in vop_stdpathconf() should always be correct otherwise.

[kib]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

I actually do not understand what was said by ngie at all. But, reading your response, I now got another question: does smbfs support fifos at all ? AFAIR no, and then reporting values would be weird.

[jhb]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

I believe Ngie is saying that fifo_pathconf() was intentionally returning EINVAL for some of the values listed in vop_stdpathconf() and thus only whitelisting certain pathconf options. I don't believe that is correct and that all of the values reported by vop_stdpathconf() are in fact valid for FIFOs (and my question to Ngie is if she feels the same way). Put another way, the question is: are there any values in vop_stdpathconf() which are not valid for FIFOs?

Re: smbfs, I believe Ngie mentioned this just as an example of a case where it's ok to use vop_stdpathconf() but not in conjunction with FIFOs? The only pathconf method I changed in smbfs is for non-FIFO files.

[kib]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

The call only uses the vnode to look up the filesystem. In other words, with the patch, smbfs returns PIPE_BUF for PC_PIPE_BUF. The same issue is already present for devfs, BTW.

[ngie]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

I was trying to compare and contrast fifofs and smbfs "whitelisting" supported *pathconf constants before this change, and loosening the requirements to handle *pathconf constants per vop_stdpathconf(..) after this change.

re: fifofs: I would have to look at mkfifo and related system calls -- my concern is there might be a requirement buried somewhere that would fail after this change, but virtue of the fact that very little is handled before this change, and pretty much everything is handled after this change.

re: smbfs: I'm not concerned about the change. It's good that our smbfs implementation will start reporting more valid information (assuming that the underlying implementation supports it -- if that's not true, that's a bug in smbfs).

343 ↗

(On Diff #30595)

*but virtue of the fact -> by virtue of the fact

[ngie]

One way to build confidence with this change is to run pjdfstest:

# 0. Elevate privileges because pjdfstest requires root
su
# 1. Run against UFS.
env TMPDIR=/path/to/temporary/ufs/mountpoint kyua test -k /usr/tests/sys/pjdfstest
# 2. Run against ZFS.
env TMPDIR=/path/to/temporary/zpool/mountpoint kyua test -k /usr/tests/sys/pjdfstest

More details about pjdfstest can be found here: https://wiki.freebsd.org/Pjdfstest (as well as a link to the README that describes how it can be run via kyua). I would really appreciate any and all feedback related to this wiki page.

[jhb]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

From my reading of http://pubs.opengroup.org/onlinepubs/009695399/functions/fpathconf.html, returning _PC_PIPE_BUF doesn't seem to imply that a FIFO can be created in a directory. Rather, that if one can create a FIFO, what would it's buffer size be.

In terms of exposing more variables for FIFOs, it seems if anything more correct. The page for mkfifo at opengroup.org specifically references NAME_MAX for example.

On the other hand, fifo_pathconf() probably shouldn't exist (which after this patch it does not) since the only FIFO-specific variable (PIPE_BUF) has to already be handled by the parent filesystem's pathconf method anyway to properly handle a PIPE_BUF request against a directory in the filesystem. Other filesystem properties like NAME_MAX and LINK_MAX are properties of the parent filesystem anyway, so a correct FIFO-specific pathconf would always need to do the same overrides for FS-specific limits as the non-FIFO pathconf. The pathconf VOP for a FIFO should probably just be the VOP for the parent filesystem without any further changes. ZFS currently gets this wrong (even with this patch) as it will now report an incorrect PC_LINK_MAX for a FIFO for example (since the maximum number of links isn't related to fifofs but to what ZFS as a filesystem can store).

[kib]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

You argument could be reformulated as stating that any value of PIPE_BUF for smbfs is valid since we cannot create a fifo at all.

Anyway, I think that the patch is an improvement so this really irrelevant detail should be not a reason for delay.

[ngie]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

Sounds good -- could you please run pjdfstest?

[jhb]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

Yes, I think any value of PIPE_BUF for smbfs would be valid. Or rather, since you can't open() or mkfifo() a FIFO on smbfs, the asking for PIPE_BUF is undefined and there is no valid value.

I have run pjdfstest on ZFS and am waiting for UFS to finish. However, pjdfstest doesn't actually test pathconf. It uses pathconf in a couple of places to query NAME_MAX and PATH_MAX for use in other tests, but it doesn't do any tests of pathconf itself.

[jhb]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

UFS passed pjdfstest as well.

[ngie]

sys/fs/fifofs/fifo_vnops.c

343 ↗

(On Diff #30595)

Sounds good -- I know pjdfstest doesn't exercise all of *pathconf, but at least it exercises some of the support. I'll add more tests for *pathconf in pjdfstest later.