Re: [Qemu-devel] Safely reopening image files by stashing fds

[Blue Swirl]

On Fri, Aug 5, 2011 at 8:40 AM, Stefan Hajnoczi <address@hidden> wrote:
> We've discussed safe methods for reopening image files (e.g. useful for
> changing the hostcache parameter).  The problem is that closing the file first
> and then opening it again exposes us to the error case where the open fails.
> At that point we cannot get to the file anymore and our options are to
> terminate QEMU, pause the VM, or offline the block device.
>
> This window of vulnerability can be eliminated by keeping the file descriptor
> around and falling back to it should the open fail.
>
> The challenge for the file descriptor approach is that image formats, like
> VMDK, can span multiple files.  Therefore the solution is not as simple as
> stashing a single file descriptor and reopening from it.
>
> Here is the outline for an fd stashing mechanism that can handle reopening
> multi-file images and could also solve the file descriptor passing problem for
> libvirt:
>
> 1. Extract monitor getfd/closefd functionality
>
> The monitor already supports fd stashing with getfd/closefd commands.  But the
> fd stash code is part of Monitor and we need to extract it into its own 
> object.
>
> /* A stashed file descriptor */
> typedef FDEntry {
>        const char *name;
>        int fd;
>        QLIST_ENTRY(FDEntry) next;
> } FDEntry;
>
> /* A container for stashing file descriptors */
> typedef struct FDStash {
>        QLIST_HEAD(, FDEntry) fds;
> } FDStash;
>
> void fdstash_init(FDStash *stash);
>
> /**
>  * Clear stashed file descriptors and close them
>  */
> void fdstash_cleanup(FDStash *stash);
>
> /**
>  * Stash a file descriptor and give up ownership
>  *
>  * If a file descriptor is already present with the same name the old fd is
>  * closed and replaced by the new one.
>  */
> void fdstash_give(FDStash *stash, const char *name, int fd);
>
> /**
>  * Find and take ownership of a stashed file descriptor
>  *
>  * Return the file descriptor or -ENOENT if not found.
>  */
> int fdstash_take(FDStash *stash, const char *name);
>
> The monitor is refactored to use this code instead of open coding fd stashing.
>
> 2. Introduce a function to extract open file descriptors from an block device
>
> Add a new .bdrv_extract_fds(BlockDriverState *bs, FDStash *stash) interface,
> which defaults to calling bdrv_extract_fds(bs->file, stash).
>
> VMDK and protocols can implement this function to support extracting open fds
> from a block device.  Note that they need to dup(2) fds before giving them to
> the fdstash, otherwise the fd will be closed when the block device is
> closed/deleted.
>
> 3. Rework bdrv_open() to take a FDStash
>
> Check the FDStash before opening an image file on the host file system.  This
> makes it possible to open an image file and use existing stashed fds.
>
> 4. Implement bdrv_reopen()
>
> First call bdrv_extract_fds() to stash the file descriptors, then close the
> block device.  Try opening the new image but if that fails, reopen using the
> stashed file descriptors.
>
> Thoughts?

I was previously thinking that the fd store should be tightly coupled
with block layer, but maybe it would be better to make this totally
generic like Avi(?) proposed, then all files (logs etc) could benefit
from this functionality.

I'd then handle fd stashing in qemu_open() etc. Stashing would not
need major changes anywhere, except reopen should be handled with
qemu_reopen() instead of qemu_close() + qemu_open() sequence. From the
other side (monitor), the interface could be like you present in 1.