[PATCH] init: add root=PARTUUID=UUID+/-%d support

[PATCH] init: add root=PARTUUID=UUID+/-%d support

[Will Drewry]

Not sure if this is practical and/or interesting to others, but it's a
minimal change --

Expands root=PARTUUID=UUID syntax to support selecting a root partition
by integer offset from a known, unique partition.  This approach
provides similar properties to specifying a device and partition number,
but using the UUID as the unique path prior to evaluating the offset.

For example,
  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B+1
selects the partition with UUID 99DE.. then select the next
partition.

This change is motivated by a particular usecase in Chromium OS where
the bootloader can easily determine what partition it is on (by UUID)
but doesn't perform general partition table walking.

That said, support for this approach provides a direct mechanism for the
user to modify the root partition to boot without specifically needing
to extract each PARTUUID or update the bootloader explicitly when the
root partition UUID is changed (if it is recreated to be larger, for
instance, restored, etc).  Pinning to a /boot partition UUID with an
offset allows the arbitrary root partition reconfiguration/modifications
with slightly less ambiguity than just [dev][partition] and less
stringency than the specific root partition UUID.

Signed-off-by: Will Drewry <wad@chromium.org>
---
 init/do_mounts.c |   38 ++++++++++++++++++++++++++++++++++----
 1 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index c0851a8..d1a7a33 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -85,12 +85,15 @@ no_match:
 
 /**
  * devt_from_partuuid - looks up the dev_t of a partition by its UUID
- * @uuid:	36 byte char array containing a hex ascii UUID
+ * @uuid:	min 36 byte char array containing a hex ascii UUID
  *
  * The function will return the first partition which contains a matching
  * UUID value in its partition_meta_info struct.  This does not search
  * by filesystem UUIDs.
  *
+ * If @uuid is of the format UUID+/-%u, then the partition offset from the
+ * found partition will be returned.
+ *
  * Returns the matching dev_t on success or 0 on failure.
  */
 static dev_t devt_from_partuuid(char *uuid_str)
@@ -98,6 +101,16 @@ static dev_t devt_from_partuuid(char *uuid_str)
 	dev_t res = 0;
 	struct device *dev = NULL;
 	u8 uuid[16];
+	struct gendisk *disk;
+	struct hd_struct *part;
+	unsigned int offset = 0;
+	u8 op = 0;
+
+	if (strlen(uuid_str) < 36)
+		goto done;
+
+	if (uuid_str[36])
+		sscanf(&uuid_str[36], "%c%u", &op, &offset);
 
 	/* Pack the requested UUID in the expected format. */
 	part_pack_uuid(uuid_str, uuid);
@@ -107,8 +120,25 @@ static dev_t devt_from_partuuid(char *uuid_str)
 		goto done;
 
 	res = dev->devt;
-	put_device(dev);
 
+	/* Attempt to find the partition by offset. */
+	if (!offset)
+		goto no_offset;
+
+	disk = part_to_disk(dev_to_part(dev));
+	part = NULL;
+	res = 0;
+	if (op == '+')
+		part = disk_get_part(disk, dev_to_part(dev)->partno + offset);
+	else if (op == '-')
+		part = disk_get_part(disk, dev_to_part(dev)->partno - offset);
+	if (part) {
+		res = part_devt(part);
+		put_device(part_to_dev(part));
+	}
+
+no_offset:
+	put_device(dev);
 done:
 	return res;
 }
@@ -126,6 +156,8 @@ done:
  *	   used when disk name of partitioned disk ends on a digit.
  *	6) PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF representing the
  *	   unique id of a partition if the partition table provides it.
+ *	7) PARTUUID=<UUID><+/-><decimal> to select a partition in relation to
+ *	   a partition with a known unique id.
  *
  *	If name doesn't have fall into the categories above, we return (0,0).
  *	block_class is used to check if something is a disk name. If the disk
@@ -143,8 +175,6 @@ dev_t name_to_dev_t(char *name)
 #ifdef CONFIG_BLOCK
 	if (strncmp(name, "PARTUUID=", 9) == 0) {
 		name += 9;
-		if (strlen(name) != 36)
-			goto fail;
 		res = devt_from_partuuid(name);
 		if (!res)
 			goto fail;
-- 
1.7.0.4

Re: [PATCH] init: add root=PARTUUID=UUID+/-%d support

[Kay Sievers]

On Wed, Jun 1, 2011 at 21:49, Will Drewry <wad@chromium.org> wrote:
> Not sure if this is practical and/or interesting to others, but it's a
> minimal change --
>
> Expands root=PARTUUID=UUID syntax to support selecting a root partition
> by integer offset from a known, unique partition.  This approach
> provides similar properties to specifying a device and partition number,
> but using the UUID as the unique path prior to evaluating the offset.
>
> For example,
>  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B+1
> selects the partition with UUID 99DE.. then select the next
> partition.
>
> This change is motivated by a particular usecase in Chromium OS where
> the bootloader can easily determine what partition it is on (by UUID)
> but doesn't perform general partition table walking.
>
> That said, support for this approach provides a direct mechanism for the
> user to modify the root partition to boot without specifically needing
> to extract each PARTUUID or update the bootloader explicitly when the
> root partition UUID is changed (if it is recreated to be larger, for
> instance, restored, etc).  Pinning to a /boot partition UUID with an
> offset allows the arbitrary root partition reconfiguration/modifications
> with slightly less ambiguity than just [dev][partition] and less
> stringency than the specific root partition UUID.

Kind of a creative notation to add/substract something from a uuid. :)

