diff mbox series

[v10,6/9] file-posix: support BDRV_REQ_ALLOCATE

Message ID 20181203101429.88735-7-anton.nefedov@virtuozzo.com
State New
Headers show
Series qcow2: cluster space preallocation | expand

Commit Message

Anton Nefedov Dec. 3, 2018, 10:14 a.m. UTC 
Current write_zeroes implementation is good enough to satisfy this flag too

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 block/file-posix.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Vladimir Sementsov-Ogievskiy Dec. 5, 2018, 1:25 p.m. UTC | #1 
03.12.2018 13:14, Anton Nefedov wrote:
> Current write_zeroes implementation is good enough to satisfy this flag too
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>   block/file-posix.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 07bbdab953..b0b7ab0159 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -603,6 +603,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>           } else {
>               s->discard_zeroes = true;
>               s->has_fallocate = true;
> +            bs->supported_zero_flags = BDRV_REQ_ALLOCATE;
>           }
>       } else {
>           if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
> @@ -646,10 +647,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>   #ifdef CONFIG_XFS
>       if (platform_test_xfs_fd(s->fd)) {
>           s->is_xfs = true;
> +        bs->supported_zero_flags = BDRV_REQ_ALLOCATE;

why we should handle xfs separately? is there a case with xfs, not handled by previous generic if ()?

>       }
>   #endif
>   
> -    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
> +    bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
>       ret = 0;
>   fail:
>       if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
> @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>           }
>           s->has_fallocate = false;
>       }
> +
> +    if (!s->has_fallocate) {
> +        aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE;
> +    }
>   #endif
>   
>       return -ENOTSUP;
>
Anton Nefedov Dec. 5, 2018, 2:11 p.m. UTC | #2 
On 5/12/2018 4:25 PM, Vladimir Sementsov-Ogievskiy wrote:
> 03.12.2018 13:14, Anton Nefedov wrote:
>> Current write_zeroes implementation is good enough to satisfy this flag too
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>    block/file-posix.c | 8 +++++++-
>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 07bbdab953..b0b7ab0159 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -603,6 +603,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>            } else {
>>                s->discard_zeroes = true;
>>                s->has_fallocate = true;
>> +            bs->supported_zero_flags = BDRV_REQ_ALLOCATE;
>>            }
>>        } else {
>>            if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
>> @@ -646,10 +647,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>    #ifdef CONFIG_XFS
>>        if (platform_test_xfs_fd(s->fd)) {
>>            s->is_xfs = true;
>> +        bs->supported_zero_flags = BDRV_REQ_ALLOCATE;
> 
> why we should handle xfs separately? is there a case with xfs, not handled by previous generic if ()?
> 

The driver supports ALLOCATE either when it's XFS, or when fallocate is
available. Currently there is no test for fallocate, it's just implied
it's supported until ENOTSUP returned.
I think it's safer (for possible future changes) to set it twice even
though you're right and first condition currently covers the XFS 
condition too.

>>        }
>>    #endif
>>    
>> -    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
>> +    bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
>>        ret = 0;
>>    fail:
>>        if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
>> @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>>            }
>>            s->has_fallocate = false;
>>        }
>> +
>> +    if (!s->has_fallocate) {
>> +        aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE;
>> +    }
>>    #endif
>>    
>>        return -ENOTSUP;
>>
> 
>
Alberto Garcia Dec. 7, 2018, 3:09 p.m. UTC | #3 
On Mon 03 Dec 2018 11:14:58 AM CET, Anton Nefedov wrote:
> Current write_zeroes implementation is good enough to satisfy this flag too
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto
Vladimir Sementsov-Ogievskiy Dec. 12, 2018, 5:19 p.m. UTC | #4 
05.12.2018 17:11, Anton Nefedov wrote:
> On 5/12/2018 4:25 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 03.12.2018 13:14, Anton Nefedov wrote:
>>> Current write_zeroes implementation is good enough to satisfy this flag too
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> ---
>>>     block/file-posix.c | 8 +++++++-
>>>     1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index 07bbdab953..b0b7ab0159 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -603,6 +603,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>             } else {
>>>                 s->discard_zeroes = true;
>>>                 s->has_fallocate = true;
>>> +            bs->supported_zero_flags = BDRV_REQ_ALLOCATE;
>>>             }
>>>         } else {
>>>             if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
>>> @@ -646,10 +647,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>     #ifdef CONFIG_XFS
>>>         if (platform_test_xfs_fd(s->fd)) {
>>>             s->is_xfs = true;
>>> +        bs->supported_zero_flags = BDRV_REQ_ALLOCATE;
>>
>> why we should handle xfs separately? is there a case with xfs, not handled by previous generic if ()?
>>
> 
> The driver supports ALLOCATE either when it's XFS, or when fallocate is
> available. Currently there is no test for fallocate, it's just implied
> it's supported until ENOTSUP returned.
> I think it's safer (for possible future changes) to set it twice even
> though you're right and first condition currently covers the XFS
> condition too.

Aha, ok.



> 
>>>         }
>>>     #endif
>>>     
>>> -    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
>>> +    bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
>>>         ret = 0;
>>>     fail:
>>>         if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
>>> @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>>>             }
>>>             s->has_fallocate = false;
>>>         }
>>> +
>>> +    if (!s->has_fallocate) {
>>> +        aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE;
>>> +    }

hm, if CONFIG_FALLOCATE is disabled, flag will remain in supported_zero_flags

>>>     #endif
>>>     
>>>         return -ENOTSUP;
>>>
>>
>>
Anton Nefedov Dec. 13, 2018, 12:01 p.m. UTC | #5 
On 12/12/2018 8:19 PM, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2018 17:11, Anton Nefedov wrote:
>> On 5/12/2018 4:25 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 03.12.2018 13:14, Anton Nefedov wrote:
>>>>          }
>>>>      #endif
>>>>      
>>>> -    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
>>>> +    bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
>>>>          ret = 0;
>>>>      fail:
>>>>          if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
>>>> @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>>>>              }
>>>>              s->has_fallocate = false;
>>>>          }
>>>> +
>>>> +    if (!s->has_fallocate) {
>>>> +        aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE;
>>>> +    }
 >>>>>     #endif
> 
> hm, if CONFIG_FALLOCATE is disabled, flag will remain in supported_zero_flags
> 

right..
I think there should be a separate patch to reset s->has_* in
non-CONFIG_FALLOCATE* cases. Then I'll move this hunk just one line down
under the following #endif
diff mbox series

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index 07bbdab953..b0b7ab0159 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -603,6 +603,7 @@  static int raw_open_common(BlockDriverState *bs, QDict *options,
         } else {
             s->discard_zeroes = true;
             s->has_fallocate = true;
+            bs->supported_zero_flags = BDRV_REQ_ALLOCATE;
         }
     } else {
         if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
@@ -646,10 +647,11 @@  static int raw_open_common(BlockDriverState *bs, QDict *options,
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
         s->is_xfs = true;
+        bs->supported_zero_flags = BDRV_REQ_ALLOCATE;
     }
 #endif
 
-    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
+    bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
     ret = 0;
 fail:
     if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
@@ -1520,6 +1522,10 @@  static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
         }
         s->has_fallocate = false;
     }
+
+    if (!s->has_fallocate) {
+        aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE;
+    }
 #endif
 
     return -ENOTSUP;