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@xxxxxxxxxx> >>>> --- >>>> 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
