Re: [PATCH 4/5] kpartx: default to running in sync mode

["Benjamin Marzinski" <bmarzins@redhat.com>]

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)