| Message ID | 1464943756-14143-4-git-send-email-famz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben: > Respect the locking mode from CLI or QMP, and set the open flags > accordingly. > > Signed-off-by: Fam Zheng <famz@redhat.com> > Reviewed-by: Max Reitz <mreitz@redhat.com> > --- > blockdev.c | 23 +++++++++++++++++++++++ > qemu-options.hx | 1 + > 2 files changed, 24 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index 717785e..5acb286 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -356,6 +356,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags, > const char *discard; > Error *local_error = NULL; > const char *aio; > + const char *lock_mode; > > if (bdrv_flags) { > if (!qemu_opt_get_bool(opts, "read-only", false)) { > @@ -382,6 +383,18 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags, > return; > } > } > + > + lock_mode = qemu_opt_get(opts, "lock-mode") ? : "off"; > + if (!strcmp(lock_mode, "exclusive")) { > + /* Default */ > + } else if (!strcmp(lock_mode, "shared")) { > + *bdrv_flags |= BDRV_O_SHARED_LOCK; > + } else if (!strcmp(lock_mode, "off")) { > + *bdrv_flags |= BDRV_O_NO_LOCK; > + } else { > + error_setg(errp, "invalid lock mode"); > + return; > + } > } > > /* disk I/O throttling */ > @@ -4296,6 +4309,11 @@ QemuOptsList qemu_common_drive_opts = { > .type = QEMU_OPT_BOOL, > .help = "whether to account for failed I/O operations " > "in the statistics", > + },{ > + .name = "lock-mode", > + .type = QEMU_OPT_STRING, > + .help = "how to lock the image (exclusive, shared, off. " > + "default: exclusive)", > }, > { /* end of list */ } > }, > @@ -4325,6 +4343,11 @@ static QemuOptsList qemu_root_bds_opts = { > .name = "detect-zeroes", > .type = QEMU_OPT_STRING, > .help = "try to optimize zero writes (off, on, unmap)", > + },{ > + .name = "lock-mode", > + .type = QEMU_OPT_STRING, > + .help = "how to lock the image (exclusive, shared, off. " > + "default: exclusive)", > }, > { /* end of list */ } > }, This is the wrong level to implement the feature. You would only be able to configure the locking on the top level image this way (because what we're doing here is BB, not BDS configuration). If you want to allow configuration per node, you need to put the options into bdrv_runtime_opts and interpret them in bdrv_open_common(). Otherwise we would have to specify in the blockdev-add documentation that this works only on the top level, but I don't see a reason for such a restriction. Kevin
Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben: > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben: > > Respect the locking mode from CLI or QMP, and set the open flags > > accordingly. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > Reviewed-by: Max Reitz <mreitz@redhat.com> > This is the wrong level to implement the feature. You would only be able > to configure the locking on the top level image this way (because what > we're doing here is BB, not BDS configuration). If you want to allow > configuration per node, you need to put the options into > bdrv_runtime_opts and interpret them in bdrv_open_common(). > > Otherwise we would have to specify in the blockdev-add documentation > that this works only on the top level, but I don't see a reason for > such a restriction. And actually, after some more thinking about block device configuration, I'm not sure any more whether letting the user configure this on the node level, at least as the primary interface. A node usually knows by itself what locking mode it needs to request from its children, depending on the locking mode that the parent node requested for it. It could be passing down the locking mode (raw format), it could require a stricter locking mode (non-raw formats never work with r/w sharing) or it could even be less strict (backing files are normally ro/ and can therefore be shared, even if the guest can't share its image). The real origin of the locking requirement is the top level. So maybe the right interface for guest devices is adding a qdev option that tells whether the guest can share the image. For NBD servers, we'd add a QMP option that tells whether client can share the image. And starting from these requirements, the locking mode would propagate through the graph, with each node deciding what it needs to request from its children in order to achieve the protection that its parent requested. And at this point I start wondering... Doesn't this look an awful lot like op blockers? (The new ones.) Should image locking be integrated there? I still see a (limited) use for the node-level configuration: The user might want to request a stricter locking mode than is necessary because they foresee an operation that will change the requirements (e.g. commit to a backing file) and they don't want to risk failure then. Any opinions? Kevin
On Tue, 07/05 15:37, Kevin Wolf wrote: > Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben: > > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben: > > > Respect the locking mode from CLI or QMP, and set the open flags > > > accordingly. > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > This is the wrong level to implement the feature. You would only be able > > to configure the locking on the top level image this way (because what > > we're doing here is BB, not BDS configuration). If you want to allow > > configuration per node, you need to put the options into > > bdrv_runtime_opts and interpret them in bdrv_open_common(). > > > > Otherwise we would have to specify in the blockdev-add documentation > > that this works only on the top level, but I don't see a reason for > > such a restriction. > > And actually, after some more thinking about block device configuration, > I'm not sure any more whether letting the user configure this on the > node level, at least as the primary interface. > > A node usually knows by itself what locking mode it needs to request > from its children, depending on the locking mode that the parent node > requested for it. It could be passing down the locking mode (raw > format), it could require a stricter locking mode (non-raw formats never > work with r/w sharing) or it could even be less strict (backing files > are normally ro/ and can therefore be shared, even if the guest can't > share its image). > > The real origin of the locking requirement is the top level. So maybe > the right interface for guest devices is adding a qdev option that tells > whether the guest can share the image. For NBD servers, we'd add a QMP I think most block devices are not designed to share the data, so in general it's hard to imagine this as a device property. > option that tells whether client can share the image. And starting from > these requirements, the locking mode would propagate through the graph, > with each node deciding what it needs to request from its children in > order to achieve the protection that its parent requested. > > And at this point I start wondering... Doesn't this look an awful lot > like op blockers? (The new ones.) Should image locking be integrated > there? > > I still see a (limited) use for the node-level configuration: The user > might want to request a stricter locking mode than is necessary because > they foresee an operation that will change the requirements (e.g. commit > to a backing file) and they don't want to risk failure then. > > Any opinions? Who is going to enable the default auto lock with an unattached (no BB or no device) image, such as the qemu-img case? Lock mode there needs to be configurable too, but moving the option away from the BB/BDS makes this trickier to do. Fam
Am 08.07.2016 um 04:56 hat Fam Zheng geschrieben: > On Tue, 07/05 15:37, Kevin Wolf wrote: > > Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben: > > > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben: > > > > Respect the locking mode from CLI or QMP, and set the open flags > > > > accordingly. > > > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > > > This is the wrong level to implement the feature. You would only be able > > > to configure the locking on the top level image this way (because what > > > we're doing here is BB, not BDS configuration). If you want to allow > > > configuration per node, you need to put the options into > > > bdrv_runtime_opts and interpret them in bdrv_open_common(). > > > > > > Otherwise we would have to specify in the blockdev-add documentation > > > that this works only on the top level, but I don't see a reason for > > > such a restriction. > > > > And actually, after some more thinking about block device configuration, > > I'm not sure any more whether letting the user configure this on the > > node level, at least as the primary interface. > > > > A node usually knows by itself what locking mode it needs to request > > from its children, depending on the locking mode that the parent node > > requested for it. It could be passing down the locking mode (raw > > format), it could require a stricter locking mode (non-raw formats never > > work with r/w sharing) or it could even be less strict (backing files > > are normally ro/ and can therefore be shared, even if the guest can't > > share its image). > > > > The real origin of the locking requirement is the top level. So maybe > > the right interface for guest devices is adding a qdev option that tells > > whether the guest can share the image. For NBD servers, we'd add a QMP > > I think most block devices are not designed to share the data, so in general > it's hard to imagine this as a device property. Well, it's really a guest OS (or even guest application) property, but obviously that doesn't exist. And the device is the qemu component that is the closest to the guest. We generally have options about behaviour that the guest expects at the device level. > > option that tells whether client can share the image. And starting from > > these requirements, the locking mode would propagate through the graph, > > with each node deciding what it needs to request from its children in > > order to achieve the protection that its parent requested. > > > > And at this point I start wondering... Doesn't this look an awful lot > > like op blockers? (The new ones.) Should image locking be integrated > > there? > > > > I still see a (limited) use for the node-level configuration: The user > > might want to request a stricter locking mode than is necessary because > > they foresee an operation that will change the requirements (e.g. commit > > to a backing file) and they don't want to risk failure then. > > > > Any opinions? > > Who is going to enable the default auto lock with an unattached (no BB or no > device) image, such as the qemu-img case? Lock mode there needs to be > configurable too, but moving the option away from the BB/BDS makes this > trickier to do. Unattached BDSes don't get I/O. qemu-img does use a BB. Do you actually need to configure the locking mode for qemu-img or is just a switch for ignoring locks enough? Your -L could be implemented more or less the same way as it is now. You got a user option and you pass it down to the lower layers. The exact way of how you pass it down (flags vs. possibly some op blocker thing) could change, but it doesn't really change much conceptually. Kevin
On 08.07.2016 11:50, Kevin Wolf wrote: > Am 08.07.2016 um 04:56 hat Fam Zheng geschrieben: >> On Tue, 07/05 15:37, Kevin Wolf wrote: >>> Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben: >>>> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben: >>>>> Respect the locking mode from CLI or QMP, and set the open flags >>>>> accordingly. >>>>> >>>>> Signed-off-by: Fam Zheng <famz@redhat.com> >>>>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>> >>>> This is the wrong level to implement the feature. You would only be able >>>> to configure the locking on the top level image this way (because what >>>> we're doing here is BB, not BDS configuration). If you want to allow >>>> configuration per node, you need to put the options into >>>> bdrv_runtime_opts and interpret them in bdrv_open_common(). >>>> >>>> Otherwise we would have to specify in the blockdev-add documentation >>>> that this works only on the top level, but I don't see a reason for >>>> such a restriction. >>> >>> And actually, after some more thinking about block device configuration, >>> I'm not sure any more whether letting the user configure this on the >>> node level, at least as the primary interface. >>> >>> A node usually knows by itself what locking mode it needs to request >>> from its children, depending on the locking mode that the parent node >>> requested for it. It could be passing down the locking mode (raw >>> format), it could require a stricter locking mode (non-raw formats never >>> work with r/w sharing) or it could even be less strict (backing files >>> are normally ro/ and can therefore be shared, even if the guest can't >>> share its image). >>> >>> The real origin of the locking requirement is the top level. So maybe >>> the right interface for guest devices is adding a qdev option that tells >>> whether the guest can share the image. For NBD servers, we'd add a QMP >> >> I think most block devices are not designed to share the data, so in general >> it's hard to imagine this as a device property. > > Well, it's really a guest OS (or even guest application) property, but > obviously that doesn't exist. And the device is the qemu component that > is the closest to the guest. We generally have options about behaviour > that the guest expects at the device level. Can the guest device really make a qualified decision here? If you're using raw as the image format, sharing may be fine, if you're using qcow2, it most likely is not. I think whether to lock a BDS chain is a host-side property and has not a lot to do with the guest, thus it should be a BDS property. I can imagine that a guest may say that sharing should be disallowed under all circumstances, but a guest is never able to decide to allow sharing. Max
Am 08.07.2016 um 15:05 hat Max Reitz geschrieben: > On 08.07.2016 11:50, Kevin Wolf wrote: > > Am 08.07.2016 um 04:56 hat Fam Zheng geschrieben: > >> On Tue, 07/05 15:37, Kevin Wolf wrote: > >>> Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben: > >>>> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben: > >>>>> Respect the locking mode from CLI or QMP, and set the open flags > >>>>> accordingly. > >>>>> > >>>>> Signed-off-by: Fam Zheng <famz@redhat.com> > >>>>> Reviewed-by: Max Reitz <mreitz@redhat.com> > >>> > >>>> This is the wrong level to implement the feature. You would only be able > >>>> to configure the locking on the top level image this way (because what > >>>> we're doing here is BB, not BDS configuration). If you want to allow > >>>> configuration per node, you need to put the options into > >>>> bdrv_runtime_opts and interpret them in bdrv_open_common(). > >>>> > >>>> Otherwise we would have to specify in the blockdev-add documentation > >>>> that this works only on the top level, but I don't see a reason for > >>>> such a restriction. > >>> > >>> And actually, after some more thinking about block device configuration, > >>> I'm not sure any more whether letting the user configure this on the > >>> node level, at least as the primary interface. > >>> > >>> A node usually knows by itself what locking mode it needs to request > >>> from its children, depending on the locking mode that the parent node > >>> requested for it. It could be passing down the locking mode (raw > >>> format), it could require a stricter locking mode (non-raw formats never > >>> work with r/w sharing) or it could even be less strict (backing files > >>> are normally ro/ and can therefore be shared, even if the guest can't > >>> share its image). > >>> > >>> The real origin of the locking requirement is the top level. So maybe > >>> the right interface for guest devices is adding a qdev option that tells > >>> whether the guest can share the image. For NBD servers, we'd add a QMP > >> > >> I think most block devices are not designed to share the data, so in general > >> it's hard to imagine this as a device property. > > > > Well, it's really a guest OS (or even guest application) property, but > > obviously that doesn't exist. And the device is the qemu component that > > is the closest to the guest. We generally have options about behaviour > > that the guest expects at the device level. > > Can the guest device really make a qualified decision here? If you're > using raw as the image format, sharing may be fine, if you're using > qcow2, it most likely is not. Just noticed while looking at v7 that I never replied here... Yes, you're right that the guest device can't make the decision for the exact locking mode of the images. But the device is the point where the user has to decide whether or not sharing is okay. For everything below it, qemu knows by itself whether it can share the image. This is essentially what I already described above: > >>> A node usually knows by itself what locking mode it needs to request > >>> from its children, depending on the locking mode that the parent node > >>> requested for it. It could be passing down the locking mode (raw > >>> format), it could require a stricter locking mode (non-raw formats never > >>> work with r/w sharing) or it could even be less strict (backing files > >>> are normally ro/ and can therefore be shared, even if the guest can't > >>> share its image). > I think whether to lock a BDS chain is a host-side property and has not > a lot to do with the guest, thus it should be a BDS property. I can > imagine that a guest may say that sharing should be disallowed under all > circumstances, but a guest is never able to decide to allow sharing. Well, yes and no. The decision which lock mode to use is at the node level, no doubt. But it's not something that a user can configure. Non-raw images simply can't be shared and the user can't do anything about it. Why should they have an option to specify a lock mode when there is only one correct setting? The only possible exception where an option on the node level could make sense (which I already mentioned earlier in this thread) is if the user wants to be stricter than what qemu needs. Kevin
On Tue, Sep 06, 2016 at 06:45:55PM +0200, Kevin Wolf wrote: > Am 08.07.2016 um 15:05 hat Max Reitz geschrieben: > > I think whether to lock a BDS chain is a host-side property and has not > > a lot to do with the guest, thus it should be a BDS property. I can > > imagine that a guest may say that sharing should be disallowed under all > > circumstances, but a guest is never able to decide to allow sharing. > > Well, yes and no. The decision which lock mode to use is at the node > level, no doubt. But it's not something that a user can configure. > Non-raw images simply can't be shared and the user can't do anything > about it. Why should they have an option to specify a lock mode when > there is only one correct setting? I'm not sure that's true for all non-raw images. I understand things like qcow2 won't work, because of cached metadata which can be updated by QEMU during writes, but I think the luks format ought to be safe as the cached metadata is never changed. Regards, Daniel
Am 06.09.2016 um 18:51 hat Daniel P. Berrange geschrieben: > On Tue, Sep 06, 2016 at 06:45:55PM +0200, Kevin Wolf wrote: > > Am 08.07.2016 um 15:05 hat Max Reitz geschrieben: > > > I think whether to lock a BDS chain is a host-side property and has not > > > a lot to do with the guest, thus it should be a BDS property. I can > > > imagine that a guest may say that sharing should be disallowed under all > > > circumstances, but a guest is never able to decide to allow sharing. > > > > Well, yes and no. The decision which lock mode to use is at the node > > level, no doubt. But it's not something that a user can configure. > > Non-raw images simply can't be shared and the user can't do anything > > about it. Why should they have an option to specify a lock mode when > > there is only one correct setting? > > I'm not sure that's true for all non-raw images. I understand things like > qcow2 won't work, because of cached metadata which can be updated by QEMU > during writes, but I think the luks format ought to be safe as the cached > metadata is never changed. Yes, I guess I've been overgeneralising. Doesn't really make a difference for my point, though, the luks driver still knows what it needs to request from the lower layer without user intervention. Kevin
diff --git a/blockdev.c b/blockdev.c index 717785e..5acb286 100644 --- a/blockdev.c +++ b/blockdev.c @@ -356,6 +356,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags, const char *discard; Error *local_error = NULL; const char *aio; + const char *lock_mode; if (bdrv_flags) { if (!qemu_opt_get_bool(opts, "read-only", false)) { @@ -382,6 +383,18 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags, return; } } + + lock_mode = qemu_opt_get(opts, "lock-mode") ? : "off"; + if (!strcmp(lock_mode, "exclusive")) { + /* Default */ + } else if (!strcmp(lock_mode, "shared")) { + *bdrv_flags |= BDRV_O_SHARED_LOCK; + } else if (!strcmp(lock_mode, "off")) { + *bdrv_flags |= BDRV_O_NO_LOCK; + } else { + error_setg(errp, "invalid lock mode"); + return; + } } /* disk I/O throttling */ @@ -4296,6 +4309,11 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_BOOL, .help = "whether to account for failed I/O operations " "in the statistics", + },{ + .name = "lock-mode", + .type = QEMU_OPT_STRING, + .help = "how to lock the image (exclusive, shared, off. " + "default: exclusive)", }, { /* end of list */ } }, @@ -4325,6 +4343,11 @@ static QemuOptsList qemu_root_bds_opts = { .name = "detect-zeroes", .type = QEMU_OPT_STRING, .help = "try to optimize zero writes (off, on, unmap)", + },{ + .name = "lock-mode", + .type = QEMU_OPT_STRING, + .help = "how to lock the image (exclusive, shared, off. " + "default: exclusive)", }, { /* end of list */ } }, diff --git a/qemu-options.hx b/qemu-options.hx index 6106520..731cdda 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -524,6 +524,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, " [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n" " [,readonly=on|off][,copy-on-read=on|off]\n" " [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n" + " [,lock-mode=exclusive|shared|off]\n" " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n" " [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n" " [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"