[PATCH] md: allow changing set_name of running array

[PATCH] md: allow changing set_name of running array

[Michał Mirosław]

Allow changing active array's set_name. This is the only way to
safely update superblock on an array which carries a mounted fs.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/md/md.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index caca5d689cdc..59aa10669bef 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5051,6 +5051,84 @@ static struct md_sysfs_entry md_consistency_policy =
 __ATTR(consistency_policy, S_IRUGO | S_IWUSR, consistency_policy_show,
        consistency_policy_store);
 
+static ssize_t
+set_name_show(struct mddev *mddev, char *page)
+{
+	struct mdp_superblock_1 *sb;
+	struct md_rdev *rdev;
+	int err;
+
+	err = mddev_lock(mddev);
+	if (err)
+		return err;
+
+	/* only version-1 superblocks carry a MD set's name */
+	err = -ENXIO;
+	if (mddev->major_version != 1)
+		goto out_unlock;
+
+	err = -EINVAL;
+	rdev_for_each(rdev, mddev) {
+		if (WARN_ON(!rdev->sb_loaded))
+			continue;
+
+		sb = page_address(rdev->sb_page);
+		err = sprintf(page, "%.32s\n", sb->set_name);
+		break;
+	}
+
+out_unlock:
+	mddev_unlock(mddev);
+	return err;
+}
+
+static ssize_t
+set_name_store(struct mddev *mddev, const char *buf, size_t buflen)
+{
+	struct mdp_superblock_1 *sb;
+	struct md_rdev *rdev;
+	size_t len = buflen;
+	int err;
+
+	if (len && buf[len - 1] == '\n')
+		--len;
+
+	if (len > sizeof(sb->set_name))
+		return -ENOSPC;
+
+	err = mddev_lock(mddev);
+	if (err)
+		return err;
+
+	/* only version-1 superblocks carry a MD set's name */
+	err = -ENXIO;
+	if (mddev->major_version != 1)
+		goto out_unlock;
+
+	err = -EROFS;
+	if (mddev->ro)
+		goto out_unlock;
+
+	rdev_for_each(rdev, mddev) {
+		if (WARN_ON(!rdev->sb_loaded))
+			continue;
+
+		sb = page_address(rdev->sb_page);
+		memcpy(sb->set_name, buf, len);
+		memset(&sb->set_name[len], 0, sizeof(sb->set_name) - len);
+	}
+
+	md_update_sb(mddev, 1);
+	err = buflen;
+
+out_unlock:
+	mddev_unlock(mddev);
+	return err;
+}
+
+static struct md_sysfs_entry md_set_name =
+__ATTR(set_name, S_IRUGO | S_IWUSR, set_name_show, set_name_store);
+
 static struct attribute *md_default_attrs[] = {
 	&md_level.attr,
 	&md_layout.attr,
@@ -5067,6 +5145,7 @@ static struct attribute *md_default_attrs[] = {
 	&md_array_size.attr,
 	&max_corr_read_errors.attr,
 	&md_consistency_policy.attr,
+	&md_set_name.attr,
 	NULL,
 };
 
-- 
2.11.0

Re: [PATCH] md: allow changing set_name of running array

[NeilBrown]

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

On Wed, Aug 30 2017, Michał Mirosław wrote:

> Allow changing active array's set_name. This is the only way to
> safely update superblock on an array which carries a mounted fs.

Do you really need to change the set_name of an active array?

The name is only used when the array is actived, so wait until the next
time the array is stopped, and change the name then.

You can boot with a rescue CD or similar and use "--assemble
--update=name", or with a bit of effort you could get the normal boot
sequence to change the name.

I wouldn't object to adding something to mdadm so that it would read
something from mdadm.conf, and update the set name at boot time.

What is the underlying problem that you are trying to solve here?

Thanks,
NeilBrown


>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/md/md.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index caca5d689cdc..59aa10669bef 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5051,6 +5051,84 @@ static struct md_sysfs_entry md_consistency_policy =
>  __ATTR(consistency_policy, S_IRUGO | S_IWUSR, consistency_policy_show,
>         consistency_policy_store);
>  
> +static ssize_t
> +set_name_show(struct mddev *mddev, char *page)
> +{
> +	struct mdp_superblock_1 *sb;
> +	struct md_rdev *rdev;
> +	int err;
> +
> +	err = mddev_lock(mddev);
> +	if (err)
> +		return err;
> +
> +	/* only version-1 superblocks carry a MD set's name */
> +	err = -ENXIO;
> +	if (mddev->major_version != 1)
> +		goto out_unlock;
> +
> +	err = -EINVAL;
> +	rdev_for_each(rdev, mddev) {
> +		if (WARN_ON(!rdev->sb_loaded))
> +			continue;
> +
> +		sb = page_address(rdev->sb_page);
> +		err = sprintf(page, "%.32s\n", sb->set_name);
> +		break;
> +	}
> +
> +out_unlock:
> +	mddev_unlock(mddev);
> +	return err;
> +}
> +
> +static ssize_t
> +set_name_store(struct mddev *mddev, const char *buf, size_t buflen)
> +{
> +	struct mdp_superblock_1 *sb;
> +	struct md_rdev *rdev;
> +	size_t len = buflen;
> +	int err;
> +
> +	if (len && buf[len - 1] == '\n')
> +		--len;
> +
> +	if (len > sizeof(sb->set_name))
> +		return -ENOSPC;
> +
> +	err = mddev_lock(mddev);
> +	if (err)
> +		return err;
> +
> +	/* only version-1 superblocks carry a MD set's name */
> +	err = -ENXIO;
> +	if (mddev->major_version != 1)
> +		goto out_unlock;
> +
> +	err = -EROFS;
> +	if (mddev->ro)
> +		goto out_unlock;
> +
> +	rdev_for_each(rdev, mddev) {
> +		if (WARN_ON(!rdev->sb_loaded))
> +			continue;
> +
> +		sb = page_address(rdev->sb_page);
> +		memcpy(sb->set_name, buf, len);
> +		memset(&sb->set_name[len], 0, sizeof(sb->set_name) - len);
> +	}
> +
> +	md_update_sb(mddev, 1);
> +	err = buflen;
> +
> +out_unlock:
> +	mddev_unlock(mddev);
> +	return err;
> +}
> +
> +static struct md_sysfs_entry md_set_name =
> +__ATTR(set_name, S_IRUGO | S_IWUSR, set_name_show, set_name_store);
> +
>  static struct attribute *md_default_attrs[] = {
>  	&md_level.attr,
>  	&md_layout.attr,
> @@ -5067,6 +5145,7 @@ static struct attribute *md_default_attrs[] = {
>  	&md_array_size.attr,
>  	&max_corr_read_errors.attr,
>  	&md_consistency_policy.attr,
> +	&md_set_name.attr,
>  	NULL,
>  };
>  
> -- 
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] md: allow changing set_name of running array

[Michał Mirosław]

On Fri, Sep 01, 2017 at 10:07:29AM +1000, NeilBrown wrote:
> On Wed, Aug 30 2017, Michał Mirosław wrote:
> > Allow changing active array's set_name. This is the only way to
> > safely update superblock on an array which carries a mounted fs.
> 
> Do you really need to change the set_name of an active array?
> 
> The name is only used when the array is actived, so wait until the next
> time the array is stopped, and change the name then.
> 
> You can boot with a rescue CD or similar and use "--assemble
> --update=name", or with a bit of effort you could get the normal boot
> sequence to change the name.
> 
> I wouldn't object to adding something to mdadm so that it would read
> something from mdadm.conf, and update the set name at boot time.
> 
> What is the underlying problem that you are trying to solve here?

I had to fix /dev/md* naming on a system with no physical access.

The problem was that despite matching mdadm.conf entries, arrays started
with random 127-i indexes (because superblocks' set_names didn't match
hostname, I guess).

Best Regards,
Michał Mirosław

Re: [PATCH] md: allow changing set_name of running array

[NeilBrown]

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

On Fri, Sep 01 2017, Michał Mirosław wrote:

> On Fri, Sep 01, 2017 at 10:07:29AM +1000, NeilBrown wrote:
>> On Wed, Aug 30 2017, Michał Mirosław wrote:
>> > Allow changing active array's set_name. This is the only way to
>> > safely update superblock on an array which carries a mounted fs.
>> 
>> Do you really need to change the set_name of an active array?
>> 
>> The name is only used when the array is actived, so wait until the next
>> time the array is stopped, and change the name then.
>> 
>> You can boot with a rescue CD or similar and use "--assemble
>> --update=name", or with a bit of effort you could get the normal boot
>> sequence to change the name.
>> 
>> I wouldn't object to adding something to mdadm so that it would read
>> something from mdadm.conf, and update the set name at boot time.
>> 
>> What is the underlying problem that you are trying to solve here?
>
> I had to fix /dev/md* naming on a system with no physical access.
>
> The problem was that despite matching mdadm.conf entries, arrays started
> with random 127-i indexes (because superblocks' set_names didn't match
> hostname, I guess).

That's odd.  If an array is listed in mdadm.conf, that is enough to tell
mdadm that it is "local" so that it doesn't need the hostname to match.
Can you show me exactly what was in your mdadm.conf?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] md: allow changing set_name of running array

