[Qemu-devel] [PATCH for-2.4] macio: re-add TRIM support

[Qemu-devel] [PATCH for-2.4] macio: re-add TRIM support

[Aurelien Jarno]

Commit bd4214fc dropped TRIM support by mistake. Given it is still
advertised to the host when using a drive with discard=on, this cause
the IDE bus to hang when the host issues a TRIM command.

This patch fixes that by re-adding the TRIM code, ported to the new
new DMA implementation.

Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/ide/macio.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index a55a479..66ac2ba 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -208,6 +208,33 @@ static void pmac_dma_write(BlockBackend *blk,
                               cb, io);
 }
 
+static void pmac_dma_trim(BlockBackend *blk,
+                        int64_t offset, int bytes,
+                        void (*cb)(void *opaque, int ret), void *opaque)
+{
+    DBDMA_io *io = opaque;
+    MACIOIDEState *m = io->opaque;
+    IDEState *s = idebus_active_if(&m->bus);
+    dma_addr_t dma_addr, dma_len;
+    void *mem;
+
+    qemu_iovec_destroy(&io->iov);
+    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
+
+    dma_addr = io->addr;
+    dma_len = io->len;
+    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
+                         DMA_DIRECTION_TO_DEVICE);
+
+    qemu_iovec_add(&io->iov, mem, io->len);
+    s->io_buffer_size -= io->len;
+    s->io_buffer_index += io->len;
+    io->len = 0;
+
+    m->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov, (bytes >> 9),
+                              cb, io);
+}
+
 static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
 {
     DBDMA_io *io = opaque;
@@ -313,6 +340,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_TRIM:
+        pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
     }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH for-2.4] macio: re-add TRIM support

[John Snow]

On 07/29/2015 03:27 PM, Aurelien Jarno wrote:
> Commit bd4214fc dropped TRIM support by mistake. Given it is still
> advertised to the host when using a drive with discard=on, this cause
> the IDE bus to hang when the host issues a TRIM command.
> 
> This patch fixes that by re-adding the TRIM code, ported to the new
> new DMA implementation.
> 
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: John Snow <jsnow@redhat.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/ide/macio.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index a55a479..66ac2ba 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -208,6 +208,33 @@ static void pmac_dma_write(BlockBackend *blk,
>                                cb, io);
>  }
>  
> +static void pmac_dma_trim(BlockBackend *blk,
> +                        int64_t offset, int bytes,
> +                        void (*cb)(void *opaque, int ret), void *opaque)
> +{
> +    DBDMA_io *io = opaque;
> +    MACIOIDEState *m = io->opaque;
> +    IDEState *s = idebus_active_if(&m->bus);
> +    dma_addr_t dma_addr, dma_len;
> +    void *mem;
> +
> +    qemu_iovec_destroy(&io->iov);
> +    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
> +
> +    dma_addr = io->addr;
> +    dma_len = io->len;
> +    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
> +                         DMA_DIRECTION_TO_DEVICE);
> +
> +    qemu_iovec_add(&io->iov, mem, io->len);
> +    s->io_buffer_size -= io->len;
> +    s->io_buffer_index += io->len;
> +    io->len = 0;
> +
> +    m->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov, (bytes >> 9),
> +                              cb, io);
> +}
> +
>  static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>  {
>      DBDMA_io *io = opaque;
> @@ -313,6 +340,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>          pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
>      case IDE_DMA_TRIM:
> +        pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
>      }
>  
> 

Looks sane, and tested myself.

Not that this is a problem you introduced, but we don't *unmap* this
memory anywhere, something else I overlooked for the initial patch.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH for-2.4] macio: re-add TRIM support

[Mark Cave-Ayland]

On 29/07/15 20:27, Aurelien Jarno wrote:

