diff mbox

[v9,1/9] mirror: inherit supported write/zero flags

Message ID 1525791496-125188-2-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive)
State New, archived
Headers show

Commit Message

Anton Nefedov May 8, 2018, 2:58 p.m. UTC 
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Eric Blake May 15, 2018, 2:35 p.m. UTC | #1 
On 05/08/2018 09:58 AM, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/mirror.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

That said,

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 820f512..a22ddef 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1098,6 +1098,15 @@ static BlockDriver bdrv_mirror_top = {
>       .bdrv_child_perm            = bdrv_mirror_top_child_perm,
>   };
>   
> +static void mirror_top_set_supported_flags(BlockDriverState *bs)
> +{
> +    bs->supported_write_flags = BDRV_REQ_FUA &
> +        bs->backing->bs->supported_write_flags;
> +    bs->supported_zero_flags =
> +        (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
> +        bs->backing->bs->supported_zero_flags;
> +}
> +

This is a pretty short static function...

>   static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>                                int creation_flags, BlockDriverState *target,
>                                const char *replaces, int64_t speed,
> @@ -1163,6 +1172,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>           return;
>       }
>   
> +    mirror_top_set_supported_flags(mirror_top_bs);

...with exactly one caller.  Wouldn't it be easier to just inline it?

> +
>       /* Make sure that the source is not resized while the job is running */
>       s = block_job_create(job_id, driver, NULL, mirror_top_bs,
>                            BLK_PERM_CONSISTENT_READ,
>
Anton Nefedov May 15, 2018, 2:59 p.m. UTC | #2 
On 15/5/2018 5:35 PM, Eric Blake wrote:
> On 05/08/2018 09:58 AM, Anton Nefedov wrote:
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 820f512..a22ddef 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1098,6 +1098,15 @@ static BlockDriver bdrv_mirror_top = {
>>       .bdrv_child_perm            = bdrv_mirror_top_child_perm,
>>   };
>> +static void mirror_top_set_supported_flags(BlockDriverState *bs)
>> +{
>> +    bs->supported_write_flags = BDRV_REQ_FUA &
>> +        bs->backing->bs->supported_write_flags;
>> +    bs->supported_zero_flags =
>> +        (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
>> +        bs->backing->bs->supported_zero_flags;
>> +}
>> +
> 
> This is a pretty short static function...
> 
>>   static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>>                                int creation_flags, BlockDriverState 
>> *target,
>>                                const char *replaces, int64_t speed,
>> @@ -1163,6 +1172,8 @@ static void mirror_start_job(const char *job_id, 
>> BlockDriverState *bs,
>>           return;
>>       }
>> +    mirror_top_set_supported_flags(mirror_top_bs);
> 
> ...with exactly one caller.  Wouldn't it be easier to just inline it?
> 

idk, I felt mirror_start_job() was quite massive already, even
considering there are just a few new lines.
diff mbox

Patch

diff --git a/block/mirror.c b/block/mirror.c
index 820f512..a22ddef 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1098,6 +1098,15 @@  static BlockDriver bdrv_mirror_top = {
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
 };
 
+static void mirror_top_set_supported_flags(BlockDriverState *bs)
+{
+    bs->supported_write_flags = BDRV_REQ_FUA &
+        bs->backing->bs->supported_write_flags;
+    bs->supported_zero_flags =
+        (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
+        bs->backing->bs->supported_zero_flags;
+}
+
 static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                              int creation_flags, BlockDriverState *target,
                              const char *replaces, int64_t speed,
@@ -1163,6 +1172,8 @@  static void mirror_start_job(const char *job_id, BlockDriverState *bs,
         return;
     }
 
+    mirror_top_set_supported_flags(mirror_top_bs);
+
     /* Make sure that the source is not resized while the job is running */
     s = block_job_create(job_id, driver, NULL, mirror_top_bs,
                          BLK_PERM_CONSISTENT_READ,