If we are not sure we'll not need another magic thing in the future,
we might want to go for a named parameter to add?
Maybe instead of:
  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B+2
we could do:
  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B,PARTNROFF=2

Might be it's a but over the top, but the key is named PARTUUID, and a
+- to it doesn't seem to explain what it does and can not really be
expanded for even more creative uses. :)

Kay

Re: [PATCH] init: add root=PARTUUID=UUID+/-%d support

[Will Drewry]

On Thu, Jun 2, 2011 at 7:49 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Wed, Jun 1, 2011 at 21:49, Will Drewry <wad@chromium.org> wrote:
>> Not sure if this is practical and/or interesting to others, but it's a
>> minimal change --
>>
>> Expands root=PARTUUID=UUID syntax to support selecting a root partition
>> by integer offset from a known, unique partition.  This approach
>> provides similar properties to specifying a device and partition number,
>> but using the UUID as the unique path prior to evaluating the offset.
>>
>> For example,
>>  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B+1
>> selects the partition with UUID 99DE.. then select the next
>> partition.
>>
>> This change is motivated by a particular usecase in Chromium OS where
>> the bootloader can easily determine what partition it is on (by UUID)
>> but doesn't perform general partition table walking.
>>
>> That said, support for this approach provides a direct mechanism for the
>> user to modify the root partition to boot without specifically needing
>> to extract each PARTUUID or update the bootloader explicitly when the
>> root partition UUID is changed (if it is recreated to be larger, for
>> instance, restored, etc).  Pinning to a /boot partition UUID with an
>> offset allows the arbitrary root partition reconfiguration/modifications
>> with slightly less ambiguity than just [dev][partition] and less
>> stringency than the specific root partition UUID.
>
> Kind of a creative notation to add/substract something from a uuid. :)

Necessity can sometimes do that :)

> If we are not sure we'll not need another magic thing in the future,
> we might want to go for a named parameter to add?
> Maybe instead of:
>  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B+2
> we could do:
>  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B,PARTNROFF=2
>
> Might be it's a but over the top, but the key is named PARTUUID, and a
> +- to it doesn't seem to explain what it does and can not really be
> expanded for even more creative uses. :)

Good call!  I'll take a pass at integrating that. I can see a couple
different ways of handling the parsing (e.g., when and what does it
affect).  Do you think it would make sense to keep it isolated to just
being a PARTUUID subargument or would it make more sense for
PARTNROFF= to be evaluated as a generic root= attribute?  I can see
benefits to both, but one is considerably more minimal to implement :)
 [Maybe I'll try both and see how they look.]

thanks!

Re: [PATCH] init: add root=PARTUUID=UUID+/-%d support

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

On Thu, 02 Jun 2011 08:48:16 CDT, Will Drewry said:

> Good call!  I'll take a pass at integrating that. I can see a couple
> different ways of handling the parsing (e.g., when and what does it
> affect).  Do you think it would make sense to keep it isolated to just
> being a PARTUUID subargument or would it make more sense for
> PARTNROFF= to be evaluated as a generic root= attribute?  I can see
> benefits to both, but one is considerably more minimal to implement :)
>  [Maybe I'll try both and see how they look.]

I've seen multi-boot configs that have several disks similarly partitioned - a /boot
on /dev/sdN1 and / on /dev/sdN2.  For those configs, being able to say "use
partition 2 of whatever disk you booted" would be one less thing you have
to worry about getting out of sync.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

[PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Will Drewry]

Expand root=PARTUUID=UUID syntax to support selecting a root partition
by integer offset from a known, unique partition.  This approach
provides similar properties to specifying a device and partition number,
but using the UUID as the unique path prior to evaluating the offset.

For example,
  root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B,PARTNROFF=1
selects the partition with UUID 99DE.. then select the next
partition.

This change is motivated by a particular usecase in Chromium OS where
the bootloader can easily determine what partition it is on (by UUID)
but doesn't perform general partition table walking.

That said, support for this model provides a direct mechanism for the
user to modify the root partition to boot without specifically needing
to extract each UUID or update the bootloader explicitly when the root
partition UUID is changed (if it is recreated to be larger, for
instance).  Pinning to a /boot-style partition UUID allows the arbitrary
root partition reconfiguration/modifications with slightly less
ambiguity than just [dev][partition] and less stringency than the
specific root partition UUID.

At present, PARTNROFF is tied to PARTUUID use and must follow
immediately.  It is possible to expand support to other values of root=
or to add other key-value attributes and orderings, but it seems
unnecessary to add the infrastructure without a concrete need.

v2: - convert from +/-%u to ,PARTNROFF=%d

Signed-off-by: Will Drewry <wad@chromium.org>
---
 init/do_mounts.c |   34 ++++++++++++++++++++++++++++++----
 1 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index c0851a8..35329b2 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -85,12 +85,15 @@ no_match:
 
 /**
  * devt_from_partuuid - looks up the dev_t of a partition by its UUID
- * @uuid:	36 byte char array containing a hex ascii UUID
+ * @uuid:	min 36 byte char array containing a hex ascii UUID
  *
  * The function will return the first partition which contains a matching
  * UUID value in its partition_meta_info struct.  This does not search
  * by filesystem UUIDs.
  *
+ * If @uuid is followed by a ",PARTNROFF=%d", then the number will be
+ * extracted and used as an offset from the partition identified by the UUID.
+ *
  * Returns the matching dev_t on success or 0 on failure.
  */
 static dev_t devt_from_partuuid(char *uuid_str)
@@ -98,6 +101,16 @@ static dev_t devt_from_partuuid(char *uuid_str)
 	dev_t res = 0;
 	struct device *dev = NULL;
 	u8 uuid[16];
+	struct gendisk *disk;
+	struct hd_struct *part;
+	int offset = 0;
+
+	if (strlen(uuid_str) < 36)
+		goto done;
+
+	/* Check for optional partition number offset attributes. */
+	if (uuid_str[36])
+		sscanf(&uuid_str[36], ",PARTNROFF=%d", &offset);
 
 	/* Pack the requested UUID in the expected format. */
 	part_pack_uuid(uuid_str, uuid);
@@ -107,8 +120,21 @@ static dev_t devt_from_partuuid(char *uuid_str)
 		goto done;
 
 	res = dev->devt;
-	put_device(dev);
 
+	/* Attempt to find the partition by offset. */
+	if (!offset)
+		goto no_offset;
+
+	res = 0;
+	disk = part_to_disk(dev_to_part(dev));
+	part = disk_get_part(disk, dev_to_part(dev)->partno + offset);
+	if (part) {
+		res = part_devt(part);
+		put_device(part_to_dev(part));
+	}
+
+no_offset:
+	put_device(dev);
 done:
 	return res;
 }
