| Message ID | OF49F7969B.B27015BD-ON48258049.00328EDA-48258049.00330CC8@zte.com.cn (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On 10/11/2016 11:17 AM, tang.junhui@zte.com.cn wrote: > Hello Hannes, Ben, > Could you have a review for this patch, any comment will be highly > appreciated. > > Thanks, > Tang > > > > > 发件人: Christophe Varoqui <christophe.varoqui@opensvc.com> > 收件人: tang.junhui@zte.com.cn, > 抄送: Bart Van Assche <bart.vanassche@sandisk.com>, device-mapper > development <dm-devel@redhat.com>, zhang.kai16@zte.com.cn > 日期: 2016/10/11 14:59 > 主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q > command logic > 发件人: dm-devel-bounces@redhat.com > ------------------------------------------------------------------------ > > > > Hannes, Ben, > > are you ok with the solution to these two issues. > Seems sane to me. > This actually is only part of the story. The whole idea of issuing 'multipath' is to check if a given path _should_ be multipathed (as this is typically called from an udev event). But as it's called from an udev event we cannot rely on the multipath daemon to be started; we might just handle an event which came in before multipathd got started from systemd. So checking for the PID file is not enough, we need to check if the daemon will be started eventually. And in fact checking the PID file or calling mpath_connect() is equivalent, with the added advantage the mpath_connect() will start the daemon _if enabled by systemd_. So this patch doesn't help much, as it doesn't solve the main problem of figuring out if multipathd _should_ be started. I've done a patch for checking the '.wants' directories from systemd, but this obviously will only work if the OS is systemd-based. And it's not really perfect, as there are corner-cases where just checking for the .wants directory is not enough. Cheers, Hannes
On Tue, Oct 11, 2016 at 12:33:43PM +0200, Hannes Reinecke wrote: > On 10/11/2016 11:17 AM, tang.junhui@zte.com.cn wrote: > >Hello Hannes, Ben, > >Could you have a review for this patch, any comment will be highly > >appreciated. > > > >Thanks, > >Tang > > > > > > > > > >发件人: Christophe Varoqui <christophe.varoqui@opensvc.com> > >收件人: tang.junhui@zte.com.cn, > >抄送: Bart Van Assche <bart.vanassche@sandisk.com>, device-mapper > >development <dm-devel@redhat.com>, zhang.kai16@zte.com.cn > >日期: 2016/10/11 14:59 > >主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q > >command logic > >发件人: dm-devel-bounces@redhat.com > >------------------------------------------------------------------------ > > > > > > > >Hannes, Ben, > > > >are you ok with the solution to these two issues. > >Seems sane to me. > > > This actually is only part of the story. > > The whole idea of issuing 'multipath' is to check if a given path _should_ > be multipathed (as this is typically called from an udev event). > But as it's called from an udev event we cannot rely on the multipath daemon > to be started; we might just handle an event which came in before multipathd > got started from systemd. > So checking for the PID file is not enough, we need to check if the daemon > will be started eventually. > And in fact checking the PID file or calling mpath_connect() is equivalent, > with the added advantage the mpath_connect() will start the daemon _if > enabled by systemd_. > So this patch doesn't help much, as it doesn't solve the main problem of > figuring out if multipathd _should_ be started. > > I've done a patch for checking the '.wants' directories from systemd, but > this obviously will only work if the OS is systemd-based. > And it's not really perfect, as there are corner-cases where just checking > for the .wants directory is not enough. I think you are dealing with a different issue. the "-q" option for multipath just overrides the default behaviour of not enabling queue_if_no_path when creating a multipath device is multipathd isn't running. For this, we don't care if multipathd will start running in the future, because if/when multipathd does start up, it will set the queue_if_no_path flag itself. We only care about now, when we know it's not running. Really, I doubt that anyone ever uses the -q option. So your change in how multipath checks if multipathd is running is fine by me. However, you also changed what the "-q" option does. Previously, it just kept multipath from disabling "queue_if_no_path" on devices that were configured to have it, when multipathd wasn't running. With your code, doesn't it now make all devices set queue_if_no_path if multipathd isn't running, including devices that weren't configured to set it even when multipathd is running? Is there a reason for this? In general, setting "queue_if_no_path" when multiapthd isn't running is a bad idea, since the paths will never come back, and nothing will ever disable the queueing. That's why I said that I doubt anybody uses the option. Forcing all devices to set "queue_if_no_path", even if they weren't configured to, seems even less useful. Or is there actually a use-case that I'm missing here? -Ben > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > hare@suse.de +49 911 74053 688 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hello Ben, In multipath manual document, multipath -q command is described as: "-q allow device tables with queue_if_no_path when multiathd is not running". However the command does not take effect when we testing it. About use-case, In my opinion, sometimes we need to stop multipath service temporarily (for example: multipath version upgrade), but we do not want IO interrupt during those times even path down existed, so we can execute “multiath -q” to keep IO queue until we starting service after version upgrade. Cheers, Tang 发件人: "Benjamin Marzinski" <bmarzins@redhat.com> 收件人: Hannes Reinecke <hare@suse.de>, 抄送: Bart Van Assche <bart.vanassche@sandisk.com>, device-mapper development <dm-devel@redhat.com>, tang.junhui@zte.com.cn, zhang.kai16@zte.com.cn 日期: 2016/10/12 10:53 主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic 发件人: dm-devel-bounces@redhat.com On Tue, Oct 11, 2016 at 12:33:43PM +0200, Hannes Reinecke wrote: > On 10/11/2016 11:17 AM, tang.junhui@zte.com.cn wrote: > >Hello Hannes, Ben, > >Could you have a review for this patch, any comment will be highly > >appreciated. > > > >Thanks, > >Tang > > > > > > > > > >发件人: Christophe Varoqui <christophe.varoqui@opensvc.com> > >收件人: tang.junhui@zte.com.cn, > >抄送: Bart Van Assche <bart.vanassche@sandisk.com>, device-mapper > >development <dm-devel@redhat.com>, zhang.kai16@zte.com.cn > >日期: 2016/10/11 14:59 > >主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q > >command logic > >发件人: dm-devel-bounces@redhat.com > >------------------------------------------------------------------------ > > > > > > > >Hannes, Ben, > > > >are you ok with the solution to these two issues. > >Seems sane to me. > > > This actually is only part of the story. > > The whole idea of issuing 'multipath' is to check if a given path _should_ > be multipathed (as this is typically called from an udev event). > But as it's called from an udev event we cannot rely on the multipath daemon > to be started; we might just handle an event which came in before multipathd > got started from systemd. > So checking for the PID file is not enough, we need to check if the daemon > will be started eventually. > And in fact checking the PID file or calling mpath_connect() is equivalent, > with the added advantage the mpath_connect() will start the daemon _if > enabled by systemd_. > So this patch doesn't help much, as it doesn't solve the main problem of > figuring out if multipathd _should_ be started. > > I've done a patch for checking the '.wants' directories from systemd, but > this obviously will only work if the OS is systemd-based. > And it's not really perfect, as there are corner-cases where just checking > for the .wants directory is not enough. I think you are dealing with a different issue. the "-q" option for multipath just overrides the default behaviour of not enabling queue_if_no_path when creating a multipath device is multipathd isn't running. For this, we don't care if multipathd will start running in the future, because if/when multipathd does start up, it will set the queue_if_no_path flag itself. We only care about now, when we know it's not running. Really, I doubt that anyone ever uses the -q option. So your change in how multipath checks if multipathd is running is fine by me. However, you also changed what the "-q" option does. Previously, it just kept multipath from disabling "queue_if_no_path" on devices that were configured to have it, when multipathd wasn't running. With your code, doesn't it now make all devices set queue_if_no_path if multipathd isn't running, including devices that weren't configured to set it even when multipathd is running? Is there a reason for this? In general, setting "queue_if_no_path" when multiapthd isn't running is a bad idea, since the paths will never come back, and nothing will ever disable the queueing. That's why I said that I doubt anybody uses the option. Forcing all devices to set "queue_if_no_path", even if they weren't configured to, seems even less useful. Or is there actually a use-case that I'm missing here? -Ben > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > hare@suse.de +49 911 74053 688 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hello Hannes, check_daemon() now is only used to determine whether to set/remove queue_if_no_path feature for mapped devices, which you said maybe another issue, we are looking forward for your patch. Thanks, Tang 发件人: Hannes Reinecke <hare@suse.de> 收件人: tang.junhui@zte.com.cn, bmarzins@redhat.com, 抄送: Bart Van Assche <bart.vanassche@sandisk.com>, device-mapper development <dm-devel@redhat.com>, zhang.kai16@zte.com.cn 日期: 2016/10/11 18:42 主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic 发件人: dm-devel-bounces@redhat.com On 10/11/2016 11:17 AM, tang.junhui@zte.com.cn wrote: > Hello Hannes, Ben, > Could you have a review for this patch, any comment will be highly > appreciated. > > Thanks, > Tang > > > > > 发件人: Christophe Varoqui <christophe.varoqui@opensvc.com> > 收件人: tang.junhui@zte.com.cn, > 抄送: Bart Van Assche <bart.vanassche@sandisk.com>, device-mapper > development <dm-devel@redhat.com>, zhang.kai16@zte.com.cn > 日期: 2016/10/11 14:59 > 主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q > command logic > 发件人: dm-devel-bounces@redhat.com > ------------------------------------------------------------------------ > > > > Hannes, Ben, > > are you ok with the solution to these two issues. > Seems sane to me. > This actually is only part of the story. The whole idea of issuing 'multipath' is to check if a given path _should_ be multipathed (as this is typically called from an udev event). But as it's called from an udev event we cannot rely on the multipath daemon to be started; we might just handle an event which came in before multipathd got started from systemd. So checking for the PID file is not enough, we need to check if the daemon will be started eventually. And in fact checking the PID file or calling mpath_connect() is equivalent, with the added advantage the mpath_connect() will start the daemon _if enabled by systemd_. So this patch doesn't help much, as it doesn't solve the main problem of figuring out if multipathd _should_ be started. I've done a patch for checking the '.wants' directories from systemd, but this obviously will only work if the OS is systemd-based. And it's not really perfect, as there are corner-cases where just checking for the .wants directory is not enough. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 707e6be..d8a17a6 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -715,36 +715,36 @@ deadmap (struct multipath * mpp) return 1; /* dead */ } -int check_daemon(void) +static int +check_daemon(void) { int fd; - char *reply; - int ret = 0; - unsigned int timeout; - struct config *conf; - - fd = mpath_connect(); - if (fd == -1) - return 0; + struct flock lock; - if (send_packet(fd, "show daemon") != 0) - goto out; - conf = get_multipath_config(); - timeout = conf->uxsock_timeout; - put_multipath_config(conf); - if (recv_packet(fd, &reply, timeout) != 0) - goto out; - - if (strstr(reply, "shutdown")) - goto out_free; - - ret = 1; + fd = open(DEFAULT_PIDFILE, O_RDONLY); + if (fd < 0) { + if (errno == ENOENT) + return 0; + if (errno == EMFILE) + condlog(0, "failed to open file, increase max_fds at %s", DEFAULT_CONFIGFILE); + else + condlog(0, "can not open pidfile %s: %s", DEFAULT_PIDFILE, strerror(errno)); + return -1; + } -out_free: - FREE(reply); -out: - mpath_disconnect(fd); - return ret; + lock.l_type = F_WRLCK; + lock.l_start = 0; + lock.l_whence = SEEK_SET; + lock.l_len = 0; + if (fcntl(fd, F_GETLK, &lock) < 0) { + condlog(0, "can not get file locker, %s: %s", DEFAULT_PIDFILE, strerror(errno)); + close(fd); + return -1; + } + close(fd); + if (lock.l_type == F_UNLCK) + return 0; + return 1;