[PATCH v7 01/47] block: Add child access functions

Max Reitz posted 47 patches 5 years, 4 months ago
There is a newer version of this series
[PATCH v7 01/47] block: Add child access functions
Posted by Max Reitz 5 years, 4 months ago
There are BDS children that the general block layer code can access,
namely bs->file and bs->backing.  Since the introduction of filters and
external data files, their meaning is not quite clear.  bs->backing can
be a COW source, or it can be a filtered child; bs->file can be a
filtered child, it can be data and metadata storage, or it can be just
metadata storage.

This overloading really is not helpful.  This patch adds functions that
retrieve the correct child for each exact purpose.  Later patches in
this series will make use of them.  Doing so will allow us to handle
filter nodes in a meaningful way.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block_int.h | 44 +++++++++++++++++--
 block.c                   | 90 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1b86b59af1..bb3457c5e8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -90,9 +90,17 @@ struct BlockDriver {
     int instance_size;
 
     /* set to true if the BlockDriver is a block filter. Block filters pass
-     * certain callbacks that refer to data (see block.c) to their bs->file if
-     * the driver doesn't implement them. Drivers that do not wish to forward
-     * must implement them and return -ENOTSUP.
+     * certain callbacks that refer to data (see block.c) to their bs->file
+     * or bs->backing (whichever one exists) if the driver doesn't implement
+     * them. Drivers that do not wish to forward must implement them and return
+     * -ENOTSUP.
+     * Note that filters are not allowed to modify data.
+     *
+     * Filters generally cannot have more than a single filtered child,
+     * because the data they present must at all times be the same as
+     * that on their filtered child.  That would be impossible to
+     * achieve for multiple filtered children.
+     * (And this filtered child must then be bs->file or bs->backing.)
      */
     bool is_filter;
     /*
@@ -1370,4 +1378,34 @@ BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
                                            BlockDriverState **bitmap_bs,
                                            Error **errp);
 
+BdrvChild *bdrv_cow_child(BlockDriverState *bs);
+BdrvChild *bdrv_filter_child(BlockDriverState *bs);
+BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
+BdrvChild *bdrv_primary_child(BlockDriverState *bs);
+
+static inline BlockDriverState *child_bs(BdrvChild *child)
+{
+    return child ? child->bs : NULL;
+}
+
+static inline BlockDriverState *bdrv_cow_bs(BlockDriverState *bs)
+{
+    return child_bs(bdrv_cow_child(bs));
+}
+
+static inline BlockDriverState *bdrv_filter_bs(BlockDriverState *bs)
+{
+    return child_bs(bdrv_filter_child(bs));
+}
+
+static inline BlockDriverState *bdrv_filter_or_cow_bs(BlockDriverState *bs)
+{
+    return child_bs(bdrv_filter_or_cow_child(bs));
+}
+
+static inline BlockDriverState *bdrv_primary_bs(BlockDriverState *bs)
+{
+    return child_bs(bdrv_primary_child(bs));
+}
+
 #endif /* BLOCK_INT_H */
diff --git a/block.c b/block.c
index 144f52e413..5a42ef49fd 100644
--- a/block.c
+++ b/block.c
@@ -6918,3 +6918,93 @@ int bdrv_make_empty(BdrvChild *c, Error **errp)
 
     return 0;
 }
