[Libguestfs] [libnbd PATCH v3 2/5] python: Alter lock for persistent buffer

[Richard W.M. Jones]

On Thu, Jun 09, 2022 at 11:24:48AM -0500, Eric Blake wrote:
> On Thu, Jun 09, 2022 at 03:27:36PM +0100, Richard W.M. Jones wrote:
> 
> > > Post-patch:
> > > nbd> b = nbd.Buffer(10)
> > > nbd> c = h.aio_pread(b, 0)
> > > nbd> b._o.pop()
> > > Traceback (most recent call last):
> > >   File "/usr/lib64/python3.10/code.py", line 90, in runcode
> > >     exec(code, self.locals)
> > >   File "<console>", line 1, in <module>
> > > BufferError: Existing exports of data: object cannot be re-sized
> > 
> > Wow what a horrible error message!  However it comes from Python, not
> > us, so there's not a lot we can do about it.
> 
> Yeah, it doesn't tell you how to find which object is still hanging on
> to the buffer export.  And during development, I had several times
> where I didn't call enough Py_DECREF(), which was interesting to track
> down where the stale references were being kept when all I had was
> this message to go on.
> 
> > 
> > > nbd> h.poll(-1)
> > > nbd> h.aio_command_completed(c)
> > > True
> > > nbd> b._o.pop()
> > > 0
> > > nbd> b.size()
> > > 9
> 
> But I'm pretty happy about the results - Python's ability to magically
> lock a bytearray from being resized while a memoryview is active, so
> that we can then peer into its underlying C memory without copying,
> then restore full functionality when we're done with the underlying
> memory, is pretty cool.
> 
> As to your question about ref-counting:
> 
> > > +++ b/generator/Python.ml
> > > @@ -424,16 +424,12 @@ let
> > >         pr "  %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n;
> > >         pr "  if (!%s_view) goto out;\n" n;
> > >         pr "  py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
> > > -       pr "  /* Increment refcount since buffer may be saved by libnbd. */\n";
> > > -       pr "  Py_INCREF (%s);\n" n;
> > > -       pr "  completion_user_data->buf = %s;\n" n
> > > +       pr "  completion_user_data->view = %s_view;\n" n
> 
> Pre-patch and post-patch: buf_view provides a new object (reference
> count incremented to at least 1), as a result of calling
> nbd_internal_py_get_aio_view.  Pre-patch did not want that reference
> count left around, so it cleans up that reference in the same function
> at [1]; rather, pre-patch wanted to save a different object (buf) and
> had to both Py_INCREF() it as well as assigning buf into
> completion_user_data for later cleanup [2].  Post-patch WANTS to use
> the reference count on view as our long-term lock, so it no longer
> needs in-function cleanup at [1], and no longer has to mess with the
> reference counts on buf.
> 
> > >      | BytesPersistOut (n, _) ->
> > >         pr "  %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n;
> > >         pr "  if (!%s_view) goto out;\n" n;
> > >         pr "  py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
> > > -       pr "  /* Increment refcount since buffer may be saved by libnbd. */\n";
> > > -       pr "  Py_INCREF (%s);\n" n;
> > > -       pr "  completion_user_data->buf = %s;\n" n
> > > +       pr "  completion_user_data->view = %s_view;\n" n
> > >      | Closure { cbname } ->
> > >         pr "  %s.user_data = %s_user_data = alloc_user_data ();\n" cbname cbname;
> > >         pr "  if (%s_user_data == NULL) goto out;\n" cbname;
> > > @@ -538,8 +534,7 @@ let
> > >         pr "  if (%s.obj)\n" n;
> > >         pr "    PyBuffer_Release (&%s);\n" n
> > >      | BytesOut (n, _) -> pr "  Py_XDECREF (%s);\n" n
> > > -    | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
> > > -       pr "  Py_XDECREF (%s_view);\n" n
> > > +    | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> ()
> 
> [1] mentioned above, where we cleaned up buf_view pre-patch but not
> post-patch.
> 
> > >      | Closure { cbname } ->
> > >         pr "  free_user_data (%s_user_data);\n" cbname
> > >      | Enum _ -> ()
> > > @@ -588,7 +583,7 @@ let
> > >    pr " */\n";
> > >    pr "struct user_data {\n";
> > >    pr "  PyObject *fn;    /* Optional pointer to Python function. */\n";
> > > -  pr "  PyObject *buf;   /* Optional pointer to persistent buffer. */\n";
> > > +  pr "  PyObject *view;  /* Optional PyMemoryView of persistent buffer. */\n";
> > >    pr "};\n";
> > >    pr "\n";
> > >    pr "static struct user_data *\n";
> > > @@ -609,7 +604,7 @@ let
> > >    pr "\n";
> > >    pr "  if (data) {\n";
> > >    pr "    Py_XDECREF (data->fn);\n";
> > > -  pr "    Py_XDECREF (data->buf);\n";
> > > +  pr "    Py_XDECREF (data->view);\n";
> 
> [2], where we cleaned up buf pre-patch, and buf_view post-patch.
> 
> > >    pr "    free (data);\n";
> > >    pr "  }\n";
> > >    pr "}\n";
> > 
> > I gather it works, but I don't understand why.  What increments the
> > ref count on data->view?
> 
> I hope that answered your question.

Sure thing, thanks.

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html