* [PATCH - mdadm] mdopen: always try create_named_array() @ 2023-03-14 0:06 NeilBrown 2023-03-14 9:11 ` Mariusz Tkaczyk ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: NeilBrown @ 2023-03-14 0:06 UTC (permalink / raw) To: Jes Sorensen Cc: Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov mdopen() will use create_named_array() to ask the kernel to create the given md array, but only if it is given a number or name. If it is NOT given a name and is required to choose one itself using find_free_devnm() it does NOT use create_named_array(). On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in failure to assemble an array. This can particularly seen when the "name" of the array begins with a host name different to the name of the host running the command. So add the missing call to create_named_array(). Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 Signed-off-by: NeilBrown <neilb@suse.de> --- mdopen.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mdopen.c b/mdopen.c index d18c931996d2..810f79a3d19a 100644 --- a/mdopen.c +++ b/mdopen.c @@ -370,6 +370,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, } if (block_udev) udev_block(devnm); + create_named_array(devnm); } sprintf(devname, "/dev/%s", devnm); -- 2.39.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-14 0:06 [PATCH - mdadm] mdopen: always try create_named_array() NeilBrown @ 2023-03-14 9:11 ` Mariusz Tkaczyk 2023-03-19 16:36 ` Jes Sorensen 2023-03-22 3:00 ` Xiao Ni 2 siblings, 0 replies; 14+ messages in thread From: Mariusz Tkaczyk @ 2023-03-14 9:11 UTC (permalink / raw) To: NeilBrown Cc: Jes Sorensen, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Tue, 14 Mar 2023 11:06:25 +1100 "NeilBrown" <neilb@suse.de> wrote: > mdopen() will use create_named_array() to ask the kernel to create the > given md array, but only if it is given a number or name. > If it is NOT given a name and is required to choose one itself using > find_free_devnm() it does NOT use create_named_array(). > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > failure to assemble an array. This can particularly seen when the > "name" of the array begins with a host name different to the name of the > host running the command. > > So add the missing call to create_named_array(). > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 > Signed-off-by: NeilBrown <neilb@suse.de> > --- > mdopen.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mdopen.c b/mdopen.c > index d18c931996d2..810f79a3d19a 100644 > --- a/mdopen.c > +++ b/mdopen.c > @@ -370,6 +370,7 @@ int create_mddev(char *dev, char *name, int autof, int > trustworthy, } > if (block_udev) > udev_block(devnm); > + create_named_array(devnm); > } > > sprintf(devname, "/dev/%s", devnm); Hi Neil, Thanks for handling that. I didn't have time to get into this topic yet. I trust you and I believe that it is a good fix but I cannot verify it by standard code review- the code is to complex. Could you please add test for it to prove it works this way at least for following cases? - mdadm -CR /dev/md/name - mdadm -CR name - mdadm -CR /dev/md_name - mdadm -CR /dev/mdX, where X is a number. You can disable creation via open temporarily by changing value in /sys/module/md_mod/parameters/new_array that should be enough for verification. For code: Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> Thanks, Mariusz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-14 0:06 [PATCH - mdadm] mdopen: always try create_named_array() NeilBrown 2023-03-14 9:11 ` Mariusz Tkaczyk @ 2023-03-19 16:36 ` Jes Sorensen 2023-03-19 22:09 ` NeilBrown 2023-03-22 3:00 ` Xiao Ni 2 siblings, 1 reply; 14+ messages in thread From: Jes Sorensen @ 2023-03-19 16:36 UTC (permalink / raw) To: NeilBrown Cc: Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On 3/13/23 20:06, NeilBrown wrote: > > mdopen() will use create_named_array() to ask the kernel to create the > given md array, but only if it is given a number or name. > If it is NOT given a name and is required to choose one itself using > find_free_devnm() it does NOT use create_named_array(). > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > failure to assemble an array. This can particularly seen when the > "name" of the array begins with a host name different to the name of the > host running the command. > > So add the missing call to create_named_array(). > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 > Signed-off-by: NeilBrown <neilb@suse.de> Applied! Thanks, Jes ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-19 16:36 ` Jes Sorensen @ 2023-03-19 22:09 ` NeilBrown 2023-03-20 17:15 ` Jes Sorensen 0 siblings, 1 reply; 14+ messages in thread From: NeilBrown @ 2023-03-19 22:09 UTC (permalink / raw) To: Jes Sorensen Cc: Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Mon, 20 Mar 2023, Jes Sorensen wrote: > On 3/13/23 20:06, NeilBrown wrote: > > > > mdopen() will use create_named_array() to ask the kernel to create the > > given md array, but only if it is given a number or name. > > If it is NOT given a name and is required to choose one itself using > > find_free_devnm() it does NOT use create_named_array(). > > > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > > failure to assemble an array. This can particularly seen when the > > "name" of the array begins with a host name different to the name of the > > host running the command. > > > > So add the missing call to create_named_array(). > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 > > Signed-off-by: NeilBrown <neilb@suse.de> > > Applied! Thanks. Do you have plans for releasing 4.3? I'd like this patch to be in a numbered release for at least a few months before we change the kernel Kconfig to allow md to be built without CONFIG_BLOCK_LEGACY_AUTOLOAD. Thanks, NeilBrown ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-19 22:09 ` NeilBrown @ 2023-03-20 17:15 ` Jes Sorensen 2023-03-21 8:49 ` Mariusz Tkaczyk 0 siblings, 1 reply; 14+ messages in thread From: Jes Sorensen @ 2023-03-20 17:15 UTC (permalink / raw) To: NeilBrown Cc: Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On 3/19/23 18:09, NeilBrown wrote: > On Mon, 20 Mar 2023, Jes Sorensen wrote: >> On 3/13/23 20:06, NeilBrown wrote: >>> >>> mdopen() will use create_named_array() to ask the kernel to create the >>> given md array, but only if it is given a number or name. >>> If it is NOT given a name and is required to choose one itself using >>> find_free_devnm() it does NOT use create_named_array(). >>> >>> On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in >>> failure to assemble an array. This can particularly seen when the >>> "name" of the array begins with a host name different to the name of the >>> host running the command. >>> >>> So add the missing call to create_named_array(). >>> >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 >>> Signed-off-by: NeilBrown <neilb@suse.de> >> >> Applied! > > Thanks. > > Do you have plans for releasing 4.3? I'd like this patch to be in a > numbered release for at least a few months before we change the kernel > Kconfig to allow md to be built without CONFIG_BLOCK_LEGACY_AUTOLOAD. No immediate plans, but no reason why we shouldn't do it. I think Mariusz has some pending changes he wants to get in as well, like the error number stuff. So lets make this a call for submitting changes for mdadm that are needed for the next release. Thanks, Jes ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-20 17:15 ` Jes Sorensen @ 2023-03-21 8:49 ` Mariusz Tkaczyk 0 siblings, 0 replies; 14+ messages in thread From: Mariusz Tkaczyk @ 2023-03-21 8:49 UTC (permalink / raw) To: Jes Sorensen Cc: NeilBrown, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Mon, 20 Mar 2023 13:15:35 -0400 Jes Sorensen <jes@trained-monkey.org> wrote: > On 3/19/23 18:09, NeilBrown wrote: > > On Mon, 20 Mar 2023, Jes Sorensen wrote: > >> On 3/13/23 20:06, NeilBrown wrote: > >>> > >>> mdopen() will use create_named_array() to ask the kernel to create the > >>> given md array, but only if it is given a number or name. > >>> If it is NOT given a name and is required to choose one itself using > >>> find_free_devnm() it does NOT use create_named_array(). > >>> > >>> On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > >>> failure to assemble an array. This can particularly seen when the > >>> "name" of the array begins with a host name different to the name of the > >>> host running the command. > >>> > >>> So add the missing call to create_named_array(). > >>> > >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 > >>> Signed-off-by: NeilBrown <neilb@suse.de> > >> > >> Applied! > > > > Thanks. > > > > Do you have plans for releasing 4.3? I'd like this patch to be in a > > numbered release for at least a few months before we change the kernel > > Kconfig to allow md to be built without CONFIG_BLOCK_LEGACY_AUTOLOAD. > > No immediate plans, but no reason why we shouldn't do it. I think > Mariusz has some pending changes he wants to get in as well, like the > error number stuff. > > So lets make this a call for submitting changes for mdadm that are > needed for the next release. > Hi Jes, My patches around names (where I added those errors) will be too risky to be merged and released in short period. I'm going to limit the supported characters for names- it could bring regressions in some non obvious cases and I would like to give user time to accept it and adopt their solutions to new naming policy. Let me see if the patches recently merged doesn't bring any critical regression, and we are free to go I think :) I will back with a results soon. Thanks, Mariusz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-14 0:06 [PATCH - mdadm] mdopen: always try create_named_array() NeilBrown 2023-03-14 9:11 ` Mariusz Tkaczyk 2023-03-19 16:36 ` Jes Sorensen @ 2023-03-22 3:00 ` Xiao Ni 2023-03-22 3:24 ` NeilBrown 2 siblings, 1 reply; 14+ messages in thread From: Xiao Ni @ 2023-03-22 3:00 UTC (permalink / raw) To: NeilBrown Cc: Jes Sorensen, Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Tue, Mar 14, 2023 at 8:08 AM NeilBrown <neilb@suse.de> wrote: > > > mdopen() will use create_named_array() to ask the kernel to create the > given md array, but only if it is given a number or name. > If it is NOT given a name and is required to choose one itself using > find_free_devnm() it does NOT use create_named_array(). > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > failure to assemble an array. This can particularly seen when the > "name" of the array begins with a host name different to the name of the > host running the command. > > So add the missing call to create_named_array(). > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 Hi Neil I have two questions, hope you can help to understand the function create_mddev better. Frist, from the comment7 of the bug you mentioned: There are two different sorts names. Note that you almost acknowledged this by writing "name for my md device node" while the documentation only talks about names for "md devices", not for "md device nodes". There are 1/ there are names in /dev or /dev/md/ (device nodes) 2/ there are names that appear in /proc/mdstat and in /sys/block/ (devices) Thanks for the clarification. But it looks like it doesn't work like what you said. For example: mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc --name=test cat /proc/mdstat Personalities : [raid0] md127 : active raid0 sdc[1] sda[0] 3906764800 blocks super 1.2 512k chunks cd /sys/block/md127/md/ In /proc/mdstat and /sys/block, they all use md127 rather than the name(root) Before this patch, it creates a symbol link with the name root rather than test ll /dev/md/root lrwxrwxrwx. 1 root root 8 Mar 21 22:35 /dev/md/root -> ../md127 So "test" which is specified by --name looks like it has little usage. By the way, after this patch, the symbol link /dev/md/root can't be created anymore. Is it a regression problem? Second, are there possibilities that the arguments "dev" and "name" of function create_mddev are null at the same time? After some tests, I found dev can't be null when creating a raid device. It can be checked before calling create_mddev. And we must get a name after creating a raid device. So when assembling a raid device, the name must not be null. So the dev and name can't be null at the same time, right? Best Regards Xiao > Signed-off-by: NeilBrown <neilb@suse.de> > --- > mdopen.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mdopen.c b/mdopen.c > index d18c931996d2..810f79a3d19a 100644 > --- a/mdopen.c > +++ b/mdopen.c > @@ -370,6 +370,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, > } > if (block_udev) > udev_block(devnm); > + create_named_array(devnm); > } > > sprintf(devname, "/dev/%s", devnm); > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-22 3:00 ` Xiao Ni @ 2023-03-22 3:24 ` NeilBrown 2023-03-22 6:51 ` Mariusz Tkaczyk ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: NeilBrown @ 2023-03-22 3:24 UTC (permalink / raw) To: Xiao Ni Cc: Jes Sorensen, Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Wed, 22 Mar 2023, Xiao Ni wrote: > On Tue, Mar 14, 2023 at 8:08 AM NeilBrown <neilb@suse.de> wrote: > > > > > > mdopen() will use create_named_array() to ask the kernel to create the > > given md array, but only if it is given a number or name. > > If it is NOT given a name and is required to choose one itself using > > find_free_devnm() it does NOT use create_named_array(). > > > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > > failure to assemble an array. This can particularly seen when the > > "name" of the array begins with a host name different to the name of the > > host running the command. > > > > So add the missing call to create_named_array(). > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 > > Hi Neil > > I have two questions, hope you can help to understand the function > create_mddev better. > > Frist, from the comment7 of the bug you mentioned: > > There are two different sorts names. Note that you almost > acknowledged this by writing "name for my md device node" while the > documentation only talks about names for "md devices", not for "md > device nodes". > > There are > 1/ there are names in /dev or /dev/md/ (device nodes) > 2/ there are names that appear in /proc/mdstat and in /sys/block/ (devices) > > Thanks for the clarification. But it looks like it doesn't work like > what you said. > For example: > mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc --name=test > cat /proc/mdstat > Personalities : [raid0] > md127 : active raid0 sdc[1] sda[0] > 3906764800 blocks super 1.2 512k chunks > cd /sys/block/md127/md/ > > In /proc/mdstat and /sys/block, they all use md127 rather than the name(root) Try again with "CREATE names=yes" in /etc/mdadm.conf. mdadm generally tries to keep: - the names in /dev/ - the names in /dev/md/ - the names in /proc/mdstat - the names stored in the metadata in sync. It can only do this when: - you enabled "names=yes" - you don't confuse it by specifying a device name (/dev/md/root) that is different from the metadata names "test". If you don't have "names=yes" then the name in /proc/mdstat and the name in /dev/md* will be numeric. The name in /dev/md/ and the name in the metadata can be different and will usually be the same. If you explicitly give a different name with --name= than the device name then obviously they will be different. If you then stop the array and restart with "mdadm -As" or "mdadm -I /dev/sda; mdadm -I /dev/sdb" then mdadm will create a name in /dev/md/ that matches the name in the metadata. > > Before this patch, it creates a symbol link with the name root rather than test > ll /dev/md/root > lrwxrwxrwx. 1 root root 8 Mar 21 22:35 /dev/md/root -> ../md127 That is what you asked it to do. > > So "test" which is specified by --name looks like it has little usage. > It is stored in the metadata. You can see it in --examine output. If you reassemble the array without specifying a device name, it will use the name "test". > > By the way, after this patch, the symbol link /dev/md/root can't be > created anymore. > Is it a regression problem? I cannot reproduce any problem like that. Please provide a sequence of steps so that I can try to duplicate it. > > Second, are there possibilities that the arguments "dev" and "name" of > function create_mddev > are null at the same time? No. For Build or Create, dev is never NULL. For Assemble and Incremental, name is never NULL. > After some tests, I found dev can't be null when creating a raid > device. It can be checked before > calling create_mddev. And we must get a name after creating a raid > device. So when assembling > a raid device, the name must not be null. So the dev and name can't be > null at the same time, right? Correct. NeilBrown > > Best Regards > Xiao > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > mdopen.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/mdopen.c b/mdopen.c > > index d18c931996d2..810f79a3d19a 100644 > > --- a/mdopen.c > > +++ b/mdopen.c > > @@ -370,6 +370,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy, > > } > > if (block_udev) > > udev_block(devnm); > > + create_named_array(devnm); > > } > > > > sprintf(devname, "/dev/%s", devnm); > > -- > > 2.39.2 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-22 3:24 ` NeilBrown @ 2023-03-22 6:51 ` Mariusz Tkaczyk 2023-03-23 2:09 ` Xiao Ni 2023-03-29 21:51 ` NeilBrown 2 siblings, 0 replies; 14+ messages in thread From: Mariusz Tkaczyk @ 2023-03-22 6:51 UTC (permalink / raw) To: NeilBrown Cc: Xiao Ni, Jes Sorensen, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov, Logan Gunthorpe On Wed, 22 Mar 2023 14:24:49 +1100 "NeilBrown" <neilb@suse.de> wrote: > On Wed, 22 Mar 2023, Xiao Ni wrote: > > On Tue, Mar 14, 2023 at 8:08 AM NeilBrown <neilb@suse.de> wrote: > > > > > > > > > mdopen() will use create_named_array() to ask the kernel to create the > > > given md array, but only if it is given a number or name. > > > If it is NOT given a name and is required to choose one itself using > > > find_free_devnm() it does NOT use create_named_array(). > > > > > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > > > failure to assemble an array. This can particularly seen when the > > > "name" of the array begins with a host name different to the name of the > > > host running the command. > > > > > > So add the missing call to create_named_array(). > > > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 > > > > Hi Neil > > > > I have two questions, hope you can help to understand the function > > create_mddev better. > > > > Frist, from the comment7 of the bug you mentioned: > > > > There are two different sorts names. Note that you almost > > acknowledged this by writing "name for my md device node" while the > > documentation only talks about names for "md devices", not for "md > > device nodes". > > > > There are > > 1/ there are names in /dev or /dev/md/ (device nodes) > > 2/ there are names that appear in /proc/mdstat and in /sys/block/ (devices) > > > > Thanks for the clarification. But it looks like it doesn't work like > > what you said. > > For example: > > mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc --name=test > > cat /proc/mdstat > > Personalities : [raid0] > > md127 : active raid0 sdc[1] sda[0] > > 3906764800 blocks super 1.2 512k chunks > > cd /sys/block/md127/md/ > > > > In /proc/mdstat and /sys/block, they all use md127 rather than the > > name(root) > > Try again with "CREATE names=yes" in /etc/mdadm.conf. > > mdadm generally tries to keep: > - the names in /dev/ > - the names in /dev/md/ > - the names in /proc/mdstat > - the names stored in the metadata > > in sync. It can only do this when: > - you enabled "names=yes" > - you don't confuse it by specifying a device name (/dev/md/root) that > is different from the metadata names "test". > > If you don't have "names=yes" then the name in /proc/mdstat and the name > in /dev/md* will be numeric. The name in /dev/md/ and the name in the > metadata can be different and will usually be the same. > > If you explicitly give a different name with --name= than the device > name then obviously they will be different. If you then stop the array > and restart with "mdadm -As" or "mdadm -I /dev/sda; mdadm -I /dev/sdb" > then mdadm will create a name in /dev/md/ that matches the name in the > metadata. > > > > > Before this patch, it creates a symbol link with the name root rather than > > test ll /dev/md/root > > lrwxrwxrwx. 1 root root 8 Mar 21 22:35 /dev/md/root -> ../md127 > > That is what you asked it to do. > > > > > So "test" which is specified by --name looks like it has little usage. > > > > It is stored in the metadata. You can see it in --examine output. If > you reassemble the array without specifying a device name, it will use > the name "test". > > > > > By the way, after this patch, the symbol link /dev/md/root can't be > > created anymore. > > Is it a regression problem? > > I cannot reproduce any problem like that. Please provide a sequence of > steps so that I can try to duplicate it. Hi, It is not caused by this patch. Regression is caused by: https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=8a4ce2c053866ac97feb436c4c85a54446ee0016 We noticed that yesterday. In our case, udev fails to create link, timeout happens. This is caused by missing MD_DEVNAME property in --detail --export. At that is all I know for now. Work in progress. Steps: #mdadm -CR imsm -e imsm -n4 /dev/nvme[0-3]n1 #mdadm -CR vol -l5 -n4 /dev/nvme[0-3]n1 --assume-clean Thanks, Mariusz > > > > > Second, are there possibilities that the arguments "dev" and "name" of > > function create_mddev > > are null at the same time? > > No. For Build or Create, dev is never NULL. For Assemble and > Incremental, name is never NULL. > > > > After some tests, I found dev can't be null when creating a raid > > device. It can be checked before > > calling create_mddev. And we must get a name after creating a raid > > device. So when assembling > > a raid device, the name must not be null. So the dev and name can't be > > null at the same time, right? > > Correct. > > NeilBrown > > > > > > Best Regards > > Xiao > > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > mdopen.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/mdopen.c b/mdopen.c > > > index d18c931996d2..810f79a3d19a 100644 > > > --- a/mdopen.c > > > +++ b/mdopen.c > > > @@ -370,6 +370,7 @@ int create_mddev(char *dev, char *name, int autof, > > > int trustworthy, } > > > if (block_udev) > > > udev_block(devnm); > > > + create_named_array(devnm); > > > } > > > > > > sprintf(devname, "/dev/%s", devnm); > > > -- > > > 2.39.2 > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-22 3:24 ` NeilBrown 2023-03-22 6:51 ` Mariusz Tkaczyk @ 2023-03-23 2:09 ` Xiao Ni 2023-03-29 21:51 ` NeilBrown 2 siblings, 0 replies; 14+ messages in thread From: Xiao Ni @ 2023-03-23 2:09 UTC (permalink / raw) To: NeilBrown Cc: Jes Sorensen, Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Wed, Mar 22, 2023 at 11:25 AM NeilBrown <neilb@suse.de> wrote: > > On Wed, 22 Mar 2023, Xiao Ni wrote: > > On Tue, Mar 14, 2023 at 8:08 AM NeilBrown <neilb@suse.de> wrote: > > > > > > > > > mdopen() will use create_named_array() to ask the kernel to create the > > > given md array, but only if it is given a number or name. > > > If it is NOT given a name and is required to choose one itself using > > > find_free_devnm() it does NOT use create_named_array(). > > > > > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in > > > failure to assemble an array. This can particularly seen when the > > > "name" of the array begins with a host name different to the name of the > > > host running the command. > > > > > > So add the missing call to create_named_array(). > > > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074 > > > > Hi Neil > > > > I have two questions, hope you can help to understand the function > > create_mddev better. > > > > Frist, from the comment7 of the bug you mentioned: > > > > There are two different sorts names. Note that you almost > > acknowledged this by writing "name for my md device node" while the > > documentation only talks about names for "md devices", not for "md > > device nodes". > > > > There are > > 1/ there are names in /dev or /dev/md/ (device nodes) > > 2/ there are names that appear in /proc/mdstat and in /sys/block/ (devices) > > > > Thanks for the clarification. But it looks like it doesn't work like > > what you said. > > For example: > > mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc --name=test > > cat /proc/mdstat > > Personalities : [raid0] > > md127 : active raid0 sdc[1] sda[0] > > 3906764800 blocks super 1.2 512k chunks > > cd /sys/block/md127/md/ > > > > In /proc/mdstat and /sys/block, they all use md127 rather than the name(root) > > Try again with "CREATE names=yes" in /etc/mdadm.conf. > > mdadm generally tries to keep: > - the names in /dev/ > - the names in /dev/md/ > - the names in /proc/mdstat > - the names stored in the metadata > > in sync. It can only do this when: > - you enabled "names=yes" > - you don't confuse it by specifying a device name (/dev/md/root) that > is different from the metadata names "test". > > If you don't have "names=yes" then the name in /proc/mdstat and the name > in /dev/md* will be numeric. The name in /dev/md/ and the name in the > metadata can be different and will usually be the same. > > If you explicitly give a different name with --name= than the device > name then obviously they will be different. If you then stop the array > and restart with "mdadm -As" or "mdadm -I /dev/sda; mdadm -I /dev/sdb" > then mdadm will create a name in /dev/md/ that matches the name in the > metadata. Hi Neil My last email uses non plain text mode. So many people can't see it. I send this again with plain text mode. Thanks for your explanation. It looks like I understand it. In the function create_mddev, it tries to extract the number and name from dev(device node name) or name(metadata name). If it doesn't use --name when creating a raid device, it gets the number and name from dev. 1. If it can get a number, we use the mdNN as the raid name. So the device node name and raid name are the same(.e.g md0). 2. If it can't get a number and doesn't set 'Create names=yes', it automatically chooses a number. In this case the device node name and the raid name are different. For example: mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc The device node name is md127, the raid name is root. Because it doesn't specify --name, so the metadata name is root too. mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc --name=test The device node name is md127, the raid name is root, the metadata name is test. When assembling raid device, it'll use md127 as the device node name too. But the raid name will change to md_test. So it's not a good command to create the raid device. If you want to specify the raid name in /dev/md/root, it's better not to use --name. 3. If it can't get a number and sets 'Create names=yes' in mdadm.conf, it can use the raid name as the device node name. For example: mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc The device node name is root, the raid name is root and the metadata name is root too. > > > > > Before this patch, it creates a symbol link with the name root rather than test > > ll /dev/md/root > > lrwxrwxrwx. 1 root root 8 Mar 21 22:35 /dev/md/root -> ../md127 > > That is what you asked it to do. > > > > > So "test" which is specified by --name looks like it has little usage. > > > > It is stored in the metadata. You can see it in --examine output. If > you reassemble the array without specifying a device name, it will use > the name "test". So we can call it metadata name :) > > > > > By the way, after this patch, the symbol link /dev/md/root can't be > > created anymore. > > Is it a regression problem? > > I cannot reproduce any problem like that. Please provide a sequence of > steps so that I can try to duplicate it. In the next email, Mariusz has given the reproduction steps. And it's another patch that causes the regression. > > > > > Second, are there possibilities that the arguments "dev" and "name" of > > function create_mddev > > are null at the same time? > > No. For Build or Create, dev is never NULL. For Assemble and > Incremental, name is never NULL. > > > > After some tests, I found dev can't be null when creating a raid > > device. It can be checked before > > calling create_mddev. And we must get a name after creating a raid > > device. So when assembling > > a raid device, the name must not be null. So the dev and name can't be > > null at the same time, right? > > Correct. Thanks for the confirmation. Now there is a comment like this: "If both name and dev are NULL, we choose a name 'mdXX' or 'mdpXX'". I'll try to write some patches to optimize the function mddev_create and those comments. Best Regards Xiao ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-22 3:24 ` NeilBrown 2023-03-22 6:51 ` Mariusz Tkaczyk 2023-03-23 2:09 ` Xiao Ni @ 2023-03-29 21:51 ` NeilBrown 2023-03-30 0:10 ` Xiao Ni 2 siblings, 1 reply; 14+ messages in thread From: NeilBrown @ 2023-03-29 21:51 UTC (permalink / raw) To: Xiao Ni Cc: Jes Sorensen, Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Wed, 22 Mar 2023, NeilBrown wrote: > On Wed, 22 Mar 2023, Xiao Ni wrote: > > > > > Second, are there possibilities that the arguments "dev" and "name" of > > function create_mddev > > are null at the same time? > > No. For Build or Create, dev is never NULL. For Assemble and > Incremental, name is never NULL. > I should clarify this a bit. For Assemble and Incremental, "name" is never NULL *but* it might be an empty string. So: if (name && name[0] == 0) name = NULL; might cause it to become NULL. So you cannot assume there is always either a valid "dev" or a valid "name". "dev" might be NULL, and "name" might be "". NeilBrown ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-29 21:51 ` NeilBrown @ 2023-03-30 0:10 ` Xiao Ni 2023-03-30 3:15 ` NeilBrown 0 siblings, 1 reply; 14+ messages in thread From: Xiao Ni @ 2023-03-30 0:10 UTC (permalink / raw) To: NeilBrown Cc: Jes Sorensen, Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Thu, Mar 30, 2023 at 5:52 AM NeilBrown <neilb@suse.de> wrote: > > On Wed, 22 Mar 2023, NeilBrown wrote: > > On Wed, 22 Mar 2023, Xiao Ni wrote: > > > > > > > > Second, are there possibilities that the arguments "dev" and "name" of > > > function create_mddev > > > are null at the same time? > > > > No. For Build or Create, dev is never NULL. For Assemble and > > Incremental, name is never NULL. > > > > I should clarify this a bit. For Assemble and Incremental, "name" is > never NULL *but* it might be an empty string. > So: > if (name && name[0] == 0) > name = NULL; > > might cause it to become NULL. So you cannot assume there is always > either a valid "dev" or a valid "name". "dev" might be NULL, and "name" > might be "". > > NeilBrown > Hi Neil The input argument name should be the metadata name. For incremental and assemble, why are there possibilities that the metadata name is invalid? A raid device should have a valid metadata name, right? -- Best Regards Xiao Ni ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-30 0:10 ` Xiao Ni @ 2023-03-30 3:15 ` NeilBrown 2023-03-30 4:55 ` Xiao Ni 0 siblings, 1 reply; 14+ messages in thread From: NeilBrown @ 2023-03-30 3:15 UTC (permalink / raw) To: Xiao Ni Cc: Jes Sorensen, Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Thu, 30 Mar 2023, Xiao Ni wrote: > On Thu, Mar 30, 2023 at 5:52 AM NeilBrown <neilb@suse.de> wrote: > > > > On Wed, 22 Mar 2023, NeilBrown wrote: > > > On Wed, 22 Mar 2023, Xiao Ni wrote: > > > > > > > > > > > Second, are there possibilities that the arguments "dev" and "name" of > > > > function create_mddev > > > > are null at the same time? > > > > > > No. For Build or Create, dev is never NULL. For Assemble and > > > Incremental, name is never NULL. > > > > > > > I should clarify this a bit. For Assemble and Incremental, "name" is > > never NULL *but* it might be an empty string. > > So: > > if (name && name[0] == 0) > > name = NULL; > > > > might cause it to become NULL. So you cannot assume there is always > > either a valid "dev" or a valid "name". "dev" might be NULL, and "name" > > might be "". > > > > NeilBrown > > > > Hi Neil > > The input argument name should be the metadata name. For incremental > and assemble, why are there possibilities that the metadata name is > invalid? A raid device should have a valid metadata name, right? "should" do, yes. But you can never completely trust data on disk. It is safest to be prepared for "name" being "". NeilBrown ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH - mdadm] mdopen: always try create_named_array() 2023-03-30 3:15 ` NeilBrown @ 2023-03-30 4:55 ` Xiao Ni 0 siblings, 0 replies; 14+ messages in thread From: Xiao Ni @ 2023-03-30 4:55 UTC (permalink / raw) To: NeilBrown Cc: Jes Sorensen, Mariusz Tkaczyk, Song Liu, Linux regressions mailing list, linux-raid, LKML, Nikolay Kichukov On Thu, Mar 30, 2023 at 11:15 AM NeilBrown <neilb@suse.de> wrote: > > On Thu, 30 Mar 2023, Xiao Ni wrote: > > On Thu, Mar 30, 2023 at 5:52 AM NeilBrown <neilb@suse.de> wrote: > > > > > > On Wed, 22 Mar 2023, NeilBrown wrote: > > > > On Wed, 22 Mar 2023, Xiao Ni wrote: > > > > > > > > > > > > > > Second, are there possibilities that the arguments "dev" and "name" of > > > > > function create_mddev > > > > > are null at the same time? > > > > > > > > No. For Build or Create, dev is never NULL. For Assemble and > > > > Incremental, name is never NULL. > > > > > > > > > > I should clarify this a bit. For Assemble and Incremental, "name" is > > > never NULL *but* it might be an empty string. > > > So: > > > if (name && name[0] == 0) > > > name = NULL; > > > > > > might cause it to become NULL. So you cannot assume there is always > > > either a valid "dev" or a valid "name". "dev" might be NULL, and "name" > > > might be "". > > > > > > NeilBrown > > > > > > > Hi Neil > > > > The input argument name should be the metadata name. For incremental > > and assemble, why are there possibilities that the metadata name is > > invalid? A raid device should have a valid metadata name, right? > > "should" do, yes. But you can never completely trust data on disk. It > is safest to be prepared for "name" being "". > > NeilBrown > That's right, thanks for this explanation :) -- Best Regards Xiao Ni ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-03-30 4:55 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-14 0:06 [PATCH - mdadm] mdopen: always try create_named_array() NeilBrown 2023-03-14 9:11 ` Mariusz Tkaczyk 2023-03-19 16:36 ` Jes Sorensen 2023-03-19 22:09 ` NeilBrown 2023-03-20 17:15 ` Jes Sorensen 2023-03-21 8:49 ` Mariusz Tkaczyk 2023-03-22 3:00 ` Xiao Ni 2023-03-22 3:24 ` NeilBrown 2023-03-22 6:51 ` Mariusz Tkaczyk 2023-03-23 2:09 ` Xiao Ni 2023-03-29 21:51 ` NeilBrown 2023-03-30 0:10 ` Xiao Ni 2023-03-30 3:15 ` NeilBrown 2023-03-30 4:55 ` Xiao Ni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).