CHROMIUM: add boot-to-device mapper support.

This is a wrap-up of three patches pending upstream approval.
I'm bundling them because they are interdependent, and it'll be
easier to drop it on rebase later.

1. dm: allow a dm-fs-style device to be shared via dm-ioctl

Integrates feedback from Alisdair, Mike, and Kiyoshi.

Two main changes occur here:

- One function is added which allows for a programmatically created
mapped device to be inserted into the dm-ioctl hash table.  This binds
the device to a name and, optional, uuid which is needed by udev and
allows for userspace management of the mapped device.

- dm_table_complete() was extended to handle all of the final
functional changes required for the table to be operational once
called.

2. init: boot to device-mapper targets without an initr*

Add a dm= kernel parameter modeled after the md= parameter from
do_mounts_md.  It allows for device-mapper targets to be configured at
boot time for use early in the boot process (as the root device or
otherwise).  It also replaces /dev/XXX calls with major:minor opportunistically.

The format is dm="name uuid ro,table line 1,table line 2,...".  The
parser expects the comma to be safe to use as a newline substitute but,
otherwise, uses the normal separator of space.  Some attempt has been
made to make it forgiving of additional spaces (using skip_spaces()).

A mapped device created during boot will be assigned a minor of 0 and
may be access via /dev/dm-0.

An example dm-linear root with no uuid may look like:

root=/dev/dm-0  dm="lroot none ro, 0 4096 linear /dev/ubdb 0, 4096 4096 linear /dv/ubdc 0"

Once udev is started, /dev/dm-0 will become /dev/mapper/lroot.


Older upstream threads:
http://marc.info/?l=dm-devel&m=127429492521964&w=2
http://marc.info/?l=dm-devel&m=127429499422096&w=2
http://marc.info/?l=dm-devel&m=127429493922000&w=2

Latest upstream threads:
https://patchwork.kernel.org/patch/104859/
https://patchwork.kernel.org/patch/104860/
https://patchwork.kernel.org/patch/104861/

Signed-off-by: Will Drewry <wad@chromium.org>;

[Olof Johansson, 2010-05-12 20:49:47]

This looks like exactly the kind of patch that should go upstream first. :)

I'm not crazy about adding new bootargs that contain a lot of hardcoded things,
especially not major/minors, etc. I don't know if there's a way around it
though? Can root=/dev/mapper/foo be used instead, or does the parser not handle
that properly?

[Will Drewry, 2010-05-12 21:03:03]

On 2010/05/12 20:49:47, olofj wrote:
> This looks like exactly the kind of patch that should go upstream first. :)

I hope it does, but if we want to be able to test verified boot this century, we
might want to put it in sooner :p

> I'm not crazy about adding new bootargs that contain a lot of hardcoded
things,
> especially not major/minors, etc. I don't know if there's a way around it
> though? Can root=/dev/mapper/foo be used instead, or does the parser not
handle
> that properly?

yeah me either. So it turns out I'm dumb :)  It can use: root=/dev/dm-[MINOR]
and we can avoid forcing a major.

When I was testing, I thought that was the source of trouble, but it turns out
it's not.

