From owner-svn-src-all@freebsd.org Mon Aug 28 21:45:03 2017 Return-Path: Delivered-To: svn-src-all@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 F376BE1475F; Mon, 28 Aug 2017 21:45:03 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id B5DF9658E3; Mon, 28 Aug 2017 21:45:03 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id CB0FB10AF3A; Mon, 28 Aug 2017 17:44:55 -0400 (EDT) From: John Baldwin To: Ryan Libby Cc: Maxim Sobolev , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers Subject: Re: svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys Date: Mon, 28 Aug 2017 13:49:39 -0700 Message-ID: <2937323.CvTEtZnL2T@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.1-STABLE; KDE/4.14.30; amd64; ; ) In-Reply-To: References: <201708281554.v7SFs8fr014268@repo.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Mon, 28 Aug 2017 17:44:55 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Aug 2017 21:45:04 -0000 On Monday, August 28, 2017 12:46:48 PM Ryan Libby wrote: > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev 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/D10457 > >> > > >> > 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