[3/7] nbd: use BDS refcount
diff mbox

Message ID 1372744789-997-4-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 2, 2013, 5:59 a.m. UTC
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(-)

Comments

Paolo Bonzini July 2, 2013, 10:16 a.m. UTC | #1
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)
>
Fam Zheng July 3, 2013, 1:10 a.m. UTC | #2
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)
> > 
>
Paolo Bonzini July 3, 2013, 5:58 a.m. UTC | #3
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
Fam Zheng July 3, 2013, 6:30 a.m. UTC | #4
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?
Paolo Bonzini July 3, 2013, 7:28 a.m. UTC | #5
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

Patch
diff mbox

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)