Message ID | 1438198068-32428-1-git-send-email-aurelien@aurel32.net |
---|---|
State | New |
Headers | show |
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>
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.
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.
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; }
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(+)