[Michał Mirosław]

On Mon, Sep 04, 2017 at 12:22:33PM +1000, NeilBrown wrote:
> On Fri, Sep 01 2017, Michał Mirosław wrote:
> 
> > On Fri, Sep 01, 2017 at 10:07:29AM +1000, NeilBrown wrote:
> >> On Wed, Aug 30 2017, Michał Mirosław wrote:
> >> > Allow changing active array's set_name. This is the only way to
> >> > safely update superblock on an array which carries a mounted fs.
> >> 
> >> Do you really need to change the set_name of an active array?
> >> 
> >> The name is only used when the array is actived, so wait until the next
> >> time the array is stopped, and change the name then.
> >> 
> >> You can boot with a rescue CD or similar and use "--assemble
> >> --update=name", or with a bit of effort you could get the normal boot
> >> sequence to change the name.
> >> 
> >> I wouldn't object to adding something to mdadm so that it would read
> >> something from mdadm.conf, and update the set name at boot time.
> >> 
> >> What is the underlying problem that you are trying to solve here?
> >
> > I had to fix /dev/md* naming on a system with no physical access.
> >
> > The problem was that despite matching mdadm.conf entries, arrays started
> > with random 127-i indexes (because superblocks' set_names didn't match
> > hostname, I guess).
> 
> That's odd.  If an array is listed in mdadm.conf, that is enough to tell
> mdadm that it is "local" so that it doesn't need the hostname to match.
> Can you show me exactly what was in your mdadm.conf?

