[v3] file-posix: allow -EBUSY -EINVAL errors during write zeros on block

[PATCH V3] file-posix: allow -EBUSY -EINVAL errors during write zeros on block

Posted by ChangLimin 3 years, 2 months ago

Since Linux 5.10, write zeros to a multipath device using
ioctl(fd, BLKZEROOUT, range) with cache none or directsync return -EBUSY
permanently.

Similar to handle_aiocb_write_zeroes_unmap, handle_aiocb_write_zeroes_block
allow -EBUSY and -EINVAL errors during ioctl(fd, BLKZEROOUT, range).

Reference commit in Linux 5.10:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=384d87ef2c954fc58e6c5fd8253e4a1984f5fe02

Although it will be fixed in 5.12, I think it's good to avoid similar problem in the future.
https://lore.kernel.org/linux-block/53689a67-7591-0ad8-3e7d-dca9a626cd99@kernel.dk/

Signed-off-by: ChangLimin <changlm@chinatelecom.cn>
---
 block/file-posix.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 05079b40ca..4e132db929 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1629,8 +1629,13 @@ static ssize_t handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
         } while (errno == EINTR);

         ret = translate_err(-errno);
-        if (ret == -ENOTSUP) {
-            s->has_write_zeroes = false;
+        switch (ret) {
+        case -ENOTSUP:
+            s->has_write_zeroes = false; /* fall through */
+        case -EINVAL:
+        case -EBUSY:
+            return -ENOTSUP;
+            break;
         }
     }
 #endif
--
2.27.0

Re: [PATCH V3] file-posix: allow -EBUSY -EINVAL errors during write zeros on block

Posted by John Snow 3 years, 1 month ago

On 3/9/21 7:16 PM, ChangLimin wrote:
> Since Linux 5.10, write zeros to a multipath device using
> ioctl(fd, BLKZEROOUT, range) with cache none or directsync return -EBUSY
> permanently.
> 

When do we get -EINVAL? Both of the commits referenced below don't 
specifically mention it, so I am not sure in which circumstances that 
might arise.

> Similar to handle_aiocb_write_zeroes_unmap, handle_aiocb_write_zeroes_block
> allow -EBUSY and -EINVAL errors during ioctl(fd, BLKZEROOUT, range).
> 
> Reference commit in Linux 5.10:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=384d87ef2c954fc58e6c5fd8253e4a1984f5fe02 
> <https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=384d87ef2c954fc58e6c5fd8253e4a1984f5fe02>
> 
> Although it will be fixed in 5.12, I think it's good to avoid similar 
> problem in the future.
> https://lore.kernel.org/linux-block/53689a67-7591-0ad8-3e7d-dca9a626cd99@kernel.dk/ 
> <https://lore.kernel.org/linux-block/53689a67-7591-0ad8-3e7d-dca9a626cd99@kernel.dk/>
> 

Wait, if they're fixing the function to actually apply a different 
fallback path, shouldn't we *not* allow EBUSY?

> Signed-off-by: ChangLimin <changlm@chinatelecom.cn>
> ---
>   block/file-posix.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 05079b40ca..4e132db929 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1629,8 +1629,13 @@ static ssize_t 
> handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
>           } while (errno == EINTR);
> 
>           ret = translate_err(-errno);
> -        if (ret == -ENOTSUP) {
> -            s->has_write_zeroes = false;
> +        switch (ret) {
> +        case -ENOTSUP:
> +            s->has_write_zeroes = false; /* fall through */
> +        case -EINVAL:
> +        case -EBUSY:
> +            return -ENOTSUP;
> +            break;

oh, we're not "allowing" them, we're treating the failure *more 
seriously* so that we avoid attempting to call this function ever again 
for this FD.

Can you please add a brief comment here, something like:

/* Linux 5.10/5.11 may return these for multipath devices */

>           }
>       }
>   #endif
> --
> 2.27.0
>

Re: [PATCH V3] file-posix: allow -EBUSY -EINVAL errors during write zeros on block

Posted by Vladimir Sementsov-Ogievskiy via 3 years, 1 month ago

18.03.2021 02:23, John Snow wrote:
> On 3/9/21 7:16 PM, ChangLimin wrote:
>> Since Linux 5.10, write zeros to a multipath device using
>> ioctl(fd, BLKZEROOUT, range) with cache none or directsync return -EBUSY
>> permanently.
>>
> 
> When do we get -EINVAL? Both of the commits referenced below don't specifically mention it, so I am not sure in which circumstances that might arise.
> 
>> Similar to handle_aiocb_write_zeroes_unmap, handle_aiocb_write_zeroes_block
>> allow -EBUSY and -EINVAL errors during ioctl(fd, BLKZEROOUT, range).
>>
>> Reference commit in Linux 5.10:
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=384d87ef2c954fc58e6c5fd8253e4a1984f5fe02 <https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=384d87ef2c954fc58e6c5fd8253e4a1984f5fe02>
>>
>> Although it will be fixed in 5.12, I think it's good to avoid similar problem in the future.
>> https://lore.kernel.org/linux-block/53689a67-7591-0ad8-3e7d-dca9a626cd99@kernel.dk/ <https://lore.kernel.org/linux-block/53689a67-7591-0ad8-3e7d-dca9a626cd99@kernel.dk/>
>>
> 
> Wait, if they're fixing the function to actually apply a different fallback path, shouldn't we *not* allow EBUSY?
> 
>> Signed-off-by: ChangLimin <changlm@chinatelecom.cn>
>> ---
>>   block/file-posix.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 05079b40ca..4e132db929 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1629,8 +1629,13 @@ static ssize_t handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
>>           } while (errno == EINTR);
>>
>>           ret = translate_err(-errno);
>> -        if (ret == -ENOTSUP) {
>> -            s->has_write_zeroes = false;
>> +        switch (ret) {
>> +        case -ENOTSUP:
>> +            s->has_write_zeroes = false; /* fall through */
>> +        case -EINVAL:
>> +        case -EBUSY:
>> +            return -ENOTSUP;
>> +            break;
> 
> oh, we're not "allowing" them, we're treating the failure *more seriously* so that we avoid attempting to call this function ever again for this FD.

No. s->has_write_zeroes is set to false only for ENOTSUP came from translate_err, this behavior is not changed.

The only thing the patch does is _returning_ ENOTSUP instead of EINVAL and EBUSY. And I don't understand why. I think that at least for EINVAL it's just wrong.

> 
> Can you please add a brief comment here, something like:
> 
> /* Linux 5.10/5.11 may return these for multipath devices */
> 
>>           }
>>       }
>>   #endif
>> -- 
>> 2.27.0
>>
> 
> 


-- 
Best regards,
Vladimir