Date: Wed, 12 Jul 2017 09:28:42 -0700 From: John Baldwin <jhb@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r320900 - in head/sys: fs/cd9660 fs/ext2fs fs/fifofs fs/msdosfs fs/nandfs fs/nfsclient fs/smbfs fs/tmpfs ufs/ufs Message-ID: <2198764.5xJVkxmjDt@ralph.baldwin.cx> In-Reply-To: <20170712184348.Q1271@besplex.bde.org> References: <201707112155.v6BLtKbZ006618@repo.freebsd.org> <20170712184348.Q1271@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday, July 12, 2017 08:08:42 PM Bruce Evans wrote: > On Tue, 11 Jul 2017, John Baldwin wrote: > > > Log: > > Consistently use vop_stdpathconf() for default pathconf values. > > > > 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. > > This is sort of backwards. Not many settings are the same for most > file systems, and many that did were wrong. Now more are wrong. > > > Modified: head/sys/fs/cd9660/cd9660_vnops.c > > ============================================================================== > > --- head/sys/fs/cd9660/cd9660_vnops.c Tue Jul 11 21:52:11 2017 (r320899) > > +++ head/sys/fs/cd9660/cd9660_vnops.c Tue Jul 11 21:55:20 2017 (r320900) > > @@ -792,20 +792,11 @@ cd9660_pathconf(ap) > > else > > *ap->a_retval = 37; > > return (0); > > - case _PC_PATH_MAX: > > - *ap->a_retval = PATH_MAX; > > - return (0); > > PATH_MAX is one of the few that is system-wide. > > > - case _PC_PIPE_BUF: > > - *ap->a_retval = PIPE_BUF; > > - return (0); > > PIPE_BUF isn't system wide. cd9660 does support fifos, but setting it > for files that aren't fifos or directories is bogus. POSIX allows such > bogus settings, and needs complicated wording to describe this. E.g., > PIPE_BUF for a directory is not completely bogus and it means that the > value for the directory applies to all fifos in the directory and is > ignored for all non-fifos in the directory. It is unclear if PIPE_BUF > must be set for a directory if there is any fifo in the directory. > FreeBSD's man pages give no details about this. PIPE_BUF is system wide because all FIFOs in FreeBSD are implemented via a common fifo implementation that has a single size. The wording about PIPE_BUF from http://pubs.opengroup.org/onlinepubs/009695399/functions/fpathconf.html: If path refers to a FIFO, or fildes refers to a pipe or FIFO, the value returned shall apply to the referenced object. If path or fildes refers to a directory, the value returned shall apply to any FIFO that exists or can be created within the directory. If path or fildes refers to any other type of file, it is unspecified whether an implementation supports an association of the variable name with the specified file. This leaves the value for non-directories and non-FIFOs undefined. We could perhaps add more complexity to fail with EINVAL for types other than VFIFO or VDIR, and filesystems that do not support FIFOs could fail _PC_PIPE_BUF in their fs-specific vnop, but this would not be any more correct as both behaviors fall into "undefined", but the current one is simpler to implement. > > - case _PC_CHOWN_RESTRICTED: > > - *ap->a_retval = 1; > > - return (0); > > This is system-wide. > > > case _PC_NO_TRUNC: > > *ap->a_retval = 1; > > return (0); > > This is almost system-wide. > > > default: > > - return (EINVAL); > > + return (vop_stdpathconf(ap)); > > This is fail-unsafe. It gives bogus settings like PIPE_BUF for all cases. > Case statements in file systems need to be even larger to kill defaults, > and the code becomes harder to read since it is harder to see the final > values. > > vop_stdpathconf() has defaults for: > - ASYNC_IO. Is this system-wide (all in vfs?) Yes. > - NAME_MAX. This is very far from system-wide > - PATH_MAX. This is system-wide. > - LINK_MAX. This is very far from system-wide, especially after expansion > of nlink_t. Most file systems get this wrong. LINK_MAX is bogusly > defined to be the constant 32767, but the value is very variable. > zfs_pathconf() returns INT_MAX and zfs uses something like int32_t > internally, but zfs_getattr() clamps to LINK_MAX. ext2fs limits to > 32000 for ext2 and 65000 for ext4 internally but ext2_pathconf() > only returns this limit in some cases -- other cases return INT_MAX. In theory if a filesystem supported an infinite link count or name count it would have no filesystem-specific limit and having LINK_MAX and NAME_MAX provide the system limits gives a reasonable default in that case. I don't know if any filesystem supports infinite limits in practice, but it's also true that the correct behavior in each filesystem would be to return a value like MIN(fs_limit, system_limit), not just the native filesystem limit. Perhaps this could be implemented by a post-processing step for VOP_PATHCONF that truncates values that exceed system limits to system limits. > - MAX_CANON. This is garbage except for terminals (and directories). > Its value is also wrong. > - MAX_INPUT. Like MAX_CANON. Again, POSIX leaves it undefined for other types, so it doesn't matter what we return. Quoting from the same URL for a note on these two: If path or fildes does not refer to a terminal file, it is unspecified whether an implementation supports an association of the variable name with the specified file. > - PIPE_BUF. Like MAX_CANON, for fifos instead of terminals. > - CHOWN_RESTRICTED. Wrong for file systems that don't support ownerships > or changing them. Might depend on mount options. > - VDISABLE. Like MAX_CANON. Same note for MAX_CANON applies. > > This is easy to improve: > - NAME_MAX, LINK_MAX: move to individual fs's That probably is true. In this case I was just trying to reduce overrides to the ones that were really fs-specific. The existing places that used NAME_MAX and LINK_MAX were also likely wrong as they did not return a filesystem-specific value. > - MAX_CANON, MAX_INPUT, VDISABLE: move to devfs_pathconf(). That is probably fine as well as it is simple and doesn't add complexity. > - PIPE_BUF: move to fifo_pathconf()? What about directories? I think this one should be standard for the reasons I gave above. If we really wanted to, we could make it conditional on the type (VFIFO and VDIR) without adding a lot of complexity. > > Modified: head/sys/fs/fifofs/fifo_vnops.c > > ============================================================================== > > --- head/sys/fs/fifofs/fifo_vnops.c Tue Jul 11 21:52:11 2017 (r320899) > > +++ head/sys/fs/fifofs/fifo_vnops.c Tue Jul 11 21:55:20 2017 (r320900) > > @@ -337,34 +336,6 @@ fifo_print(ap) > > fifo_printinfo(ap->a_vp); > > printf("\n"); > > return (0); > > -} > > - > > -/* > > - * Return POSIX pathconf information applicable to fifo's. > > - */ > > -static int > > -fifo_pathconf(ap) > > - struct vop_pathconf_args /* { > > - struct vnode *a_vp; > > - int a_name; > > - int *a_retval; > > - } */ *ap; > > -{ > > - > > - switch (ap->a_name) { > > - case _PC_LINK_MAX: > > - *ap->a_retval = LINK_MAX; > > - return (0); > > - case _PC_PIPE_BUF: > > - *ap->a_retval = PIPE_BUF; > > - return (0); > > - case _PC_CHOWN_RESTRICTED: > > - *ap->a_retval = 1; > > - return (0); > > - default: > > - return (EINVAL); > > - } > > - /* NOTREACHED */ > > } > > This was small and hopefully correct. Not many settings apply to fifos. > It seems a but too small to be correct -- why would LINK_MAX apply but > not PATH_MAX? Using the default, a pathconf which wants to set only 3 > value would need about 15 cases to kill all the defaults. This was not really correct since at least NAME_MAX was missing, but it also should never have been used as a full VOP. The parent filesystem in which the FIFO resides dictates values like LINK_MAX and NAME_MAX. The only safe way to use fifo_pathconf() is to have a per-fs wrapper that supplies FS-specific properties like NAME_MAX and LINK_MAX and only invokes fifo_pathconf for FIFO-specific properties. However, the only FIFO-specific property is PIPE_BUF, and the parent fs already has to handle that in the FS-specific pathconf VOP to support PIPE_BUF on directories, so the FS-specific pathconf VOP already works correctly for FIFOs and there is no need for a fifo_pathconf. > > Modified: head/sys/fs/msdosfs/msdosfs_vnops.c > > ============================================================================== > > --- head/sys/fs/msdosfs/msdosfs_vnops.c Tue Jul 11 21:52:11 2017 (r320899) > > +++ head/sys/fs/msdosfs/msdosfs_vnops.c Tue Jul 11 21:55:20 2017 (r320900) > > @@ -1875,17 +1875,11 @@ msdosfs_pathconf(struct vop_pathconf_args *ap) > > case _PC_NAME_MAX: > > *ap->a_retval = pmp->pm_flags & MSDOSFSMNT_LONGNAME ? WIN_MAXLEN : 12; > > return (0); > > - case _PC_PATH_MAX: > > - *ap->a_retval = PATH_MAX; > > - return (0); > > - case _PC_CHOWN_RESTRICTED: > > - *ap->a_retval = 1; > > - return (0); > > case _PC_NO_TRUNC: > > *ap->a_retval = 0; > > return (0); > > default: > > - return (EINVAL); > > + return (vop_stdpathconf(ap)); > > msdosfs doesn't support pipes. It used to indicate this by not having > _PC_PIPE_MAX. Now it gets this via pollution in vop_stdpathconf(ap). >From what I can tell (and from the note I referenced above), POSIX doesn't define if the variable is defined on filesystems that don't support FIFOs. It only says that the value on a directory must be true for any FIFO that "exists or can be created". Since mkfifo() will fail and open() will never return a vnode with VFIFO on such filesystems, the value is meaningless. > msdosfs also has unusual handling for LINK_MAX, NAME_MAX and NO_TRUNC. > It still has this, with buggy limits in all cases: > - msdosfs doesn't support links. It returns 1 for LINK_MAX instead of -1 > (unsupported) > - NAME_MAX doesn't really apply to shortnames. 12 is returned instead > of the 2 limits 8 and 3. > - msdosfs truncates only for shortnames, so NO_TRUNC = 0 is wrong for > !shortnames. > > Almost all file systems except msdosfs return 1 for NO_TRUNC, but this > is not defaulted. That may indeed be a good candidate for moving to stdpathconf. > > Modified: head/sys/ufs/ufs/ufs_vnops.c > > ============================================================================== > > --- head/sys/ufs/ufs/ufs_vnops.c Tue Jul 11 21:52:11 2017 (r320899) > > +++ head/sys/ufs/ufs/ufs_vnops.c Tue Jul 11 21:55:20 2017 (r320900) > > @@ -2442,21 +2442,9 @@ ufs_pathconf(ap) > > > > error = 0; > > switch (ap->a_name) { > > - case _PC_LINK_MAX: > > - *ap->a_retval = LINK_MAX; > > - break; > > case _PC_NAME_MAX: > > *ap->a_retval = UFS_MAXNAMLEN; > > break; > > LINK_MAX is basically ffs's limit. > > NAME_MAX _is_ ffs's limit, but it is still defaulted. > > I expect that some of the other defaulted limits will become more variable > (so unsuitable as defaults). LINK_MAX is almost there. > > > @@ -2505,11 +2493,6 @@ ufs_pathconf(ap) > > case _PC_MIN_HOLE_SIZE: > > *ap->a_retval = ap->a_vp->v_mount->mnt_stat.f_iosize; > > break; > > - case _PC_ASYNC_IO: > > - /* _PC_ASYNC_IO should have been handled by upper layers. */ > > - KASSERT(0, ("_PC_ASYNC_IO should not get here")); > > - error = EINVAL; > > - break; > > case _PC_PRIO_IO: > > *ap->a_retval = 0; > > break; > > But some of the newer limits like are in vfs so should have been defaulted? _PC_ASYNC_IO used to be explicitly handled in the pathconf system calls as a special case. I had moved it to vop_stdpathconf() so that it was handled in one place (rather than 3), but had missed these specific handlers in existing pathconf VOPs. System-wide pathconf values should really be in one place in vop_stdpathconf(). We might want to alter the list of things in vop_stdpathconf(), but I think all filesystems should fall through to it and only override values that are filesystem-specific. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2198764.5xJVkxmjDt>