Date: Fri, 22 May 2020 08:15:29 -0700 From: Conrad Meyer <cem@freebsd.org> To: Andriy Gapon <avg@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r361363 - in head/lib/libprocstat: . zfs Message-ID: <CAG6CVpU%2BykwgVYDJYBu0MtQJjZKxKCx%2BxOqzbAxvZBUGdde3Ag@mail.gmail.com> In-Reply-To: <202005221120.04MBKOiH003190@repo.freebsd.org> References: <202005221120.04MBKOiH003190@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Andriy, Would it make sense to also export sizes of those members? Currently the code assumes the members may be relocated in the struct, but never changed in size. If they can be moved around, maybe they might be enlarged or shrunk at some time too? Maybe not; I am not very familiar with ZFS. Best, Conrad On Fri, May 22, 2020 at 4:20 AM Andriy Gapon <avg@freebsd.org> wrote: > > Author: avg > Date: Fri May 22 11:20:23 2020 > New Revision: 361363 > URL: https://svnweb.freebsd.org/changeset/base/361363 > > Log: > libprocstat: fix ZFS support > > First of all, znode_phys_t hasn't been used for storing file attributes > for a long time now. Modern ZFS versions use a System Attribute table > with a flexible layout. But more importantly all the required > information is available in znode_t itself. > > It's not easy to include zfs_znode.h in userland without breaking code > because the most interesting parts of the header are kernel-only. And > hardcoding field offsets is too fragile. So, I created a new > compilation unit that includes zfs_znode.h using some mild kludges to > get it and its dependencies to compile in userland. The compilation > unit exports interesting field offsets and does not have any other code. > > PR: 194117 > Reviewed by: markj > MFC after: 2 weeks > Sponsored by: Panzura > Differential Revision: https://reviews.freebsd.org/D24941 > > Added: > head/lib/libprocstat/zfs_defs.c (contents, props changed) > head/lib/libprocstat/zfs_defs.h (contents, props changed) > Modified: > head/lib/libprocstat/Makefile > head/lib/libprocstat/zfs.c > head/lib/libprocstat/zfs/Makefile > > Modified: head/lib/libprocstat/Makefile > ============================================================================== > --- head/lib/libprocstat/Makefile Fri May 22 10:50:29 2020 (r361362) > +++ head/lib/libprocstat/Makefile Fri May 22 11:20:23 2020 (r361363) > @@ -57,16 +57,17 @@ MLINKS+=libprocstat.3 procstat_close.3 \ > # XXX This is a hack. > .if ${MK_CDDL} != "no" > CFLAGS+= -DLIBPROCSTAT_ZFS > -OBJS+= zfs/zfs.o > -SOBJS+= zfs/zfs.pico > -POBJS+= zfs/zfs.po > +SRCS+= zfs.c > +OBJS+= zfs/zfs_defs.o > +SOBJS+= zfs/zfs_defs.pico > +POBJS+= zfs/zfs_defs.po > SUBDIR= zfs > -zfs/zfs.o: .PHONY > - @cd ${.CURDIR}/zfs && ${MAKE} zfs.o > -zfs/zfs.pico: .PHONY > - @cd ${.CURDIR}/zfs && ${MAKE} zfs.pico > -zfs/zfs.po: .PHONY > - @cd ${.CURDIR}/zfs && ${MAKE} zfs.po > +zfs/zfs_defs.o: .PHONY > + @cd ${.CURDIR}/zfs && ${MAKE} zfs_defs.o > +zfs/zfs_defs.pico: .PHONY > + @cd ${.CURDIR}/zfs && ${MAKE} zfs_defs.pico > +zfs/zfs_defs.po: .PHONY > + @cd ${.CURDIR}/zfs && ${MAKE} zfs_defs.po > .endif > > .include <bsd.lib.mk> > > Modified: head/lib/libprocstat/zfs.c > ============================================================================== > --- head/lib/libprocstat/zfs.c Fri May 22 10:50:29 2020 (r361362) > +++ head/lib/libprocstat/zfs.c Fri May 22 11:20:23 2020 (r361363) > @@ -31,22 +31,13 @@ > #include <sys/param.h> > #define _KERNEL > #include <sys/mount.h> > -#include <sys/taskqueue.h> > #undef _KERNEL > +#include <sys/queue.h> > +#include <sys/stat.h> > #include <sys/sysctl.h> > +#include <sys/time.h> > +#include <sys/vnode.h> > > -#undef lbolt > -#undef lbolt64 > -#undef gethrestime_sec > -#include <sys/zfs_context.h> > -#include <sys/spa.h> > -#include <sys/spa_impl.h> > -#include <sys/dmu.h> > -#include <sys/zap.h> > -#include <sys/fs/zfs.h> > -#include <sys/zfs_znode.h> > -#include <sys/zfs_sa.h> > - > #include <netinet/in.h> > > #include <err.h> > @@ -57,24 +48,15 @@ > #define ZFS > #include "libprocstat.h" > #include "common_kvm.h" > +#include "zfs_defs.h" > > -/* > - * Offset calculations that are used to get data from znode without having the > - * definition. > - */ > -#define LOCATION_ZID (2 * sizeof(void *)) > -#define LOCATION_ZPHYS(zsize) ((zsize) - (2 * sizeof(void *) + sizeof(struct task))) > - > int > zfs_filestat(kvm_t *kd, struct vnode *vp, struct vnstat *vn) > { > > - znode_phys_t zphys; > struct mount mount, *mountptr; > - uint64_t *zid; > - void *znodeptr, *vnodeptr; > + void *znodeptr; > char *dataptr; > - void *zphys_addr; > size_t len; > int size; > > @@ -83,33 +65,27 @@ zfs_filestat(kvm_t *kd, struct vnode *vp, struct vnsta > warnx("error getting sysctl"); > return (1); > } > - znodeptr = malloc(size); > - if (znodeptr == NULL) { > + dataptr = malloc(size); > + if (dataptr == NULL) { > warnx("error allocating memory for znode storage"); > return (1); > } > - /* Since we have problems including vnode.h, we'll use the wrappers. */ > - vnodeptr = getvnodedata(vp); > - if (!kvm_read_all(kd, (unsigned long)vnodeptr, znodeptr, > - (size_t)size)) { > - warnx("can't read znode at %p", (void *)vnodeptr); > + > + if ((size_t)size < offsetof_z_id + sizeof(uint64_t) || > + (size_t)size < offsetof_z_mode + sizeof(mode_t) || > + (size_t)size < offsetof_z_size + sizeof(uint64_t)) { > + warnx("znode_t size is too small"); > goto bad; > } > > - /* > - * z_id field is stored in the third pointer. We therefore skip the two > - * first bytes. > - * > - * Pointer to the z_phys structure is the next last pointer. Therefore > - * go back two bytes from the end. > - */ > - dataptr = znodeptr; > - zid = (uint64_t *)(dataptr + LOCATION_ZID); > - zphys_addr = *(void **)(dataptr + LOCATION_ZPHYS(size)); > + if ((size_t)size != sizeof_znode_t) > + warnx("znode_t size mismatch, data could be wrong"); > > - if (!kvm_read_all(kd, (unsigned long)zphys_addr, &zphys, > - sizeof(zphys))) { > - warnx("can't read znode_phys at %p", zphys_addr); > + /* Since we have problems including vnode.h, we'll use the wrappers. */ > + znodeptr = getvnodedata(vp); > + if (!kvm_read_all(kd, (unsigned long)znodeptr, dataptr, > + (size_t)size)) { > + warnx("can't read znode at %p", (void *)znodeptr); > goto bad; > } > > @@ -119,18 +95,18 @@ zfs_filestat(kvm_t *kd, struct vnode *vp, struct vnsta > warnx("can't read mount at %p", (void *)mountptr); > goto bad; > } > - vn->vn_fsid = mount.mnt_stat.f_fsid.val[0]; > - vn->vn_fileid = *zid; > + > /* > - * XXX: Shows up wrong in output, but UFS has this error too. Could > - * be that we're casting mode-variables from 64-bit to 8-bit or simply > - * error in the mode-to-string function. > + * XXX Assume that this is a znode, but it can be a special node > + * under .zfs/. > */ > - vn->vn_mode = (mode_t)zphys.zp_mode; > - vn->vn_size = (u_long)zphys.zp_size; > - free(znodeptr); > + vn->vn_fsid = mount.mnt_stat.f_fsid.val[0]; > + vn->vn_fileid = *(uint64_t *)(void *)(dataptr + offsetof_z_id); > + vn->vn_mode = *(mode_t *)(void *)(dataptr + offsetof_z_mode); > + vn->vn_size = *(uint64_t *)(void *)(dataptr + offsetof_z_size); > + free(dataptr); > return (0); > bad: > - free(znodeptr); > + free(dataptr); > return (1); > } > > Modified: head/lib/libprocstat/zfs/Makefile > ============================================================================== > --- head/lib/libprocstat/zfs/Makefile Fri May 22 10:50:29 2020 (r361362) > +++ head/lib/libprocstat/zfs/Makefile Fri May 22 11:20:23 2020 (r361363) > @@ -2,20 +2,20 @@ > > .PATH: ${.CURDIR:H} > > -SRCS= zfs.c > -OBJS= zfs.o > +SRCS= zfs_defs.c > +OBJS= zfs_defs.o > WARNS?= 1 > > CFLAGS+= -I${SRCTOP}/sys/cddl/compat/opensolaris > CFLAGS+= -I${SRCTOP}/cddl/compat/opensolaris/include > CFLAGS+= -I${SRCTOP}/cddl/compat/opensolaris/lib/libumem > -CFLAGS+= -I${SRCTOP}/cddl/contrib/opensolaris/lib/libzpool/common > CFLAGS+= -I${SRCTOP}/sys/cddl/contrib/opensolaris/common/zfs > CFLAGS+= -I${SRCTOP}/sys/cddl/contrib/opensolaris/uts/common/fs/zfs > CFLAGS+= -I${SRCTOP}/sys/cddl/contrib/opensolaris/uts/common > CFLAGS+= -I${SRCTOP}/cddl/contrib/opensolaris/head > CFLAGS+= -I${.CURDIR:H} > CFLAGS+= -DNEED_SOLARIS_BOOLEAN > +CFLAGS+= -fno-builtin -nostdlib > > all: ${OBJS} > CLEANFILES= ${OBJS} > > Added: head/lib/libprocstat/zfs_defs.c > ============================================================================== > --- /dev/null 00:00:00 1970 (empty, because file is newly added) > +++ head/lib/libprocstat/zfs_defs.c Fri May 22 11:20:23 2020 (r361363) > @@ -0,0 +1,59 @@ > +/*- > + * SPDX-License-Identifier: BSD-2-Clause-FreeBSD > + * > + * Copyright (c) 2020 Andriy Gapon <avg@FreeBSD.org> > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include <sys/cdefs.h> > +__FBSDID("$FreeBSD$"); > + > +/* Pretend we are kernel to get the same binary layout. */ > +#define _KERNEL > + > +/* A hack to deal with kpilite.h. */ > +#define KLD_MODULE > + > +/* > + * Prevent some headers from getting included and fake some types > + * in order to allow this file to compile without bringing in > + * too many kernel build dependencies. > + */ > +#define _OPENSOLARIS_SYS_PATHNAME_H_ > +#define _OPENSOLARIS_SYS_POLICY_H_ > +#define _OPENSOLARIS_SYS_VNODE_H_ > +#define _VNODE_PAGER_ > + > +typedef struct vnode vnode_t; > +typedef struct vattr vattr_t; > +typedef struct xvattr xvattr_t; > +typedef struct vsecattr vsecattr_t; > +typedef enum vtype vtype_t; > + > +#include <sys/zfs_context.h> > +#include <sys/zfs_znode.h> > + > +size_t sizeof_znode_t = sizeof(znode_t); > +size_t offsetof_z_id = offsetof(znode_t, z_id); > +size_t offsetof_z_size = offsetof(znode_t, z_size); > +size_t offsetof_z_mode = offsetof(znode_t, z_mode); > > Added: head/lib/libprocstat/zfs_defs.h > ============================================================================== > --- /dev/null 00:00:00 1970 (empty, because file is newly added) > +++ head/lib/libprocstat/zfs_defs.h Fri May 22 11:20:23 2020 (r361363) > @@ -0,0 +1,38 @@ > +/*- > + * SPDX-License-Identifier: BSD-2-Clause-FreeBSD > + * > + * Copyright (c) 2020 Andriy Gapon <avg@FreeBSD.org> > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + * > + * $FreeBSD$ > + */ > + > +#ifndef _LIBPROCSTAT_ZFS_DEFS_H > +#define _LIBPROCSTAT_ZFS_DEFS_H > + > +extern size_t sizeof_znode_t; > +extern size_t offsetof_z_id; > +extern size_t offsetof_z_size; > +extern size_t offsetof_z_mode; > + > +#endif /* _LIBPROCSTAT_ZFS_DEFS_H */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpU%2BykwgVYDJYBu0MtQJjZKxKCx%2BxOqzbAxvZBUGdde3Ag>