Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Paolo Bonzini]

On 19/09/2017 15:23, Paolo Bonzini wrote:
> On 19/09/2017 15:12, Daniel P. Berrange wrote:
>> On Tue, Sep 19, 2017 at 02:57:00PM +0200, Paolo Bonzini wrote:
>>> On 19/09/2017 14:53, Daniel P. Berrange wrote:
> +/* Try to reconnect while sending the CDB.  */
> +for (attempts = 0; attempts < PR_MAX_RECONNECT_ATTEMPTS; attempts++) 
> {

 I'm curious why you need to loop here. The helper daemon should be running
 already, as you're not spawning it on demand IIUC. So it shoudl either
 succeed first time, or fail every time.
>>>
>>> You're focusing on the usecase where the helper daemon is spawned per-VM
>>> by the system libvirtd, which I agree is the most important one.
>>> However, the other usecase is the one with a global daemon, access to
>>> which is controlled via Unix permissions.  This is not SELinux-friendly,
>>> but it is nicer for testing and it is also the only possibility for user
>>> libvirtd.
>>>
>>> In that case, upgrading QEMU on the host could result in a "systemctl
>>> restart qemu-pr-helper.service" (or, hopefully unlikely, a crash could
>>> result in systemd respawning the daemon).  Reconnect is useful in that case.
>>
>> If using systemd socket activation and you restart the daemon, the listening
>> socket should be preserved, so you shouldn't need to reconnect - the client
>> should get queued until it has started again (likewise on crash).
> 
> Oh, that's cool.  I didn't know that.

... wait, I do need to reconnect, because the active socket is closed.
I only don't need to retry and wait between the retries.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Paolo Bonzini]

On 19/09/2017 15:26, Daniel P. Berrange wrote:
> On Tue, Sep 19, 2017 at 03:23:09PM +0200, Paolo Bonzini wrote:
>> On 19/09/2017 15:12, Daniel P. Berrange wrote:
>>> On Tue, Sep 19, 2017 at 02:57:00PM +0200, Paolo Bonzini wrote:
 On 19/09/2017 14:53, Daniel P. Berrange wrote:
>> +/* Try to reconnect while sending the CDB.  */
>> +for (attempts = 0; attempts < PR_MAX_RECONNECT_ATTEMPTS; 
>> attempts++) {
>
> I'm curious why you need to loop here. The helper daemon should be running
> already, as you're not spawning it on demand IIUC. So it shoudl either
> succeed first time, or fail every time.

 You're focusing on the usecase where the helper daemon is spawned per-VM
 by the system libvirtd, which I agree is the most important one.
 However, the other usecase is the one with a global daemon, access to
 which is controlled via Unix permissions.  This is not SELinux-friendly,
 but it is nicer for testing and it is also the only possibility for user
 libvirtd.

 In that case, upgrading QEMU on the host could result in a "systemctl
 restart qemu-pr-helper.service" (or, hopefully unlikely, a crash could
 result in systemd respawning the daemon).  Reconnect is useful in that 
 case.
>>>
>>> If using systemd socket activation and you restart the daemon, the listening
>>> socket should be preserved, so you shouldn't need to reconnect - the client
>>> should get queued until it has started again (likewise on crash).
>>
>> Oh, that's cool.  I didn't know that.  However, systemd socket
>> activation is optional, and it's only a handful of lines so I think it's
>> a bit nicer behavior (chardevs for example have options to reconnect).
> 
> The downside is that if someone forget to start the daemon, or enable
> the socket,  QEMU will spin for 5 seconds trying to reconnect, instead
> of reporting an error immediately. 

Not exactly:

- the daemon must be present at startup.  Reconnect is only tried when
sending a command.

- it's not a busy wait, pr_manager_helper_run runs in an
util/thread-pool.c worker thread (see pr_manager_execute in patch 1).

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Paolo Bonzini]

On 19/09/2017 15:12, Daniel P. Berrange wrote:
> On Tue, Sep 19, 2017 at 02:57:00PM +0200, Paolo Bonzini wrote:
>> On 19/09/2017 14:53, Daniel P. Berrange wrote:
 +/* Try to reconnect while sending the CDB.  */
 +for (attempts = 0; attempts < PR_MAX_RECONNECT_ATTEMPTS; attempts++) {
>>>
>>> I'm curious why you need to loop here. The helper daemon should be running
>>> already, as you're not spawning it on demand IIUC. So it shoudl either
>>> succeed first time, or fail every time.
>>
>> You're focusing on the usecase where the helper daemon is spawned per-VM
>> by the system libvirtd, which I agree is the most important one.
>> However, the other usecase is the one with a global daemon, access to
>> which is controlled via Unix permissions.  This is not SELinux-friendly,
>> but it is nicer for testing and it is also the only possibility for user
>> libvirtd.
>>
>> In that case, upgrading QEMU on the host could result in a "systemctl
>> restart qemu-pr-helper.service" (or, hopefully unlikely, a crash could
>> result in systemd respawning the daemon).  Reconnect is useful in that case.
> 
> If using systemd socket activation and you restart the daemon, the listening
> socket should be preserved, so you shouldn't need to reconnect - the client
> should get queued until it has started again (likewise on crash).