> Commit bd4214fc dropped TRIM support by mistake. Given it is still
> advertised to the host when using a drive with discard=on, this cause
> the IDE bus to hang when the host issues a TRIM command.
> 
> This patch fixes that by re-adding the TRIM code, ported to the new
> new DMA implementation.
> 
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: John Snow <jsnow@redhat.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/ide/macio.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index a55a479..66ac2ba 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -208,6 +208,33 @@ static void pmac_dma_write(BlockBackend *blk,
>                                cb, io);
>  }
>  
> +static void pmac_dma_trim(BlockBackend *blk,
> +                        int64_t offset, int bytes,
> +                        void (*cb)(void *opaque, int ret), void *opaque)
> +{
> +    DBDMA_io *io = opaque;
> +    MACIOIDEState *m = io->opaque;
> +    IDEState *s = idebus_active_if(&m->bus);
> +    dma_addr_t dma_addr, dma_len;
> +    void *mem;
> +
> +    qemu_iovec_destroy(&io->iov);
> +    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
> +
> +    dma_addr = io->addr;
> +    dma_len = io->len;
> +    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
> +                         DMA_DIRECTION_TO_DEVICE);
> +
> +    qemu_iovec_add(&io->iov, mem, io->len);
> +    s->io_buffer_size -= io->len;
> +    s->io_buffer_index += io->len;
> +    io->len = 0;
> +
> +    m->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov, (bytes >> 9),
> +                              cb, io);
> +}
> +
>  static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>  {
>      DBDMA_io *io = opaque;
> @@ -313,6 +340,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>          pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
>      case IDE_DMA_TRIM:
> +        pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
>      }

Apologies for the slow response on this (it has been a busy week here).
The basic patch itself looks good, my only question would be about the
effect on the Darwin/OS X family, since like the previous code the TRIM
request is rounded to the nearest sector whereas these OSs do use
byte-aligned DMA buffers.

Having said that, since the option has to be explicitly enabled for the
guest (and does the OS driver even support TRIM on these OSs?) then I
think this patch is fine, and I can confirm it passes my tests here.


ATB,

Mark.

Re: [Qemu-devel] [PATCH for-2.4] macio: re-add TRIM support

[Aurelien Jarno]

On 2015-08-01 12:10, Mark Cave-Ayland wrote:
> On 29/07/15 20:27, Aurelien Jarno wrote:
> 
> > Commit bd4214fc dropped TRIM support by mistake. Given it is still
> > advertised to the host when using a drive with discard=on, this cause
> > the IDE bus to hang when the host issues a TRIM command.
> > 
> > This patch fixes that by re-adding the TRIM code, ported to the new
> > new DMA implementation.
> > 
> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Cc: John Snow <jsnow@redhat.com>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  hw/ide/macio.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> > index a55a479..66ac2ba 100644
> > --- a/hw/ide/macio.c
> > +++ b/hw/ide/macio.c
> > @@ -208,6 +208,33 @@ static void pmac_dma_write(BlockBackend *blk,
> >                                cb, io);
> >  }
> >  
> > +static void pmac_dma_trim(BlockBackend *blk,
> > +                        int64_t offset, int bytes,
> > +                        void (*cb)(void *opaque, int ret), void *opaque)
> > +{
> > +    DBDMA_io *io = opaque;
> > +    MACIOIDEState *m = io->opaque;
> > +    IDEState *s = idebus_active_if(&m->bus);
> > +    dma_addr_t dma_addr, dma_len;
> > +    void *mem;
> > +
> > +    qemu_iovec_destroy(&io->iov);
> > +    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
> > +
> > +    dma_addr = io->addr;
> > +    dma_len = io->len;
> > +    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
> > +                         DMA_DIRECTION_TO_DEVICE);
> > +
> > +    qemu_iovec_add(&io->iov, mem, io->len);
> > +    s->io_buffer_size -= io->len;
> > +    s->io_buffer_index += io->len;
> > +    io->len = 0;
> > +
> > +    m->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov, (bytes >> 9),
> > +                              cb, io);
> > +}
> > +
> >  static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
> >  {
> >      DBDMA_io *io = opaque;
> > @@ -313,6 +340,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
> >          pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
> >          break;
> >      case IDE_DMA_TRIM:
> > +        pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
> >          break;
> >      }
> 
> Apologies for the slow response on this (it has been a busy week here).
> The basic patch itself looks good, my only question would be about the
> effect on the Darwin/OS X family, since like the previous code the TRIM
> request is rounded to the nearest sector whereas these OSs do use
> byte-aligned DMA buffers.

Indeed it's likely not to work. That said the previous code was doing
the same, the TRIM part was not taken care of in the unaligned support
code.

> Having said that, since the option has to be explicitly enabled for the
> guest (and does the OS driver even support TRIM on these OSs?) then I
> think this patch is fine, and I can confirm it passes my tests here.

I don't know about Darwin/OS X family. That said, while TRIM support has
to be manually enabled under Linux, some distributions do this
automatically at the installation time, either by using the discard
mount option in /etc/fstab or by providing a regular cron job calling
fstrim.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net