From owner-svn-src-head@freebsd.org Wed Aug 30 17:10:24 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 68492E061C5 for ; Wed, 30 Aug 2017 17:10:24 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: from mail-vk0-x22c.google.com (mail-vk0-x22c.google.com [IPv6:2607:f8b0:400c:c05::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1A7FF73E24 for ; Wed, 30 Aug 2017 17:10:24 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: by mail-vk0-x22c.google.com with SMTP id x85so4717311vkx.5 for ; Wed, 30 Aug 2017 10:10:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sippysoft-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=VRmifCXV04XZjA9bR6qwZbFC5kvWDzphiDj5KqxTRlc=; b=q7S+PnWdK/IZ5F2kCGc7W4rvw6Yrpi+hyxf1PTVPgmtHm1yfzi8Qb6hpf8D2EixRUR gObMWwiA3+YQa2q+WPo74Fxms5t8ewguQpbyMLlA4ZYs4v3cA3+ftFlonoSyX+ELrnqv bFUXTFyQuHGlyrayBji9PDit2Fnn6mDQlOI7sKlxcSGHCOXlR+ypf/6Pr3PNsR+ffSOI EQaaaZYLh9T5gtW7xxG1ZBwJrx/uMoGqUAge5bXDV7yz82mXmeR9u5uaqQrARUCZ/TLV qNPjl9F8SsuQogkSFV41D8TRUQc1nv6jXflyvXjVEaTBfRsDakFChVNGCpYNWv+8etH9 ADFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=VRmifCXV04XZjA9bR6qwZbFC5kvWDzphiDj5KqxTRlc=; b=O+QQCVLExzJcwjCWw/FTZV4WL//QokM9njiW8iIswkhQoZDzou1VBLeaDC+VMAz95n 0OJOEgLin5H6lhpx7hQoYATtwmBcpRIRGfvGM+aa9BRlttVOQjYIgkaNTRol2LKf3YAw QWEmbtQ9MkQnuqFUO+Amx3CndQgHxCYkt6C2jr3ErdgYxIWh3eRgQpol/hdf2oYQ2Q++ NXnPHaq/stT5tVKBOPishvRVDWarVjz8/vpTzKOCjZ2NVvf4Ig4DdFOKYDzKMJeViNAG enHAAehUJLRgiAMGMQ3BVdZKggxFpEzWELd/PT69s4J8rOUAmgdbR++wDcE6V43Z1moa pRBg== X-Gm-Message-State: AHYfb5gFrr/GYSazIGC5cwHa62x1w77WnW6avli6Y/es0YNPGBR6nkcr SQHJwTpsI23xwlCm46aoY0cx4WSYzpHN X-Google-Smtp-Source: ADKCNb5Wocgl4g4R+hwvN7eGaKAjLYDlmhI285/UeRizfzwWsuymOAEBE7Ux7FEd4OklUw1mcIhOdHX9OjQgOsm2Wao= X-Received: by 10.31.193.73 with SMTP id r70mr1125529vkf.54.1504113022978; Wed, 30 Aug 2017 10:10:22 -0700 (PDT) MIME-Version: 1.0 Sender: sobomax@sippysoft.com Received: by 10.176.6.137 with HTTP; Wed, 30 Aug 2017 10:10:22 -0700 (PDT) In-Reply-To: <1504016363.56799.71.camel@freebsd.org> References: <201708281554.v7SFs8fr014268@repo.freebsd.org> <2937323.CvTEtZnL2T@ralph.baldwin.cx> <1504016363.56799.71.camel@freebsd.org> From: Maxim Sobolev Date: Wed, 30 Aug 2017 10:10:22 -0700 X-Google-Sender-Auth: DdIuuYQEu-0YCUlrK1LDV0h3seU Message-ID: Subject: Re: svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys To: Ian Lepore Cc: John Baldwin , Ryan Libby , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.23 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 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: Wed, 30 Aug 2017 17:10:24 -0000 Hi Ian, I've committed support for the md_label into 32-bit-to-64-bit ioctl translation later and tested it for a bit here. It would be nice if you give it a go as well so if there are any bugs I'd like to iron them out before I MFC this piece in few weeks. Interestingly enough, I found mdconfig -l[v] is not working when i386 binary is run on amd64 kernels, but it seems to have nothing to do with this change. It fails to mmap("/dev/devstat") in the geom userland libs. Turns out the mdconfig(8) is not using ioctl() but rather geom layer for the "list / query" functionality. In fact there is no CLI code in the system that would exercise either MDIOCLIST or MDIOCQUERY. No userland code for the MDIOCLIST at all in the base and the only code that uses MDIOCQUERY is in some bsnmpd modules. For that reason I could not verify that it works 100%, but I've tested allocating device with and without label using i386 binary on amd64 and it seems to DTRT. The label is retrieved properly when I query device with native 64-bit mdconfig. It would be really nice to add extra mode flag into mdconfig so that it goes via those ioctl() rather than geom to fetch the device data data. If nothing else it would provide a basis to create an unit test for those ioctls. -Max On Tue, Aug 29, 2017 at 7:19 AM, Ian Lepore wrote: > On Mon, 2017-08-28 at 16:40 -0700, Maxim Sobolev wrote: > > John, well, this depends on how you look at it. The padding element size > is > > "int", which when you account for the alignment has the nice property on > > both 32 and 64-bit arches that no matter what kind of element you add > > (char, short, int or void *), you only need to bring down MDNPAD by 1 to > > keep the structure size the same. It also has a second interesting > property > > that number of padding elements needed stays the same no matter if > > sizeof(void *) is 64 or 32, otherwise you would have to have MDNPAD > diverge > > on 32 and 64 bit arches after adding pointer which has different sizes > > (just like you suggested it should originally). I am not 100% sure if it > > was intentionally designed like this by PHK from the day one, but I found > > it quite interesting. I am not quite sure if having sizeof(md_ioctl) is > > ever been required to stay the same between 64 and 32 bit arches. I don't > > think there is any support for having 32-bit mdconfig run on 64-bit > kernel. > > > > -Max > > > > mdconfig works in 32-bit jails on a 64-bit host, and I rely on that > being the case. With poudriere evolving to become a general image > building tool, and with it being so jail-centric, it's likely to be > another use case. > > -- Ian > > > On Mon, Aug 28, 2017 at 1:49 PM, John Baldwin > > wrote: > > > > > > > > On Monday, August 28, 2017 12:46:48 PM Ryan Libby wrote: > > > > > > > > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev > > > org> > > > wrote: > > > > > > > > > > > > > > Hi John, > > > > > > > > > > Thanks for your feedback! To address the points that you've > > > > > raised: > > > > > > > > > > 1. I've tested on both 32 and 64 bit platforms, it seems not to > > > > > be the > > > > > case. See imp's comment and my reply here > > > > > https://reviews.freebsd.org/D10457#216855 . Did I miss > > > > > something? Can > > > you > > > > > > > > > > > > > > post piece of C code that produces different sizeof(struct old) > > > > > vs. > > > > > sizeof(struct new) on some platform? > > > > [...] > > > > > > > > > > On Mon, Aug 28, 2017 at 9:19 AM, John Baldwin > > > > > wrote: > > > > > > > > > > > > > > > > > On Monday, August 28, 2017 03:54:08 PM Maxim Sobolev wrote: > > > > > > > > > > > > > > Author: sobomax > > > > > > > Date: Mon Aug 28 15:54:07 2017 > > > > > > > New Revision: 322969 > > > > > > > URL: https://svnweb.freebsd.org/changeset/base/322969 > > > > > > > > > > > > > > Log: > > > > > > > Add ability to label md(4) devices. > > > > > > > > > > > > > > This feature comes from the fact that we rely memory- > > > > > > > backed md(4) > > > > > > > in our build process heavily. However, if the build goes > > > > > > > haywire > > > > > > > the allocated resources (i.e. swap and memory-backed > > > > > > > md(4)'s) need > > > > > > > to be purged. It is extremely useful to have ability to > > > > > > > attach > > > > > > > arbitrary labels to each of the virtual disks so that > > > > > > > they can > > > > > > > be identified and GC'ed if neecessary. > > > > > > > > > > > > > > MFC after: 4 weeks > > > > > > > Differential Revision: https://reviews.freebsd.org/D > > > > > > > 10457 > > > > > > > > > > > > > > Modified: > > > > > > > head/sbin/mdconfig/mdconfig.8 > > > > > > > head/sbin/mdconfig/mdconfig.c > > > > > > > head/sys/dev/md/md.c > > > > > > > head/sys/sys/mdioctl.h > > > > > > > > > > > > > > Modified: head/sys/sys/mdioctl.h > > > > > > > =========================================================== > > > > > > > = > > > > > > ================== > > > > > > > > > > > > > > --- head/sys/sys/mdioctl.h Mon Aug 28 14:49:26 2017 > > > (r322968) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +++ head/sys/sys/mdioctl.h Mon Aug 28 15:54:07 2017 > > > (r322969) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -49,7 +49,7 @@ enum md_types {MD_MALLOC, MD_PRELOAD, > > > > > > > MD_VNODE, > > > MD_SWA > > > > > > > > > > > > > > > > > > > > > > > > > > > > > * Ioctl definitions for memory disk pseudo-device. > > > > > > > */ > > > > > > > > > > > > > > -#define MDNPAD 97 > > > > > > > +#define MDNPAD 96 > > > > > > > struct md_ioctl { > > > > > > > unsigned md_version; /* Structure layout > > > > > > > version */ > > > > > > > unsigned md_unit; /* unit number */ > > > > > > > @@ -61,6 +61,7 @@ struct md_ioctl { > > > > > > > u_int64_t md_base; /* base address */ > > > > > > > int md_fwheads; /* firmware heads */ > > > > > > > int md_fwsectors; /* firmware sectors > > > > > > > */ > > > > > > > + char *md_label; /* label of the > > > > > > > device */ > > > > > > > int md_pad[MDNPAD]; /* padding for future > > > > > > > ideas */ > > > > > > > }; > > > > > > This isn't correct on 64-bit platforms. MDNPAD needs to be > > > > > > 95 on > > > those > > > > > > > > > > > > > > > > > > > > > platforms. > > > > [...] > > > > > > > > Can you report sizeof(md_ioctl) before and after for 32-bit and > > > > 64-bit? > > > > I think it may be: > > > > 32-bit before: 440 > > > > 32-bit after: 440 > > > > 64-bit before: 448 > > > > 64-bit after: 448 > > > > > > > > In other words, it looks like it used to produce different sizes > > > > on the > > > > different architectures, and still does. It also looks like 32- > > > > bit > > > > before and after and 64-bit before included some undeclared > > > > padding > > > > after md_pad, so that this would fail: > > > > CTASSERT(sizeof(md_ioctl) == offsetof(struct md_ioctl, md_pad) + > > > > sizeof(((struct md_ioctl *)NULL)->md_pad)); > > > Ugh, yes. To me that means that MDNPAD is actually wrong and > > > should be > > > fixed to account for the implicit padding. That probably would > > > result in > > > requiring separate values for MDNPAD. The current change as-is > > > certainly > > > looks wrong (and would be wrong if the padding were accurate) so it > > > needs > > > to be fixed to reflect reality. > > > > > > -- > > > John Baldwin > > > > > > > >