@@ -126,6 +152,8 @@ done:
  *	   used when disk name of partitioned disk ends on a digit.
  *	6) PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF representing the
  *	   unique id of a partition if the partition table provides it.
+ *	7) PARTUUID=<UUID>,PARTNROFF=<int> to select a partition in relation to
+ *	   a partition with a known unique id.
  *
  *	If name doesn't have fall into the categories above, we return (0,0).
  *	block_class is used to check if something is a disk name. If the disk
@@ -143,8 +171,6 @@ dev_t name_to_dev_t(char *name)
 #ifdef CONFIG_BLOCK
 	if (strncmp(name, "PARTUUID=", 9) == 0) {
 		name += 9;
-		if (strlen(name) != 36)
-			goto fail;
 		res = devt_from_partuuid(name);
 		if (!res)
 			goto fail;
-- 
1.7.0.4

Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Andrew Morton]

On Tue,  7 Jun 2011 17:19:33 -0500
Will Drewry <wad@chromium.org> wrote:

> Expand root=PARTUUID=UUID syntax to support selecting a root partition
> by integer offset from a known, unique partition.  This approach
> provides similar properties to specifying a device and partition number,
> but using the UUID as the unique path prior to evaluating the offset.
> 
> For example,
>   root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B,PARTNROFF=1
> selects the partition with UUID 99DE.. then select the next
> partition.
> 
> This change is motivated by a particular usecase in Chromium OS where
> the bootloader can easily determine what partition it is on (by UUID)
> but doesn't perform general partition table walking.
> 
> That said, support for this model provides a direct mechanism for the
> user to modify the root partition to boot without specifically needing
> to extract each UUID or update the bootloader explicitly when the root
> partition UUID is changed (if it is recreated to be larger, for
> instance).  Pinning to a /boot-style partition UUID allows the arbitrary
> root partition reconfiguration/modifications with slightly less
> ambiguity than just [dev][partition] and less stringency than the
> specific root partition UUID.
> 
> At present, PARTNROFF is tied to PARTUUID use and must follow
> immediately.  It is possible to expand support to other values of root=
> or to add other key-value attributes and orderings, but it seems
> unnecessary to add the infrastructure without a concrete need.
> 
> ...
>
>  init/do_mounts.c |   34 ++++++++++++++++++++++++++++++----
>  1 files changed, 30 insertions(+), 4 deletions(-)

I have bad news.

akpm:/usr/src/linux-3.0-rc2> grep -rl 'root=' Documentation
Documentation/scsi/sym53c8xx_2.txt
Documentation/scsi/ncr53c8xx.txt
Documentation/kernel-parameters.txt
Documentation/virtual/uml/UserModeLinux-HOWTO.txt
Documentation/virtual/lguest/lguest.txt
Documentation/filesystems/nfs/nfsroot.txt
Documentation/filesystems/affs.txt
Documentation/filesystems/ubifs.txt
Documentation/m68k/kernel-options.txt
Documentation/power/swsusp-dmcrypt.txt
Documentation/early-userspace/README
Documentation/kdump/kdump.txt
Documentation/devicetree/booting-without-of.txt
Documentation/security/Smack.txt
Documentation/md.txt
Documentation/ia64/xen.txt
Documentation/intel_txt.txt
Documentation/initrd.txt
Documentation/x86/boot.txt
Documentation/sound/oss/mwave
Documentation/arm/SA1100/Assabet
Documentation/frv/booting.txt
Documentation/networking/cs89x0.txt
Documentation/networking/cxgb.txt
Documentation/init.txt

