Skip to contentSkip to conversation

Block patches

GIT PULL In review Kevin Wolf 2 participants 11 messages Jul 26, 2010 – Jul 27, 2010

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

Some more fixes for 0.13.

The following changes since commit cdcf9153e5e17dde340135fee5dcc7c299f2d4f5:

etrax: Update ethernet mgm-ctrl reg on writes (2010-07-25 21:03:56 +0200)

are available in the git repository at:
git://repo.or.cz/qemu/kevin.git for-anthony

Alex Williamson (1):
virtio-blk: Create exit function to unregister savevm

Aurelien Jarno (1):
ide/atapi: add support for GET EVENT STATUS NOTIFICATION

Blue Swirl (1):
Fix -snapshot deleting images on disk change

Bruce Rogers (1):
move 'unsafe' to end of caching modes in help

Christoph Hellwig (1):
block: default to 0 minimal / optiomal I/O size

Stefan Weil (1):
block: Use error codes from lower levels for error message

Yoshiaki Tamura (1):
block migration: propagate return value when bdrv_write() returns < 0

 block-migration.c |    6 +++++-
 block.c           |   32 ++++++++++++++++++++++++--------
 block.h           |    1 +
 block_int.h       |    4 ++--
 blockdev.c        |    1 +
 hw/ide/core.c     |   15 +++++++++++++++
 hw/virtio-blk.c   |    8 ++++++++
 hw/virtio-pci.c   |    1 +
 hw/virtio.h       |    1 +
 qemu-options.hx   |    2 +-
 10 files changed, 59 insertions(+), 12 deletions(-)
author
#1 lore raw

PATCH 1/7 ide/atapi: add support for GET EVENT STATUS NOTIFICATION

From: Aurelien Jarno <[email protected]>

The GET EVENT STATUS NOTIFICATION is a mandatory command according to MMC-3, even if event status notification is not supported.

This patch adds support for this command. It returns NEA ("No Event Available") with an empty "Supported Event Classes" to show that it doesn't event support status notification. If asychronous operation is requested, which requires NCQ support, it returns an error according to the specifications.

This fixes HAL support on FreeBSD and derivatives, which fill up the logs every second with:

acd0: FAILURE - unknown CMD (0x03) ILLEGAL REQUEST asc=0x20 ascq=0x00

---
 hw/ide/core.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
hw/ide/core.c +15 -0
index e20f2e7..9e1bdd5 100644
@@ -1643,6 +1643,21 @@ static void ide_atapi_cmd(IDEState *s)
             ide_atapi_cmd_reply(s, len, max_len);
             break;
         }
+    case GPCMD_GET_EVENT_STATUS_NOTIFICATION:
+        max_len = ube16_to_cpu(packet + 7);
+
+        if (packet[1] & 0x01) { /* polling */
+            /* We don't support any event class (yet). */
+            cpu_to_ube16(buf, 0x00); /* No event descriptor returned */
+            buf[2] = 0x80;           /* No Event Available (NEA) */
+            buf[3] = 0x00;           /* Empty supported event classes */
+            ide_atapi_cmd_reply(s, 4, max_len);
+        } else { /* asynchronous mode */
+            /* Only polling is supported, asynchronous mode is not. */
+            ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
+                                ASC_INV_FIELD_IN_CMD_PACKET);
+        }
+        break;
     default:
         ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
                             ASC_ILLEGAL_OPCODE);
Signature · 1 line
1.7.1.1
author
#2 lore raw

PATCH 2/7 block migration: propagate return value when bdrv_write() returns < 0

In reply to Kevin Wolf

From: Yoshiaki Tamura <[email protected]>

Currently block_load() doesn't check return value of bdrv_write(), and even the destination weren't prepared to execute block migration, it proceeds and guest boots on the target. This patch fix this issue.

---
 block-migration.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
block-migration.c +5 -1
index a77106e..8eda307 100644
@@ -586,6 +586,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
         addr >>= BDRV_SECTOR_BITS;
 
         if (flags & BLK_MIG_FLAG_DEVICE_BLOCK) {
+            int ret;
             /* get device name */
             len = qemu_get_byte(f);
             qemu_get_buffer(f, (uint8_t *)device_name, len);
@@ -601,9 +602,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
             buf = qemu_malloc(BLOCK_SIZE);
 
             qemu_get_buffer(f, buf, BLOCK_SIZE);
-            bdrv_write(bs, addr, buf, BDRV_SECTORS_PER_DIRTY_CHUNK);
+            ret = bdrv_write(bs, addr, buf, BDRV_SECTORS_PER_DIRTY_CHUNK);
 
             qemu_free(buf);
+            if (ret < 0) {
+                return ret;
+            }
         } else if (flags & BLK_MIG_FLAG_PROGRESS) {
             if (!banner_printed) {
                 printf("Receiving block device images\n");
Signature · 1 line
1.7.1.1
author
#3 lore raw

PATCH 3/7 virtio-blk: Create exit function to unregister savevm

In reply to Kevin Wolf

From: Alex Williamson <[email protected]>

Otherwise we can't migrate after we've removed a virtio block device.

---
 hw/virtio-blk.c |    8 ++++++++
 hw/virtio-pci.c |    1 +
 hw/virtio.h     |    1 +
 3 files changed, 10 insertions(+), 0 deletions(-)
hw/virtio-blk.c +8 -0
index f50069d..490cd41 100644
@@ -28,6 +28,7 @@ typedef struct VirtIOBlock
     BlockConf *conf;
     unsigned short sector_mask;
     char sn[BLOCK_SERIAL_STRLEN];
+    DeviceState *qdev;
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -522,9 +523,16 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
 
     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
+    s->qdev = dev;
     register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
                     virtio_blk_save, virtio_blk_load, s);
     bdrv_set_removable(s->bs, 0);
 
     return &s->vdev;
 }