+
+/*
+ * Return the child that @bs acts as an overlay for, and from which data may be
+ * copied in COW or COR operations.  Usually this is the backing file.
+ */
+BdrvChild *bdrv_cow_child(BlockDriverState *bs)
+{
+    if (!bs || !bs->drv) {
+        return NULL;
+    }
+
+    if (bs->drv->is_filter) {
+        return NULL;
+    }
+
+    if (!bs->backing) {
+        return NULL;
+    }
+
+    assert(bs->backing->role & BDRV_CHILD_COW);
+    return bs->backing;
+}
+
+/*
+ * If @bs acts as a filter for exactly one of its children, return
+ * that child.
+ */
+BdrvChild *bdrv_filter_child(BlockDriverState *bs)
+{
+    BdrvChild *c;
+
+    if (!bs || !bs->drv) {
+        return NULL;
+    }
+
+    if (!bs->drv->is_filter) {
+        return NULL;
+    }
+
+    /* Only one of @backing or @file may be used */
+    assert(!(bs->backing && bs->file));
+
+    c = bs->backing ?: bs->file;
+    if (!c) {
+        return NULL;
+    }
+
+    assert(c->role & BDRV_CHILD_FILTERED);
+    return c;
+}
+
+/*
+ * Return either the result of bdrv_cow_child() or bdrv_filter_child(),
+ * whichever is non-NULL.
+ *
+ * Return NULL if both are NULL.
+ */
+BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs)
+{
+    BdrvChild *cow_child = bdrv_cow_child(bs);
+    BdrvChild *filter_child = bdrv_filter_child(bs);
+
+    /* Filter nodes cannot have COW backing files */
+    assert(!(cow_child && filter_child));
+
+    return cow_child ?: filter_child;
+}
+
+/*
+ * Return the primary child of this node: For filters, that is the
+ * filtered child.  For other nodes, that is usually the child storing
+ * metadata.
+ * (A generally more helpful description is that this is (usually) the
+ * child that has the same filename as @bs.)
+ *
+ * Drivers do not necessarily have a primary child; for example quorum
+ * does not.
+ */
+BdrvChild *bdrv_primary_child(BlockDriverState *bs)
+{
+    BdrvChild *c;
+
+    QLIST_FOREACH(c, &bs->children, next) {
+        if (c->role & BDRV_CHILD_PRIMARY) {
+            return c;
+        }
+    }
+
+    return NULL;
+}
-- 
2.26.2


Re: [PATCH v7 01/47] block: Add child access functions
Posted by Andrey Shinkevich 5 years, 4 months ago
On 25.06.2020 18:21, Max Reitz wrote:
> There are BDS children that the general block layer code can access,
> namely bs->file and bs->backing.  Since the introduction of filters and
> external data files, their meaning is not quite clear.  bs->backing can
> be a COW source, or it can be a filtered child; bs->file can be a
> filtered child, it can be data and metadata storage, or it can be just
> metadata storage.
>
> This overloading really is not helpful.  This patch adds functions that
> retrieve the correct child for each exact purpose.  Later patches in
> this series will make use of them.  Doing so will allow us to handle
> filter nodes in a meaningful way.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/block/block_int.h | 44 +++++++++++++++++--
>   block.c                   | 90 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 131 insertions(+), 3 deletions(-)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1b86b59af1..bb3457c5e8 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -90,9 +90,17 @@ struct BlockDriver {
>       int instance_size;
>   
> ...


Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>


Re: [PATCH v7 01/47] block: Add child access functions
Posted by Vladimir Sementsov-Ogievskiy 5 years, 4 months ago
25.06.2020 18:21, Max Reitz wrote:
> There are BDS children that the general block layer code can access,
> namely bs->file and bs->backing.  Since the introduction of filters and
> external data files, their meaning is not quite clear.  bs->backing can
> be a COW source, or it can be a filtered child; bs->file can be a
> filtered child, it can be data and metadata storage, or it can be just
> metadata storage.
> 
> This overloading really is not helpful.  This patch adds functions that
> retrieve the correct child for each exact purpose.  Later patches in
> this series will make use of them.  Doing so will allow us to handle
> filter nodes in a meaningful way.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

[..]

> +/*
> + * Return the primary child of this node: For filters, that is the
> + * filtered child.  For other nodes, that is usually the child storing
> + * metadata.
> + * (A generally more helpful description is that this is (usually) the
> + * child that has the same filename as @bs.)
> + *
> + * Drivers do not necessarily have a primary child; for example quorum
> + * does not.
> + */
> +BdrvChild *bdrv_primary_child(BlockDriverState *bs)
> +{
> +    BdrvChild *c;
> +
> +    QLIST_FOREACH(c, &bs->children, next) {
> +        if (c->role & BDRV_CHILD_PRIMARY) {
> +            return c;
> +        }
> +    }
> +
> +    return NULL;
> +}
> 

Suggest squash-in to also assert that not more than one primary child:
--- a/block.c
+++ b/block.c
@@ -6998,13 +6998,14 @@ BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs)
   */
  BdrvChild *bdrv_primary_child(BlockDriverState *bs)
  {
-    BdrvChild *c;
+    BdrvChild *c, *found = NULL;
  
      QLIST_FOREACH(c, &bs->children, next) {
          if (c->role & BDRV_CHILD_PRIMARY) {
-            return c;
+            assert(!found);
+            found = c;
          }
      }
  
-    return NULL;
+    return c;
  }