Please work out which Documentation files need updating?

> index c0851a8..35329b2 100644
> --- a/init/do_mounts.c
> +++ b/init/do_mounts.c
> @@ -85,12 +85,15 @@ no_match:
>  
>  /**
>   * devt_from_partuuid - looks up the dev_t of a partition by its UUID
> - * @uuid:	36 byte char array containing a hex ascii UUID
> + * @uuid:	min 36 byte char array containing a hex ascii UUID
>   *
>   * The function will return the first partition which contains a matching
>   * UUID value in its partition_meta_info struct.  This does not search
>   * by filesystem UUIDs.
>   *
> + * If @uuid is followed by a ",PARTNROFF=%d", then the number will be
> + * extracted and used as an offset from the partition identified by the UUID.
> + *
>   * Returns the matching dev_t on success or 0 on failure.
>   */
>  static dev_t devt_from_partuuid(char *uuid_str)
> @@ -98,6 +101,16 @@ static dev_t devt_from_partuuid(char *uuid_str)
>  	dev_t res = 0;
>  	struct device *dev = NULL;
>  	u8 uuid[16];
> +	struct gendisk *disk;
> +	struct hd_struct *part;
> +	int offset = 0;
> +
> +	if (strlen(uuid_str) < 36)
> +		goto done;
> +
> +	/* Check for optional partition number offset attributes. */
> +	if (uuid_str[36])
> +		sscanf(&uuid_str[36], ",PARTNROFF=%d", &offset);

Syntax errors here will be silently ignored.  I expect that's common in
this area of code, but please take a look, see if we should do
something to help the poor user.

> @@ -143,8 +171,6 @@ dev_t name_to_dev_t(char *name)
>  #ifdef CONFIG_BLOCK
>  	if (strncmp(name, "PARTUUID=", 9) == 0) {
>  		name += 9;
> -		if (strlen(name) != 36)
> -			goto fail;
>  		res = devt_from_partuuid(name);
>  		if (!res)
>  			goto fail;

The code changes the behaviour of name_to_dev_t(), so we should
consider all callers.  That's hibernation, block2mtd and md.  Have you
thought about (or perhaps tested) these code paths?  Does it make sense
to use the extended syntax in these cases?  Is it possible to do so? 
Does it work?  etc.

Thanks.

Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Will Drewry]

On Thu, Jun 9, 2011 at 5:33 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue,  7 Jun 2011 17:19:33 -0500
> Will Drewry <wad@chromium.org> wrote:
>
>> Expand root=PARTUUID=UUID syntax to support selecting a root partition
>> by integer offset from a known, unique partition.  This approach
>> provides similar properties to specifying a device and partition number,
>> but using the UUID as the unique path prior to evaluating the offset.
>>
>> For example,
>>   root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B,PARTNROFF=1
>> selects the partition with UUID 99DE.. then select the next
>> partition.
>>
>> This change is motivated by a particular usecase in Chromium OS where
>> the bootloader can easily determine what partition it is on (by UUID)
>> but doesn't perform general partition table walking.
>>
>> That said, support for this model provides a direct mechanism for the
>> user to modify the root partition to boot without specifically needing
>> to extract each UUID or update the bootloader explicitly when the root
>> partition UUID is changed (if it is recreated to be larger, for
>> instance).  Pinning to a /boot-style partition UUID allows the arbitrary
>> root partition reconfiguration/modifications with slightly less
>> ambiguity than just [dev][partition] and less stringency than the
>> specific root partition UUID.
>>
>> At present, PARTNROFF is tied to PARTUUID use and must follow
>> immediately.  It is possible to expand support to other values of root=
>> or to add other key-value attributes and orderings, but it seems
>> unnecessary to add the infrastructure without a concrete need.
>>
>> ...
>>
>>  init/do_mounts.c |   34 ++++++++++++++++++++++++++++++----
>>  1 files changed, 30 insertions(+), 4 deletions(-)
>
> I have bad news.
>
> akpm:/usr/src/linux-3.0-rc2> grep -rl 'root=' Documentation
> Documentation/scsi/sym53c8xx_2.txt
> Documentation/scsi/ncr53c8xx.txt
> Documentation/kernel-parameters.txt
> Documentation/virtual/uml/UserModeLinux-HOWTO.txt
> Documentation/virtual/lguest/lguest.txt
> Documentation/filesystems/nfs/nfsroot.txt
> Documentation/filesystems/affs.txt
> Documentation/filesystems/ubifs.txt
> Documentation/m68k/kernel-options.txt
> Documentation/power/swsusp-dmcrypt.txt
> Documentation/early-userspace/README
> Documentation/kdump/kdump.txt
> Documentation/devicetree/booting-without-of.txt
> Documentation/security/Smack.txt
> Documentation/md.txt
> Documentation/ia64/xen.txt
> Documentation/intel_txt.txt
> Documentation/initrd.txt
> Documentation/x86/boot.txt
> Documentation/sound/oss/mwave
> Documentation/arm/SA1100/Assabet
> Documentation/frv/booting.txt
> Documentation/networking/cs89x0.txt
> Documentation/networking/cxgb.txt
> Documentation/init.txt
>
> Please work out which Documentation files need updating?