This is what was I had when it would reassign arrays on boot. System
hostname is 'rere' - it was renamed some time after creating arrays.

---8<---
HOMEHOST <system>
MAILADDR root
ARRAY /dev/md/0  metadata=1.2 UUID=d5ea39a9:5ec27c36:c83296d4:70f879d4 name=rere2:0
ARRAY /dev/md/1  metadata=1.2 UUID=9bf69b92:2e7ba905:5f959c09:c8fecb99 name=rere2:1
---8<---

I don't have old versions of mdadm.conf, so had to recreate this one.
IIRC I verified UUIDs back then, but don't remember whether I modified
names or not. Thinking of it, how would mdadm behave if it had matching
UUIDs and mismatching array names in mdadm.conf?

Best Regards,
Michał Mirosław

Re: [PATCH] md: allow changing set_name of running array

[NeilBrown]

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

On Mon, Sep 04 2017, Michał Mirosław wrote:

> On Mon, Sep 04, 2017 at 12:22:33PM +1000, NeilBrown wrote:
>> On Fri, Sep 01 2017, Michał Mirosław wrote:
>> 
>> > On Fri, Sep 01, 2017 at 10:07:29AM +1000, NeilBrown wrote:
>> >> On Wed, Aug 30 2017, Michał Mirosław wrote:
>> >> > Allow changing active array's set_name. This is the only way to
>> >> > safely update superblock on an array which carries a mounted fs.
>> >> 
>> >> Do you really need to change the set_name of an active array?
>> >> 
>> >> The name is only used when the array is actived, so wait until the next
>> >> time the array is stopped, and change the name then.
>> >> 
>> >> You can boot with a rescue CD or similar and use "--assemble
>> >> --update=name", or with a bit of effort you could get the normal boot
>> >> sequence to change the name.
>> >> 
>> >> I wouldn't object to adding something to mdadm so that it would read
>> >> something from mdadm.conf, and update the set name at boot time.
>> >> 
>> >> What is the underlying problem that you are trying to solve here?
>> >
>> > I had to fix /dev/md* naming on a system with no physical access.
>> >
>> > The problem was that despite matching mdadm.conf entries, arrays started
>> > with random 127-i indexes (because superblocks' set_names didn't match
>> > hostname, I guess).
>> 
>> That's odd.  If an array is listed in mdadm.conf, that is enough to tell
>> mdadm that it is "local" so that it doesn't need the hostname to match.
>> Can you show me exactly what was in your mdadm.conf?
>
> This is what was I had when it would reassign arrays on boot. System
> hostname is 'rere' - it was renamed some time after creating arrays.
>
> ---8<---
> HOMEHOST <system>
> MAILADDR root
> ARRAY /dev/md/0  metadata=1.2 UUID=d5ea39a9:5ec27c36:c83296d4:70f879d4 name=rere2:0
> ARRAY /dev/md/1  metadata=1.2 UUID=9bf69b92:2e7ba905:5f959c09:c8fecb99 name=rere2:1
> ---8<---
>
> I don't have old versions of mdadm.conf, so had to recreate this one.
> IIRC I verified UUIDs back then, but don't remember whether I modified
> names or not. Thinking of it, how would mdadm behave if it had matching
> UUIDs and mismatching array names in mdadm.conf?