with or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH v7 01/47] block: Add child access functions
Posted by Max Reitz 5 years, 4 months ago
On 13.07.20 11:06, Vladimir Sementsov-Ogievskiy wrote:
> 25.06.2020 18:21, Max Reitz wrote:
>> There are BDS children that the general block layer code can access,
>> namely bs->file and bs->backing.  Since the introduction of filters and
>> external data files, their meaning is not quite clear.  bs->backing can
>> be a COW source, or it can be a filtered child; bs->file can be a
>> filtered child, it can be data and metadata storage, or it can be just
>> metadata storage.
>>
>> This overloading really is not helpful.  This patch adds functions that
>> retrieve the correct child for each exact purpose.  Later patches in
>> this series will make use of them.  Doing so will allow us to handle
>> filter nodes in a meaningful way.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
> 
> [..]
> 
>> +/*
>> + * Return the primary child of this node: For filters, that is the
>> + * filtered child.  For other nodes, that is usually the child storing
>> + * metadata.
>> + * (A generally more helpful description is that this is (usually) the
>> + * child that has the same filename as @bs.)
>> + *
>> + * Drivers do not necessarily have a primary child; for example quorum
>> + * does not.
>> + */
>> +BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>> +{
>> +    BdrvChild *c;
>> +
>> +    QLIST_FOREACH(c, &bs->children, next) {
>> +        if (c->role & BDRV_CHILD_PRIMARY) {
>> +            return c;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>>
> 
> Suggest squash-in to also assert that not more than one primary child:
> --- a/block.c
> +++ b/block.c
> @@ -6998,13 +6998,14 @@ BdrvChild
> *bdrv_filter_or_cow_child(BlockDriverState *bs)
>   */
>  BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>  {
> -    BdrvChild *c;
> +    BdrvChild *c, *found = NULL;
>  
>      QLIST_FOREACH(c, &bs->children, next) {
>          if (c->role & BDRV_CHILD_PRIMARY) {
> -            return c;
> +            assert(!found);

Hm, why not.

> +            found = c;
>          }
>      }
>  
> -    return NULL;
> +    return c;
>  }
> 
> 
> with or without:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

In any case, thanks for reviewing!

Max

Re: [PATCH v7 01/47] block: Add child access functions
Posted by Christophe de Dinechin 5 years, 3 months ago
On 2020-07-13 at 11:06 CEST, Vladimir Sementsov-Ogievskiy wrote...
> 25.06.2020 18:21, Max Reitz wrote:
>> There are BDS children that the general block layer code can access,
>> namely bs->file and bs->backing.  Since the introduction of filters and
>> external data files, their meaning is not quite clear.  bs->backing can
>> be a COW source, or it can be a filtered child; bs->file can be a
>> filtered child, it can be data and metadata storage, or it can be just
>> metadata storage.
>>
>> This overloading really is not helpful.  This patch adds functions that
>> retrieve the correct child for each exact purpose.  Later patches in
>> this series will make use of them.  Doing so will allow us to handle
>> filter nodes in a meaningful way.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>
> [..]
>
>> +/*
>> + * Return the primary child of this node: For filters, that is the
>> + * filtered child.  For other nodes, that is usually the child storing
>> + * metadata.
>> + * (A generally more helpful description is that this is (usually) the
>> + * child that has the same filename as @bs.)
>> + *
>> + * Drivers do not necessarily have a primary child; for example quorum
>> + * does not.
>> + */
>> +BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>> +{
>> +    BdrvChild *c;
>> +
>> +    QLIST_FOREACH(c, &bs->children, next) {
>> +        if (c->role & BDRV_CHILD_PRIMARY) {
>> +            return c;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>>
>
> Suggest squash-in to also assert that not more than one primary child:
> --- a/block.c
> +++ b/block.c
> @@ -6998,13 +6998,14 @@ BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs)
>    */
>   BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>   {
> -    BdrvChild *c;
> +    BdrvChild *c, *found = NULL;
>
>       QLIST_FOREACH(c, &bs->children, next) {
>           if (c->role & BDRV_CHILD_PRIMARY) {
> -            return c;
> +            assert(!found);
> +            found = c;
>           }
>       }
>
> -    return NULL;
> +    return c;

Shouldn't that be "return found"?
>   }
>
>
> with or without:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


--
Cheers,
Christophe de Dinechin (IRC c3d)


Re: [PATCH v7 01/47] block: Add child access functions
Posted by Vladimir Sementsov-Ogievskiy 5 years, 3 months ago
28.07.2020 19:09, Christophe de Dinechin wrote:
> 
> On 2020-07-13 at 11:06 CEST, Vladimir Sementsov-Ogievskiy wrote...
>> 25.06.2020 18:21, Max Reitz wrote:
>>> There are BDS children that the general block layer code can access,
>>> namely bs->file and bs->backing.  Since the introduction of filters and
>>> external data files, their meaning is not quite clear.  bs->backing can
>>> be a COW source, or it can be a filtered child; bs->file can be a
>>> filtered child, it can be data and metadata storage, or it can be just
>>> metadata storage.
>>>
>>> This overloading really is not helpful.  This patch adds functions that
>>> retrieve the correct child for each exact purpose.  Later patches in
>>> this series will make use of them.  Doing so will allow us to handle
>>> filter nodes in a meaningful way.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>
>> [..]
>>
>>> +/*
>>> + * Return the primary child of this node: For filters, that is the
>>> + * filtered child.  For other nodes, that is usually the child storing
>>> + * metadata.
>>> + * (A generally more helpful description is that this is (usually) the
>>> + * child that has the same filename as @bs.)
>>> + *
>>> + * Drivers do not necessarily have a primary child; for example quorum
>>> + * does not.
>>> + */
>>> +BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>>> +{
>>> +    BdrvChild *c;
>>> +
>>> +    QLIST_FOREACH(c, &bs->children, next) {
>>> +        if (c->role & BDRV_CHILD_PRIMARY) {
>>> +            return c;
>>> +        }
>>> +    }
>>> +
>>> +    return NULL;
>>> +}
>>>
>>
>> Suggest squash-in to also assert that not more than one primary child:
>> --- a/block.c
>> +++ b/block.c
>> @@ -6998,13 +6998,14 @@ BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs)
>>     */
>>    BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>>    {
>> -    BdrvChild *c;
>> +    BdrvChild *c, *found = NULL;
>>
>>        QLIST_FOREACH(c, &bs->children, next) {
>>            if (c->role & BDRV_CHILD_PRIMARY) {
>> -            return c;
>> +            assert(!found);
>> +            found = c;
>>            }
>>        }
>>
>> -    return NULL;
>> +    return c;
> 
> Shouldn't that be "return found"?

Oops, you are right of course!

>>    }
>>
>>
>> with or without:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v7 01/47] block: Add child access functions
Posted by Vladimir Sementsov-Ogievskiy 5 years, 4 months ago
25.06.2020 18:21, Max Reitz wrote:
> There are BDS children that the general block layer code can access,
> namely bs->file and bs->backing.  Since the introduction of filters and
> external data files, their meaning is not quite clear.  bs->backing can
> be a COW source, or it can be a filtered child; bs->file can be a
> filtered child, it can be data and metadata storage, or it can be just
> metadata storage.
> 
> This overloading really is not helpful.  This patch adds functions that
> retrieve the correct child for each exact purpose.  Later patches in
> this series will make use of them.  Doing so will allow us to handle
> filter nodes in a meaningful way.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/block/block_int.h | 44 +++++++++++++++++--
>   block.c                   | 90 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 131 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1b86b59af1..bb3457c5e8 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h

[..]

> +/*
> + * If @bs acts as a filter for exactly one of its children, return
> + * that child.
> + */
> +BdrvChild *bdrv_filter_child(BlockDriverState *bs)

Hmm you called it filter_child instead of filterED_child..

> +{
> +    BdrvChild *c;
> +
> +    if (!bs || !bs->drv) {
> +        return NULL;
> +    }
> +
> +    if (!bs->drv->is_filter) {
> +        return NULL;
> +    }
> +
> +    /* Only one of @backing or @file may be used */
> +    assert(!(bs->backing && bs->file));
> +
> +    c = bs->backing ?: bs->file;
> +    if (!c) {
> +        return NULL;
> +    }
> +
> +    assert(c->role & BDRV_CHILD_FILTERED);

But the role is still called CHILD_FILTERED

> +    return c;
> +}

(just note that it's a bit inconsistent, keep my r-b anyway)


-- 
Best regards,
Vladimir