I wouldn't say that it's bad news - I'll track down the place that
seems to make the most sense.

>> index c0851a8..35329b2 100644
>> --- a/init/do_mounts.c
>> +++ b/init/do_mounts.c
>> @@ -85,12 +85,15 @@ no_match:
>>
>>  /**
>>   * devt_from_partuuid - looks up the dev_t of a partition by its UUID
>> - * @uuid:    36 byte char array containing a hex ascii UUID
>> + * @uuid:    min 36 byte char array containing a hex ascii UUID
>>   *
>>   * The function will return the first partition which contains a matching
>>   * UUID value in its partition_meta_info struct.  This does not search
>>   * by filesystem UUIDs.
>>   *
>> + * If @uuid is followed by a ",PARTNROFF=%d", then the number will be
>> + * extracted and used as an offset from the partition identified by the UUID.
>> + *
>>   * Returns the matching dev_t on success or 0 on failure.
>>   */
>>  static dev_t devt_from_partuuid(char *uuid_str)
>> @@ -98,6 +101,16 @@ static dev_t devt_from_partuuid(char *uuid_str)
>>       dev_t res = 0;
>>       struct device *dev = NULL;
>>       u8 uuid[16];
>> +     struct gendisk *disk;
>> +     struct hd_struct *part;
>> +     int offset = 0;
>> +
>> +     if (strlen(uuid_str) < 36)
>> +             goto done;
>> +
>> +     /* Check for optional partition number offset attributes. */
>> +     if (uuid_str[36])
>> +             sscanf(&uuid_str[36], ",PARTNROFF=%d", &offset);
>
> Syntax errors here will be silently ignored.  I expect that's common in
> this area of code, but please take a look, see if we should do
> something to help the poor user.

Sounds good. I can see a few ways to bubble up useful information and
still trigger the classic error path (potential partitions) too, I
think.

>> @@ -143,8 +171,6 @@ dev_t name_to_dev_t(char *name)
>>  #ifdef CONFIG_BLOCK
>>       if (strncmp(name, "PARTUUID=", 9) == 0) {
>>               name += 9;
>> -             if (strlen(name) != 36)
>> -                     goto fail;
>>               res = devt_from_partuuid(name);
>>               if (!res)
>>                       goto fail;
>
> The code changes the behaviour of name_to_dev_t(), so we should
> consider all callers.  That's hibernation, block2mtd and md.  Have you
> thought about (or perhaps tested) these code paths?  Does it make sense
> to use the extended syntax in these cases?  Is it possible to do so?
> Does it work?  etc.

Good question(s) - given that name_to_dev_t is init-internal, I had
forgotten there were still a fair number of consumers.  I'll review
the code paths in the places where I can't test them outright and make
a note and/or appropriate changes.

Thanks for the feedback!
will

Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Will Drewry]

