Re: [Qemu-block] [PATCH] block: Simplify bdrv_can_write_zeroes_with_unmap()

[Stefan Hajnoczi]

On Fri, Jan 26, 2018 at 01:34:39PM -0600, Eric Blake wrote:
> We don't need the can_write_zeroes_with_unmap field in
> BlockDriverInfo, because it is redundant information with
> supported_zero_flags & BDRV_REQ_MAY_UNMAP.  Note that
> BlockDriverInfo and supported_zero_flags are both per-device
> settings, rather than global state about the driver as a
> whole, which means one or both of these bits of information
> can already be conditional.  Let's audit how they were set:
> 
> crypto: always setting can_write_ to false is pointless (the
> struct starts life zero-initialized), no use of supported_
> 
> nbd: just recently fixed to set can_write_ if supported_
> includes MAY_UNMAP (thus this commit effectively reverts
> bca80059e and solves the problem mentioned there in a more
> global way)
> 
> file-posix, iscsi, qcow2: can_write_ is conditional, while
> supported_ was unconditional; but passing MAY_UNMAP would
> fail with ENOTSUP if the condition wasn't met
> 
> qed: can_write_ is unconditional, but pwrite_zeroes lacks
> support for MAY_UNMAP and supported_ is not set. Perhaps
> support can be added later (since it would be similar to
> qcow2), but for now claiming false is no real loss
> 
> all other drivers: can_write_ is not set, and supported_ is
> either unset or a passthrough
> 
> Simplify the code by moving the conditional into
> supported_zero_flags for all drivers, then dropping the
> now-unused BDI field.  For callers that relied on
> bdrv_can_write_zeroes_with_unmap(), we return the same
> per-device settings for drivers that had conditions (no
> observable change in behavior there); and can now return
> true (instead of false) for drivers that support passthrough
> (for example, the commit driver) which gives those drivers
> the same fix as nbd just got in bca80059e.  For callers that
> relied on supported_zero_flags, we now have a few more places
> that can avoid a wasted call to pwrite_zeroes() that will
> just fail with ENOTSUP.
> 
> Suggested-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> The commit id mentioned above is dependent on me not having to respin
> my latest NBD pull request:
> 
> Based-on: <address@hidden>
> ([PULL 0/8] NBD patches through 26 Jan)
> ---
>  include/block/block.h | 11 -----------
>  block.c               |  8 +-------
>  block/crypto.c        |  1 -
>  block/file-posix.c    |  3 +--
>  block/iscsi.c         |  6 ++++--
>  block/nbd.c           | 11 -----------
>  block/qcow2.c         |  3 +--
>  block/qed.c           |  1 -
>  8 files changed, 7 insertions(+), 37 deletions(-)

Reviewed-by: Stefan Hajnoczi <address@hidden>

signature.asc
Description: PGP signature