[As to /dev/mapper/*, because I'm not traversing the ioctl interface, we don't
get a named /dev/mapper node -- just the initial device.]

Is that a little more sane? :)

thanks!

[Olof Johansson, 2010-05-12 21:13:29]

On 2010/05/12 21:03:03, Will Drewry wrote:
> On 2010/05/12 20:49:47, olofj wrote:
> > This looks like exactly the kind of patch that should go upstream first. :)
> 
> I hope it does, but if we want to be able to test verified boot this century,
we
> might want to put it in sooner :p

I would personally like to see it posted on LKML and the dm list, even if it's
not yet picked up, just to get it out there.

So while I didn't mean "picked up by Linus", I would like to see it submitted
before we pick it up. Chances are this will go in through akpm.

We can always update the local patch as it gets revised, but if there's too much
of a delay I'm sure you'll be too busy with whatever else is on your plate at
that time, and the interest to get it upstream fades. Been there, done that,
prefer to avoid it in the future. :)

> yeah me either. So it turns out I'm dumb :)  It can use: root=/dev/dm-[MINOR]
> and we can avoid forcing a major.

Cool, excellent. Feel free to update the patch description accordingly. :)

> Is that a little more sane? :)

Yep.


-Olof

[Will Drewry, 2010-05-12 21:29:01]

On 2010/05/12 21:13:29, olofj wrote:
> On 2010/05/12 21:03:03, Will Drewry wrote:
> > On 2010/05/12 20:49:47, olofj wrote:
> > > This looks like exactly the kind of patch that should go upstream first.
:)
> > 
> > I hope it does, but if we want to be able to test verified boot this
century,
> we
> > might want to put it in sooner :p
> 
> I would personally like to see it posted on LKML and the dm list, even if it's
> not yet picked up, just to get it out there.

Already done :)

>
> So while I didn't mean "picked up by Linus", I would like to see it submitted
> before we pick it up. Chances are this will go in through akpm.

Yep already have some feedback too.
 
> We can always update the local patch as it gets revised, but if there's too
much
> of a delay I'm sure you'll be too busy with whatever else is on your plate at
> that time, and the interest to get it upstream fades. Been there, done that,
> prefer to avoid it in the future. :)

I agree.  It's hard to justify making continued changes when it works-for-you :)
 I'm going to start on the requested changes to see if I can get something
acceptable (after replying), but I'd like to at least have this somewhere
comfortable for us so that we we're also happy with the dm-verity code, we can
commit both and start testing them even though more changes are guaranteed to be
needed!
 
> > yeah me either. So it turns out I'm dumb :)  It can use:
root=/dev/dm-[MINOR]
> > and we can avoid forcing a major.
> 
> Cool, excellent. Feel free to update the patch description accordingly. :)

Updated with the link to dm-devel too.

[Olof Johansson, 2010-05-12 21:34:37]

On 2010/05/12 21:29:01, Will Drewry wrote:
> On 2010/05/12 21:13:29, olofj wrote:
> > On 2010/05/12 21:03:03, Will Drewry wrote:
> > > On 2010/05/12 20:49:47, olofj wrote:
> > > > This looks like exactly the kind of patch that should go upstream first.
> :)
> > > 
> > > I hope it does, but if we want to be able to test verified boot this
> century,
> > we
> > > might want to put it in sooner :p
> > 
> > I would personally like to see it posted on LKML and the dm list, even if
it's
> > not yet picked up, just to get it out there.
> 
> Already done :)
> 
> >
> > So while I didn't mean "picked up by Linus", I would like to see it
submitted
> > before we pick it up. Chances are this will go in through akpm.
> 
> Yep already have some feedback too.

Good 

> > We can always update the local patch as it gets revised, but if there's too
> much
> > of a delay I'm sure you'll be too busy with whatever else is on your plate
at
> > that time, and the interest to get it upstream fades. Been there, done that,
> > prefer to avoid it in the future. :)
> 
> I agree.  It's hard to justify making continued changes when it works-for-you
:)
>  I'm going to start on the requested changes to see if I can get something
> acceptable (after replying), but I'd like to at least have this somewhere
> comfortable for us so that we we're also happy with the dm-verity code, we can
> commit both and start testing them even though more changes are guaranteed to
be
> needed!

Yep, it's a trade-off. Having it posted (and actively worked on with the
commenters) is sufficient for me to believe it will make it in.

If not, we can always drop it when we rebase the kernel next time. :-)

> Updated with the link to dm-devel too.

Great!


So:

LGTM, the others might want to chime in though.

[hughd, 2010-05-12 23:31:24]

On Wed, May 12, 2010 at 2:34 PM, <olofj@chromium.org> wrote:

>
> LGTM, the others might want to chime in though.
>
> http://codereview.chromium.org/2020011/show
>

The reassurances to Olof seem good, thanks Will.
If there weren't the md= precedent, I'd be very dubious about this;
but since there is, and you've been publishing, it should be okay.

A few minutiae in do_mounts_dm.c concern me.

Three things about the comma handling:

1. The "*endp = 0" parse_fail tests would be better as "*endp != ',' ?
    The comments below say it was a comma but better to check.

2.  Updating *endp with '\0' or ' ': that's convenient and what I'd want
     to do, but can you reassure me that it's safe, that we really are
     free to modify that input buffer e.g. that it's a temporary copy,
     not what would be shown in /proc/cmdline later on: an example
     of somewhere else doing the same will reassure me.

3. Those comma,separated,target,params: can you reassure me
    that none of those params will ever themselves contain commas
    (which we would then be mis-stripping here) - I ask because I met
    that problem once with mempolicy args to a tmpfs mount, and we
    had to hack around it by checking for a digit following the comma.

And the fourth thing: that little block /* From drivers/md/dm.h */
will have to go: I guess move the two function declarations and
(all) Suspend feature flags over to include/linux/device-mapper.h.

Otherwise, looks good so far as I can tell.

Hugh

[Will Drewry, 2010-05-14 14:58:18]

On 2010/05/12 23:31:24, hughd wrote:
> On Wed, May 12, 2010 at 2:34 PM, <mailto:olofj@chromium.org> wrote:
> 
> >
> > LGTM, the others might want to chime in though.
> >
> > http://codereview.chromium.org/2020011/show
> >
> 
> The reassurances to Olof seem good, thanks Will.
> If there weren't the md= precedent, I'd be very dubious about this;
> but since there is, and you've been publishing, it should be okay.
>

Thanks for the review! I agree, but if upstream is willing to allow dm=, I'll be
really happy as it's the cleanest solution I could see :/

> A few minutiae in do_mounts_dm.c concern me.
> 
> Three things about the comma handling:
> 
> 1. The "*endp = 0" parse_fail tests would be better as "*endp != ',' ?
>     The comments below say it was a comma but better to check.

Agreed. I'll tighten it up

> 2.  Updating *endp with '\0' or ' ': that's convenient and what I'd want
>      to do, but can you reassure me that it's safe, that we really are
>      free to modify that input buffer e.g. that it's a temporary copy,
>      not what would be shown in /proc/cmdline later on: an example
>      of somewhere else doing the same will reassure me.

Since I'm using strlcpy, there's no reason to write the NUL. Instead I can just
track the size in a variable and max strlcpy there.
 
> 3. Those comma,separated,target,params: can you reassure me
>     that none of those params will ever themselves contain commas
>     (which we would then be mis-stripping here) - I ask because I met
>     that problem once with mempolicy args to a tmpfs mount, and we
>     had to hack around it by checking for a digit following the comma.

I failed to realize that modern kernel parameters can be quoted so I'll move to
using the same format device mapper uses now dm="a b c d".  I'll just need to
make sure quotes aren't acceptable in the existing table format and have to
figure out a good way to encode a "newline" equivalent (maybe a comma).

FWIW, dm has been solid about not allowing anything other than spaces as
separators so a comma should be safe, but I'll have to cross-check.

> And the fourth thing: that little block /* From drivers/md/dm.h */
> will have to go: I guess move the two function declarations and
> (all) Suspend feature flags over to include/linux/device-mapper.h.


Agreed! I was looking for direction from upstream on this one (device-mapper.h,
dm-ioctl.h, etc).

> Otherwise, looks good so far as I can tell.
> 
> Hugh


Thanks! I just lost a big response to this in codereview :(

However, I'm working with upstream to make this right device-mapper-wise:
http://lkml.org/lkml/2010/5/12/399
Hopefully, I'll have a full new set of patches to upload this weekend or monday.
 If upstream is seeming happier, then I'll ping you guys for another review
since I'd like to start getting the ability to test these changes into the image
(this and dm-verity) since I'm running out of time :) 

Would it make sense to target this and dm-verity changes to a separate branch so
that the work can be more iterative prior to pulling into the mainline?  I'm
trying to get this pulled together for the next milestone so I'll need it merged
in soonish anyhow, but having a polished version of this and dm-verity is a
challenge given the limited ability to do iterative code reviews :)

thanks!
will

[hughd, 2010-05-14 19:54:01]

On Fri, May 14, 2010 at 7:58 AM, <wad@chromium.org> wrote:

>
> Thanks! I just lost a big response to this in codereview :(
>
> However, I'm working with upstream to make this right device-mapper-wise:
> http://lkml.org/lkml/2010/5/12/399


Thanks, good to see Alasdair on it.


>
> Hopefully, I'll have a full new set of patches to upload this weekend or
> monday.
>  If upstream is seeming happier, then I'll ping you guys for another review
> since I'd like to start getting the ability to test these changes into the
> image
> (this and dm-verity) since I'm running out of time :)
>
> Would it make sense to target this and dm-verity changes to a separate
> branch so
> that the work can be more iterative prior to pulling into the mainline?
>  I'm
> trying to get this pulled together for the next milestone so I'll need it
> merged
> in soonish anyhow, but having a polished version of this and dm-verity is a
> challenge given the limited ability to do iterative code reviews :)
>

I'm all for you going ahead with this work, refining it along the way, to
get to
testing asap.  Whether a separate branch is the best way to do that, I've no
idea - hope Mandeep can advise.

Hugh


>
> thanks!
> will
>
>
> http://codereview.chromium.org/2020011/show
>

[Will Drewry, 2010-05-19 19:33:16]

On 2010/05/14 19:54:01, hughd wrote:
> On Fri, May 14, 2010 at 7:58 AM, <mailto:wad@chromium.org> wrote:
> 
> >
> > Thanks! I just lost a big response to this in codereview :(
> >
> > However, I'm working with upstream to make this right device-mapper-wise:
> > http://lkml.org/lkml/2010/5/12/399
> 
> 
> Thanks, good to see Alasdair on it.

Yup! I just hope I can get more of his time!

> 
> >
> > Hopefully, I'll have a full new set of patches to upload this weekend or
> > monday.
> >  If upstream is seeming happier, then I'll ping you guys for another review
> > since I'd like to start getting the ability to test these changes into the
> > image
> > (this and dm-verity) since I'm running out of time :)
> >
> > Would it make sense to target this and dm-verity changes to a separate
> > branch so
> > that the work can be more iterative prior to pulling into the mainline?
> >  I'm
> > trying to get this pulled together for the next milestone so I'll need it
> > merged
> > in soonish anyhow, but having a polished version of this and dm-verity is a
> > challenge given the limited ability to do iterative code reviews :)
> >
> 
> I'm all for you going ahead with this work, refining it along the way, to
> get to
> testing asap.  Whether a separate branch is the best way to do that, I've no
> idea - hope Mandeep can advise.

Cool - I've brought the CL description and code into sync with the proposed
upstream changes.  It is more invasive to address Alasdair and Mike's feedback,
but I'm hoping it is more likely to be accepted now.

PTAL if you have the chance.

[ wrt to the lib/string code, let me know if I should just backport the changes
to lib/string.c and lib/string.h in a standalone cl.  That may conflict with
other standalone cases in our tree (apparmor, ?), but I can go that route too. ]

thanks!

[hughd, 2010-05-21 00:34:26]

On 2010/05/19 19:33:16, Will Drewry wrote:

> PTAL if you have the chance.
> 
> [ wrt to the lib/string code, let me know if I should just backport the
changes
> to lib/string.c and lib/string.h in a standalone cl.  That may conflict with
> other standalone cases in our tree (apparmor, ?), but I can go that route too.
]

I'm happy with the temporary way in which you've added a local skip_spaces().

I was slightly bothered by your base_path[32], rightly commented to match
name_to_dev_t(), and the loss of -EOVERFLOW; but on reflection think both are
okay, not worth worrying about.

The one thing I do dislike is your use of "0" for readwrite and "1" for
readonly: I would much prefer "rw" and "ro" there.

[Will Drewry, 2010-06-08 15:57:40]

On 2010/05/21 00:34:26, hughd wrote:
> On 2010/05/19 19:33:16, Will Drewry wrote:
> 
> > PTAL if you have the chance.
> > 
> > [ wrt to the lib/string code, let me know if I should just backport the
> changes
> > to lib/string.c and lib/string.h in a standalone cl.  That may conflict with
> > other standalone cases in our tree (apparmor, ?), but I can go that route
too.
> ]
> 
> I'm happy with the temporary way in which you've added a local skip_spaces().

great!

> I was slightly bothered by your base_path[32], rightly commented to match
> name_to_dev_t(), and the loss of -EOVERFLOW; but on reflection think both are
> okay, not worth worrying about.

This is gone now too.  An init maintainer was against name_to_dev_t use outside
of init/.  So I reverted the dm-table.c change and added a function which walks
the target parameters and attempts to substitute major:minor in at boot-time.

> The one thing I do dislike is your use of "0" for readwrite and "1" for
> readonly: I would much prefer "rw" and "ro" there.

done!

PTAL  - I'd love to land these changes. FWIW, I push a new iteration upstream
last night which I hope will be more in line with both what init and dm people
want to see.

thanks!

[Will Drewry, 2010-06-09 19:13:27]

On 2010/06/08 15:57:40, Will Drewry wrote:
> On 2010/05/21 00:34:26, hughd wrote:
> > On 2010/05/19 19:33:16, Will Drewry wrote:
> > 
> > > PTAL if you have the chance.
> > > 
> > > [ wrt to the lib/string code, let me know if I should just backport the
> > changes
> > > to lib/string.c and lib/string.h in a standalone cl.  That may conflict
with
> > > other standalone cases in our tree (apparmor, ?), but I can go that route
> too.
> > ]
> > 
> > I'm happy with the temporary way in which you've added a local
skip_spaces().
> 
> great!
> 
> > I was slightly bothered by your base_path[32], rightly commented to match
> > name_to_dev_t(), and the loss of -EOVERFLOW; but on reflection think both
are
> > okay, not worth worrying about.
> 
> This is gone now too.  An init maintainer was against name_to_dev_t use
outside
> of init/.  So I reverted the dm-table.c change and added a function which
walks
> the target parameters and attempts to substitute major:minor in at boot-time.
> 
> > The one thing I do dislike is your use of "0" for readwrite and "1" for
> > readonly: I would much prefer "rw" and "ro" there.
> 
> done!
> 
> PTAL  - I'd love to land these changes. FWIW, I push a new iteration upstream
> last night which I hope will be more in line with both what init and dm people
> want to see.
> 
> thanks!

While I'm bugging people in kernel-land -- any further thoughts on this change?

thanks!
will

[hughd, 2010-06-09 22:09:04]

On Wed, Jun 9, 2010 at 12:13 PM,  <wad@chromium.org> wrote:

>> PTAL  - I'd love to land these changes. FWIW, I push a new iteration
>> upstream
>> last night which I hope will be more in line with both what init and dm
>> people
>> want to see.
>
>> thanks!
>
> While I'm bugging people in kernel-land -- any further thoughts on this
> change?
>
> thanks!
> will
>
> http://codereview.chromium.org/2020011/show

LGTM - but would LEBTM if you changed that "onyl" to "only"!

Hugh

[Will Drewry, 2010-06-09 22:27:14]

On 2010/06/09 22:09:04, hughd wrote:
> On Wed, Jun 9, 2010 at 12:13 PM,  <mailto:wad@chromium.org> wrote:
> 
> >> PTAL &nbsp;- I'd love to land these changes. FWIW, I push a new iteration
> >> upstream
> >> last night which I hope will be more in line with both what init and dm
> >> people
> >> want to see.
> >
> >> thanks!
> >
> > While I'm bugging people in kernel-land -- any further thoughts on this
> > change?
> >
> > thanks!
> > will
> >
> > http://codereview.chromium.org/2020011/show
> 
> LGTM - but would LEBTM if you changed that "onyl" to "only"!
> 
> Hugh
> 


Fixed and pushing!  Thanks!  Now to send out the CL for the config options ;)
will

[piman, 2010-06-09 22:45:27]

BTW someone will need to move these patches over to the ARM kernel branches.

[Will Drewry, 2010-06-09 22:51:51]

What's the best way to do that?

On Wed, Jun 9, 2010 at 5:45 PM,  <piman@chromium.org> wrote:
> BTW someone will need to move these patches over to the ARM kernel branches.
>
> http://codereview.chromium.org/2020011/show
>

[piman, 2010-06-09 23:28:40]

On Wed, Jun 9, 2010 at 3:51 PM, Will Drewry <wad@chromium.org> wrote:

> What's the best way to do that?
>

We chatted on IRC, it doesn't break anything until the userland or the image
scripts depend on it, but In the mean time a bug was filed.


> On Wed, Jun 9, 2010 at 5:45 PM,  <piman@chromium.org> wrote:
> > BTW someone will need to move these patches over to the ARM kernel
> branches.
> >
> > http://codereview.chromium.org/2020011/show
> >
>