On Thu, Jun 9, 2011 at 9:56 PM, Will Drewry <wad@chromium.org> wrote:
> On Thu, Jun 9, 2011 at 5:33 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Tue,  7 Jun 2011 17:19:33 -0500
>> Will Drewry <wad@chromium.org> wrote:
>>
>>> Expand root=PARTUUID=UUID syntax to support selecting a root partition
>>> by integer offset from a known, unique partition.  This approach
>>> provides similar properties to specifying a device and partition number,
>>> but using the UUID as the unique path prior to evaluating the offset.
>>>
>>> For example,
>>>   root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B,PARTNROFF=1
>>> selects the partition with UUID 99DE.. then select the next
>>> partition.
>>>
>>> This change is motivated by a particular usecase in Chromium OS where
>>> the bootloader can easily determine what partition it is on (by UUID)
>>> but doesn't perform general partition table walking.
>>>
>>> That said, support for this model provides a direct mechanism for the
>>> user to modify the root partition to boot without specifically needing
>>> to extract each UUID or update the bootloader explicitly when the root
>>> partition UUID is changed (if it is recreated to be larger, for
>>> instance).  Pinning to a /boot-style partition UUID allows the arbitrary
>>> root partition reconfiguration/modifications with slightly less
>>> ambiguity than just [dev][partition] and less stringency than the
>>> specific root partition UUID.
>>>
>>> At present, PARTNROFF is tied to PARTUUID use and must follow
>>> immediately.  It is possible to expand support to other values of root=
>>> or to add other key-value attributes and orderings, but it seems
>>> unnecessary to add the infrastructure without a concrete need.
>>>
>>> ...
>>>
>>>  init/do_mounts.c |   34 ++++++++++++++++++++++++++++++----
>>>  1 files changed, 30 insertions(+), 4 deletions(-)
>>
>> I have bad news.
>>
>> akpm:/usr/src/linux-3.0-rc2> grep -rl 'root=' Documentation
>> Documentation/scsi/sym53c8xx_2.txt
>> Documentation/scsi/ncr53c8xx.txt
>> Documentation/kernel-parameters.txt
>> Documentation/virtual/uml/UserModeLinux-HOWTO.txt
>> Documentation/virtual/lguest/lguest.txt
>> Documentation/filesystems/nfs/nfsroot.txt
>> Documentation/filesystems/affs.txt
>> Documentation/filesystems/ubifs.txt
>> Documentation/m68k/kernel-options.txt
>> Documentation/power/swsusp-dmcrypt.txt
>> Documentation/early-userspace/README
>> Documentation/kdump/kdump.txt
>> Documentation/devicetree/booting-without-of.txt
>> Documentation/security/Smack.txt
>> Documentation/md.txt
>> Documentation/ia64/xen.txt
>> Documentation/intel_txt.txt
>> Documentation/initrd.txt
>> Documentation/x86/boot.txt
>> Documentation/sound/oss/mwave
>> Documentation/arm/SA1100/Assabet
>> Documentation/frv/booting.txt
>> Documentation/networking/cs89x0.txt
>> Documentation/networking/cxgb.txt
>> Documentation/init.txt
>>
>> Please work out which Documentation files need updating?
>
> I wouldn't say that it's bad news - I'll track down the place that
> seems to make the most sense.
>
>>> index c0851a8..35329b2 100644
>>> --- a/init/do_mounts.c
>>> +++ b/init/do_mounts.c
>>> @@ -85,12 +85,15 @@ no_match:
>>>
>>>  /**
>>>   * devt_from_partuuid - looks up the dev_t of a partition by its UUID
>>> - * @uuid:    36 byte char array containing a hex ascii UUID
>>> + * @uuid:    min 36 byte char array containing a hex ascii UUID
>>>   *
>>>   * The function will return the first partition which contains a matching
>>>   * UUID value in its partition_meta_info struct.  This does not search
>>>   * by filesystem UUIDs.
>>>   *
>>> + * If @uuid is followed by a ",PARTNROFF=%d", then the number will be
>>> + * extracted and used as an offset from the partition identified by the UUID.
>>> + *
>>>   * Returns the matching dev_t on success or 0 on failure.
>>>   */
>>>  static dev_t devt_from_partuuid(char *uuid_str)
>>> @@ -98,6 +101,16 @@ static dev_t devt_from_partuuid(char *uuid_str)
>>>       dev_t res = 0;
>>>       struct device *dev = NULL;
>>>       u8 uuid[16];
>>> +     struct gendisk *disk;
>>> +     struct hd_struct *part;
>>> +     int offset = 0;
>>> +
>>> +     if (strlen(uuid_str) < 36)
>>> +             goto done;
>>> +
>>> +     /* Check for optional partition number offset attributes. */
>>> +     if (uuid_str[36])
>>> +             sscanf(&uuid_str[36], ",PARTNROFF=%d", &offset);
>>
>> Syntax errors here will be silently ignored.  I expect that's common in
>> this area of code, but please take a look, see if we should do
>> something to help the poor user.
>
> Sounds good. I can see a few ways to bubble up useful information and
> still trigger the classic error path (potential partitions) too, I
> think.
>
>>> @@ -143,8 +171,6 @@ dev_t name_to_dev_t(char *name)
>>>  #ifdef CONFIG_BLOCK
>>>       if (strncmp(name, "PARTUUID=", 9) == 0) {
>>>               name += 9;
>>> -             if (strlen(name) != 36)
>>> -                     goto fail;
>>>               res = devt_from_partuuid(name);
>>>               if (!res)
>>>                       goto fail;
>>
>> The code changes the behaviour of name_to_dev_t(), so we should
>> consider all callers.  That's hibernation, block2mtd and md.  Have you
>> thought about (or perhaps tested) these code paths?  Does it make sense
>> to use the extended syntax in these cases?  Is it possible to do so?
>> Does it work?  etc.
>
> Good question(s) - given that name_to_dev_t is init-internal, I had
> forgotten there were still a fair number of consumers.  I'll review
> the code paths in the places where I can't test them outright and make
> a note and/or appropriate changes.

Upon further inspection, the code changes would not directly break any
existing code, but PARTUUID=...,PARTNROFF= would not be usable via the
other entrypoints to name_to_dev_t.  E.g., block2mtd or md because
they take in comma-separated parameters prior to calling
name_to_dev_t.  That seems like it'd be less than ideal.

Kay,  Do you have a strong preference around the ,PARTNROFF= syntax?
I'm not sure what the cleanest approach is, but I'm inclined to finish
other patch cleanup (and documentation) and repost with '/' as the
separator instead of ','.   At present ',' is reused across several
places where devices are supplied by "name", but '/' is expected as
part of the normal path semantic.  The other option would be to use
':' instead. ':' isn't usually overloaded as it is expected as part of
a the device major:minor naming scheme, but slashes seem more sane
even if weird:

  PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF/PARTNROFF=1

I'll repost along these lines unless someone indicates that this is
too grotesque to consider.

Thanks!
will

Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Kay Sievers]

