Re: [Qemu-devel] [PATCH v2] nbd: Don't trim unrequested bytes

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]

On 05/25/2016 04:59 AM, Eric Blake wrote:
> Similar to commit df7b97ff, we are mishandling clients that
> give an unaligned NBD_CMD_TRIM request, and potentially
> trimming bytes that occur before their request; which in turn
> can cause potential unintended data loss (unlikely in
> practice, since most clients are sane and issue aligned trim
> requests).  However, while we fixed read and write by switching
> to the byte interfaces of blk_, we don't yet have a byte
> interface for discard.  On the other hand, trim is advisory, so
> rounding the user's request to simply ignore the first and last
> unaligned sectors (or the entire request, if it is sub-sector
> in length) is just fine.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> v2: don't underflow request.len on sub-sector requests
> 
>  nbd/server.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index fa862cd..b2cfeb9 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1153,12 +1153,20 @@ static void nbd_trip(void *opaque)
>          break;
>      case NBD_CMD_TRIM:
>          TRACE("Request type is TRIM");
> -        ret = blk_co_discard(exp->blk, (request.from + exp->dev_offset)
> -                                       / BDRV_SECTOR_SIZE,
> -                             request.len / BDRV_SECTOR_SIZE);
> -        if (ret < 0) {
> -            LOG("discard failed");
> -            reply.error = -ret;
> +        /* Ignore unaligned head or tail, until block layer adds byte
> +         * interface */
> +        if (request.len >= BDRV_SECTOR_SIZE) {
> +            request.len -= (request.from + request.len) % BDRV_SECTOR_SIZE;
> +            ret = blk_co_discard(exp->blk,
> +                                 DIV_ROUND_UP(request.from + exp->dev_offset,
> +                                              BDRV_SECTOR_SIZE),
> +                                 request.len / BDRV_SECTOR_SIZE);

This can end up calling blk_co_discard(, , 0) - do we need to audit
whether all underlying drivers handle a 0-length request, and/or
special-case this in blk_co_discard() to be an explicit no-op?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]