| Message ID | 7bfce913f68d95284746e7cf3693703f5361b26f.1454940776.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
Am 08.02.2016 um 15:14 hat Alberto Garcia geschrieben: > When x-blockdev-del is performed on a BlockBackend that has inserted > media it will only succeed if the BDS doesn't have any additional > references. > > The only problem with this is that if the BDS was created separately > using blockdev-add then the backend won't be able to be destroyed > unless the BDS is ejected first. This is an unnecessary restriction. > > Now that we have a list of monitor-owned BDSs we can allow > x-blockdev-del to work in this scenario if the BDS has exactly one > extra reference and that reference is from the monitor. > > Signed-off-by: Alberto Garcia <berto@igalia.com> This means that what you created with two blockdev-add commands would be destroyed with a single blockdev-del command. I think we all agreed that blockdev-add and blockdev-del should always come in pairs. Kevin
On Tue 09 Feb 2016 11:32:20 AM CET, Kevin Wolf <kwolf@redhat.com> wrote: >> When x-blockdev-del is performed on a BlockBackend that has inserted >> media it will only succeed if the BDS doesn't have any additional >> references. >> >> The only problem with this is that if the BDS was created separately >> using blockdev-add then the backend won't be able to be destroyed >> unless the BDS is ejected first. This is an unnecessary restriction. >> >> Now that we have a list of monitor-owned BDSs we can allow >> x-blockdev-del to work in this scenario if the BDS has exactly one >> extra reference and that reference is from the monitor. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> > > This means that what you created with two blockdev-add commands would be > destroyed with a single blockdev-del command. I think we all agreed that > blockdev-add and blockdev-del should always come in pairs. No, no. I think you are misunderstanding the use case. Consider this example (testAttachMedia() from iotest 139): 1) blockdev-add creates 'drive0' with a BDS called 'node0' 2) 'node0' is removed from 'drive0' (and therefore destroyed) 3) 'node1' is added using blockdev-add 4) 'node1' is inserted into 'drive0' Now we want to destroy both 'drive0' and 'node1'. With the current code we can only do it like this: 5) 'node1' is removed from 'drive0', but not destroyed because it still has the monitor reference. 6) 'drive0' is destroyed with x-blockdev-del 7) 'node1' is destroyed with x-blockdev-del The order of 6) and 7) can be changed without problems. However, 5) is necessary because otherwise 'x-blockdev-del drive0' won't work ('node1' has 2 references at that point). I claim that step 5) should be unnecessary. If we can guarantee that 'node1' has only two references: the one held by 'drive0' and the monitor reference, then it should be possible to do 'x-blockdev-del drive0' when 'node1' is inserted. This would destroy 'drive0' but 'node1' would still remain because of its monitor reference. Now we can do 'x-blockdev-del node1' and complete the operation. Berto
On 02/08/2016 07:14 AM, Alberto Garcia wrote: When sending a multi-patch series, you should always include a 0/3 cover letter. The cover letter is optional only for a lone patch. > When x-blockdev-del is performed on a BlockBackend that has inserted > media it will only succeed if the BDS doesn't have any additional > references. > > The only problem with this is that if the BDS was created separately > using blockdev-add then the backend won't be able to be destroyed > unless the BDS is ejected first. This is an unnecessary restriction. > > Now that we have a list of monitor-owned BDSs we can allow > x-blockdev-del to work in this scenario if the BDS has exactly one > extra reference and that reference is from the monitor. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > blockdev.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index e1b6b0f..847058d 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3966,9 +3966,16 @@ void qmp_x_blockdev_del(bool has_id, const char *id, > } > > if (bs->refcnt > 1) { > - error_setg(errp, "Block device %s is in use", > - bdrv_get_device_or_node_name(bs)); > - goto out; > + /* We allow deleting a BlockBackend that has a BDS with an > + * extra reference if that extra reference is from the > + * monitor. */ > + bool bs_has_only_monitor_ref = > + blk && bs->monitor_list.tqe_prev && bs->refcnt == 2; > + if (!bs_has_only_monitor_ref) { I don't think the temporary bool or nested 'if' are necessary; but at the same time, I don't think the following is any more legible: /* Prohibit deleting a BlockBackend whose BDS is in use by any more than a single monitor */ if (bs->refcnt > 1 + (blk && bs->monitor_list.tqe_prev)) { error_setg(... so I could live with your patch as-is: Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue 09 Feb 2016 04:34:09 PM CET, Eric Blake wrote: > On 02/08/2016 07:14 AM, Alberto Garcia wrote: > > When sending a multi-patch series, you should always include a 0/3 cover > letter. The cover letter is optional only for a lone patch. Sorry, I didn't know. The description of the first patch already contains everything that would go into the cover letter, that's why I decided to do it like this. I'll include the cover letter in the future. >> if (bs->refcnt > 1) { >> - error_setg(errp, "Block device %s is in use", >> - bdrv_get_device_or_node_name(bs)); >> - goto out; >> + /* We allow deleting a BlockBackend that has a BDS with an >> + * extra reference if that extra reference is from the >> + * monitor. */ >> + bool bs_has_only_monitor_ref = >> + blk && bs->monitor_list.tqe_prev && bs->refcnt == 2; >> + if (!bs_has_only_monitor_ref) { > > I don't think the temporary bool or nested 'if' are necessary; but at > the same time, I don't think the following is any more legible: > > /* Prohibit deleting a BlockBackend whose BDS is in use by any more than > a single monitor */ > if (bs->refcnt > 1 + (blk && bs->monitor_list.tqe_prev)) { > error_setg(... Exactly, I considered several options and I thought the one I finally chose would be the easiest to read. Thanks! Berto
diff --git a/blockdev.c b/blockdev.c index e1b6b0f..847058d 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3966,9 +3966,16 @@ void qmp_x_blockdev_del(bool has_id, const char *id, } if (bs->refcnt > 1) { - error_setg(errp, "Block device %s is in use", - bdrv_get_device_or_node_name(bs)); - goto out; + /* We allow deleting a BlockBackend that has a BDS with an + * extra reference if that extra reference is from the + * monitor. */ + bool bs_has_only_monitor_ref = + blk && bs->monitor_list.tqe_prev && bs->refcnt == 2; + if (!bs_has_only_monitor_ref) { + error_setg(errp, "Block device %s is in use", + bdrv_get_device_or_node_name(bs)); + goto out; + } } }
When x-blockdev-del is performed on a BlockBackend that has inserted media it will only succeed if the BDS doesn't have any additional references. The only problem with this is that if the BDS was created separately using blockdev-add then the backend won't be able to be destroyed unless the BDS is ejected first. This is an unnecessary restriction. Now that we have a list of monitor-owned BDSs we can allow x-blockdev-del to work in this scenario if the BDS has exactly one extra reference and that reference is from the monitor. Signed-off-by: Alberto Garcia <berto@igalia.com> --- blockdev.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)