On Fri, Jun 24, 2011 at 21:53, Will Drewry <wad@chromium.org> wrote:

> Upon further inspection, the code changes would not directly break any
> existing code, but PARTUUID=...,PARTNROFF= would not be usable via the
> other entrypoints to name_to_dev_t.  E.g., block2mtd or md because
> they take in comma-separated parameters prior to calling
> name_to_dev_t.  That seems like it'd be less than ideal.
>
> Kay,  Do you have a strong preference around the ,PARTNROFF= syntax?
> I'm not sure what the cleanest approach is, but I'm inclined to finish
> other patch cleanup (and documentation) and repost with '/' as the
> separator instead of ','.   At present ',' is reused across several
> places where devices are supplied by "name", but '/' is expected as
> part of the normal path semantic.  The other option would be to use
> ':' instead. ':' isn't usually overloaded as it is expected as part of
> a the device major:minor naming scheme, but slashes seem more sane
> even if weird:
>
>  PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF/PARTNROFF=1
>
> I'll repost along these lines unless someone indicates that this is
> too grotesque to consider.

Hmm, '/' might look a bit strange in the context of a path, which is
the common use case for root=.

We catch PARTUUID before parsing any of the other options, right? So
we can't really break anything.

Fstab and mount options in general all use ',' to separate keywords,
and I think we should do the same here. I think the ',' still looks
more natural than an artificial '/' to avoid possibly breaking
something we don't even call into. :)

Thanks,
Kay

Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Will Drewry]

On Sun, Jun 26, 2011 at 11:43 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Fri, Jun 24, 2011 at 21:53, Will Drewry <wad@chromium.org> wrote:
>
>> Upon further inspection, the code changes would not directly break any
>> existing code, but PARTUUID=...,PARTNROFF= would not be usable via the
>> other entrypoints to name_to_dev_t.  E.g., block2mtd or md because
>> they take in comma-separated parameters prior to calling
>> name_to_dev_t.  That seems like it'd be less than ideal.
>>
>> Kay,  Do you have a strong preference around the ,PARTNROFF= syntax?
>> I'm not sure what the cleanest approach is, but I'm inclined to finish
>> other patch cleanup (and documentation) and repost with '/' as the
>> separator instead of ','.   At present ',' is reused across several
>> places where devices are supplied by "name", but '/' is expected as
>> part of the normal path semantic.  The other option would be to use
>> ':' instead. ':' isn't usually overloaded as it is expected as part of
>> a the device major:minor naming scheme, but slashes seem more sane
>> even if weird:
>>
>>  PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF/PARTNROFF=1
>>
>> I'll repost along these lines unless someone indicates that this is
>> too grotesque to consider.
>
> Hmm, '/' might look a bit strange in the context of a path, which is
> the common use case for root=.

Yeah - that was why I was considering colons too, but slashes seemed
less likely to have other weird expectations.

> We catch PARTUUID before parsing any of the other options, right? So
> we can't really break anything.

For root= parsing, it's always okay.  The problem is if you want to
use PARTUUID=...,PARTNR=.. as a device "name" in an argument to md= or
to mtd2block.  Both of those take their device name from a
comma-separated argument list.  While they could be fixed up to
support this syntax, it seems that it would make things pretty
confusing.  (The other option is to just not support this naming
scheme via those input paths.)

> Fstab and mount options in general all use ',' to separate keywords,
> and I think we should do the same here. I think the ',' still looks
> more natural than an artificial '/' to avoid possibly breaking
> something we don't even call into. :)

I think the comma looks better too, but I wasn't sure if it'd be fair
game to provide a early-init device resolution path that solely worked
for root= and not for the other consumers at that stage.  I'll see if
there is another way around this, but I'm not seeing anything offhand.

thanks!
will

Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Kay Sievers]