+
+void virtio_blk_exit(VirtIODevice *vdev)
+{
+    VirtIOBlock *s = to_virtio_blk(vdev);
+    unregister_savevm(s->qdev, "virtio-blk", s);
+}
hw/virtio-pci.c +1 -0
index 31a711e..17c3d15 100644
@@ -569,6 +569,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
+    virtio_blk_exit(proxy->vdev);
     blockdev_mark_auto_del(proxy->block.bs);
     return virtio_exit_pci(pci_dev);
 }
hw/virtio.h +1 -0
index e4306cd..30e472a 100644
@@ -194,6 +194,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf);
 
 
 void virtio_net_exit(VirtIODevice *vdev);
+void virtio_blk_exit(VirtIODevice *vdev);
 
 #define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
 	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
Signature · 1 line
1.7.1.1
author
#4 lore raw

PATCH 4/7 move 'unsafe' to end of caching modes in help

In reply to Kevin Wolf

From: Bruce Rogers <[email protected]>

Libvirt parses qemu help output to determine qemu features. In particular it probes for the following: "cache=writethrough|writeback|none". The addition of the unsafe cache mode was inserted within this string, as opposed to being added to the end, which impacted libvirt's probe. Unbreak libvirt by keeping the existing cache modes intact and add unsafe to the end.

This problem only manifests itself if a caching mode is explicitly specified in the libvirt xml, in which case older syntax for caching is passed to qemu, which it no longer understands.

---
 qemu-options.hx |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
qemu-options.hx +1 -1
index 0d7dd90..9ecc54e 100644
@@ -118,7 +118,7 @@ ETEXI
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
-    "       [,cache=writethrough|writeback|unsafe|none][,format=f]\n"
+    "       [,cache=writethrough|writeback|none|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
     "       [,readonly=on|off]\n"
     "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
Signature · 1 line
1.7.1.1
#5 lore raw

[Qemu-devel] Re: [PATCH 4/7] move 'unsafe' to end of caching modes in help

Hi Kevin,

There's an on-going discussion on this patch. Could you drop it from your pull request but leave it in your tree until the discussion is resolved?

Thanks,

Anthony Liguori

On Jul 26, 2010 9:01 AM, "Kevin Wolf" <[email protected]> wrote:

From: Bruce Rogers <[email protected]>

Libvirt parses qemu help output to determine qemu features. In particular it probes for the following: "cache=writethrough|writeback|none". The addition of the unsafe cache mode was inserted within this string, as opposed to being added to the end, which impacted libvirt's probe. Unbreak libvirt by keeping the existing cache modes intact and add unsafe to the end.

This problem only manifests itself if a caching mode is explicitly specified in the libvirt xml, in which case older syntax for caching is passed to qemu, which it no longer understands.

---
 qemu-options.hx |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
qemu-options.hx +1 -1
index 0d7dd90..9ecc54e 100644
@@ -118,7 +118,7 @@ ETEXI
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
    "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
    "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
-    "       [,cache=writethrough|writeback|unsafe|none][,format=f]\n"
+    "       [,cache=writethrough|writeback|none|unsafe][,format=f]\n"
    "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
    "       [,readonly=on|off]\n"
    "                use 'file' as a drive image\n", QEMU_ARCH_ALL)

--
1.7.1.1

author
#6 lore raw

[Qemu-devel] Re: [PATCH 4/7] move 'unsafe' to end of caching modes in help

Am 26.07.2010 19:28, schrieb Anthony Liguori:

Quoted 5 lines

Hi Kevin,

There's an on-going discussion on this patch. Could you drop it from your pull request but leave it in your tree until the discussion is resolved?

Right, so I guess waiting some hours was enough and you're going to pull it without changes now?

Kevin

#7 lore raw

[Qemu-devel] Re: [PATCH 4/7] move 'unsafe' to end of caching modes in help

Quoted 11 lines · Kevin Wolf

Am 26.07.2010 19:28, schrieb Anthony Liguori:

Quoted 6 lines

Hi Kevin,

There's an on-going discussion on this patch. Could you drop it from your pull request but leave it in your tree until the discussion is resolved?

Right, so I guess waiting some hours was enough and you're going to pull it without changes now?

Yeah, I'll do the pull now. Sorry for the noise :-)

Regards,

Anthony Liguori

Quoted 2 lines

Kevin

author
#8 lore raw

PATCH 5/7 block: default to 0 minimal / optiomal I/O size

In reply to Kevin Wolf

From: Christoph Hellwig <[email protected]>

Currently we set them to 512 bytes unless manually specified. Unforuntaly some brain-dead partitioning tools create unaligned partitions if they get low enough optiomal I/O size values, so don't report any at all unless explicitly set.

---
 block_int.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
block_int.h +2 -2
index 96ff4cf..f075a8c 100644
@@ -243,7 +243,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
                        _conf.logical_block_size, 512),                  \
     DEFINE_PROP_UINT16("physical_block_size", _state,                   \
                        _conf.physical_block_size, 512),                 \
-    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 512),  \
-    DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 512)
+    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
+    DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0)
 
 #endif /* BLOCK_INT_H */
Signature · 1 line
1.7.1.1
author
#9 lore raw

PATCH 6/7 block: Use error codes from lower levels for error message

In reply to Kevin Wolf

From: Stefan Weil <[email protected]>

"No such file or directory" is a misleading error message when a user tries to open a file with wrong permissions.

---
 block.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)
block.c +19 -8
index f837876..49e6cbc 100644
@@ -330,7 +330,7 @@ BlockDriver *bdrv_find_protocol(const char *filename)
     return NULL;
 }
 
-static BlockDriver *find_image_format(const char *filename)
+static int find_image_format(const char *filename, BlockDriver **pdrv)
 {
     int ret, score, score_max;
     BlockDriver *drv1, *drv;
@@ -338,19 +338,27 @@ static BlockDriver *find_image_format(const char *filename)
     BlockDriverState *bs;
 
     ret = bdrv_file_open(&bs, filename, 0);
-    if (ret < 0)
-        return NULL;
+    if (ret < 0) {
+        *pdrv = NULL;
+        return ret;
+    }
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
     if (bs->sg || !bdrv_is_inserted(bs)) {
         bdrv_delete(bs);
-        return bdrv_find_format("raw");
+        drv = bdrv_find_format("raw");
+        if (!drv) {
+            ret = -ENOENT;
+        }
+        *pdrv = drv;
+        return ret;
     }
 
     ret = bdrv_pread(bs, 0, buf, sizeof(buf));
     bdrv_delete(bs);
     if (ret < 0) {
-        return NULL;
+        *pdrv = NULL;
+        return ret;
     }
 
     score_max = 0;
@@ -364,7 +372,11 @@ static BlockDriver *find_image_format(const char *filename)
             }
         }
     }
-    return drv;
+    if (!drv) {
+        ret = -ENOENT;
+    }
+    *pdrv = drv;
+    return ret;
 }
 
 /**
@@ -571,12 +583,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
 
     /* Find the right image format driver */
     if (!drv) {
-        drv = find_image_format(filename);
+        ret = find_image_format(filename, &drv);
         probed = 1;
     }
 
     if (!drv) {
-        ret = -ENOENT;
         goto unlink_and_fail;
     }
 
Signature · 1 line
1.7.1.1
author
#10 lore raw

PATCH 7/7 Fix -snapshot deleting images on disk change

In reply to Kevin Wolf

From: Blue Swirl <[email protected]>

Block device change command did not copy BDRV_O_SNAPSHOT flag. Thus the new image did not have this flag and the file got deleted during opening.

Fix by copying BDRV_O_SNAPSHOT flag.

---
 block.c    |    5 +++++
 block.h    |    1 +
 blockdev.c |    1 +
 3 files changed, 7 insertions(+), 0 deletions(-)
block.c +5 -0
index 49e6cbc..452ae94 100644
@@ -1811,6 +1811,11 @@ int bdrv_can_snapshot(BlockDriverState *bs)
     return 1;
 }
 
+int bdrv_is_snapshot(BlockDriverState *bs)
+{
+    return !!(bs->open_flags & BDRV_O_SNAPSHOT);
+}
+
 BlockDriverState *bdrv_snapshots(void)
 {
     BlockDriverState *bs;
block.h +1 -0
index c2a7e4c..db131a3 100644
@@ -202,6 +202,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 int bdrv_can_snapshot(BlockDriverState *bs);
+int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
blockdev.c +1 -0
index 0a9dec3..01e402b 100644
@@ -590,6 +590,7 @@ int do_change_block(Monitor *mon, const char *device,
         return -1;
     }
     bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
+    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
     if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
         qerror_report(QERR_OPEN_FILE_FAILED, filename);
         return -1;
Signature · 1 line
1.7.1.1