Oh, that's cool.  I didn't know that.  However, systemd socket
activation is optional, and it's only a handful of lines so I think it's
a bit nicer behavior (chardevs for example have options to reconnect).

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Paolo Bonzini]

On 19/09/2017 14:53, Daniel P. Berrange wrote:
>> +/* Try to reconnect while sending the CDB.  */
>> +for (attempts = 0; attempts < PR_MAX_RECONNECT_ATTEMPTS; attempts++) {
>
> I'm curious why you need to loop here. The helper daemon should be running
> already, as you're not spawning it on demand IIUC. So it shoudl either
> succeed first time, or fail every time.

You're focusing on the usecase where the helper daemon is spawned per-VM
by the system libvirtd, which I agree is the most important one.
However, the other usecase is the one with a global daemon, access to
which is controlled via Unix permissions.  This is not SELinux-friendly,
but it is nicer for testing and it is also the only possibility for user
libvirtd.

In that case, upgrading QEMU on the host could result in a "systemctl
restart qemu-pr-helper.service" (or, hopefully unlikely, a crash could
result in systemd respawning the daemon).  Reconnect is useful in that case.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Daniel P. Berrange]

On Tue, Sep 19, 2017 at 12:24:34PM +0200, Paolo Bonzini wrote:
> This adds a concrete subclass of pr-manager that talks to qemu-pr-helper.
> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: fixed string property double-free
> fixed/cleaned up error handling
> handle buffer underrun
> 
>  scsi/Makefile.objs   |   2 +-
>  scsi/pr-manager-helper.c | 302 
> +++
>  2 files changed, 303 insertions(+), 1 deletion(-)
>  create mode 100644 scsi/pr-manager-helper.c
> 
> diff --git a/scsi/Makefile.objs b/scsi/Makefile.objs
> index 5496d2ae6a..4d25e476cf 100644
> --- a/scsi/Makefile.objs
> +++ b/scsi/Makefile.objs
> @@ -1,3 +1,3 @@
>  block-obj-y += utils.o
>  
> -block-obj-$(CONFIG_LINUX) += pr-manager.o
> +block-obj-$(CONFIG_LINUX) += pr-manager.o pr-manager-helper.o
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> new file mode 100644
> index 00..063fd35dee
> --- /dev/null
> +++ b/scsi/pr-manager-helper.c

> +/* Called with lock held.  */
> +static int pr_manager_helper_read(PRManagerHelper *pr_mgr,
> +  void *buf, int sz, Error **errp)
> +{
> +ssize_t r = qio_channel_read_all(pr_mgr->ioc, buf, sz, errp);

The return value isn't ssize_t - it just returns 0 on success, -1 on
error, never a bytes count since it always reads all requested bytes
and treats EOF as an error condition.

> +
> +if (r < 0) {
> +object_unref(OBJECT(pr_mgr->ioc));
> +pr_mgr->ioc = NULL;
> +return -EINVAL;
> +}
> +
> +return 0;
> +}
> +

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Daniel P. Berrange]

On Tue, Sep 19, 2017 at 02:57:00PM +0200, Paolo Bonzini wrote:
> On 19/09/2017 14:53, Daniel P. Berrange wrote:
> >> +/* Try to reconnect while sending the CDB.  */
> >> +for (attempts = 0; attempts < PR_MAX_RECONNECT_ATTEMPTS; attempts++) {
> >
> > I'm curious why you need to loop here. The helper daemon should be running
> > already, as you're not spawning it on demand IIUC. So it shoudl either
> > succeed first time, or fail every time.
> 
> You're focusing on the usecase where the helper daemon is spawned per-VM
> by the system libvirtd, which I agree is the most important one.
> However, the other usecase is the one with a global daemon, access to
> which is controlled via Unix permissions.  This is not SELinux-friendly,
> but it is nicer for testing and it is also the only possibility for user
> libvirtd.
> 
> In that case, upgrading QEMU on the host could result in a "systemctl
> restart qemu-pr-helper.service" (or, hopefully unlikely, a crash could
> result in systemd respawning the daemon).  Reconnect is useful in that case.

If using systemd socket activation and you restart the daemon, the listening
socket should be preserved, so you shouldn't need to reconnect - the client
should get queued until it has started again (likewise on crash).

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Daniel P. Berrange]

