| Message ID | 1493073570-17167-5-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On 04/25/2017 12:39 AM, Benjamin Marzinski wrote: > When users run kpartx, they would naturally assume that when it > completes, the devices have been created. However, kpartx runs in async > mode by default. This seems like it is likely to trip up users. So, > switch the default to sync mode, add a -n option to enable async mode, > and set async mode when kpartx is called by the udev rules. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > kpartx/kpartx.c | 10 +++++++--- > kpartx/kpartx.rules | 2 +- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > index 58e60ff..d1edd5e 100644 > --- a/kpartx/kpartx.c > +++ b/kpartx/kpartx.c > -int udev_sync = 0; > +int udev_sync = 1; > - printf("\t-s sync mode. Don't return until the partitions are created\n"); > + printf("\t-n nosync mode. Return before the partitions are created\n"); > + printf("\t-s sync mode. Don't return until the partitions are created. Default.\n"); > + case 'n': > + udev_sync = 0; > + break; > diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules > index 48a4d6c..a958791 100644 > --- a/kpartx/kpartx.rules > +++ b/kpartx/kpartx.rules > @@ -40,6 +40,6 @@ ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" > ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1" > ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end" > ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ > - RUN+="/sbin/kpartx -u -p -part /dev/$name" > + RUN+="/sbin/kpartx -un -p -part /dev/$name" I understand this was async before and is now still async after the default change in kpartx. However, I'm wondering in general how one would "wait" for kpartx mappings, since I suppose a "udevadm settle" would not suffice. Dracut probably does not care because it would loop until its required root-fs devices have appeared.
On Tue, Apr 25, 2017 at 12:12:25PM +0200, Steffen Maier wrote: > > On 04/25/2017 12:39 AM, Benjamin Marzinski wrote: > >When users run kpartx, they would naturally assume that when it > >completes, the devices have been created. However, kpartx runs in async > >mode by default. This seems like it is likely to trip up users. So, > >switch the default to sync mode, add a -n option to enable async mode, > >and set async mode when kpartx is called by the udev rules. > > > >Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > >--- > > kpartx/kpartx.c | 10 +++++++--- > > kpartx/kpartx.rules | 2 +- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > >diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > >index 58e60ff..d1edd5e 100644 > >--- a/kpartx/kpartx.c > >+++ b/kpartx/kpartx.c > > >-int udev_sync = 0; > >+int udev_sync = 1; > > >- printf("\t-s sync mode. Don't return until the partitions are created\n"); > >+ printf("\t-n nosync mode. Return before the partitions are created\n"); > >+ printf("\t-s sync mode. Don't return until the partitions are created. Default.\n"); > > >+ case 'n': > >+ udev_sync = 0; > >+ break; > > >diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules > >index 48a4d6c..a958791 100644 > >--- a/kpartx/kpartx.rules > >+++ b/kpartx/kpartx.rules > >@@ -40,6 +40,6 @@ ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" > > ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1" > > ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end" > > ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ > >- RUN+="/sbin/kpartx -u -p -part /dev/$name" > >+ RUN+="/sbin/kpartx -un -p -part /dev/$name" > > I understand this was async before and is now still async after the default > change in kpartx. However, I'm wondering in general how one would "wait" for > kpartx mappings, since I suppose a "udevadm settle" would not suffice. > Dracut probably does not care because it would loop until its required > root-fs devices have appeared. I'm pretty sure that it is inherently impossible in the general case. There is no way to know when the uevent will occur, and until the uevent occurs, there would be no way that udev settle could possibly help. You can know that a dm device has been set up when the first change event for that device occurs. So, if you knew a device was coming, you could wait for the change event. Multipathd currently listens for change events to know when a multipath device has been fully initialized. For partition devices explicitly, say you just manually ran multipath to create a multipath device, and want to know that your partition devices exist before going on. You could manually run kpartx in the now-default sync mode, and when it returned, you would know that the devices were there. However, your kpart call would be racing with the udev version, so it's possible that kpartx would return with an error because it tried to create the device and failed because the device already existed. The good news is that it would continue to try with the rest of the devices, so it really won't return until they all are there. If it is important to you, we could add a flag to kpartx to not return an error if a create fails because the device currently exists. -Ben > > -- > Mit freundlichen Grüßen / Kind regards > Steffen Maier > > Linux on z Systems Development > > IBM Deutschland Research & Development GmbH > Vorsitzende des Aufsichtsrats: Martina Koederitz > Geschaeftsfuehrung: Dirk Wittkopp > Sitz der Gesellschaft: Boeblingen > Registergericht: Amtsgericht Stuttgart, HRB 243294 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 04/25/2017 05:52 PM, Benjamin Marzinski wrote: > On Tue, Apr 25, 2017 at 12:12:25PM +0200, Steffen Maier wrote: >> On 04/25/2017 12:39 AM, Benjamin Marzinski wrote: >>> When users run kpartx, they would naturally assume that when it >>> completes, the devices have been created. However, kpartx runs in async >>> mode by default. This seems like it is likely to trip up users. So, >>> switch the default to sync mode, add a -n option to enable async mode, >>> and set async mode when kpartx is called by the udev rules. >>> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c >>> + printf("\t-n nosync mode. Return before the partitions are created\n"); >>> diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules >>> @@ -40,6 +40,6 @@ ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" >>> ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1" >>> ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end" >>> ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ >>> - RUN+="/sbin/kpartx -u -p -part /dev/$name" >>> + RUN+="/sbin/kpartx -un -p -part /dev/$name" >> >> I understand this was async before and is now still async after the default >> change in kpartx. However, I'm wondering in general how one would "wait" for >> kpartx mappings, since I suppose a "udevadm settle" would not suffice. >> Dracut probably does not care because it would loop until its required >> root-fs devices have appeared. > > I'm pretty sure that it is inherently impossible in the general case. > There is no way to know when the uevent will occur, and until the uevent > occurs, there would be no way that udev settle could possibly help. You indeed > can know that a dm device has been set up when the first change event > for that device occurs. So, if you knew a device was coming, you could > wait for the change event. Multipathd currently listens for change > events to know when a multipath device has been fully initialized. > > For partition devices explicitly, say you just manually ran multipath to > create a multipath device, and want to know that your partition devices > exist before going on. You could manually run kpartx in the now-default > sync mode, and when it returned, you would know that the devices were > there. However, your kpart call would be racing with the udev version, > so it's possible that kpartx would return with an error because it tried > to create the device and failed because the device already existed. The > good news is that it would continue to try with the rest of the devices, > so it really won't return until they all are there. If it is important > to you, we could add a flag to kpartx to not return an error if a create > fails because the device currently exists. Currently I have no real need for such addition, just wanted to understand the implications. Thanks for your explanation!
On 04/25/2017 12:39 AM, Benjamin Marzinski wrote: > When users run kpartx, they would naturally assume that when it > completes, the devices have been created. However, kpartx runs in async > mode by default. This seems like it is likely to trip up users. So, > switch the default to sync mode, add a -n option to enable async mode, > and set async mode when kpartx is called by the udev rules. >[...] > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > index 58e60ff..d1edd5e 100644 > --- a/kpartx/kpartx.c > +++ b/kpartx/kpartx.c >[...] > -static char short_opts[] = "rladfgvp:t:su"; > +static char short_opts[] = "rladfgvp:t:snu"; >[...] > - printf("\t-s sync mode. Don't return until the partitions are created\n"); > + printf("\t-n nosync mode. Return before the partitions are created\n"); > + printf("\t-s sync mode. Don't return until the partitions are created. Default.\n"); > return 1; >[...]> + case 'n': > + udev_sync = 0; > + break; New flags should be documented in its man page. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Ben, On Mon, 2017-04-24 at 17:39 -0500, Benjamin Marzinski wrote: > When users run kpartx, they would naturally assume that when it > completes, the devices have been created. However, kpartx runs in > async > mode by default. This seems like it is likely to trip up users. Is this just likely, or do you have evidence that it really did irritate users? You introduced "sync mode", together with the "-s" flag, yourself in 2010, and unless I'm mistaken, kpartx operated in "async" mode before that, too, because there was no "sync" mode. I'm not too fond of the idea to change a default which has been established for such a long time just because user mistakes are "likely". For one thing, such changes are difficult to document in a way that doesn't cause confusion. Also, if someone writes a script in which she wants kpartx to run asynchronously, it will be difficult to do so in a manner which is portable between distributions if some distributions include this patch and some don't. As an alternative, we might simply warn about the default asynchronous behavior in a prominent place in the man page. Cheers, Martin > So, > switch the default to sync mode, add a -n option to enable async > mode, > and set async mode when kpartx is called by the udev rules. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > kpartx/kpartx.c | 10 +++++++--- > kpartx/kpartx.rules | 2 +- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > index 58e60ff..d1edd5e 100644 > --- a/kpartx/kpartx.c > +++ b/kpartx/kpartx.c > @@ -58,7 +58,7 @@ struct pt { > } pts[MAXTYPES]; > > int ptct = 0; > -int udev_sync = 0; > +int udev_sync = 1; > > static void > addpts(char *t, ptreader f) > @@ -86,7 +86,7 @@ initpts(void) > addpts("ps3", read_ps3_pt); > } > > -static char short_opts[] = "rladfgvp:t:su"; > +static char short_opts[] = "rladfgvp:t:snu"; > > /* Used in gpt.c */ > int force_gpt=0; > @@ -105,7 +105,8 @@ usage(void) { > printf("\t-g force GUID partition table (GPT)\n"); > printf("\t-f force devmap create\n"); > printf("\t-v verbose\n"); > - printf("\t-s sync mode. Don't return until the partitions > are created\n"); > + printf("\t-n nosync mode. Return before the partitions are > created\n"); > + printf("\t-s sync mode. Don't return until the partitions > are created. Default.\n"); > return 1; > } > > @@ -291,6 +292,9 @@ main(int argc, char **argv){ > case 's': > udev_sync = 1; > break; > + case 'n': > + udev_sync = 0; > + break; > case 'u': > what = UPDATE; > break; > diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules > index 48a4d6c..a958791 100644 > --- a/kpartx/kpartx.rules > +++ b/kpartx/kpartx.rules > @@ -40,6 +40,6 @@ ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" > ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", > IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1" > ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end" > ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ > - RUN+="/sbin/kpartx -u -p -part /dev/$name" > + RUN+="/sbin/kpartx -un -p -part /dev/$name" > > LABEL="kpartx_end"
On Thu, May 11, 2017 at 02:42:08PM +0200, Martin Wilck wrote: > Hi Ben, > > On Mon, 2017-04-24 at 17:39 -0500, Benjamin Marzinski wrote: > > > When users run kpartx, they would naturally assume that when it > > completes, the devices have been created. However, kpartx runs in > > async > > mode by default. This seems like it is likely to trip up users. > > Is this just likely, or do you have evidence that it really did > irritate users? You introduced "sync mode", together with the "-s" > flag, yourself in 2010, and unless I'm mistaken, kpartx operated in > "async" mode before that, too, because there was no "sync" mode. Yes. I made this patch after the second time a customer complained that kpartx wasn't working correctly, when the real issue was that their scripts were assuming that after the kpartx command completed, those partition device nodes would be present. Obviously, 2 isn't a huge number, but it certainly has caused confusion in some cases. > I'm not too fond of the idea to change a default which has been > established for such a long time just because user mistakes are > "likely". For one thing, such changes are difficult to document in a > way that doesn't cause confusion. Also, if someone writes a script in > which she wants kpartx to run asynchronously, it will be difficult to > do so in a manner which is portable between distributions if some > distributions include this patch and some don't. I see your point, but assuming that I am correct that most users do assume that kpartx runs synchronously, I feel like we shouldn't keep a bad default forever, simply because it's always been that way. lvm and dmsetup wait for udev by default (well, on lvm IIRC it is a compile-time setting, but here is some SUSE documentation showing that it uses udev synchronization by default https://www.suse.com/documentation/sles-12/stor_admin/data/sec_lvm_cli.html) kpartx does seem to be the odd one out. > As an alternative, we might simply warn about the default asynchronous > behavior in a prominent place in the man page. We could, and if you are opposed to this patch, that would probably help cut down any confusion (assuming that when customers see things go wrong they read the documentation before calling support). -Ben > Cheers, > Martin > > > So, > > switch the default to sync mode, add a -n option to enable async > > mode, > > and set async mode when kpartx is called by the udev rules. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > kpartx/kpartx.c | 10 +++++++--- > > kpartx/kpartx.rules | 2 +- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > > index 58e60ff..d1edd5e 100644 > > --- a/kpartx/kpartx.c > > +++ b/kpartx/kpartx.c > > @@ -58,7 +58,7 @@ struct pt { > > } pts[MAXTYPES]; > > > > int ptct = 0; > > -int udev_sync = 0; > > +int udev_sync = 1; > > > > static void > > addpts(char *t, ptreader f) > > @@ -86,7 +86,7 @@ initpts(void) > > addpts("ps3", read_ps3_pt); > > } > > > > -static char short_opts[] = "rladfgvp:t:su"; > > +static char short_opts[] = "rladfgvp:t:snu"; > > > > /* Used in gpt.c */ > > int force_gpt=0; > > @@ -105,7 +105,8 @@ usage(void) { > > printf("\t-g force GUID partition table (GPT)\n"); > > printf("\t-f force devmap create\n"); > > printf("\t-v verbose\n"); > > - printf("\t-s sync mode. Don't return until the partitions > > are created\n"); > > + printf("\t-n nosync mode. Return before the partitions are > > created\n"); > > + printf("\t-s sync mode. Don't return until the partitions > > are created. Default.\n"); > > return 1; > > } > > > > @@ -291,6 +292,9 @@ main(int argc, char **argv){ > > case 's': > > udev_sync = 1; > > break; > > + case 'n': > > + udev_sync = 0; > > + break; > > case 'u': > > what = UPDATE; > > break; > > diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules > > index 48a4d6c..a958791 100644 > > --- a/kpartx/kpartx.rules > > +++ b/kpartx/kpartx.rules > > @@ -40,6 +40,6 @@ ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" > > ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", > > IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1" > > ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end" > > ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ > > - RUN+="/sbin/kpartx -u -p -part /dev/$name" > > + RUN+="/sbin/kpartx -un -p -part /dev/$name" > > > > LABEL="kpartx_end" > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 2017-05-11 at 12:46 -0500, Benjamin Marzinski wrote: > On Thu, May 11, 2017 at 02:42:08PM +0200, Martin Wilck wrote: > > Hi Ben, > > > > On Mon, 2017-04-24 at 17:39 -0500, Benjamin Marzinski wrote: > > > > > When users run kpartx, they would naturally assume that when it > > > completes, the devices have been created. However, kpartx runs in > > > async > > > mode by default. This seems like it is likely to trip up users. > > > > Is this just likely, or do you have evidence that it really did > > irritate users? You introduced "sync mode", together with the "-s" > > flag, yourself in 2010, and unless I'm mistaken, kpartx operated in > > "async" mode before that, too, because there was no "sync" mode. > > Yes. I made this patch after the second time a customer complained > that > kpartx wasn't working correctly, when the real issue was that their > scripts were assuming that after the kpartx command completed, those > partition device nodes would be present. Obviously, 2 isn't a huge > number, but it certainly has caused confusion in some cases. 2 > "likely", that makes a difference. > > I'm not too fond of the idea to change a default which has been > > established for such a long time just because user mistakes are > > "likely". For one thing, such changes are difficult to document in > > a > > way that doesn't cause confusion. Also, if someone writes a script > > in > > which she wants kpartx to run asynchronously, it will be difficult > > to > > do so in a manner which is portable between distributions if some > > distributions include this patch and some don't. > > I see your point, but assuming that I am correct that most users do > assume that kpartx runs synchronously, I feel like we shouldn't keep > a > bad default forever, simply because it's always been that way. lvm > and > dmsetup wait for udev by default (well, on lvm IIRC it is a compile- > time > setting, but here is some SUSE documentation showing that it uses > udev > synchronization by default > https://www.suse.com/documentation/sles-12/stor_admin/data/sec_lvm_cl > i.html) > kpartx does seem to be the odd one out. > > > As an alternative, we might simply warn about the default > > asynchronous > > behavior in a prominent place in the man page. > > We could, and if you are opposed to this patch, that would probably > help > cut down any confusion (assuming that when customers see things go > wrong > they read the documentation before calling support). I still don't like it too much. The scripting issue is my biggest concern - users need at least a reliable way to test whether or not "-n" is supported by kpartx on a given system. But I, alone, am not in a position to veto this. Does anyone else have an opinion on this change? Martin
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c index 58e60ff..d1edd5e 100644 --- a/kpartx/kpartx.c +++ b/kpartx/kpartx.c @@ -58,7 +58,7 @@ struct pt { } pts[MAXTYPES]; int ptct = 0; -int udev_sync = 0; +int udev_sync = 1; static void addpts(char *t, ptreader f) @@ -86,7 +86,7 @@ initpts(void) addpts("ps3", read_ps3_pt); } -static char short_opts[] = "rladfgvp:t:su"; +static char short_opts[] = "rladfgvp:t:snu"; /* Used in gpt.c */ int force_gpt=0; @@ -105,7 +105,8 @@ usage(void) { printf("\t-g force GUID partition table (GPT)\n"); printf("\t-f force devmap create\n"); printf("\t-v verbose\n"); - printf("\t-s sync mode. Don't return until the partitions are created\n"); + printf("\t-n nosync mode. Return before the partitions are created\n"); + printf("\t-s sync mode. Don't return until the partitions are created. Default.\n"); return 1; } @@ -291,6 +292,9 @@ main(int argc, char **argv){ case 's': udev_sync = 1; break; + case 'n': + udev_sync = 0; + break; case 'u': what = UPDATE; break; diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules index 48a4d6c..a958791 100644 --- a/kpartx/kpartx.rules +++ b/kpartx/kpartx.rules @@ -40,6 +40,6 @@ ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1" ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end" ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ - RUN+="/sbin/kpartx -u -p -part /dev/$name" + RUN+="/sbin/kpartx -un -p -part /dev/$name" LABEL="kpartx_end"
When users run kpartx, they would naturally assume that when it completes, the devices have been created. However, kpartx runs in async mode by default. This seems like it is likely to trip up users. So, switch the default to sync mode, add a -n option to enable async mode, and set async mode when kpartx is called by the udev rules. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- kpartx/kpartx.c | 10 +++++++--- kpartx/kpartx.rules | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-)