That's what I was wondering and part of why I asked to see the
mdadm.conf.

If a name is given in mdadm.conf and it doesn't match the name in the
metadata, that line will be ignored.  If mdadm doesn't find a match in
mdadm.conf and the hostname doesn't match, the the array will be treated
as "foreign" and will be assembled as e.g. /dev/md127 with a symlink
from /dev/md/0_1.
If the name and uuid in mdadm.conf do match the metadata, it will be
treat as "local" and will be given the name that you would expect
(/dev/md0, /dev/md/0).

So my guess is that the mdadm.conf you had at the time doesn't match
what you have provided above.

It would be nice to add an option to "mdadm --incremental" to tell it to
update various details like you can with "mdadm --assemble"....

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] md: allow changing set_name of running array

[Phil Turmel]

On 09/04/2017 09:00 PM, NeilBrown wrote:
> If a name is given in mdadm.conf and it doesn't match the name in the
> metadata, that line will be ignored.  If mdadm doesn't find a match in
> mdadm.conf and the hostname doesn't match, the the array will be treated
> as "foreign" and will be assembled as e.g. /dev/md127 with a symlink
> from /dev/md/0_1.
> If the name and uuid in mdadm.conf do match the metadata, it will be
> treat as "local" and will be given the name that you would expect
> (/dev/md0, /dev/md/0).
> 
> So my guess is that the mdadm.conf you had at the time doesn't match
> what you have provided above.
> 
> It would be nice to add an option to "mdadm --incremental" to tell it to
> update various details like you can with "mdadm --assemble"....

I just delete the name and metadata version from mdadm.conf.  The above
would become:

ARRAY /dev/md/0 UUID=d5ea39a9:5ec27c36:c83296d4:70f879d4
ARRAY /dev/md/1 UUID=9bf69b92:2e7ba905:5f959c09:c8fecb99

The default output of mdadm -Es is overspecified for use as mdadm.conf,
though it works on precisely the setup that produced it.

Phil