Linux-Raid Archives on lore.kernel.org
 help / color / Atom feed
* [PATCH V2 0/2] Some fixes for mdadm
@ 2020-09-15  7:44 Xiao Ni
  2020-09-15  7:44 ` [PATCH V2 1/2] Check hostname file empty or not when creating raid device Xiao Ni
  2020-09-15  7:44 ` [PATCH V2 2/2] Don't create bitmap for raid5 with journal disk Xiao Ni
  0 siblings, 2 replies; 7+ messages in thread
From: Xiao Ni @ 2020-09-15  7:44 UTC (permalink / raw)
  To: jes, linux-raid; +Cc: colyli, ncroxon, antmbox

Hi all

These two patches fix some mdadm problems. The first is to avoid one raid being
not active after boot. The second patch is to check journal device when creating
bitmap.

v2:Change patch1's comments to make it more clear

Xiao Ni (2):
  Check hostname file empty or not when creating raid device
  Don't create bitmap for raid5 with journal disk

 Create.c |  1 +
 mdadm.c  |  3 +++
 mdadm.h  |  1 +
 util.c   | 19 +++++++++++++++++++
 4 files changed, 24 insertions(+)

-- 
2.7.5


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH V2 1/2] Check hostname file empty or not when creating raid device
  2020-09-15  7:44 [PATCH V2 0/2] Some fixes for mdadm Xiao Ni
@ 2020-09-15  7:44 ` Xiao Ni
  2020-09-15 15:27   ` Andrey Jr. Melnikov
  2020-10-14 15:22   ` Jes Sorensen
  2020-09-15  7:44 ` [PATCH V2 2/2] Don't create bitmap for raid5 with journal disk Xiao Ni
  1 sibling, 2 replies; 7+ messages in thread
From: Xiao Ni @ 2020-09-15  7:44 UTC (permalink / raw)
  To: jes, linux-raid; +Cc: colyli, ncroxon, antmbox

If /etc/hostname is empty and the hostname is decided by network(dhcp, e.g.), there is a
risk that raid device will not be in active state after boot. It will be auto-read-only
state. It depends on the boot sequence. If the storage starts before network. The system
detects disks first, udev rules are triggered and raid device is assemble automatically.
But the network hasn't started successfully. So mdadm can't get the right hostname. The
raid device will be treated as a foreign raid.
Add a note message if /etc/hostname is empty when creating a raid device.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 mdadm.c |  3 +++
 mdadm.h |  1 +
 util.c  | 19 +++++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/mdadm.c b/mdadm.c
