From owner-svn-src-head@freebsd.org Thu Aug 31 15:44:59 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 D7935E00951 for ; Thu, 31 Aug 2017 15:44:59 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from nov-007-i471.relay.mailchannels.net (nov-007-i471.relay.mailchannels.net [46.232.183.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 62DD7818AE for ; Thu, 31 Aug 2017 15:44:57 +0000 (UTC) (envelope-from ian@freebsd.org) X-Sender-Id: _forwarded-from|73.78.92.27 Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id D4A3F20138A for ; Thu, 31 Aug 2017 14:26:10 +0000 (UTC) Received: from outbound1a.eu.mailhop.org (unknown [100.96.139.205]) (Authenticated sender: duocircle) by relay.mailchannels.net (Postfix) with ESMTPA id 54AFB20103A for ; Thu, 31 Aug 2017 14:26:10 +0000 (UTC) X-Sender-Id: _forwarded-from|73.78.92.27 Received: from outbound1a.eu.mailhop.org (outbound1a.eu.mailhop.org [172.20.107.195]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384) by 0.0.0.0:2500 (trex/5.9.14); Thu, 31 Aug 2017 14:26:10 +0000 X-MC-Relay: Forwarding X-MailChannels-SenderId: _forwarded-from|73.78.92.27 X-MailChannels-Auth-Id: duocircle X-Ski-Bored: 30a64b121f17183b_1504189570771_1269694573 X-MC-Loop-Signature: 1504189570771:1354609654 X-MC-Ingress-Time: 1504189570770 X-MHO-User: 50fcce08-8e58-11e7-83af-a91f44540cb3 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 73.78.92.27 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [73.78.92.27]) by outbound1.eu.mailhop.org (Halon) with ESMTPSA id 50fcce08-8e58-11e7-83af-a91f44540cb3; Thu, 31 Aug 2017 14:26:04 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id v7VEQ0vx001251; Thu, 31 Aug 2017 08:26:00 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <1504189560.56799.94.camel@freebsd.org> Subject: Re: svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys From: Ian Lepore To: Maxim Sobolev Cc: John Baldwin , Ryan Libby , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers Date: Thu, 31 Aug 2017 08:26:00 -0600 In-Reply-To: References: <201708281554.v7SFs8fr014268@repo.freebsd.org> <2937323.CvTEtZnL2T@ralph.baldwin.cx> <1504016363.56799.71.camel@freebsd.org> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.18.5.1 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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: Thu, 31 Aug 2017 15:45:00 -0000 Unfortunately, I'm not in a position to test those changes. =A0My setup that uses mdconfig in a 32-bit jail runs on freebsd 10-stable and 11- stable systems. =A0I don't have any 64-bit hardware available to run -current. -- Ian On Wed, 2017-08-30 at 10:10 -0700, Maxim Sobolev wrote: > Hi Ian, I've committed support for the md_label into 32-bit-to-64-bit=20 > 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.=A0=A0No 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. >=20 > 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. >=20 > -Max >=20 > On Tue, Aug 29, 2017 at 7:19 AM, Ian Lepore wrote: >=20 > >=20 > > On Mon, 2017-08-28 at 16:40 -0700, Maxim Sobolev wrote: > > >=20 > > > John, well, this depends on how you look at it. The padding > > > element size > > is > > >=20 > > > "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 > > >=20 > > > 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 > > >=20 > > > 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. > > >=20 > > >=20 > > > -Max > > >=20 > > mdconfig works in 32-bit jails on a 64-bit host, and I rely on that > > being the case.=A0=A0With poudriere evolving to become a general imag= e > > building tool, and with it being so jail-centric, it's likely to be > > another use case. > >=20 > > -- Ian > >=20 > > >=20 > > > On Mon, Aug 28, 2017 at 1:49 PM, John Baldwin > > > wrote: > > >=20 > > > >=20 > > > >=20 > > > > On Monday, August 28, 2017 12:46:48 PM Ryan Libby wrote: > > > > >=20 > > > > >=20 > > > > > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev > > > > bsd. > > > > > org> > > > > wrote: > > > > >=20 > > > > >=20 > > > > > >=20 > > > > > >=20 > > > > > > Hi John, > > > > > >=20 > > > > > > Thanks for your feedback! To address the points that you've > > > > > > raised: > > > > > >=20 > > > > > > 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 > > > > >=20 > > > > >=20 > > > > > >=20 > > > > > >=20 > > > > > > post piece of C code that produces different sizeof(struct > > > > > > old) > > > > > > vs. > > > > > > sizeof(struct new) on some platform? > > > > > [...] > > > > > >=20 > > > > > >=20 > > > > > > On Mon, Aug 28, 2017 at 9:19 AM, John Baldwin > > > > > org> > > > > > > wrote: > > > > > >=20 > > > > > > >=20 > > > > > > >=20 > > > > > > > On Monday, August 28, 2017 03:54:08 PM Maxim Sobolev > > > > > > > wrote: > > > > > > > >=20 > > > > > > > >=20 > > > > > > > > Author: sobomax > > > > > > > > Date: Mon Aug 28 15:54:07 2017 > > > > > > > > New Revision: 322969 > > > > > > > > URL: https://svnweb.freebsd.org/changeset/base/322969 > > > > > > > >=20 > > > > > > > > Log: > > > > > > > > =A0 Add ability to label md(4) devices. > > > > > > > >=20 > > > > > > > > =A0 This feature comes from the fact that we rely memory- > > > > > > > > backed md(4) > > > > > > > > =A0 in our build process heavily. However, if the build > > > > > > > > goes > > > > > > > > haywire > > > > > > > > =A0 the allocated resources (i.e. swap and memory-backed > > > > > > > > md(4)'s) need > > > > > > > > =A0 to be purged. It is extremely useful to have ability > > > > > > > > to > > > > > > > > attach > > > > > > > > =A0 arbitrary labels to each of the virtual disks so that > > > > > > > > they can > > > > > > > > =A0 be identified and GC'ed if neecessary. > > > > > > > >=20 > > > > > > > > =A0 MFC after:=A0=A04 weeks > > > > > > > > =A0 Differential Revision:=A0=A0=A0=A0=A0=A0https://revie= ws.freebsd.o > > > > > > > > rg/D > > > > > > > > 10457 > > > > > > > >=20 > > > > > > > > Modified: > > > > > > > > =A0 head/sbin/mdconfig/mdconfig.8 > > > > > > > > =A0 head/sbin/mdconfig/mdconfig.c > > > > > > > > =A0 head/sys/dev/md/md.c > > > > > > > > =A0 head/sys/sys/mdioctl.h > > > > > > > >=20 > > > > > > > > Modified: head/sys/sys/mdioctl.h > > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > > > > > =3D=3D=3D=3D > > > > > > > > =3D > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > > > > >=20 > > > > > > > >=20 > > > > > > > > --- head/sys/sys/mdioctl.h=A0=A0=A0=A0Mon Aug 28 14:49:26= 2017 > > > > (r322968) > > > > >=20 > > > > >=20 > > > > > >=20 > > > > > >=20 > > > > > > >=20 > > > > > > >=20 > > > > > > > >=20 > > > > > > > >=20 > > > > > > > > +++ head/sys/sys/mdioctl.h=A0=A0=A0=A0Mon Aug 28 15:54:07= 2017 > > > > (r322969) > > > > >=20 > > > > >=20 > > > > > >=20 > > > > > >=20 > > > > > > >=20 > > > > > > >=20 > > > > > > > >=20 > > > > > > > >=20 > > > > > > > > @@ -49,7 +49,7 @@ enum md_types {MD_MALLOC, MD_PRELOAD, > > > > > > > > MD_VNODE, > > > > MD_SWA > > > > >=20 > > > > >=20 > > > > > >=20 > > > > > >=20 > > > > > > >=20 > > > > > > >=20 > > > > > > > >=20 > > > > > > > >=20 > > > > > > > > =A0 * Ioctl definitions for memory disk pseudo-device. > > > > > > > > =A0 */ > > > > > > > >=20 > > > > > > > > -#define MDNPAD=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A097 > > > > > > > > +#define MDNPAD=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A096 > > > > > > > > =A0struct md_ioctl { > > > > > > > > =A0=A0=A0=A0=A0=A0unsigned=A0=A0=A0=A0=A0=A0=A0=A0md_vers= ion;=A0=A0=A0=A0=A0/* Structure > > > > > > > > layout > > > > > > > > version */ > > > > > > > > =A0=A0=A0=A0=A0=A0unsigned=A0=A0=A0=A0=A0=A0=A0=A0md_unit= ;=A0=A0=A0=A0=A0=A0=A0=A0/* unit number */ > > > > > > > > @@ -61,6 +61,7 @@ struct md_ioctl { > > > > > > > > =A0=A0=A0=A0=A0=A0u_int64_t=A0=A0=A0=A0=A0=A0=A0md_base;=A0= =A0=A0=A0=A0=A0=A0=A0/* base address > > > > > > > > */ > > > > > > > > =A0=A0=A0=A0=A0=A0int=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0md_fwheads;=A0=A0=A0=A0=A0/* firmware heads > > > > > > > > */ > > > > > > > > =A0=A0=A0=A0=A0=A0int=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0md_fwsectors;=A0=A0=A0/* firmware > > > > > > > > sectors > > > > > > > > */ > > > > > > > > +=A0=A0=A0=A0=A0char=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0*= md_label;=A0=A0=A0=A0=A0=A0/* label of the > > > > > > > > device */ > > > > > > > > =A0=A0=A0=A0=A0=A0int=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0md_pad[MDNPAD]; /* padding for > > > > > > > > future > > > > > > > > ideas */ > > > > > > > > =A0}; > > > > > > > This isn't correct on 64-bit platforms.=A0=A0MDNPAD needs t= o > > > > > > > be > > > > > > > 95 on > > > > those > > > > >=20 > > > > >=20 > > > > > >=20 > > > > > >=20 > > > > > > >=20 > > > > > > >=20 > > > > > > > platforms. > > > > > [...] > > > > >=20 > > > > > 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:=A0=A0440 > > > > > 64-bit before: 448 > > > > > 64-bit after:=A0=A0448 > > > > >=20 > > > > > In other words, it looks like it used to produce different > > > > > sizes > > > > > on the > > > > > different architectures, and still does.=A0=A0It also looks lik= e > > > > > 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) =3D=3D offsetof(struct md_ioctl, > > > > > md_pad) + > > > > > =A0=A0=A0=A0sizeof(((struct md_ioctl *)NULL)->md_pad)); > > > > Ugh, yes.=A0=A0To me that means that MDNPAD is actually wrong and > > > > should be > > > > fixed to account for the implicit padding.=A0=A0That probably wou= ld > > > > result in > > > > requiring separate values for MDNPAD.=A0=A0The 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. > > > >=20 > > > > -- > > > > John Baldwin > > > >=20 > > > >=20 > >=20