Patchwork [RFC,01/24] block: add block conversion api

login
register
mail settings
Submitter Devin Nakamura
Date July 29, 2011, 4:49 a.m.
Message ID <1311914994-20482-2-git-send-email-devin122@gmail.com>
Download mbox | patch
Permalink /patch/107328/
State New
Headers show

Comments

Devin Nakamura - July 29, 2011, 4:49 a.m.
add functions to block driver interface to support inplace image conversion

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.h     |    2 +
 block_int.h |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)
Kevin Wolf - Aug. 1, 2011, 1:34 p.m.
Am 29.07.2011 06:49, schrieb Devin Nakamura:
> add functions to block driver interface to support inplace image conversion
> 
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block.h     |    2 +
>  block_int.h |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 90 insertions(+), 0 deletions(-)
> 
> diff --git a/block.h b/block.h
> index 59cc410..a1c4cc8 100644
> --- a/block.h
> +++ b/block.h
> @@ -251,6 +251,8 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs);
>  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
>  int bdrv_in_use(BlockDriverState *bs);
>  
> +typedef struct BlockConversionOptions BlockConversionOptions;
> +
>  typedef enum {
>      BLKDBG_L1_UPDATE,
>  
> diff --git a/block_int.h b/block_int.h
> index efb6803..84bf89e 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -41,6 +41,9 @@
>  #define BLOCK_OPT_PREALLOC      "preallocation"
>  #define BLOCK_OPT_SUBFMT        "subformat"
>  
> +#define BLOCK_CRYPT_NONE    0
> +#define BLOCK_CRYPT_AES     1
> +
>  typedef struct AIOPool {
>      void (*cancel)(BlockDriverAIOCB *acb);
>      int aiocb_size;
> @@ -139,6 +142,85 @@ struct BlockDriver {
>       */
>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
>  
> +    /* In-place image conversion */
> +
> +    /**
> +     * Opens an image conversion target.
> +     *
> +     * @param bs          Basic Initialization done by
> +     *                    bdrv_open_conversion_target() Still need to set format
> +     *                    specific data.
> +     * @param usr_options Creation options.
> +     * @param drv_options Conversion Options
> +     * @return            Returns non-zero on failure.
> +     */
> +    int (*bdrv_open_conversion_target)(BlockDriverState *bs,
> +        BlockConversionOptions *drv_options, QEMUOptionParameter *usr_options);
> +
> +    /**
> +     * Gets a mapping in the image file.
> +     *
> +     * The function starts searching for a mapping at
> +     * starting_guest_offset = guest_offset + contiguous_bytes
> +     *
> +     * @param bs[in]                   The image in which to find mapping.
> +     * @param guest_offset[in,out]     On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the starting
> +     *                                 guest offset of the mapping.
> +     * @param host_offset[out]         The starting image file offset for the
> +     *                                 mapping.
> +     * @param contiguous_bytes[in,out] On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the number of
> +     *                                 bytes for which this mapping is valid.
> +     *                                 A value of 0 means there are no more
> +     *                                 mappings in the image.
> +     * @return                         Returns non-zero on error.
> +     */
> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);

Last time I suggested to change the semantics of this function as follows:

> I think it would be less surprising if the function worked like
> bdrv_is_allocated(). That is, it doesn't search for mapped clusters, but
> always returns information for exactly the given offset. It would return
> if the range starting at the given offset is used or free.
> 
> If the caller wants to find the next existing mapping, he can just take
> contiguous_bytes of the free region and add it to his current offset.

Do you believe that this change would be a bad idea or did you just
forget it?

> +    /**
> +     * Sets a mapping in the image file.
> +     *
> +     * @param bs               Usualy opened with bdrv_open_conversion_target

Typo: "Usually"

> +     * @param guest_offset     The starting guest offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param host_offset      The starting image offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param contiguous_bytes The number of bytes for which this mapping exists
> +     *                         Function fails and returns -EINVAL if not cluster
> +     *                         aligned
> +     * @return                 Returns non-zero on error
> +     */
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t host_offset, uint64_t contiguous_bytes);

Usually you would describe the -EINVAL part as part of @return. Not that
it's unclear this way, just gets a bit longer than necessary.

> +
> +    /**
> +     * Copies out the header of a conversion target
> +     *
> +     * Saves the current header for the image in a temporary file and overwrites
> +     * it with the header for the new format (at the moment the header is
> +     * assumed to be 1 sector)
> +     *
> +     * @param bs  Usualy opened with bdrv_open_conversion_target().
> +     * @return    Returns non-zero on failure
> +     */
> +    int (*bdrv_copy_header) (BlockDriverState *bs);

Is it true with the current implementation that the old header is saved?
I think it's supposed to be used if updating the header goes wrong. Who
will read the temporary file in this case? Does the user need to know
where it is or even have control over it?

> +
> +    /**
> +     * Asks the block driver what options should be used to create a conversion
> +     * target.
> +     *
> +     * @param options[out] Block conversion options
> +     */
> +    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
> +         BlockConversionOptions *options);
> +
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> @@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>                         _conf.discard_granularity, 0)
>  
> +struct BlockConversionOptions {
> +    int encryption_type;

This could be an enum.

> +    uint64_t image_size;
> +    uint64_t cluster_size;
> +    uint64_t allocation_size;
> +};

