Re: [Libguestfs] [nbdkit PATCH v2 5/5] RFC: python: Track and cache per-connection state in C struct

Richard W.M. Jones
2018-04-19T17:40:50Z

On Wed, Apr 11, 2018 at 12:03:42AM -0500, Eric Blake
wrote:
> Now that we have FUA support, the C code can call can_fua as
> frequently as on every write.  If the python script has a
> can_fua, we can avoid doubling the calls into python by
> caching the per-connection results, done by wrapping the
> python handle in a C struct.
>
> This commit is marked RFC because it might be nicer if the C
> code implemented the caching for ALL plugins (TODO already
> mentions that).

It's not very clear to me (and this applies to all can_* functions) if
it's OK to call them only once per connection.

I can't think of any argument against that.  Is there a potential
meaning for a plugin to "change its mind" during a connection?

Rich.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/python/python.c | 83
++++++++++++++++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 32 deletions(-)
>
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index ad79c80..757a0e9 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -389,51 +389,66 @@ py_config_complete (void)
>    return 0;
>  }
>
> +/* All per-connection state */
> +typedef struct ConnHandle {
> +  PyObject *obj;
> +  int fua;
> +} ConnHandle;
> +
>  static void *
>  py_open (int readonly)
>  {
>    PyObject *fn;
> -  PyObject *handle;
> +  ConnHandle *h = malloc (sizeof *h);
>
> +  if (!h) {
> +    nbdkit_error ("%s: %m", script);
> +    return NULL;
> +  }
>    if (!callback_defined ("open", &fn)) {
>      nbdkit_error ("%s: missing callback: %s", script,
"open");
> +    free (h);
>      return NULL;
>    }
>
>    PyErr_Clear ();
>
> -  handle = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True :
Py_False,
> +  h->obj = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True :
Py_False,
>                                           NULL);
>    Py_DECREF (fn);
> -  if (check_python_failure ("open") == -1)
> +  if (check_python_failure ("open") == -1) {
> +    free (h);
>      return NULL;
> +  }
>
> -  return handle;
> +  h->fua = -1;
> +  return h;
>  }
>
>  static void
>  py_close (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>
>    if (callback_defined ("close", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      check_python_failure ("close");
>      Py_XDECREF (r);
>    }
>
> -  Py_DECREF (obj);
> +  Py_DECREF (h->obj);
> +  free (h);
>  }
>
>  static int64_t
>  py_get_size (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int64_t ret;
> @@ -445,7 +460,7 @@ py_get_size (void *handle)
>
>    PyErr_Clear ();
>
> -  r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +  r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>    Py_DECREF (fn);
>    if (check_python_failure ("get_size") == -1)
>      return -1;
> @@ -462,7 +477,7 @@ static int
>  py_pread (void *handle, void *buf,
>            uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>
> @@ -474,7 +489,7 @@ py_pread (void *handle, void *buf,
>
>    PyErr_Clear ();
>
> -  r = PyObject_CallFunction (fn, "OiL", obj, count, offset,
NULL);
> +  r = PyObject_CallFunction (fn, "OiL", h->obj, count,
offset, NULL);
>    Py_DECREF (fn);
>    if (check_python_failure ("pread") == -1)
>      return -1;
> @@ -504,7 +519,7 @@ static int
>  py_pwrite (void *handle, const void *buf,
>             uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *args;
>    PyObject *kwargs;
> @@ -516,7 +531,7 @@ py_pwrite (void *handle, const void *buf,
>    if (callback_defined ("pwrite", &fn)) {
>      PyErr_Clear ();
>
> -    args = Py_BuildValue ("ONL", obj,
> +    args = Py_BuildValue ("ONL", h->obj,
>                            PyByteArray_FromStringAndSize (buf, count),
>                            offset);
>      if (!args) {
> @@ -559,7 +574,7 @@ py_pwrite (void *handle, const void *buf,
>  static int
>  py_flush (void *handle, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>
> @@ -567,7 +582,7 @@ py_flush (void *handle, uint32_t flags)
>    if (callback_defined ("flush", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("flush") == -1)
>        return -1;
> @@ -584,7 +599,7 @@ py_flush (void *handle, uint32_t flags)
>  static int
>  py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *args;
>    PyObject *kwargs;
> @@ -596,7 +611,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>    if (callback_defined ("trim", &fn)) {
>      PyErr_Clear ();
>
> -    args = Py_BuildValue ("OiL", obj, count, offset);
> +    args = Py_BuildValue ("OiL", h->obj, count, offset);
>      if (!args) {
>        check_python_failure ("trim");
>        Py_DECREF (fn);
> @@ -637,7 +652,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>  static int
>  py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *args;
>    PyObject *kwargs;
> @@ -660,7 +675,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>      PyErr_Clear ();
>
>      last_error = 0;
> -    args = Py_BuildValue ("OiL", obj, count, offset);
> +    args = Py_BuildValue ("OiL", h->obj, count, offset);
>      if (!args) {
>        check_python_failure ("zero");
>        Py_DECREF (fn);
> @@ -718,7 +733,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>  static int
>  py_can_write (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -726,7 +741,7 @@ py_can_write (void *handle)
>    if (callback_defined ("can_write", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_write") == -1)
>        return -1;
> @@ -741,7 +756,7 @@ py_can_write (void *handle)
>  static int
>  py_can_flush (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -749,7 +764,7 @@ py_can_flush (void *handle)
>    if (callback_defined ("can_flush", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_flush") == -1)
>        return -1;
> @@ -764,7 +779,7 @@ py_can_flush (void *handle)
>  static int
>  py_is_rotational (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -772,7 +787,7 @@ py_is_rotational (void *handle)
>    if (callback_defined ("is_rotational", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("is_rotational") == -1)
>        return -1;
> @@ -787,7 +802,7 @@ py_is_rotational (void *handle)
>  static int
>  py_can_trim (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -795,7 +810,7 @@ py_can_trim (void *handle)
>    if (callback_defined ("can_trim", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_trim") == -1)
>        return -1;
> @@ -810,7 +825,7 @@ py_can_trim (void *handle)
>  static int
>  py_can_zero (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -818,7 +833,7 @@ py_can_zero (void *handle)
>    if (callback_defined ("can_zero", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_zero") == -1)
>        return -1;
> @@ -833,16 +848,20 @@ py_can_zero (void *handle)
>  static int
>  py_can_fua (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int fua;
>
> +  /* Check the cache first */
> +  if (h->fua != -1)
> +    return h->fua;
> +
>    /* We have to convert a python bool into the nbdkit tristate. */
>    if (callback_defined ("can_fua", &fn)) {
>      PyErr_Clear ();
>
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_fua") == -1)
>        return -1;
> @@ -861,7 +880,7 @@ py_can_fua (void *handle)
>    else
>      fua = NBDKIT_FUA_NONE;
>
> -  return fua;
> +  return h->fua = fua;
>  }
>
>  #define py_config_help \
> --
> 2.14.3
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.