[v5,07/10] block: Implement bdrv_{pread, pwrite, pwrite_zeroes}() using generated_co_wrapper

bdrv_{pread,pwrite}() now return -EIO instead of -EINVAL when 'bytes' is
negative, making them consistent with bdrv_{preadv,pwritev}() and
bdrv_co_{pread,pwrite,preadv,pwritev}().

bdrv_pwrite_zeroes() now also calls trace_bdrv_co_pwrite_zeroes() and
clears the BDRV_REQ_MAY_UNMAP flag when appropriate, which it didn't
previously.

Signed-off-by: Alberto Faria <afaria@redhat.com>
---

I audited all bdrv_{pread,pwrite}() callers to make sure that changing
the -EINVAL return code to -EIO wont't break things. However, there are
about 140 call sites, so the probability of me having missed something
isn't negligible. If someone more accustomed to the code base is able to
double-check this, that would be very much appreciated.

As a precaution, I also dropped Paolo's R-b.

 block/io.c               | 41 ----------------------------------------
 include/block/block-io.h | 15 +++++++++------
 2 files changed, 9 insertions(+), 47 deletions(-)

On Thu, Jun 09, 2022 at 04:27:41PM +0100, Alberto Faria wrote:
> bdrv_{pread,pwrite}() now return -EIO instead of -EINVAL when 'bytes' is
> negative, making them consistent with bdrv_{preadv,pwritev}() and
> bdrv_co_{pread,pwrite,preadv,pwritev}().
> 
> bdrv_pwrite_zeroes() now also calls trace_bdrv_co_pwrite_zeroes() and
> clears the BDRV_REQ_MAY_UNMAP flag when appropriate, which it didn't
> previously.
> 
> Signed-off-by: Alberto Faria <afaria@redhat.com>
> ---
> 
> I audited all bdrv_{pread,pwrite}() callers to make sure that changing
> the -EINVAL return code to -EIO wont't break things. However, there are
> about 140 call sites, so the probability of me having missed something
> isn't negligible. If someone more accustomed to the code base is able to
> double-check this, that would be very much appreciated.

I did not get through all of the callers (you are right, there ARE a
lot), but the ones I checked, particularly in block/qcow2-*.c, appear
to handle -EIO just fine.

I did notice, however, that qcow2-bitmap.c:free_bitmap_clusters()
returns an int failure, but none of its three callers
(qcow2_co_remove_persistent_dirty_bitmap, and twice in
qcow2_store_persistent_dirty_bitmaps) care about the return value.
That may be worth a separate cleanup patch.

> 
> As a precaution, I also dropped Paolo's R-b.
> 
>  block/io.c               | 41 ----------------------------------------
>  include/block/block-io.h | 15 +++++++++------
>  2 files changed, 9 insertions(+), 47 deletions(-)

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

On Thu, Jun 23, 2022 at 10:47 PM Eric Blake <eblake@redhat.com> wrote:
> I did not get through all of the callers (you are right, there ARE a
> lot), but the ones I checked, particularly in block/qcow2-*.c, appear
> to handle -EIO just fine.
>
> I did notice, however, that qcow2-bitmap.c:free_bitmap_clusters()
> returns an int failure, but none of its three callers
> (qcow2_co_remove_persistent_dirty_bitmap, and twice in
> qcow2_store_persistent_dirty_bitmaps) care about the return value.
> That may be worth a separate cleanup patch.

Thanks, Eric. I decided to add a check for this kind of thing to the
static analyzer I've been working on, which I sent as an RFC [1].

Alberto

[1] https://lore.kernel.org/qemu-devel/20220702113331.2003820-1-afaria@redhat.com/

On 09.06.22 17:27, Alberto Faria wrote:
> bdrv_{pread,pwrite}() now return -EIO instead of -EINVAL when 'bytes' is
> negative, making them consistent with bdrv_{preadv,pwritev}() and
> bdrv_co_{pread,pwrite,preadv,pwritev}().
>
> bdrv_pwrite_zeroes() now also calls trace_bdrv_co_pwrite_zeroes() and
> clears the BDRV_REQ_MAY_UNMAP flag when appropriate, which it didn't
> previously.
>
> Signed-off-by: Alberto Faria <afaria@redhat.com>
> ---
>
> I audited all bdrv_{pread,pwrite}() callers to make sure that changing
> the -EINVAL return code to -EIO wont't break things. However, there are
> about 140 call sites, so the probability of me having missed something
> isn't negligible. If someone more accustomed to the code base is able to
> double-check this, that would be very much appreciated.

FWIW I checked all call sits when reviewing patch 3, and I can’t 
remember any case where the follow-up check was anything but `ret < 0`.  
The only difference should be a couple of error_setg_errno() calls, 
which will change from “Invalid argument” to “I/O error”.

I guess the real problem wouldn’t be checking the immediate call sites, 
but the fact that most call sites just pass the error code to their 
caller in turn, and that’s really something that’s unreasonable to 
check, I believe.

Honestly, I don’t think it really matters (how likely is `bytes < 0`?), 
nor could I imagine a case where EINVAL vs. EIO would cause any 
difference in behavior.  That’s to say, I’d be disappointed if it did.

(Grepping for 'if.*EINVAL' and 'if.*EIO' in block/ only yields one case 
in block/nbd.c where I can’t quickly absolutely rule out it won’t make a 
difference, but I think that check is only for error codes coming from 
handling network requests.)

> As a precaution, I also dropped Paolo's R-b.
>
>   block/io.c               | 41 ----------------------------------------
>   include/block/block-io.h | 15 +++++++++------
>   2 files changed, 9 insertions(+), 47 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>