Comments explaining the difference between cluster_size and allocation_size?

Kevin
Devin Nakamura - Aug. 2, 2011, 4:43 a.m.
On Mon, Aug 1, 2011 at 9:34 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> +    /**
>> +     * Gets a mapping in the image file.
>> +     *
>> +     * The function starts searching for a mapping at
>> +     * starting_guest_offset = guest_offset + contiguous_bytes
>> +     *
>> +     * @param bs[in]                   The image in which to find mapping.
>> +     * @param guest_offset[in,out]     On function entry used to calculate
>> +     *                                 starting search address.
>> +     *                                 On function exit contains the starting
>> +     *                                 guest offset of the mapping.
>> +     * @param host_offset[out]         The starting image file offset for the
>> +     *                                 mapping.
>> +     * @param contiguous_bytes[in,out] On function entry used to calculate
>> +     *                                 starting search address.
>> +     *                                 On function exit contains the number of
>> +     *                                 bytes for which this mapping is valid.
>> +     *                                 A value of 0 means there are no more
>> +     *                                 mappings in the image.
>> +     * @return                         Returns non-zero on error.
>> +     */
>> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
>> +        uint64_t *host_offset, uint64_t *contiguous_bytes);
>
> Last time I suggested to change the semantics of this function as follows:
>
>> I think it would be less surprising if the function worked like
>> bdrv_is_allocated(). That is, it doesn't search for mapped clusters, but
>> always returns information for exactly the given offset. It would return
>> if the range starting at the given offset is used or free.
>>
>> If the caller wants to find the next existing mapping, he can just take
>> contiguous_bytes of the free region and add it to his current offset.
>
> Do you believe that this change would be a bad idea or did you just
> forget it?
I was just kept putting it off because for some reason I had the idea
that it would be a lot of work to change it.  I've done it now and it
turns out I only had to change about 3 line of code.

>> +
>> +    /**
>> +     * Copies out the header of a conversion target
>> +     *
>> +     * Saves the current header for the image in a temporary file and overwrites
>> +     * it with the header for the new format (at the moment the header is
>> +     * assumed to be 1 sector)
>> +     *
>> +     * @param bs  Usualy opened with bdrv_open_conversion_target().
>> +     * @return    Returns non-zero on failure
>> +     */
>> +    int (*bdrv_copy_header) (BlockDriverState *bs);
>
> Is it true with the current implementation that the old header is saved?
> I think it's supposed to be used if updating the header goes wrong. Who
> will read the temporary file in this case? Does the user need to know
> where it is or even have control over it?
Yes, the header is in fact save in the current implementation. Its
always saved to headerbackup.temp, or something to that effect. Soon I
plan on replacing that with a randomly generated temp file and
informing the user of the name.  Also, I plan on implementing the
ability to override it with a command line option

