| Message ID | 1372744789-997-4-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 02/07/2013 07:59, Fam Zheng ha scritto: > Previously, nbd call drive_get_ref on the drive of bs. A BDS doesn't > always have associated dinfo, it's more proper to use bdrv_get_ref(). This has the important side effect of marking the exported disk as "in_use" (to use the terms before the series). Right now you can serve a disk and, at the same time, stream it or mirror it or create a live snapshot of it. Do we really want to block anything for a device being served? Perhaps truncation, but maybe not even that. The NBD server is meant to be as unobtrusive as possible (in some sense NBD accesses are the same as guest accesses). Paolo > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > blockdev-nbd.c | 9 +-------- > nbd.c | 5 +++++ > 2 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index 95f10c8..d8bcd6f 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -69,12 +69,6 @@ static void nbd_close_notifier(Notifier *n, void *data) > g_free(cn); > } > > -static void nbd_server_put_ref(NBDExport *exp) > -{ > - BlockDriverState *bs = nbd_export_get_blockdev(exp); > - drive_put_ref(drive_get_by_blockdev(bs)); > -} > - > void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, > Error **errp) > { > @@ -106,10 +100,9 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, > } > > exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, > - nbd_server_put_ref); > + NULL); > > nbd_export_set_name(exp, device); > - drive_get_ref(drive_get_by_blockdev(bs)); > > n = g_malloc0(sizeof(NBDCloseNotifier)); > n->n.notify = nbd_close_notifier; > diff --git a/nbd.c b/nbd.c > index 2606403..f28b9fb 100644 > --- a/nbd.c > +++ b/nbd.c > @@ -881,6 +881,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, > exp->nbdflags = nbdflags; > exp->size = size == -1 ? bdrv_getlength(bs) : size; > exp->close = close; > + bdrv_get_ref(bs); > return exp; > } > > @@ -927,6 +928,10 @@ void nbd_export_close(NBDExport *exp) > } > nbd_export_set_name(exp, NULL); > nbd_export_put(exp); > + if (exp->bs) { > + bdrv_put_ref(exp->bs); > + exp->bs = NULL; > + } > } > > void nbd_export_get(NBDExport *exp) >
On Tue, 07/02 12:16, Paolo Bonzini wrote: > Il 02/07/2013 07:59, Fam Zheng ha scritto: > > Previously, nbd call drive_get_ref on the drive of bs. A BDS doesn't > > always have associated dinfo, it's more proper to use bdrv_get_ref(). > > This has the important side effect of marking the exported disk as > "in_use" (to use the terms before the series). Right now you can serve > a disk and, at the same time, stream it or mirror it or create a live > snapshot of it. > > Do we really want to block anything for a device being served? Perhaps > truncation, but maybe not even that. The NBD server is meant to be as > unobtrusive as possible (in some sense NBD accesses are the same as > guest accesses). > > Paolo > OK, it is better to work like that. But I don't quite understand why was there drive_get_ref() on the device (w/o the series), as there's already a close notifier? And it just drive_put_ref() when bs is closed? > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > blockdev-nbd.c | 9 +-------- > > nbd.c | 5 +++++ > > 2 files changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > > index 95f10c8..d8bcd6f 100644 > > --- a/blockdev-nbd.c > > +++ b/blockdev-nbd.c > > @@ -69,12 +69,6 @@ static void nbd_close_notifier(Notifier *n, void *data) > > g_free(cn); > > } > > > > -static void nbd_server_put_ref(NBDExport *exp) > > -{ > > - BlockDriverState *bs = nbd_export_get_blockdev(exp); > > - drive_put_ref(drive_get_by_blockdev(bs)); > > -} > > - > > void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, > > Error **errp) > > { > > @@ -106,10 +100,9 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, > > } > > > > exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, > > - nbd_server_put_ref); > > + NULL); > > > > nbd_export_set_name(exp, device); > > - drive_get_ref(drive_get_by_blockdev(bs)); > > > > n = g_malloc0(sizeof(NBDCloseNotifier)); > > n->n.notify = nbd_close_notifier; > > diff --git a/nbd.c b/nbd.c > > index 2606403..f28b9fb 100644 > > --- a/nbd.c > > +++ b/nbd.c > > @@ -881,6 +881,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, > > exp->nbdflags = nbdflags; > > exp->size = size == -1 ? bdrv_getlength(bs) : size; > > exp->close = close; > > + bdrv_get_ref(bs); > > return exp; > > } > > > > @@ -927,6 +928,10 @@ void nbd_export_close(NBDExport *exp) > > } > > nbd_export_set_name(exp, NULL); > > nbd_export_put(exp); > > + if (exp->bs) { > > + bdrv_put_ref(exp->bs); > > + exp->bs = NULL; > > + } > > } > > > > void nbd_export_get(NBDExport *exp) > > >
Il 03/07/2013 03:10, Fam Zheng ha scritto: > > This has the important side effect of marking the exported disk as > > "in_use" (to use the terms before the series). Right now you can serve > > a disk and, at the same time, stream it or mirror it or create a live > > snapshot of it. > > > > Do we really want to block anything for a device being served? Perhaps > > truncation, but maybe not even that. The NBD server is meant to be as > > unobtrusive as possible (in some sense NBD accesses are the same as > > guest accesses). > > OK, it is better to work like that. But I don't quite understand why was > there drive_get_ref() on the device (w/o the series), as there's already > a close notifier? And it just drive_put_ref() when bs is closed? The close notifier runs when the user invokes a drive_del or eject command from the monitor. The drive_get_ref/drive_put_ref delays the bdrv_delete until after nbd.c has cleaned up all the connections. Paolo
On Wed, 07/03 07:58, Paolo Bonzini wrote: > Il 03/07/2013 03:10, Fam Zheng ha scritto: > > > This has the important side effect of marking the exported disk as > > > "in_use" (to use the terms before the series). Right now you can serve > > > a disk and, at the same time, stream it or mirror it or create a live > > > snapshot of it. > > > > > > Do we really want to block anything for a device being served? Perhaps > > > truncation, but maybe not even that. The NBD server is meant to be as > > > unobtrusive as possible (in some sense NBD accesses are the same as > > > guest accesses). > > > > OK, it is better to work like that. But I don't quite understand why was > > there drive_get_ref() on the device (w/o the series), as there's already > > a close notifier? And it just drive_put_ref() when bs is closed? > > The close notifier runs when the user invokes a drive_del or eject > command from the monitor. The drive_get_ref/drive_put_ref delays the > bdrv_delete until after nbd.c has cleaned up all the connections. But drive_put_ref is called by close notifier. I think it can be omitted, registering a close notifier is enough, and close the export when drive_del calls it. It doesn't make more sense w/ drive_get_ref, does it?
Il 03/07/2013 08:30, Fam Zheng ha scritto: >> > The close notifier runs when the user invokes a drive_del or eject >> > command from the monitor. The drive_get_ref/drive_put_ref delays the >> > bdrv_delete until after nbd.c has cleaned up all the connections. > But drive_put_ref is called by close notifier. Not necessarily. nbd_export_close calls nbd_client_close, which shuts down the socket. However, if requests are being processed, they will complete after nbd_export_close returns. Completing the requests leads to the following call chain: nbd_request_put (from nbd_trip) calls nbd_client_put calls nbd_export_put calls exp->close (if refcount goes to 0) calls drive_put_ref Completion will happen as soon as the main loop runs again, because after shutdown() the reads and writes will fail. Still, it is asynchronous, hence the call to drive_put_ref is also asynchronous. > I think it can be > omitted, registering a close notifier is enough, and close the export > when drive_del calls it. It doesn't make more sense w/ drive_get_ref, > does it? I think that would cause a dangling pointer if NBD requests are being processed at the time drive_del runs. Paolo
diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 95f10c8..d8bcd6f 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -69,12 +69,6 @@ static void nbd_close_notifier(Notifier *n, void *data) g_free(cn); } -static void nbd_server_put_ref(NBDExport *exp) -{ - BlockDriverState *bs = nbd_export_get_blockdev(exp); - drive_put_ref(drive_get_by_blockdev(bs)); -} - void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, Error **errp) { @@ -106,10 +100,9 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, } exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, - nbd_server_put_ref); + NULL); nbd_export_set_name(exp, device); - drive_get_ref(drive_get_by_blockdev(bs)); n = g_malloc0(sizeof(NBDCloseNotifier)); n->n.notify = nbd_close_notifier; diff --git a/nbd.c b/nbd.c index 2606403..f28b9fb 100644 --- a/nbd.c +++ b/nbd.c @@ -881,6 +881,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, exp->nbdflags = nbdflags; exp->size = size == -1 ? bdrv_getlength(bs) : size; exp->close = close; + bdrv_get_ref(bs); return exp; } @@ -927,6 +928,10 @@ void nbd_export_close(NBDExport *exp) } nbd_export_set_name(exp, NULL); nbd_export_put(exp); + if (exp->bs) { + bdrv_put_ref(exp->bs); + exp->bs = NULL; + } } void nbd_export_get(NBDExport *exp)
Previously, nbd call drive_get_ref on the drive of bs. A BDS doesn't always have associated dinfo, it's more proper to use bdrv_get_ref(). Signed-off-by: Fam Zheng <famz@redhat.com> --- blockdev-nbd.c | 9 +-------- nbd.c | 5 +++++ 2 files changed, 6 insertions(+), 8 deletions(-)