From owner-svn-src-head@freebsd.org Fri May 22 15:15:43 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id E982C2CB51F; Fri, 22 May 2020 15:15:43 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-ot1-f42.google.com (mail-ot1-f42.google.com [209.85.210.42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49T96R4nlGz4Qs7; Fri, 22 May 2020 15:15:43 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-ot1-f42.google.com with SMTP id h7so8492140otr.3; Fri, 22 May 2020 08:15:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:cc; bh=e1/Dmks/v/RA9RP/XSf7ki7g1Ee51JEZmUdsDTZXj0w=; b=VZVvPx2tEHZLblsbdVvFo8WhUnfoi2raRaUg0rXJ+r/aDu9fKgHJcDoyTporKSCNss ZBmMsqx1JK8xgsMQOk2h5Tg0j9sN7zAnbOmtZLpNvnhexD8YtajQRFURmcZnOvXXaW9T X3cqbY0zT89v5CLahl+BJzNJ3ksYTE1/VIKnZPJr1hToJMP2SpUDYdl01YZ6lgs7nZuD x9fGjKWL1W5tQpGMDg7tcmNp/jqDe/9E8cEdtwuvmkVdkMQIyk2Adkubr7iU5DLpgZgI iqAw/uj+QPJl222I98TFv2MJ0Eg+ceIkcbzTal9NUklIZ1pJVLdGGcjJDhqw0YJsIuA5 /P/Q== X-Gm-Message-State: AOAM5321/oe233nMR0eYL1g7vQYQ3WSkV+xlWDXSRQNr2/vSpm91mEAi qtzDJVeOhpth69eQfT/+LU/dgqce X-Google-Smtp-Source: ABdhPJxcG9Kom0l9f1S9jHKPQmHZeqr+rMJhlQE/M8Id++zSKvXop4ETOoB3lw7SSOolMjpe8k928w== X-Received: by 2002:a9d:70d4:: with SMTP id w20mr11469684otj.179.1590160541458; Fri, 22 May 2020 08:15:41 -0700 (PDT) Received: from mail-oi1-f181.google.com (mail-oi1-f181.google.com. [209.85.167.181]) by smtp.gmail.com with ESMTPSA id p67sm2646568oic.46.2020.05.22.08.15.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 22 May 2020 08:15:41 -0700 (PDT) Received: by mail-oi1-f181.google.com with SMTP id l6so9569517oic.9; Fri, 22 May 2020 08:15:41 -0700 (PDT) X-Received: by 2002:aca:eb96:: with SMTP id j144mr2746581oih.48.1590160540784; Fri, 22 May 2020 08:15:40 -0700 (PDT) MIME-Version: 1.0 References: <202005221120.04MBKOiH003190@repo.freebsd.org> In-Reply-To: <202005221120.04MBKOiH003190@repo.freebsd.org> Reply-To: cem@freebsd.org From: Conrad Meyer Date: Fri, 22 May 2020 08:15:29 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r361363 - in head/lib/libprocstat: . zfs To: Andriy Gapon Cc: src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 49T96R4nlGz4Qs7 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; TAGGED_FROM(0.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 May 2020 15:15:44 -0000 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 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 > > 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 > #define _KERNEL > #include > -#include > #undef _KERNEL > +#include > +#include > #include > +#include > +#include > > -#undef lbolt > -#undef lbolt64 > -#undef gethrestime_sec > -#include > -#include > -#include > -#include > -#include > -#include > -#include > -#include > - > #include > > #include > @@ -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 > + * > + * 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 > +__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 > +#include > + > +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 > + * > + * 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 */