>> +    uint64_t image_size;
>> +    uint64_t cluster_size;
>> +    uint64_t allocation_size;
>> +};
>
> Comments explaining the difference between cluster_size and allocation_size?
>
At the moment there is none, and I don't think allocation_size has a
future since the allocation size of the source image isn't really
important.
Stefan Hajnoczi - Aug. 2, 2011, 8:56 a.m.
On Fri, Jul 29, 2011 at 12:49:31AM -0400, Devin Nakamura wrote:
> +    /**
> +     * Gets a mapping in the image file.
> +     *
> +     * The function starts searching for a mapping at
> +     * starting_guest_offset = guest_offset + contiguous_bytes
> +     *
> +     * @param bs[in]                   The image in which to find mapping.
> +     * @param guest_offset[in,out]     On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the starting
> +     *                                 guest offset of the mapping.
> +     * @param host_offset[out]         The starting image file offset for the
> +     *                                 mapping.
> +     * @param contiguous_bytes[in,out] On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the number of
> +     *                                 bytes for which this mapping is valid.
> +     *                                 A value of 0 means there are no more
> +     *                                 mappings in the image.
> +     * @return                         Returns non-zero on error.
> +     */
> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);

Will the output value of guest_offset ever be smaller than the input
value?

It seems like this function is designed to be used as an iterator (hence
the starting address calculation).  Explicitly stating that it can be
used as an iterator with contiguous_bytes starting at 0 would be
helpful.

> +     * @param guest_offset     The starting guest offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param host_offset      The starting image offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param contiguous_bytes The number of bytes for which this mapping exists
> +     *                         Function fails and returns -EINVAL if not cluster
> +     *                         aligned
> +     * @return                 Returns non-zero on error
> +     */
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t host_offset, uint64_t contiguous_bytes);

What flush semantics does this function have?  Do I need to call
bdrv_flush() to ensure the metadata updates are on disk?

> +
> +    /**
> +     * Copies out the header of a conversion target
> +     *
> +     * Saves the current header for the image in a temporary file and overwrites
> +     * it with the header for the new format (at the moment the header is
> +     * assumed to be 1 sector)
> +     *
> +     * @param bs  Usualy opened with bdrv_open_conversion_target().
> +     * @return    Returns non-zero on failure
> +     */
> +    int (*bdrv_copy_header) (BlockDriverState *bs);

What is the purpose of the temporary file, what filename does it need to
have, etc?  (I know some of the answers, but please document them.)