On Sun, Jun 26, 2011 at 20:00, Will Drewry <wad@chromium.org> wrote:
> On Sun, Jun 26, 2011 at 11:43 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Fri, Jun 24, 2011 at 21:53, Will Drewry <wad@chromium.org> wrote:
>>
>>> Upon further inspection, the code changes would not directly break any
>>> existing code, but PARTUUID=...,PARTNROFF= would not be usable via the
>>> other entrypoints to name_to_dev_t.  E.g., block2mtd or md because
>>> they take in comma-separated parameters prior to calling
>>> name_to_dev_t.  That seems like it'd be less than ideal.
>>>
>>> Kay,  Do you have a strong preference around the ,PARTNROFF= syntax?
>>> I'm not sure what the cleanest approach is, but I'm inclined to finish
>>> other patch cleanup (and documentation) and repost with '/' as the
>>> separator instead of ','.   At present ',' is reused across several
>>> places where devices are supplied by "name", but '/' is expected as
>>> part of the normal path semantic.  The other option would be to use
>>> ':' instead. ':' isn't usually overloaded as it is expected as part of
>>> a the device major:minor naming scheme, but slashes seem more sane
>>> even if weird:
>>>
>>>  PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF/PARTNROFF=1
>>>
>>> I'll repost along these lines unless someone indicates that this is
>>> too grotesque to consider.
>>
>> Hmm, '/' might look a bit strange in the context of a path, which is
>> the common use case for root=.
>
> Yeah - that was why I was considering colons too, but slashes seemed
> less likely to have other weird expectations.
>
>> We catch PARTUUID before parsing any of the other options, right? So
>> we can't really break anything.
>
> For root= parsing, it's always okay.  The problem is if you want to
> use PARTUUID=...,PARTNR=.. as a device "name" in an argument to md= or
> to mtd2block.  Both of those take their device name from a
> comma-separated argument list.  While they could be fixed up to
> support this syntax, it seems that it would make things pretty
> confusing.  (The other option is to just not support this naming
> scheme via those input paths.)
>
>> Fstab and mount options in general all use ',' to separate keywords,
>> and I think we should do the same here. I think the ',' still looks
>> more natural than an artificial '/' to avoid possibly breaking
>> something we don't even call into. :)
>
> I think the comma looks better too, but I wasn't sure if it'd be fair
> game to provide a early-init device resolution path that solely worked
> for root= and not for the other consumers at that stage.  I'll see if
> there is another way around this, but I'm not seeing anything offhand.

Maybe name_to_dev_t() could work like strtoul(), and get a separator
character passed, and return the remaining string it did not parse as
belonging to one single device. That could simplify the string
separation in callers too, if they decide to support the new options.

But I see your point, and it might be nice in general to be able to
easily identify the two PART* keys as belonging together, describing a
single device. Maybe the slash is way to go, I don't have a better
idea.

Thanks,
Kay

Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support

[Will Drewry]

On Sun, Jun 26, 2011 at 3:23 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Sun, Jun 26, 2011 at 20:00, Will Drewry <wad@chromium.org> wrote:
>> On Sun, Jun 26, 2011 at 11:43 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>> On Fri, Jun 24, 2011 at 21:53, Will Drewry <wad@chromium.org> wrote:
>>>
>>>> Upon further inspection, the code changes would not directly break any
>>>> existing code, but PARTUUID=...,PARTNROFF= would not be usable via the
>>>> other entrypoints to name_to_dev_t.  E.g., block2mtd or md because
>>>> they take in comma-separated parameters prior to calling
>>>> name_to_dev_t.  That seems like it'd be less than ideal.
>>>>
>>>> Kay,  Do you have a strong preference around the ,PARTNROFF= syntax?
>>>> I'm not sure what the cleanest approach is, but I'm inclined to finish
>>>> other patch cleanup (and documentation) and repost with '/' as the
>>>> separator instead of ','.   At present ',' is reused across several
>>>> places where devices are supplied by "name", but '/' is expected as
>>>> part of the normal path semantic.  The other option would be to use
>>>> ':' instead. ':' isn't usually overloaded as it is expected as part of
>>>> a the device major:minor naming scheme, but slashes seem more sane
>>>> even if weird:
>>>>
>>>>  PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF/PARTNROFF=1
>>>>
>>>> I'll repost along these lines unless someone indicates that this is
>>>> too grotesque to consider.
>>>
>>> Hmm, '/' might look a bit strange in the context of a path, which is
>>> the common use case for root=.
>>
>> Yeah - that was why I was considering colons too, but slashes seemed
>> less likely to have other weird expectations.
>>
>>> We catch PARTUUID before parsing any of the other options, right? So
>>> we can't really break anything.
>>
>> For root= parsing, it's always okay.  The problem is if you want to
>> use PARTUUID=...,PARTNR=.. as a device "name" in an argument to md= or
>> to mtd2block.  Both of those take their device name from a
>> comma-separated argument list.  While they could be fixed up to
>> support this syntax, it seems that it would make things pretty
>> confusing.  (The other option is to just not support this naming
>> scheme via those input paths.)
>>
>>> Fstab and mount options in general all use ',' to separate keywords,
>>> and I think we should do the same here. I think the ',' still looks
>>> more natural than an artificial '/' to avoid possibly breaking
>>> something we don't even call into. :)
>>
>> I think the comma looks better too, but I wasn't sure if it'd be fair
>> game to provide a early-init device resolution path that solely worked
>> for root= and not for the other consumers at that stage.  I'll see if
>> there is another way around this, but I'm not seeing anything offhand.
>
> Maybe name_to_dev_t() could work like strtoul(), and get a separator
> character passed, and return the remaining string it did not parse as
> belonging to one single device. That could simplify the string
> separation in callers too, if they decide to support the new options.
>
> But I see your point, and it might be nice in general to be able to
> easily identify the two PART* keys as belonging together, describing a
> single device. Maybe the slash is way to go, I don't have a better
> idea.

So I've fiddled with a few different options and went back to the
slash.  I'll repost in a new thread, but I'd appreciate any opinions.
It doesn't look too heinous in my config files and not starting with a
slash helps, but I do with it could have been cleaner.

thanks again!
will