Re: [Qemu-devel] [PATCH] ide_dma_cancel will result in partial DMA transfer (resend #4)

[malc]

On Tue, 27 Jul 2010, Andrea Arcangeli wrote:

> Subject: avoid canceling ide dma
> 
> From: Andrea Arcangeli <address@hidden>
> 
> The reason for not actually canceling the I/O is because with
> virtualization and lots of VM running, a guest fs may mistake a
> overload of the host, as an IDE timeout. So rather than canceling the
> I/O, it's safer to wait I/O completion and simulate that the I/O has
> completed just before the io cancellation was requested by the
> guest. This way if ntfs or an app writes data without checking for
> -EIO retval, and it thinks the write has succeeded, it's less likely
> to run into troubles. Similar issues for reads.
> 
> Furthermore because the DMA operation is splitted into many synchronous
> aio_read/write if there's more than one entry in the SG table, without this
> patch the DMA would be cancelled in the middle, something we've no idea if it
> happens on real hardware too or not. Overall this seems a great risk for zero
> gain.
> 
> This approach is sure safer than previous code given we can't pretend all 
> guest
> fs code out there to check for errors and reply the DMA if it was completed
> partially, given a timeout would never materialize on a real harddisk unless
> there are defective blocks (and defective blocks are practically only an issue
> for reads never for writes in any recent hardware as writing to blocks is the
> way to fix them) or the harddisk breaks as a whole.
> 
> Signed-off-by: Izik Eidus <address@hidden>
> Signed-off-by: Andrea Arcangeli <address@hidden>
> ---
> 
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 780fc5f..9f6d42a 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -40,8 +40,25 @@ void bmdma_cmd_writeb(void *opaque, uint32_t addr, 
> uint32_t val)
>      printf("%s: 0x%08x\n", __func__, val);
>  #endif
>      if (!(val & BM_CMD_START)) {
> -        /* XXX: do it better */
> -        ide_dma_cancel(bm);
> +        /*
> +      * We can't cancel Scatter Gather DMA in the middle of the
> +      * operation or a partial (not full) DMA transfer would reach
> +      * the storage so we wait for completion instead (we beahve
> +      * like if the DMA was completed by the time the guest trying
> +      * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
> +      * set).
> +      *
> +      * In the future we'll be able to safely cancel the I/O if the
> +      * whole DMA operation will be submitted to disk with a single
> +      * aio operation with preadv/pwritev.
> +      */
> +     if (bm->aiocb) {
> +             qemu_aio_flush();
> +             if (bm->aiocb)
> +                     printf("ide_dma_cancel: aiocb still pending");
> +             if (bm->status & BM_STATUS_DMAING)
> +                     printf("ide_dma_cancel: BM_STATUS_DMAING still 
> pending");
> +     }

Indentation is off.

>          bm->cmd = val & 0x09;
>      } else {
>          if (!(bm->status & BM_STATUS_DMAING)) {
> 
> 

-- 
mailto:address@hidden