> +
> +    /**
> +     * Asks the block driver what options should be used to create a conversion
> +     * target.
> +     *
> +     * @param options[out] Block conversion options
> +     */
> +    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
> +         BlockConversionOptions *options);
> +
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> @@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>                         _conf.discard_granularity, 0)
>  
> +struct BlockConversionOptions {
> +    int encryption_type;
> +    uint64_t image_size;
> +    uint64_t cluster_size;

These two fields can be extracted using existing block.h APIs.  Does it
make sense to add a bdrv_get_encryption_type() instead?  That way
qemu-img info can also show display the encryption type and you can drop
this struct.

> +    uint64_t allocation_size;

What is this?

Stefan
Kevin Wolf - Aug. 2, 2011, 9:24 a.m.
Am 02.08.2011 10:56, schrieb Stefan Hajnoczi:
>> @@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>>                         _conf.discard_granularity, 0)
>>  
>> +struct BlockConversionOptions {
>> +    int encryption_type;
>> +    uint64_t image_size;
>> +    uint64_t cluster_size;
> 
> These two fields can be extracted using existing block.h APIs.  Does it
> make sense to add a bdrv_get_encryption_type() instead?  That way
> qemu-img info can also show display the encryption type and you can drop
> this struct.

Hm... We already have BlockDriverInfo, which is used by qemu-img. Would
it make sense to add the fields there? In any case I would prefer
something that fills a whole struct at once instead of calling ten
separate functions and building the struct in the caller.

Kevin

Patch

diff --git a/block.h b/block.h
index 59cc410..a1c4cc8 100644
--- a/block.h
+++ b/block.h
@@ -251,6 +251,8 @@  int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+typedef struct BlockConversionOptions BlockConversionOptions;
+
 typedef enum {
     BLKDBG_L1_UPDATE,
 
diff --git a/block_int.h b/block_int.h
index efb6803..84bf89e 100644
--- a/block_int.h
+++ b/block_int.h
@@ -41,6 +41,9 @@ 
 #define BLOCK_OPT_PREALLOC      "preallocation"
 #define BLOCK_OPT_SUBFMT        "subformat"
 
+#define BLOCK_CRYPT_NONE    0
+#define BLOCK_CRYPT_AES     1
+
 typedef struct AIOPool {
     void (*cancel)(BlockDriverAIOCB *acb);
     int aiocb_size;
@@ -139,6 +142,85 @@  struct BlockDriver {
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    /* In-place image conversion */
+
+    /**
+     * Opens an image conversion target.
+     *
+     * @param bs          Basic Initialization done by
+     *                    bdrv_open_conversion_target() Still need to set format
+     *                    specific data.
+     * @param usr_options Creation options.
+     * @param drv_options Conversion Options
+     * @return            Returns non-zero on failure.
+     */
+    int (*bdrv_open_conversion_target)(BlockDriverState *bs,
+        BlockConversionOptions *drv_options, QEMUOptionParameter *usr_options);
+
+    /**
+     * Gets a mapping in the image file.
+     *
+     * The function starts searching for a mapping at
+     * starting_guest_offset = guest_offset + contiguous_bytes
+     *
+     * @param bs[in]                   The image in which to find mapping.
+     * @param guest_offset[in,out]     On function entry used to calculate
+     *                                 starting search address.
+     *                                 On function exit contains the starting
+     *                                 guest offset of the mapping.
+     * @param host_offset[out]         The starting image file offset for the
+     *                                 mapping.
+     * @param contiguous_bytes[in,out] On function entry used to calculate
+     *                                 starting search address.
+     *                                 On function exit contains the number of
+     *                                 bytes for which this mapping is valid.
+     *                                 A value of 0 means there are no more
+     *                                 mappings in the image.
+     * @return                         Returns non-zero on error.
+     */
+    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
+        uint64_t *host_offset, uint64_t *contiguous_bytes);
+
+    /**
+     * Sets a mapping in the image file.
+     *
+     * @param bs               Usualy opened with bdrv_open_conversion_target
+     * @param guest_offset     The starting guest offset of the mapping
+     *                         (in bytes). Function fails and returns -EINVAL if
+     *                         not cluster aligned.
+     * @param host_offset      The starting image offset of the mapping
+     *                         (in bytes). Function fails and returns -EINVAL if
+     *                         not cluster aligned.
+     * @param contiguous_bytes The number of bytes for which this mapping exists
+     *                         Function fails and returns -EINVAL if not cluster
+     *                         aligned
+     * @return                 Returns non-zero on error
+     */
+    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
+        uint64_t host_offset, uint64_t contiguous_bytes);
+
+    /**
+     * Copies out the header of a conversion target
+     *
+     * Saves the current header for the image in a temporary file and overwrites
+     * it with the header for the new format (at the moment the header is
+     * assumed to be 1 sector)
+     *
+     * @param bs  Usualy opened with bdrv_open_conversion_target().
+     * @return    Returns non-zero on failure
+     */
+    int (*bdrv_copy_header) (BlockDriverState *bs);
+
+    /**
+     * Asks the block driver what options should be used to create a conversion
+     * target.
+     *
+     * @param options[out] Block conversion options
+     */
+    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
+         BlockConversionOptions *options);
+
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
@@ -263,4 +345,10 @@  static inline unsigned int get_physical_block_exp(BlockConf *conf)
     DEFINE_PROP_UINT32("discard_granularity", _state, \
                        _conf.discard_granularity, 0)
 
+struct BlockConversionOptions {
+    int encryption_type;
+    uint64_t image_size;
+    uint64_t cluster_size;
+    uint64_t allocation_size;
+};
 #endif /* BLOCK_INT_H */