[PATCH V2] allow RAID5 to grow to RAID6 with a backup_file

[PATCH V2] allow RAID5 to grow to RAID6 with a backup_file

[Nigel Croxon]

This problem came in, as the user did not specify a full path with
the backup_file option when growing an RAID5 array to RAID6.
When the full path is specified, the symbolic link is created
properly (/run/mdadm/backup_file-mdX). But the code did not support
the symbolic link when looking for the backup_file. Added two
checks for symlink.

This addresses https://www.spinics.net/lists/raid/msg48910.html
and numerous customer reported problems.

V2:
- Removed unneeded break; in both case-statements
- Returned the error checking on call to lstat

Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
---
 Grow.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/Grow.c b/Grow.c
index 764374f..53245d7 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1135,6 +1135,15 @@ int reshape_open_backup_file(char *backup_file,
 	unsigned int dev;
 	int i;
 
+	if (lstat(backup_file, &stb) != -1) {
+		switch (stb.st_mode & S_IFMT) {
+		case S_IFLNK:
+			return 1;
+		default:
+			break;
+		}
+	}
+
 	*fdlist = open(backup_file, O_RDWR|O_CREAT|(restart ? O_TRUNC : O_EXCL),
 		       S_IRUSR | S_IWUSR);
 	*offsets = 8 * 512;
@@ -5236,8 +5245,16 @@ char *locate_backup(char *name)
 	char *fl = make_backup(name);
 	struct stat stb;
 
-	if (stat(fl, &stb) == 0 && S_ISREG(stb.st_mode))
-		return fl;
+	if (lstat(fl, &stb) == 0) {
+		switch (stb.st_mode & S_IFMT) {
+		case S_IFLNK:
+			return fl;
+		case S_IFREG:
+			return fl;
+		default:
+			break;
+		}
+	}
 
 	free(fl);
 	return NULL;
-- 
2.20.1

Re: [PATCH V2] allow RAID5 to grow to RAID6 with a backup_file

[Jes Sorensen]

On 5/5/20 2:35 PM, Nigel Croxon wrote:
> This problem came in, as the user did not specify a full path with
> the backup_file option when growing an RAID5 array to RAID6.
> When the full path is specified, the symbolic link is created
> properly (/run/mdadm/backup_file-mdX). But the code did not support
> the symbolic link when looking for the backup_file. Added two
> checks for symlink.
> 
> This addresses https://www.spinics.net/lists/raid/msg48910.html
> and numerous customer reported problems.
> 
> V2:
> - Removed unneeded break; in both case-statements
> - Returned the error checking on call to lstat
> 
> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
> ---
>  Grow.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 764374f..53245d7 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1135,6 +1135,15 @@ int reshape_open_backup_file(char *backup_file,
>  	unsigned int dev;
>  	int i;
>  
> +	if (lstat(backup_file, &stb) != -1) {
> +		switch (stb.st_mode & S_IFMT) {
> +		case S_IFLNK:
> +			return 1;
> +		default:
> +			break;
> +		}
> +	}
> +

Sorry for being a pita on this, but in this case you do the thing if the
lstat completes correctly, but what if it fails? In that case we should
error out rather than just continuing.

Jes

Re: [PATCH V2] allow RAID5 to grow to RAID6 with a backup_file

[Nigel Croxon]

On 5/8/20 10:01 AM, Jes Sorensen wrote:
> On 5/5/20 2:35 PM, Nigel Croxon wrote:
>> This problem came in, as the user did not specify a full path with
>> the backup_file option when growing an RAID5 array to RAID6.
>> When the full path is specified, the symbolic link is created
>> properly (/run/mdadm/backup_file-mdX). But the code did not support
>> the symbolic link when looking for the backup_file. Added two
>> checks for symlink.
>>
>> This addresses https://www.spinics.net/lists/raid/msg48910.html
>> and numerous customer reported problems.
>>
>> V2:
>> - Removed unneeded break; in both case-statements
>> - Returned the error checking on call to lstat
>>
>> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
>> ---
>>   Grow.c | 21 +++++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index 764374f..53245d7 100644
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -1135,6 +1135,15 @@ int reshape_open_backup_file(char *backup_file,
>>   	unsigned int dev;
>>   	int i;
>>   
>> +	if (lstat(backup_file, &stb) != -1) {
>> +		switch (stb.st_mode & S_IFMT) {
>> +		case S_IFLNK:
>> +			return 1;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
> Sorry for being a pita on this, but in this case you do the thing if the
> lstat completes correctly, but what if it fails? In that case we should
> error out rather than just continuing.
>
> Jes
>
There are two processes running in the reshape_array routine, the 
original, where the user entered the mdadm grow parameter. And the 
second, forked to call systemd mdadm-grow-continue@service.  It's in the 
original call we want the code to continue rather than fail. It needs to 
create the backup_file in the FS before the systemd thread runs.

-Nigel

Re: [PATCH V2] allow RAID5 to grow to RAID6 with a backup_file

[Nigel Croxon]

On 5/8/20 10:50 AM, Nigel Croxon wrote:
>
> On 5/8/20 10:01 AM, Jes Sorensen wrote:
>> On 5/5/20 2:35 PM, Nigel Croxon wrote:
>>> This problem came in, as the user did not specify a full path with
>>> the backup_file option when growing an RAID5 array to RAID6.
>>> When the full path is specified, the symbolic link is created
>>> properly (/run/mdadm/backup_file-mdX). But the code did not support
>>> the symbolic link when looking for the backup_file. Added two
>>> checks for symlink.
>>>
>>> This addresses https://www.spinics.net/lists/raid/msg48910.html
>>> and numerous customer reported problems.
>>>
>>> V2:
>>> - Removed unneeded break; in both case-statements
>>> - Returned the error checking on call to lstat
>>>
>>> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
>>> ---
>>>   Grow.c | 21 +++++++++++++++++++--
>>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Grow.c b/Grow.c
>>> index 764374f..53245d7 100644
>>> --- a/Grow.c
>>> +++ b/Grow.c
>>> @@ -1135,6 +1135,15 @@ int reshape_open_backup_file(char *backup_file,
>>>       unsigned int dev;
>>>       int i;
>>>   +    if (lstat(backup_file, &stb) != -1) {
>>> +        switch (stb.st_mode & S_IFMT) {
>>> +        case S_IFLNK:
>>> +            return 1;
>>> +        default:
>>> +            break;
>>> +        }
>>> +    }
>>> +
>> Sorry for being a pita on this, but in this case you do the thing if the
>> lstat completes correctly, but what if it fails? In that case we should
>> error out rather than just continuing.
>>
>> Jes
>>
> There are two processes running in the reshape_array routine, the 
> original, where the user entered the mdadm grow parameter. And the 
> second, forked to call systemd mdadm-grow-continue@service.  It's in 
> the original call we want the code to continue rather than fail. It 
> needs to create the backup_file in the FS before the systemd thread runs.
>
> -Nigel
>
Jes,

Do you have more questions?  an alternate solution?

-Nigel

Re: [PATCH V2] allow RAID5 to grow to RAID6 with a backup_file

[Jes Sorensen]

On 5/12/20 12:26 PM, Nigel Croxon wrote:
> On 5/8/20 10:50 AM, Nigel Croxon wrote:
>>
>> On 5/8/20 10:01 AM, Jes Sorensen wrote:
>>> On 5/5/20 2:35 PM, Nigel Croxon wrote:
>>>> This problem came in, as the user did not specify a full path with
>>>> the backup_file option when growing an RAID5 array to RAID6.
>>>> When the full path is specified, the symbolic link is created
>>>> properly (/run/mdadm/backup_file-mdX). But the code did not support
>>>> the symbolic link when looking for the backup_file. Added two
>>>> checks for symlink.
>>>>
>>>> This addresses https://www.spinics.net/lists/raid/msg48910.html
>>>> and numerous customer reported problems.
>>>>
>>>> V2:
>>>> - Removed unneeded break; in both case-statements
>>>> - Returned the error checking on call to lstat
>>>>
>>>> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
>>>> ---
>>>>   Grow.c | 21 +++++++++++++++++++--
>>>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Grow.c b/Grow.c
>>>> index 764374f..53245d7 100644
>>>> --- a/Grow.c
>>>> +++ b/Grow.c
>>>> @@ -1135,6 +1135,15 @@ int reshape_open_backup_file(char *backup_file,
>>>>       unsigned int dev;
>>>>       int i;
>>>>   +    if (lstat(backup_file, &stb) != -1) {
>>>> +        switch (stb.st_mode & S_IFMT) {
>>>> +        case S_IFLNK:
>>>> +            return 1;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>> Sorry for being a pita on this, but in this case you do the thing if the
>>> lstat completes correctly, but what if it fails? In that case we should
>>> error out rather than just continuing.
>>>
>>> Jes
>>>
>> There are two processes running in the reshape_array routine, the
>> original, where the user entered the mdadm grow parameter. And the
>> second, forked to call systemd mdadm-grow-continue@service.  It's in
>> the original call we want the code to continue rather than fail. It
>> needs to create the backup_file in the FS before the systemd thread runs.
>>
>> -Nigel
>>
> Jes,
> 
> Do you have more questions?  an alternate solution?

I don't like ignoring error codes like this. As a bare minimum it should
be documented, or we should call in with an argument to decide whether
or not to stat the file in the first place.

Cheers,
Jes