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

From: Eric Blake <eblake@redhat.com>
To: Alberto Faria <afaria@redhat.com>
Cc: qemu-devel@nongnu.org, "John Snow" <jsnow@redhat.com>,
	"Kevin Wolf" <kwolf@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Ari Sundholm" <ari@tuxera.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Denis V. Lunev" <den@openvz.org>, "Fam Zheng" <fam@euphon.net>,
	"Jeff Cody" <codyprime@gmail.com>,
	qemu-block@nongnu.org,
	"Vladimir Sementsov-Ogievskiy" <v.sementsov-og@mail.ru>,
	"Hanna Reitz" <hreitz@redhat.com>, "Stefan Weil" <sw@weilnetz.de>
Subject: Re: [PATCH v5 07/10] block: Implement bdrv_{pread,pwrite,pwrite_zeroes}() using generated_co_wrapper
Date: Thu, 23 Jun 2022 16:47:06 -0500	[thread overview]
Message-ID: <20220623214706.hxjpk4ub76w37g6t@redhat.com> (raw)
In-Reply-To: <20220609152744.3891847-8-afaria@redhat.com>

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org