Messages in this thread | | | Date | Fri, 24 Jun 2011 14:53:18 -0500 | Subject | Re: [PATCH v2] init: add root=PARTUUID=UUID,PARTNROFF=%d support | From | 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |