Re: PATCH V3] mkfs.btrfs: allow UUID specification at mkfs time
On Thu, May 15, 2014 at 12:53:52PM -0500, Eric Sandeen wrote: > So, in my testing, I found that re-mkfsing a device with the same UUID lead > to weird & distant segfaults in other bits of code. Probably due to the > uuid cache? - I didn't dig into it, because ... Ok, something that needs more debugging, the uuid/blkid cache is a good guess I think. > ... people didn't want to be able to create duplicate UUIDs, so I figured > the outright rejection of that was a trivial way to solve it ... > > And like I mentioned, if you really want to recreate the same UUID, you > can mkfs twice. It doesn't take long. ;) > > I dunno, maybe that's too lame. For a feature that upstream development > doesn't really seem to want, I'm not sure how much more effort I should > put into it? Well, your patch allows to create a filesytem with a given UUID in the safe case. There are workarounds if the UUID is not unique for the same fs (run twice, or wipefs first). That's enough for me to integrate the V3 patch and we can address creating duplicate UUIDs separately once we can make it work correctly. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH V3] mkfs.btrfs: allow UUID specification at mkfs time
On 5/15/14, 12:39 PM, David Sterba wrote:
> On Wed, May 14, 2014 at 05:07:04PM -0500, Eric Sandeen wrote:
@@ -125,7 +154,20 @@ int make_btrfs(int fd, const char *device, const char
*label,
memset(&super, 0, sizeof(super));
num_bytes = (num_bytes / sectorsize) * sectorsize;
- uuid_generate(super.fsid);
+ if (fs_uuid) {
+ if (uuid_parse(fs_uuid, super.fsid) != 0) {
+ fprintf(stderr, "could not parse UUID: %s\n", fs_uuid);
+ ret = -EINVAL;
+ goto out;
+ }
+ if (!test_uuid_unique(fs_uuid)) {
+ fprintf(stderr, "non-unique UUID: %s\n", fs_uuid);
+ ret = -EBUSY;
+ goto out;
+ }
>>>
>>> Why a second call to test_uuid_unique(fs_uuid) ?
>>
>> Because kdave said he thought it was worth being paranoid in an earlier
>> email,
>> if I understood him correctly.
>
> I'm thinking about it again. My original idea was not to easily allow
> to create a duplicate uuid to a regular user. But, if one uses --uuid
> already, that's something I can count as a willful action and any
> mistakes can be blamed on the user.
>
> If we end up with a warning, then the documentation should say how
> spectacularly it can blow up the system.
>
So, in my testing, I found that re-mkfsing a device with the same UUID lead
to weird & distant segfaults in other bits of code. Probably due to the
uuid cache? - I didn't dig into it, because ...
... people didn't want to be able to create duplicate UUIDs, so I figured
the outright rejection of that was a trivial way to solve it ...
And like I mentioned, if you really want to recreate the same UUID, you
can mkfs twice. It doesn't take long. ;)
I dunno, maybe that's too lame. For a feature that upstream development
doesn't really seem to want, I'm not sure how much more effort I should
put into it?
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH V3] mkfs.btrfs: allow UUID specification at mkfs time
On Wed, May 14, 2014 at 05:07:04PM -0500, Eric Sandeen wrote:
> >> @@ -125,7 +154,20 @@ int make_btrfs(int fd, const char *device, const char
> >> *label,
> >>memset(&super, 0, sizeof(super));
> >>
> >>num_bytes = (num_bytes / sectorsize) * sectorsize;
> >> - uuid_generate(super.fsid);
> >> + if (fs_uuid) {
> >> + if (uuid_parse(fs_uuid, super.fsid) != 0) {
> >> + fprintf(stderr, "could not parse UUID: %s\n", fs_uuid);
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> + if (!test_uuid_unique(fs_uuid)) {
> >> + fprintf(stderr, "non-unique UUID: %s\n", fs_uuid);
> >> + ret = -EBUSY;
> >> + goto out;
> >> + }
> >
> > Why a second call to test_uuid_unique(fs_uuid) ?
>
> Because kdave said he thought it was worth being paranoid in an earlier email,
> if I understood him correctly.
I'm thinking about it again. My original idea was not to easily allow
to create a duplicate uuid to a regular user. But, if one uses --uuid
already, that's something I can count as a willful action and any
mistakes can be blamed on the user.
If we end up with a warning, then the documentation should say how
spectacularly it can blow up the system.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH V3] mkfs.btrfs: allow UUID specification at mkfs time
On 5/14/14, 5:04 PM, Goffredo Baroncelli wrote:
> On 05/14/2014 07:39 PM, Eric Sandeen wrote:
>> Allow the specification of the filesystem UUID at mkfs time.
>>
>> Non-unique unique IDs are rejected. This includes attempting
>> to re-mkfs with the same UUID; if you really want to do that,
>> you can mkfs with a new UUID, then re-mkfs with the one you
>> wanted. ;)
>>
>> (Implemented only for mkfs.btrfs, not btrfs-convert).
>>
>> Signed-off-by: Eric Sandeen
>> ---
>>
>> NB: the prior patch didn't work well if you re-mkfs'd with
>> the same UUID; to be honest I didn't get to the bottom of it,
>> but the fact that that old UUID was already in an internal
>> list of present devices seems to have confused things.
>>
>> V2: reject non-unique unique IDs.
>> V3: test for non-unique unique IDs early in mkfs.
>>
>> @@ -1368,6 +1374,20 @@ int main(int ac, char **av)
>> "The -r option is limited to a single device\n");
>> exit(1);
>> }
>> +
>> +if (fs_uuid) {
>> +uuid_t dummy_uuid;
>> +
>> +if (uuid_parse(fs_uuid, dummy_uuid) != 0) {
>> +fprintf(stderr, "could not parse UUID: %s\n", fs_uuid);
>> +exit(1);
>> +}
>> +if (!test_uuid_unique(fs_uuid)) {
>> +fprintf(stderr, "non-unique UUID: %s\n", fs_uuid);
>> +exit(1);
>> +}
>
> My test showed that this detect a false positive when the user tries to mkfs
> two time on the same device with the same uuid.
It's not a false positive; after the first mkfs, the UUID does exist. :) See
also the commit log I wrote.
>> diff --git a/utils.c b/utils.c
>> index 3e9c527..cfac0d4 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -93,12 +93,41 @@ static u64 reference_root_table[] = {
>> [6] = BTRFS_CSUM_TREE_OBJECTID,
>> };
>>
>> -int make_btrfs(int fd, const char *device, const char *label,
>> +int test_uuid_unique(char *fs_uuid)
>> +{
>> +int unique = 1;
>> +blkid_dev_iterate iter = NULL;
>> +blkid_dev dev = NULL;
>> +blkid_cache cache = NULL;
>> +
>> +if (blkid_get_cache(&cache, 0) < 0) {
>> +printf("ERROR: lblkid cache get failed\n");
>> +return 1;
>> +}
>> +blkid_probe_all(cache);
>> +iter = blkid_dev_iterate_begin(cache);
>> +blkid_dev_set_search(iter, "UUID", fs_uuid);
>> +
>> +while (blkid_dev_next(iter, &dev) == 0) {
>
> This function should skip the check for the devices involved in the mkfs.
Perhaps. When I was doing something similar before, I ended up with
inexplicable segfaults when a device found on initial scan (?) got recreated
with the same UUID. Or something.
>> @@ -125,7 +154,20 @@ int make_btrfs(int fd, const char *device, const char
>> *label,
>> memset(&super, 0, sizeof(super));
>>
>> num_bytes = (num_bytes / sectorsize) * sectorsize;
>> -uuid_generate(super.fsid);
>> +if (fs_uuid) {
>> +if (uuid_parse(fs_uuid, super.fsid) != 0) {
>> +fprintf(stderr, "could not parse UUID: %s\n", fs_uuid);
>> +ret = -EINVAL;
>> +goto out;
>> +}
>> +if (!test_uuid_unique(fs_uuid)) {
>> +fprintf(stderr, "non-unique UUID: %s\n", fs_uuid);
>> +ret = -EBUSY;
>> +goto out;
>> +}
>
> Why a second call to test_uuid_unique(fs_uuid) ?
Because kdave said he thought it was worth being paranoid in an earlier email,
if I understood him correctly.
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH V3] mkfs.btrfs: allow UUID specification at mkfs time
On 05/14/2014 07:39 PM, Eric Sandeen wrote:
> Allow the specification of the filesystem UUID at mkfs time.
>
> Non-unique unique IDs are rejected. This includes attempting
> to re-mkfs with the same UUID; if you really want to do that,
> you can mkfs with a new UUID, then re-mkfs with the one you
> wanted. ;)
>
> (Implemented only for mkfs.btrfs, not btrfs-convert).
>
> Signed-off-by: Eric Sandeen
> ---
>
> NB: the prior patch didn't work well if you re-mkfs'd with
> the same UUID; to be honest I didn't get to the bottom of it,
> but the fact that that old UUID was already in an internal
> list of present devices seems to have confused things.
>
> V2: reject non-unique unique IDs.
> V3: test for non-unique unique IDs early in mkfs.
>
> diff --git a/btrfs-convert.c b/btrfs-convert.c
> index a8b2c51..d62d4f8 100644
> --- a/btrfs-convert.c
> +++ b/btrfs-convert.c
> @@ -2240,7 +2240,7 @@ static int do_convert(const char *devname, int
> datacsum, int packing,
> goto fail;
> }
> ret = make_btrfs(fd, devname, ext2_fs->super->s_volume_name,
> - blocks, total_bytes, blocksize, blocksize,
> + NULL, blocks, total_bytes, blocksize, blocksize,
>blocksize, blocksize, 0);
> if (ret) {
> fprintf(stderr, "unable to create initial ctree: %s\n",
> diff --git a/man/mkfs.btrfs.8.in b/man/mkfs.btrfs.8.in
> index bef509d..4450d03 100644
> --- a/man/mkfs.btrfs.8.in
> +++ b/man/mkfs.btrfs.8.in
> @@ -16,6 +16,7 @@ mkfs.btrfs \- create a btrfs filesystem
> [ \fB\-r\fP\fI rootdir\fP ]
> [ \fB\-K\fP ]
> [ \fB\-O\fP\fI feature1,feature2,...\fP ]
> +[ \fB\-U\fP\fI uuid\fP ]
> [ \fB\-h\fP ]
> [ \fB\-V\fP ]
> \fI device\fP [ \fIdevice ...\fP ]
> @@ -90,6 +91,9 @@ To see all run
>
> \fBmkfs.btrfs -O list-all\fR
> .TP
> +\fB\-U\fR, \fB\-\-uuid \fR
> +Create the filesystem with the specified UUID, which must not already exist
> on the system.
> +.TP
> \fB\-V\fR, \fB\-\-version\fR
> Print the \fBmkfs.btrfs\fP version and exit.
> .SH UNIT
> diff --git a/mkfs.c b/mkfs.c
> index dbd83f5..d67a5ba 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -288,6 +288,7 @@ static void print_usage(void)
> fprintf(stderr, "\t -r --rootdir the source directory\n");
> fprintf(stderr, "\t -K --nodiscard do not perform whole device TRIM\n");
> fprintf(stderr, "\t -O --features comma separated list of filesystem
> features\n");
> + fprintf(stderr, "\t -U --uuid specify the filesystem UUID\n");
> fprintf(stderr, "\t -V --version print the mkfs.btrfs version and
> exit\n");
> fprintf(stderr, "%s\n", BTRFS_BUILD_VERSION);
> exit(1);
> @@ -351,6 +352,7 @@ static struct option long_options[] = {
> { "rootdir", 1, NULL, 'r' },
> { "nodiscard", 0, NULL, 'K' },
> { "features", 0, NULL, 'O' },
> + { "uuid", 0, NULL, 'U' },
> { NULL, 0, NULL, 0}
> };
>
> @@ -1273,11 +1275,12 @@ int main(int ac, char **av)
> int dev_cnt = 0;
> int saved_optind;
> char estr[100];
> + char *fs_uuid = NULL;
> u64 features = DEFAULT_MKFS_FEATURES;
>
> while(1) {
> int c;
> - c = getopt_long(ac, av, "A:b:fl:n:s:m:d:L:O:r:VMK",
> + c = getopt_long(ac, av, "A:b:fl:n:s:m:d:L:O:r:U:VMK",
> long_options, &option_index);
> if (c < 0)
> break;
> @@ -1346,6 +1349,9 @@ int main(int ac, char **av)
> source_dir = optarg;
> source_dir_set = 1;
> break;
> + case 'U':
> + fs_uuid = optarg;
> + break;
> case 'K':
> discard = 0;
> break;
> @@ -1368,6 +1374,20 @@ int main(int ac, char **av)
> "The -r option is limited to a single device\n");
> exit(1);
> }
> +
> + if (fs_uuid) {
> + uuid_t dummy_uuid;
> +
> + if (uuid_parse(fs_uuid, dummy_uuid) != 0) {
> + fprintf(stderr, "could not parse UUID: %s\n", fs_uuid);
> + exit(1);
> + }
> + if (!test_uuid_unique(fs_uuid)) {
> + fprintf(stderr, "non-unique UUID: %s\n", fs_uuid);
> + exit(1);
> + }
My test showed that this detect a false positive when the user tries to mkfs
two time on the same device with the same uuid.
> + }
> +
> while (dev_cnt-- > 0) {
> file = av[optind++];
> if (is_block_device(file))
> @@ -1514,7 +1534,7 @@ int main(int ac, char **av)
>
> process_fs_features(features);
>
> - ret = make_btrfs(fd, file, label, blocks, dev_block_count,
> + ret = make_btrfs(fd, file, label, fs_uuid, blocks, dev_block_count,
>
