svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys

Maxim Sobolev sobomax at freebsd.org
Wed Aug 30 17:10:24 UTC 2017


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 <ian at freebsd.org> 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 <jhb at freebsd.org>
> > wrote:
> >
> > >
> > > On Monday, August 28, 2017 12:46:48 PM Ryan Libby wrote:
> > > >
> > > > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev <sobomax at freebsd.
> > > > 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 <jhb at freebsd.org>
> > > > > 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
> > >
> > >
>
>


More information about the svn-src-head mailing list