[Libguestfs] [PATCH nbdkit v2 3/3] tmpdisk: Implement this plugin using fileops.

[Eric Blake]

On 4/9/20 3:36 AM, Richard W.M. Jones wrote:
> This plugin now implements efficient zeroing, pre-fetch and extents,
> which should give quite a performance boost.
> 
> XXX
> On the flip side, it no longer ignores flush and FUA (which we can
> ignore because these are temporary disks), which very likely has a
> large negative impact on performance.  Fixing this would involve
> generalising fileops a little.

Indeed. Maybe adding a 'bool ignore_flush' to init_fileops.

>   static void *
>   tmpdisk_open (int readonly)
>   {
> -  struct handle *h;
> +  struct fileops *fops;

> @@ -325,29 +273,14 @@ tmpdisk_open (int readonly)
>       flags = O_RDONLY | O_CLOEXEC;
>     else
>       flags = O_RDWR | O_CLOEXEC;
> -  h->fd = open (disk, flags);
> -  if (h->fd == -1) {
> +  fd = open (disk, flags);
> +  if (fd == -1) {
>       nbdkit_error ("open: %s: %m", disk);
> -    goto error;
> +    goto error2;
>     }
>   

> +  if (init_fileops (fd, fops) == -1)
> +    goto error3;
>   
>     /* We don't need the disk to appear in the filesystem since we hold
>      * a file descriptor and access it through that, so unlink the disk.
> @@ -357,139 +290,26 @@ tmpdisk_open (int readonly)
>     rmdir (dir);
>   
>     /* Return the handle. */
> -  return h;
> +  return fops;
>   
> - error:
> -  if (h) {
> -    if (h->fd >= 0) {
> -      close (h->fd);
> -      unlink (disk);
> -    }
> -    free (h);
> -  }
> + error3:
> +  close (fd);

Here, you are closing the fd; that's a double-close if you change the 
contract of init_fileops() on error.

> @@ -505,19 +325,12 @@ static struct nbdkit_plugin plugin = {
>     .config_help       = tmpdisk_config_help,
>     .magic_config_key  = "size",
>   
> -  .can_multi_conn    = tmpdisk_can_multi_conn,
> -  .can_trim          = tmpdisk_can_trim,
> -  .can_fua           = tmpdisk_can_fua,
> -  .get_size          = tmpdisk_get_size,
> -
>     .open              = tmpdisk_open,
>     .close             = tmpdisk_close,
> -  .pread             = tmpdisk_pread,
> -  .pwrite            = tmpdisk_pwrite,
> -  .flush             = tmpdisk_flush,
> -  .trim              = tmpdisk_trim,
> +  .can_multi_conn    = tmpdisk_can_multi_conn,
>   
> -  .errno_is_preserved = 1,
> +  /* Data serving is implemented in common/fileops/fileops.c */
> +  FILEOPS_READ_WRITE_CALLBACKS
>   };
>   
>   NBDKIT_REGISTER_PLUGIN(plugin)
> 

Otherwise it looks sane.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org