index 1b3467f..e551958 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1408,6 +1408,9 @@ int main(int argc, char *argv[])
 	if (c.homehost == NULL && c.require_homehost)
 		c.homehost = conf_get_homehost(&c.require_homehost);
 	if (c.homehost == NULL || strcasecmp(c.homehost, "<system>") == 0) {
+		if (check_hostname())
+			pr_err("Note: The file /etc/hostname is empty. There is a risk the raid\n"
+				"      can't be active after boot\n");
 		if (gethostname(sys_hostname, sizeof(sys_hostname)) == 0) {
 			sys_hostname[sizeof(sys_hostname)-1] = 0;
 			c.homehost = sys_hostname;
diff --git a/mdadm.h b/mdadm.h
index 399478b..3ef1209 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1480,6 +1480,7 @@ extern int parse_cluster_confirm_arg(char *inp, char **devname, int *slot);
 extern int check_ext2(int fd, char *name);
 extern int check_reiser(int fd, char *name);
 extern int check_raid(int fd, char *name);
+extern int check_hostname(void);
 extern int check_partitions(int fd, char *dname,
 			    unsigned long long freesize,
 			    unsigned long long size);
diff --git a/util.c b/util.c
index 579dd42..de5bad0 100644
--- a/util.c
+++ b/util.c
@@ -694,6 +694,25 @@ int check_raid(int fd, char *name)
 	return 1;
 }
 
+/* It checks /etc/hostname has value or not */
+int check_hostname()
+{
+	int fd, ret = 0;
+	char buf[256];
+
+	fd = open("/etc/hostname", O_RDONLY);
+	if (fd < 0) {
+		ret = 1;
+		goto out;
+	}
+
+	if (read(fd, buf, sizeof(buf)) == 0)
+		ret = 1;
+out:
+	close(fd);
+	return ret;
+}
+
 int fstat_is_blkdev(int fd, char *devname, dev_t *rdev)
 {
 	struct stat stb;
-- 
2.7.5


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH V2 2/2] Don't create bitmap for raid5 with journal disk
  2020-09-15  7:44 [PATCH V2 0/2] Some fixes for mdadm Xiao Ni
  2020-09-15  7:44 ` [PATCH V2 1/2] Check hostname file empty or not when creating raid device Xiao Ni
@ 2020-09-15  7:44 ` Xiao Ni
  2020-10-14 15:25   ` Jes Sorensen
  1 sibling, 1 reply; 7+ messages in thread
From: Xiao Ni @ 2020-09-15  7:44 UTC (permalink / raw)
  To: jes, linux-raid; +Cc: colyli, ncroxon, antmbox

Journal disk and bitmap can't exist at the same time. It needs to check if the raid
has a journal disk when creating bitmap.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Create.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Create.c b/Create.c
index 6f84e5b..0efa19c 100644
--- a/Create.c
+++ b/Create.c
@@ -542,6 +542,7 @@ int Create(struct supertype *st, char *mddev,
 	if (!s->bitmap_file &&
 	    s->level >= 1 &&
 	    st->ss->add_internal_bitmap &&
+	    s->journaldisks == 0 &&
 	    (s->consistency_policy != CONSISTENCY_POLICY_RESYNC &&
 	     s->consistency_policy != CONSISTENCY_POLICY_PPL) &&
 	    (s->write_behind || s->size > 100*1024*1024ULL)) {
-- 
2.7.5


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/2] Check hostname file empty or not when creating raid device
  2020-09-15  7:44 ` [PATCH V2 1/2] Check hostname file empty or not when creating raid device Xiao Ni
@ 2020-09-15 15:27   ` Andrey Jr. Melnikov
  2020-10-14 15:22   ` Jes Sorensen
  1 sibling, 0 replies; 7+ messages in thread
From: Andrey Jr. Melnikov @ 2020-09-15 15:27 UTC (permalink / raw)
  To: linux-raid

Xiao Ni <xni@redhat.com> wrote:
> If /etc/hostname is empty and the hostname is decided by network(dhcp, e.g.), there is a
> risk that raid device will not be in active state after boot. It will be auto-read-only
> state. It depends on the boot sequence. If the storage starts before network. The system
> detects disks first, udev rules are triggered and raid device is assemble automatically.
> But the network hasn't started successfully. So mdadm can't get the right hostname. The
> raid device will be treated as a foreign raid.
> Add a note message if /etc/hostname is empty when creating a raid device.

> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  mdadm.c |  3 +++
>  mdadm.h |  1 +
>  util.c  | 19 +++++++++++++++++++
>  3 files changed, 23 insertions(+)

> diff --git a/mdadm.c b/mdadm.c
> index 1b3467f..e551958 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1408,6 +1408,9 @@ int main(int argc, char *argv[])
>         if (c.homehost == NULL && c.require_homehost)
>                 c.homehost = conf_get_homehost(&c.require_homehost);
>         if (c.homehost == NULL || strcasecmp(c.homehost, "<system>") == 0) {
> +               if (check_hostname())
> +                       pr_err("Note: The file /etc/hostname is empty. There is a risk the raid\n"
> +                               "      can't be active after boot\n");
>                 if (gethostname(sys_hostname, sizeof(sys_hostname)) == 0) {
>                         sys_hostname[sizeof(sys_hostname)-1] = 0;
>                         c.homehost = sys_hostname;
> diff --git a/mdadm.h b/mdadm.h
> index 399478b..3ef1209 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1480,6 +1480,7 @@ extern int parse_cluster_confirm_arg(char *inp, char **devname, int *slot);
>  extern int check_ext2(int fd, char *name);
>  extern int check_reiser(int fd, char *name);
>  extern int check_raid(int fd, char *name);
> +extern int check_hostname(void);
>  extern int check_partitions(int fd, char *dname,
>                             unsigned long long freesize,
>                             unsigned long long size);
> diff --git a/util.c b/util.c
> index 579dd42..de5bad0 100644
> --- a/util.c
> +++ b/util.c
> @@ -694,6 +694,25 @@ int check_raid(int fd, char *name)
>         return 1;
>  }
>  
> +/* It checks /etc/hostname has value or not */
> +int check_hostname()
> +{
> +       int fd, ret = 0;
> +       char buf[256];
> +
> +       fd = open("/etc/hostname", O_RDONLY);
> +       if (fd < 0) {
> +               ret = 1;
> +               goto out;
> +       }
> +
> +       if (read(fd, buf, sizeof(buf)) == 0)
> +               ret = 1;

Why not use stat() here, since you don't use file content?
Also, any error from read() - mean file "not empty". This is right?

> +out:
> +       close(fd);
> +       return ret;
> +}
> +
>  int fstat_is_blkdev(int fd, char *devname, dev_t *rdev)
>  {
>         struct stat stb;


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/2] Check hostname file empty or not when creating raid device
  2020-09-15  7:44 ` [PATCH V2 1/2] Check hostname file empty or not when creating raid device Xiao Ni
  2020-09-15 15:27   ` Andrey Jr. Melnikov
@ 2020-10-14 15:22   ` Jes Sorensen
  2020-10-15 13:40     ` Xiao Ni
  1 sibling, 1 reply; 7+ messages in thread
From: Jes Sorensen @ 2020-10-14 15:22 UTC (permalink / raw)
  To: Xiao Ni, linux-raid; +Cc: colyli, ncroxon, antmbox

On 9/15/20 3:44 AM, Xiao Ni wrote:
> If /etc/hostname is empty and the hostname is decided by network(dhcp, e.g.), there is a
> risk that raid device will not be in active state after boot. It will be auto-read-only
> state. It depends on the boot sequence. If the storage starts before network. The system
> detects disks first, udev rules are triggered and raid device is assemble automatically.
> But the network hasn't started successfully. So mdadm can't get the right hostname. The
> raid device will be treated as a foreign raid.
> Add a note message if /etc/hostname is empty when creating a raid device.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  mdadm.c |  3 +++
>  mdadm.h |  1 +
>  util.c  | 19 +++++++++++++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/mdadm.c b/mdadm.c
> index 1b3467f..e551958 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1408,6 +1408,9 @@ int main(int argc, char *argv[])
>  	if (c.homehost == NULL && c.require_homehost)
>  		c.homehost = conf_get_homehost(&c.require_homehost);
>  	if (c.homehost == NULL || strcasecmp(c.homehost, "<system>") == 0) {
> +		if (check_hostname())
> +			pr_err("Note: The file /etc/hostname is empty. There is a risk the raid\n"
> +				"      can't be active after boot\n");
>  		if (gethostname(sys_hostname, sizeof(sys_hostname)) == 0) {
>  			sys_hostname[sizeof(sys_hostname)-1] = 0;
>  			c.homehost = sys_hostname;
> diff --git a/mdadm.h b/mdadm.h
> index 399478b..3ef1209 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1480,6 +1480,7 @@ extern int parse_cluster_confirm_arg(char *inp, char **devname, int *slot);
>  extern int check_ext2(int fd, char *name);
>  extern int check_reiser(int fd, char *name);
>  extern int check_raid(int fd, char *name);
> +extern int check_hostname(void);
>  extern int check_partitions(int fd, char *dname,
>  			    unsigned long long freesize,
>  			    unsigned long long size);
> diff --git a/util.c b/util.c
> index 579dd42..de5bad0 100644
> --- a/util.c
> +++ b/util.c
> @@ -694,6 +694,25 @@ int check_raid(int fd, char *name)
>  	return 1;
>  }
>  
> +/* It checks /etc/hostname has value or not */
> +int check_hostname()
> +{
> +	int fd, ret = 0;
> +	char buf[256];
> +
> +	fd = open("/etc/hostname", O_RDONLY);
> +	if (fd < 0) {
> +		ret = 1;
> +		goto out;
> +	}

I don't think this is the right approach. If someone uses dhcp to obtain
the hostname and explicitly configured the raid to start after the
network, they shouldn't get nagged by this message.

Any reason you cannot use gethostname(2) for this?

Thanks,
Jes


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 2/2] Don't create bitmap for raid5 with journal disk
  2020-09-15  7:44 ` [PATCH V2 2/2] Don't create bitmap for raid5 with journal disk Xiao Ni
@ 2020-10-14 15:25   ` Jes Sorensen
  0 siblings, 0 replies; 7+ messages in thread
From: Jes Sorensen @ 2020-10-14 15:25 UTC (permalink / raw)
  To: Xiao Ni, linux-raid; +Cc: colyli, ncroxon, antmbox

On 9/15/20 3:44 AM, Xiao Ni wrote:
> Journal disk and bitmap can't exist at the same time. It needs to check if the raid
> has a journal disk when creating bitmap.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Create.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Create.c b/Create.c
> index 6f84e5b..0efa19c 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -542,6 +542,7 @@ int Create(struct supertype *st, char *mddev,
>  	if (!s->bitmap_file &&
>  	    s->level >= 1 &&
>  	    st->ss->add_internal_bitmap &&
> +	    s->journaldisks == 0 &&
>  	    (s->consistency_policy != CONSISTENCY_POLICY_RESYNC &&
>  	     s->consistency_policy != CONSISTENCY_POLICY_PPL) &&
>  	    (s->write_behind || s->size > 100*1024*1024ULL)) {
> 

Applied!

That said, I'd love if we could do something to get rid of some of these
very excessive if statements. It is extremely difficult to verify all
cases are correct.

Thanks,
Jes

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/2] Check hostname file empty or not when creating raid device
  2020-10-14 15:22   ` Jes Sorensen
@ 2020-10-15 13:40     ` Xiao Ni
  0 siblings, 0 replies; 7+ messages in thread
From: Xiao Ni @ 2020-10-15 13:40 UTC (permalink / raw)
  To: Jes Sorensen, linux-raid; +Cc: colyli, ncroxon, antmbox



On 10/14/2020 11:22 PM, Jes Sorensen wrote:
> On 9/15/20 3:44 AM, Xiao Ni wrote:
>> If /etc/hostname is empty and the hostname is decided by network(dhcp, e.g.), there is a
>> risk that raid device will not be in active state after boot. It will be auto-read-only
>> state. It depends on the boot sequence. If the storage starts before network. The system
>> detects disks first, udev rules are triggered and raid device is assemble automatically.
>> But the network hasn't started successfully. So mdadm can't get the right hostname. The
>> raid device will be treated as a foreign raid.
>> Add a note message if /etc/hostname is empty when creating a raid device.
>>
>> Signed-off-by: Xiao Ni <xni@redhat.com>
>> ---
>>   mdadm.c |  3 +++
>>   mdadm.h |  1 +
>>   util.c  | 19 +++++++++++++++++++
>>   3 files changed, 23 insertions(+)
>>
>> diff --git a/mdadm.c b/mdadm.c
>> index 1b3467f..e551958 100644
>> --- a/mdadm.c
>> +++ b/mdadm.c
>> @@ -1408,6 +1408,9 @@ int main(int argc, char *argv[])
>>   	if (c.homehost == NULL && c.require_homehost)
>>   		c.homehost = conf_get_homehost(&c.require_homehost);
>>   	if (c.homehost == NULL || strcasecmp(c.homehost, "<system>") == 0) {
>> +		if (check_hostname())
>> +			pr_err("Note: The file /etc/hostname is empty. There is a risk the raid\n"
>> +				"      can't be active after boot\n");
>>   		if (gethostname(sys_hostname, sizeof(sys_hostname)) == 0) {
>>   			sys_hostname[sizeof(sys_hostname)-1] = 0;
>>   			c.homehost = sys_hostname;
>> diff --git a/mdadm.h b/mdadm.h
>> index 399478b..3ef1209 100644
>> --- a/mdadm.h
>> +++ b/mdadm.h
>> @@ -1480,6 +1480,7 @@ extern int parse_cluster_confirm_arg(char *inp, char **devname, int *slot);
>>   extern int check_ext2(int fd, char *name);
>>   extern int check_reiser(int fd, char *name);
>>   extern int check_raid(int fd, char *name);
>> +extern int check_hostname(void);
>>   extern int check_partitions(int fd, char *dname,
>>   			    unsigned long long freesize,
>>   			    unsigned long long size);
>> diff --git a/util.c b/util.c
>> index 579dd42..de5bad0 100644
>> --- a/util.c
>> +++ b/util.c
>> @@ -694,6 +694,25 @@ int check_raid(int fd, char *name)
>>   	return 1;
>>   }
>>   
>> +/* It checks /etc/hostname has value or not */
>> +int check_hostname()
>> +{
>> +	int fd, ret = 0;
>> +	char buf[256];
>> +
>> +	fd = open("/etc/hostname", O_RDONLY);
>> +	if (fd < 0) {
>> +		ret = 1;
>> +		goto out;
>> +	}
> I don't think this is the right approach. If someone uses dhcp to obtain
> the hostname and explicitly configured the raid to start after the
> network, they shouldn't get nagged by this message.
>
> Any reason you cannot use gethostname(2) for this?
Hi Jes

If the raid device is used as root device and the network service needs some
files that on the raid device, so the raid device need to be assemble 
first. The
system call gethostname can't return the right hostname in this case.

But the patch has some problem. If the /etc/mdadm.conf has the raid 
information already,
it can assemble the raid successfully without considering hostname. I'll 
try to send another patch.

Regards
Xiao


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15  7:44 [PATCH V2 0/2] Some fixes for mdadm Xiao Ni
2020-09-15  7:44 ` [PATCH V2 1/2] Check hostname file empty or not when creating raid device Xiao Ni
2020-09-15 15:27   ` Andrey Jr. Melnikov
2020-10-14 15:22   ` Jes Sorensen
2020-10-15 13:40     ` Xiao Ni
2020-09-15  7:44 ` [PATCH V2 2/2] Don't create bitmap for raid5 with journal disk Xiao Ni
2020-10-14 15:25   ` Jes Sorensen

Linux-Raid Archives on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-raid/0 linux-raid/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-raid linux-raid/ https://lore.kernel.org/linux-raid \
		linux-raid@vger.kernel.org
	public-inbox-index linux-raid

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-raid


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git