On Tue, Sep 19, 2017 at 03:23:09PM +0200, Paolo Bonzini wrote:
> On 19/09/2017 15:12, Daniel P. Berrange wrote:
> > On Tue, Sep 19, 2017 at 02:57:00PM +0200, Paolo Bonzini wrote:
> >> On 19/09/2017 14:53, Daniel P. Berrange wrote:
>  +/* Try to reconnect while sending the CDB.  */
>  +for (attempts = 0; attempts < PR_MAX_RECONNECT_ATTEMPTS; 
>  attempts++) {
> >>>
> >>> I'm curious why you need to loop here. The helper daemon should be running
> >>> already, as you're not spawning it on demand IIUC. So it shoudl either
> >>> succeed first time, or fail every time.
> >>
> >> You're focusing on the usecase where the helper daemon is spawned per-VM
> >> by the system libvirtd, which I agree is the most important one.
> >> However, the other usecase is the one with a global daemon, access to
> >> which is controlled via Unix permissions.  This is not SELinux-friendly,
> >> but it is nicer for testing and it is also the only possibility for user
> >> libvirtd.
> >>
> >> In that case, upgrading QEMU on the host could result in a "systemctl
> >> restart qemu-pr-helper.service" (or, hopefully unlikely, a crash could
> >> result in systemd respawning the daemon).  Reconnect is useful in that 
> >> case.
> > 
> > If using systemd socket activation and you restart the daemon, the listening
> > socket should be preserved, so you shouldn't need to reconnect - the client
> > should get queued until it has started again (likewise on crash).
> 
> Oh, that's cool.  I didn't know that.  However, systemd socket
> activation is optional, and it's only a handful of lines so I think it's
> a bit nicer behavior (chardevs for example have options to reconnect).

The downside is that if someone forget to start the daemon, or enable
the socket,  QEMU will spin for 5 seconds trying to reconnect, instead
of reporting an error immediately. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] scsi: add persistent reservation manager using qemu-pr-helper

[Daniel P. Berrange]

On Tue, Sep 19, 2017 at 12:24:34PM +0200, Paolo Bonzini wrote:
> This adds a concrete subclass of pr-manager that talks to qemu-pr-helper.
> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: fixed string property double-free
> fixed/cleaned up error handling
> handle buffer underrun
> 
>  scsi/Makefile.objs   |   2 +-
>  scsi/pr-manager-helper.c | 302 
> +++
>  2 files changed, 303 insertions(+), 1 deletion(-)
>  create mode 100644 scsi/pr-manager-helper.c


> +static int pr_manager_helper_run(PRManager *p,
> + int fd, struct sg_io_hdr *io_hdr)
> +{
> +PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(p);
> +
> +uint32_t len;
> +PRHelperResponse resp;
> +int ret;
> +int expected_dir;
> +int attempts;
> +uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
> +
> +if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
> +return -EINVAL;
> +}
> +
> +memcpy(cdb, io_hdr->cmdp, io_hdr->cmd_len);
> +assert(cdb[0] == PERSISTENT_RESERVE_OUT || cdb[0] == 
> PERSISTENT_RESERVE_IN);
> +expected_dir =
> +(cdb[0] == PERSISTENT_RESERVE_OUT ? SG_DXFER_TO_DEV : 
> SG_DXFER_FROM_DEV);
> +if (io_hdr->dxfer_direction != expected_dir) {
> +return -EINVAL;
> +}
> +
> +len = scsi_cdb_xfer(cdb);
> +if (io_hdr->dxfer_len < len || len > PR_HELPER_DATA_SIZE) {
> +return -EINVAL;
> +}
> +
> +qemu_mutex_lock(_mgr->lock);
> +
> +/* Try to reconnect while sending the CDB.  */
> +for (attempts = 0; attempts < PR_MAX_RECONNECT_ATTEMPTS; attempts++) {

I'm curious why you need to loop here. The helper daemon should be running
already, as you're not spawning it on demand IIUC. So it shoudl either
succeed first time, or fail every time.

> +if (!pr_mgr->ioc) {
> +ret = pr_manager_helper_initialize(pr_mgr, NULL);
> +if (ret < 0) {
> +qemu_mutex_unlock(_mgr->lock);
> +g_usleep(G_USEC_PER_SEC);
> +qemu_mutex_lock(_mgr->lock);
> +continue;
> +}
> +}
> +
> +ret = pr_manager_helper_write(pr_mgr, fd, cdb, ARRAY_SIZE(cdb), 
> NULL);
> +if (ret >= 0) {
> +break;
> +}
> +}
> +if (ret < 0) {
> +goto out;
> +}

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|