[Qemu-block] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

iscsi_aio_cancel() does not increment the request's reference count,
causing a use-after-free when ABORT TASK finishes after the request has
already completed.

There are some additional issues with iscsi_aio_cancel():
1. Several ABORT TASKs may be sent for the same task if
   iscsi_aio_cancel() is invoked multiple times.  It's better to avoid
   this just in case the command identifier is reused.
2. The iscsilun->mutex protection is missing in iscsi_aio_cancel().

Reported-by: Felipe Franciosi 
Signed-off-by: Stefan Hajnoczi 
---
 block/iscsi.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 1cfe1c647c..8140baac15 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -119,6 +119,7 @@ typedef struct IscsiAIOCB {
 #ifdef __linux__
 sg_io_hdr_t *ioh;
 #endif
+bool cancelled;
 } IscsiAIOCB;
 
 /* libiscsi uses time_t so its enough to process events every second */
@@ -282,6 +283,7 @@ static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, 
struct IscsiTask *iTask)
 };
 }
 
+/* Called (via iscsi_service) with QemuMutex held. */
 static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void 
*command_data,
 void *private_data)
@@ -290,6 +292,7 @@ iscsi_abort_task_cb(struct iscsi_context *iscsi, int 
status, void *command_data,
 
 acb->status = -ECANCELED;
 iscsi_schedule_bh(acb);
+qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */
 }
 
 static void
@@ -298,14 +301,25 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
 IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
 IscsiLun *iscsilun = acb->iscsilun;
 
-if (acb->status != -EINPROGRESS) {
+qemu_mutex_lock(>mutex);
+
+/* If it was cancelled or completed already, our work is done here */
+if (acb->cancelled || acb->status != -EINPROGRESS) {
+qemu_mutex_unlock(>mutex);
 return;
 }
 
+acb->cancelled = true;
+
+qemu_aio_ref(acb); /* released in iscsi_abort_task_cb() */
+
 /* send a task mgmt call to the target to cancel the task on the target */
-iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
- iscsi_abort_task_cb, acb);
+if (iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
+ iscsi_abort_task_cb, acb) < 0) {
+qemu_aio_unref(acb); /* since iscsi_abort_task_cb() won't be called */
+}
 
+qemu_mutex_unlock(>mutex);
 }
 
 static const AIOCBInfo iscsi_aiocb_info = {
@@ -1000,6 +1014,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
 acb->bh  = NULL;
 acb->status  = -EINPROGRESS;
 acb->ioh = buf;
+acb->cancelled   = false;
 
 if (req != SG_IO) {
 iscsi_ioctl_handle_emulated(acb, req, buf);
-- 
2.14.3

[Qemu-block] [PATCH v2 2/3] block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()

[Stefan Hajnoczi]

Commit d045c466d9e62b4321fadf586d024d54ddfd8bd4 ("iscsi: do not use
aio_context_acquire/release") introduced iscsilun->mutex but appears to
have overlooked iscsi_timed_check_events() when introducing the mutex.

iscsi_service() and iscsi_set_events() must be called with
iscsilun->mutex held.

iscsi_timed_check_events() is invoked from the AioContext and does not
take the mutex.

Signed-off-by: Stefan Hajnoczi 
---
 block/iscsi.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index cd0738942c..1cfe1c647c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -339,6 +339,8 @@ static void iscsi_timed_check_events(void *opaque)
 {
 IscsiLun *iscsilun = opaque;
 
+qemu_mutex_lock(>mutex);
+
 /* check for timed out requests */
 iscsi_service(iscsilun->iscsi, 0);
 
@@ -351,6 +353,8 @@ static void iscsi_timed_check_events(void *opaque)
  * to return to service once this situation changes. */
 iscsi_set_events(iscsilun);
 
+qemu_mutex_unlock(>mutex);
+
 timer_mod(iscsilun->event_timer,
   qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
 }
-- 
2.14.3

[Qemu-block] [PATCH v2 1/3] block/iscsi: drop unused IscsiAIOCB->buf field

[Stefan Hajnoczi]

The IscsiAIOCB->buf field has not been used since commit
e49ab19fcaa617ad6cdfe1ac401327326b6a2552 ("block/iscsi: bump libiscsi
requirement to 1.9.0").  It used to be a linear buffer for old libiscsi
versions that didn't support scatter-gather.  The minimum libiscsi
version supports scatter-gather so we don't linearize buffers anymore.

Signed-off-by: Stefan Hajnoczi 
---
 block/iscsi.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a1c53711a..cd0738942c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -112,7 +112,6 @@ typedef struct IscsiAIOCB {
 QEMUBH *bh;
 IscsiLun *iscsilun;
 struct scsi_task *task;
-uint8_t *buf;
 int status;
 int64_t sector_num;
 int nb_sectors;
@@ -145,9 +144,6 @@ iscsi_bh_cb(void *p)
 
 qemu_bh_delete(acb->bh);
 
-g_free(acb->buf);
-acb->buf = NULL;
-
 acb->common.cb(acb->common.opaque, acb->status);
 
 if (acb->task != NULL) {
@@ -925,9 +921,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
 {
 IscsiAIOCB *acb = opaque;
 
-g_free(acb->buf);
-acb->buf = NULL;
-
 acb->status = 0;
 if (status < 0) {
 error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
@@ -1002,7 +995,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
 acb->iscsilun = iscsilun;
 acb->bh  = NULL;
 acb->status  = -EINPROGRESS;
-acb->buf = NULL;
 acb->ioh = buf;
 
 if (req != SG_IO) {
-- 
2.14.3

Re: [Qemu-block] [PATCH 3/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

On Fri, Feb 2, 2018 at 10:16 PM, Stefan Hajnoczi  wrote:
> The ioctl request cancellation code assumes that requests do not
> complete once TASK ABORT has been sent to the iSCSI target.  The request
> completion callback is unconditionally invoked when TASK ABORT finishes.
> Therefore the request completion callback is invoked twice if the
> request does happen to complete before TASK ABORT.
>
> Futhermore, iscsi_aio_cancel() does not increment the request's
> reference count, causing a use-after-free when TASK ABORT finishes after
> the request has already completed.
>
> The iscsilun->mutex protection is also missing in iscsi_aio_cancel().
>
> This patch rewrites iscsi_aio_cancel() and iscsi_abort_task_cb() to
> avoid double completion, use-after-free, and to take iscsilun->mutex
> when needed.
>
> Reported-by: Felipe Franciosi 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  block/iscsi.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 1cfe1c647c..4566902d43 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -282,14 +282,19 @@ static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, 
> struct IscsiTask *iTask)
>  };
>  }
>
> +/* Called (via iscsi_service) with QemuMutex held. */
>  static void
>  iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void 
> *command_data,
>  void *private_data)
>  {
>  IscsiAIOCB *acb = private_data;
>
> -acb->status = -ECANCELED;
> -iscsi_schedule_bh(acb);
> +/* Skip if the request already completed */
> +if (acb->status == -ECANCELED) {

There is a bug here.  acb->status can be -ECANCELED if the request
completed with COMMAND ABORTED.

I'll send a v2 of this patch tomorrow to solve this.

Stefan

Re: [Qemu-block] [Qemu-devel] rate limiting issues

[John Snow]

CCing qemu-block and Berto

On 02/02/2018 06:10 AM, Wolfgang Bumiller wrote:
> Summary:
> Rate limit is effectively halved when the size of written chunks adds up to
> exceeding the quota of a slice only slightly. This is surprisingly reliable.
> 
> Explanation:
> The ratelimiting code in include/qemu/ratelimit.h currently uses slices with
> quotas. Finishing up the quota for one slice means it'll wait for the end of
> this _and_ the next slice before resetting the accounting and start over.
> If that first slice was exceeded by only a tiny bit, we effectively spend 
> every
> second slice waiting around. before starting over.
> 
> Here if I use a limit of 3KiB/s I get 3KiB/s.
> Increasing the limit to 30700KiB/s gives me 30700KiB/s.
> Increasing it to 30720KiB/s reliably gives me 15000KiB/s.
> 
> Making it wait to the end of only the current slice means the excess data is 
> not
> counted at all and we may go above the limit (though by at most one 
> write-chunk,
> so I'm not sure if that's fine for most of the users, for backup jobs it seems
> to be 64k always).
> 
> I'd like to fix this and am unsure about which way to go. On the one hand I
> think the old code (before f14a39ccb922) may be fixable in a better way by not
> resetting the accounting completely but subtracting the amount of data the
> wait-period would have added.
> 
> At the same time, though, this could be simplified to not using slices but
> always comparing the amount of actually written data to the amount of data
> which should at most have been written.
> 
> Here are two approaches which seem to fix my issues:
> 
> --- Old code revised:
> 
> typedef struct {
> int64_t next_slice_time;
> uint64_t slice_quota;
> uint64_t slice_ns;
> int64_t dispatched;
> } RateLimit;
> 
> static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t n)
> {
> int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> 
> assert(limit->slice_quota && limit->slice_ns);
> 
> if (limit->next_slice_time == 0) { /* first call */
> limit->dispatched = 0;
> limit->next_slice_time = now + limit->slice_ns;
> return 0;
> }
> 
> if (limit->next_slice_time < now) {
> uint64_t passed_slices = DIV_ROUND_UP(now - limit->next_slice_time,
> limit->slice_ns);
> limit->next_slice_time = now + limit->slice_ns;
> limit->dispatched -= passed_slices * limit->slice_quota;
> }
> limit->dispatched += n;
> if (limit->dispatched+n <= limit->slice_quota) {
> return 0;
> }
> return limit->next_slice_time - now;
> }
> 
> static inline void ratelimit_set_speed(RateLimit *limit, uint64_t speed,
>uint64_t slice_ns)
> {
> limit->slice_ns = slice_ns;
> limit->slice_quota = MAX(((double)speed * slice_ns) / 10ULL, 1);
> }
> 
> ---
> 
> And this is a short slice-less version. I wonder if there's any particular
> reason for sticking to slices?
> 
> --- Version without slices:
> 
> typedef struct {
> int64_t last_time;
> uint64_t speed;
> int64_t allowed;
> } RateLimit;
> 
> static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t n)
> {
> int64_t delta, now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> 
> if (limit->last_time == 0) { /* first call */
> limit->allowed = -n;
> limit->last_time = now;
> return (n * 10ULL) / limit->speed;
> }
> 
> delta = (now - limit->last_time);
> limit->allowed += (delta * limit->speed)/10ULL - n;
> limit->last_time = now;
> if (limit->allowed < 0) {
> return ((uint64_t)-limit->allowed * 10ULL) / limit->speed;
> }
> return 0;
> }
> 
> static inline void ratelimit_set_speed(RateLimit *limit, uint64_t speed,
>uint64_t slice_ns)
> {
> (void)slice_ns; // TODO: remove
> limit->speed = speed;
> }
> 
> ---
> 
> Numerical note: a small delta means 'allowed' is incremented by 0, which
> should be fine since when we hit the quota, we'll have a longer wait after
> which the delta is for sure big enough to produce positive values.
> (I tried larger and smaller values (1KiB/s to some MiB/s)).
> Alternatively we could set last_time and do the quota increment
> conditionally only when the delta is big enough, but I have not found
> this to be necessary in my tests.
> 
> 

[Qemu-block] [PATCH 2/3] block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()

[Stefan Hajnoczi]

Commit d045c466d9e62b4321fadf586d024d54ddfd8bd4 ("iscsi: do not use
aio_context_acquire/release") introduced iscsilun->mutex but appears to
have overlooked iscsi_timed_check_events() when introducing the mutex.

iscsi_service() and iscsi_set_events() must be called with
iscsilun->mutex held.

iscsi_timed_check_events() is invoked from the AioContext and does not
take the mutex.

Signed-off-by: Stefan Hajnoczi 
---
 block/iscsi.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index cd0738942c..1cfe1c647c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -339,6 +339,8 @@ static void iscsi_timed_check_events(void *opaque)
 {
 IscsiLun *iscsilun = opaque;
 
+qemu_mutex_lock(>mutex);
+
 /* check for timed out requests */
 iscsi_service(iscsilun->iscsi, 0);
 
@@ -351,6 +353,8 @@ static void iscsi_timed_check_events(void *opaque)
  * to return to service once this situation changes. */
 iscsi_set_events(iscsilun);
 
+qemu_mutex_unlock(>mutex);
+
 timer_mod(iscsilun->event_timer,
   qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
 }
-- 
2.14.3

[Qemu-block] [PATCH 3/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

The ioctl request cancellation code assumes that requests do not
complete once TASK ABORT has been sent to the iSCSI target.  The request
completion callback is unconditionally invoked when TASK ABORT finishes.
Therefore the request completion callback is invoked twice if the
request does happen to complete before TASK ABORT.

Futhermore, iscsi_aio_cancel() does not increment the request's
reference count, causing a use-after-free when TASK ABORT finishes after
the request has already completed.

The iscsilun->mutex protection is also missing in iscsi_aio_cancel().

This patch rewrites iscsi_aio_cancel() and iscsi_abort_task_cb() to
avoid double completion, use-after-free, and to take iscsilun->mutex
when needed.

Reported-by: Felipe Franciosi 
Signed-off-by: Stefan Hajnoczi 
---
 block/iscsi.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 1cfe1c647c..4566902d43 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -282,14 +282,19 @@ static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, 
struct IscsiTask *iTask)
 };
 }
 
+/* Called (via iscsi_service) with QemuMutex held. */
 static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void 
*command_data,
 void *private_data)
 {
 IscsiAIOCB *acb = private_data;
 
-acb->status = -ECANCELED;
-iscsi_schedule_bh(acb);
+/* Skip if the request already completed */
+if (acb->status == -ECANCELED) {
+iscsi_schedule_bh(acb);
+}
+
+qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */
 }
 
 static void
@@ -298,14 +303,26 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
 IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
 IscsiLun *iscsilun = acb->iscsilun;
 
+qemu_mutex_lock(>mutex);
+
+/* If it was cancelled or completed already, our work is done here */
 if (acb->status != -EINPROGRESS) {
+qemu_mutex_unlock(>mutex);
 return;
 }
 
+/* This can still be overwritten if the request completes */
+acb->status = -ECANCELED;
+
+qemu_aio_ref(acb); /* released in iscsi_abort_task_cb() */
+
 /* send a task mgmt call to the target to cancel the task on the target */
-iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
- iscsi_abort_task_cb, acb);
+if (iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
+ iscsi_abort_task_cb, acb) < 0) {
+qemu_aio_unref(acb); /* since iscsi_abort_task_cb() won't be called */
+}
 
+qemu_mutex_unlock(>mutex);
 }
 
 static const AIOCBInfo iscsi_aiocb_info = {
-- 
2.14.3

[Qemu-block] [PATCH 1/3] block/iscsi: drop unused IscsiAIOCB->buf field

[Stefan Hajnoczi]

The IscsiAIOCB->buf field has not been used since commit
e49ab19fcaa617ad6cdfe1ac401327326b6a2552 ("block/iscsi: bump libiscsi
requirement to 1.9.0").  It used to be a linear buffer for old libiscsi
versions that didn't support scatter-gather.  The minimum libiscsi
version supports scatter-gather so we don't linearize buffers anymore.

Signed-off-by: Stefan Hajnoczi 
---
 block/iscsi.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a1c53711a..cd0738942c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -112,7 +112,6 @@ typedef struct IscsiAIOCB {
 QEMUBH *bh;
 IscsiLun *iscsilun;
 struct scsi_task *task;
-uint8_t *buf;
 int status;
 int64_t sector_num;
 int nb_sectors;
@@ -145,9 +144,6 @@ iscsi_bh_cb(void *p)
 
 qemu_bh_delete(acb->bh);
 
-g_free(acb->buf);
-acb->buf = NULL;
-
 acb->common.cb(acb->common.opaque, acb->status);
 
 if (acb->task != NULL) {
@@ -925,9 +921,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
 {
 IscsiAIOCB *acb = opaque;
 
-g_free(acb->buf);
-acb->buf = NULL;
-
 acb->status = 0;
 if (status < 0) {
 error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
@@ -1002,7 +995,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
 acb->iscsilun = iscsilun;
 acb->bh  = NULL;
 acb->status  = -EINPROGRESS;
-acb->buf = NULL;
 acb->ioh = buf;
 
 if (req != SG_IO) {
-- 
2.14.3

[Qemu-block] [PATCH 0/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

Patches 1 & 2 are cleanups.

Patch 3 fixes cancellation of ioctls.  Felipe showed me a trace where an acb is
cancelled and then completes twice.  The second time around crashes QEMU.

Compile-tested only.

Felipe: Please let us know if this fixes the issue you are seeing.  Thanks!

Stefan Hajnoczi (3):
  block/iscsi: drop unused IscsiAIOCB->buf field
  block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()
  block/iscsi: fix ioctl cancel use-after-free

 block/iscsi.c | 37 +
 1 file changed, 25 insertions(+), 12 deletions(-)

-- 
2.14.3

Re: [Qemu-block] [PULL 0/2] Block patches

[Peter Maydell]

On 1 February 2018 at 04:06, Jeff Cody  wrote:
> The following changes since commit b05631954d6dfe93340d516660397e2c1a2a5dd6:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-hppa-20180131' into 
> staging (2018-01-31 15:50:29 +)
>
> are available in the git repository at:
>
>   git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> for you to fetch changes up to 45a79646ea746fa3f32083d0aa70512aae29f6b3:
>
>   iotests: Make 200 run on tmpfs (2018-01-31 22:37:00 -0500)
>
> 
> Block patches
> 

Applied, thanks.

-- PMM

Re: [Qemu-block] [PATCH v7 for-2.12 21/25] block: Purify .bdrv_refresh_filename()

[Max Reitz]

On 2017-12-04 19:25, Max Reitz wrote:
> On 2017-12-04 17:37, Alberto Garcia wrote:
>> On Mon 20 Nov 2017 09:10:00 PM CET, Max Reitz wrote:
>>> -static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
>>> +static void blkdebug_refresh_filename(BlockDriverState *bs)
>>>  {
>>>  BDRVBlkdebugState *s = bs->opaque;
>>> -QDict *opts;
>>>  const QDictEntry *e;
>>> -bool force_json = false;
>>> -
>>> -for (e = qdict_first(options); e; e = qdict_next(options, e)) {
>>> -if (strcmp(qdict_entry_key(e), "config") &&
>>> -strcmp(qdict_entry_key(e), "x-image"))
>>> -{
>>> -force_json = true;
>>> -break;
>>> -}
>>> -}
>>> +int ret;
>>>  
>>> -if (force_json && !bs->file->bs->full_open_options) {
>>> -/* The config file cannot be recreated, so creating a plain 
>>> filename
>>> - * is impossible */
>>> +if (!bs->file->bs->exact_filename[0]) {
>>>  return;
>>>  }
>>>  
>>> -if (!force_json && bs->file->bs->exact_filename[0]) {
>>> -int ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
>>> -   "blkdebug:%s:%s", s->config_file ?: "",
>>> -   bs->file->bs->exact_filename);
>>> -if (ret >= sizeof(bs->exact_filename)) {
>>> -/* An overflow makes the filename unusable, so do not report 
>>> any */
>>> -bs->exact_filename[0] = 0;
>>> +for (e = qdict_first(bs->full_open_options); e;
>>> + e = qdict_next(bs->full_open_options, e))
>>> +{
>>> +if (strcmp(qdict_entry_key(e), "config") &&
>>> +strcmp(qdict_entry_key(e), "image") &&
>>
>> Shouldn't this be "x-image" ?
> 
> Er, yes.  It should.

Actually, it should be both.  That's because the child is attached as
"image" and not "x-image", so when the child options are gathered, they
are put under "image".

And since the child is attached using bdrv_open_child(), you have to
specify all child options in an "image" sub qdict, too (as can be seen
in iotest 099), so this is indeed correct.  (Btw, note that the old code
already put these options under "image".)

(So with "x-image" instead of "image", iotest 162 fails.)

Of course, x-image can be specified, too (although I wouldn't really
mind breaking that for users...), so we have to ignore that, still.


Before this patch, we could ignore "image" because we iterated over the
options before they were newly generated.  Now they are generated
automatically before this function is called, so there may be an "image"
key now.

x-image



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps

[Max Reitz]

On 2018-02-02 17:18, Eric Blake wrote:
> On 02/02/2018 10:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> To maintain load/store disabled bitmap there is new approach:
>>
>>  - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
>>  - store enabled bitmaps as "auto" to qcow2
>>  - store disabled bitmaps without "auto" flag to qcow2
>>  - on qcow2 open load "auto" bitmaps as enabled and others
>>as disabled (except in_use bitmaps)
>>
>> Also, adjust iotests 165 and 176 appropriately.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
> 
>> +++ b/qemu-doc.texi
>> @@ -2749,6 +2749,13 @@ used and it will be removed with no replacement.
>>  The ``convert -s snapshot_id_or_name'' argument is obsoleted
>>  by the ``convert -l snapshot_param'' argument instead.
>>  
>> +@section QEMU Machine Protocol (QMP) commands
>> +
>> +@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
>> +
>> +"autoload" parameter is now ignored. All bitmaps are automatically loaded
>> +from qcow2 image.
> 
> Won't later patches be adding the ability to enable/disable bitmaps,
> which then affects whether they are autoloaded?  So we don't forget to
> revisit this text in that patch, a better wording might be:
> 
> The "autoload" parameter is ignored; all enabled persistent dirty
> bitmaps are automatically loaded from a qcow2 image, regardless of the
> initial setting requested in this parameter.
> 
> 
>> @@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
>>  }
>>  
>>  /* Called with BQL taken. */
>> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
>> -{
>> -qemu_mutex_lock(bitmap->mutex);
>> -bitmap->autoload = autoload;
>> -qemu_mutex_unlock(bitmap->mutex);
>> -}
>> -
>> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
>> -{
>> -return bitmap->autoload;
>> -}
> 
> Will later patches be reintroducing these functions for learning which
> bitmaps are enabled/disabled?  But I'm okay with deleting them in this
> patch, even if that is more churn.

You mean bdrv_enable_dirty_bitmap(), bdrv_disable_dirty_bitmap(), and
bdrv_dirty_bitmap_enabled()? ;-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/2] qcow2: add overlap check for bitmap directory

[Max Reitz]

On 2018-02-02 14:48, Vladimir Sementsov-Ogievskiy wrote:
> 02.02.2018 16:00, Max Reitz wrote:
>> On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.01.2018 18:34, Max Reitz wrote:
 On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>    block/qcow2.h  |  7 +--
>    block/qcow2-refcount.c | 12 
>    block/qcow2.c  |  6 ++
>    3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6f0ff15dd0..8f226a3609 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -98,6 +98,7 @@
>    #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
> "overlap-check.snapshot-table"
>    #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
>    #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
> "overlap-check.bitmap-directory"
>    #define QCOW2_OPT_CACHE_SIZE "cache-size"
>    #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
>    #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap {
>    QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>    QCOW2_OL_INACTIVE_L1_BITNR    = 6,
>    QCOW2_OL_INACTIVE_L2_BITNR    = 7,
> +    QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
>    -    QCOW2_OL_MAX_BITNR    = 8,
> +    QCOW2_OL_MAX_BITNR  = 9,
>      QCOW2_OL_NONE   = 0,
>    QCOW2_OL_MAIN_HEADER    = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap {
>    /* NOTE: Checking overlaps with inactive L2 tables will result
> in bdrv
>     * reads. */
>    QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> +    QCOW2_OL_BITMAP_DIRECTORY = (1 <<
> QCOW2_OL_BITMAP_DIRECTORY_BITNR),
>    } QCow2MetadataOverlap;
>      /* Perform all overlap checks which can be done in constant
> time */
>    #define QCOW2_OL_CONSTANT \
>    (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
> QCOW2_OL_REFCOUNT_TABLE | \
> - QCOW2_OL_SNAPSHOT_TABLE)
> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>      /* Perform all overlap checks which don't require disk access */
>    #define QCOW2_OL_CACHED \
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3de1ab51ba..a7a2703f26 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -2585,6 +2585,18 @@ int
> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t
> offset,
>    }
>    }
>    +    if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
> +    (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
> +    {
> +    /* update_ext_header_and_dir_in_place firstly drop autoclear
> flag,
> + * so it will not fail */
 That's really not an argument.  bitmap_list_store() has to pass
 QCOW2_OL_BITMAP_DIRECTORY to @ign anyway.  (Because there is no reason
 not to.)
>>> in_place is a reason. When we store directory in_place, it definitely
>>> overlaps with current directory.
>> Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is
>> what that argument is for? :-)
> 
> hmm. but actually, I should not, because of zeroed autoclear flag. So,
> do you think, it is better to pass it, anyway?

Yes.  That flag describes what kind of metadata structures you are
planning to overwrite, and you *are* planning to overwrite the bitmap
directory, so you should set it.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

02.02.2018 19:18, Eric Blake wrote:

On 02/02/2018 10:07 AM, Vladimir Sementsov-Ogievskiy wrote:

To maintain load/store disabled bitmap there is new approach:

  - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
  - store enabled bitmaps as "auto" to qcow2
  - store disabled bitmaps without "auto" flag to qcow2
  - on qcow2 open load "auto" bitmaps as enabled and others
as disabled (except in_use bitmaps)

Also, adjust iotests 165 and 176 appropriately.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
+++ b/qemu-doc.texi
@@ -2749,6 +2749,13 @@ used and it will be removed with no replacement.
  The ``convert -s snapshot_id_or_name'' argument is obsoleted
  by the ``convert -l snapshot_param'' argument instead.
  
+@section QEMU Machine Protocol (QMP) commands

+
+@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
+
+"autoload" parameter is now ignored. All bitmaps are automatically loaded
+from qcow2 image.

Won't later patches be adding the ability to enable/disable bitmaps,
which then affects whether they are autoloaded?  So we don't forget to
revisit this text in that patch, a better wording might be:

The "autoload" parameter is ignored; all enabled persistent dirty
bitmaps are automatically loaded from a qcow2 image, regardless of the
initial setting requested in this parameter.



hmm.. no. all bitmaps are loaded, even disabled ones. Before this patch 
there is

no way to have disabled bitmap in qemu (by loading or by creating).
After the patch, we have a theoretical way of creating such bitmap in 
qcow2 image, then

it will be loaded as disabled.

Also, we can store bitmap with persistent=true and autoload=false before 
this patch, and there is no way
to load this bitmap before this patch, but after this patch it will be 
loaded as disabled.







@@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
  }
  
  /* Called with BQL taken. */

-void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
-{
-qemu_mutex_lock(bitmap->mutex);
-bitmap->autoload = autoload;
-qemu_mutex_unlock(bitmap->mutex);
-}
-
-bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
-{
-return bitmap->autoload;
-}

Will later patches be reintroducing these functions for learning which
bitmaps are enabled/disabled?  But I'm okay with deleting them in this
patch, even if that is more churn.



no, actually the aim of the patch is to drop buggy relation between 
autoload qmp parameter
and auto qcow2 flag (which is more like "enabled" then "autoload"). Look 
at "Reasoning" in head

letter for details.

--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

22.01.2018 22:56, John Snow wrote:


On 01/22/2018 02:51 PM, Eric Blake wrote:

On 01/22/2018 03:09 AM, Vladimir Sementsov-Ogievskiy wrote:


I have to admit exposing this interface still makes me nervous, but :)

Mechanically correct, and with suggesting phrasing changes:

Reviewed-by: John Snow 

Should we go with an x- prefix for now, and let things settle for a
release before promoting it to fully supported, just in case we find
something that justifies your nervousness in accepting the interface?


I proposed as much in a reply to the cover letter; I'm willing to take
patch one now for the sake of migration; the rest of this series I want
a test suite that helps document the anticipated use case, or otherwise
an x- prefix for the command names.


Give me a try with test, and if it will still be unclear, then let's add 
x- prefix for first steps.

(of course, my aim is to push the api without x- prefixes =)

--
Best regards,
Vladimir

[Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

To maintain load/store disabled bitmap there is new approach:

 - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
 - store enabled bitmaps as "auto" to qcow2
 - store disabled bitmaps without "auto" flag to qcow2
 - on qcow2 open load "auto" bitmaps as enabled and others
   as disabled (except in_use bitmaps)

Also, adjust iotests 165 and 176 appropriately.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

v3: fix iotests
add info into qemu-doc.texi
rm John's r-b (sorry=(

v2: add John's r-b
fix spelling

bitmaps-api and bitmaps-postcopy migrations depend on this patch, so
let's discuss (and merge if in case of consent) as soon as possible.

Reasoning:

Firstly, consider only enabled bitmaps (for now, actually, we can't
get disabled bitmap in Qemu, as block-dirty-bitmap-enable command
is not merged yet).

We (Virtuozzo) always use persistent=true and autoload=true. As, to
maintain dirty bitmap (enabled) valid, we must to load it every time,
when we load our disk for r/w, to track disk changes. I do not think,
that something like "not loading enabled dirty bitmaps, when open disk
for read-only" gives real benefits (and now it is not possible,
anyway, as loading depends on qcow2 bitmap "auto" flag, not on r-w
mode).

So, this flag is useless for now. Moreover, if we save bitmap with
autoload=false, it will not be loaded next time, and will become
invalid on first write to the disk. Creating bitmap with flags
"persistent=true, autoload=false", actually means "make it disabled
after storing" (weird semantic, isn't it?), so it will not be
automatically updated more. So, this flag is a bit dangerous.

Let's move to disabled bitmaps. Assume, that my patches will be
merged, and we will really have a possibility of enable/disable
bitmaps when we want. So, it's natural to expect, that if we have
persistent-enabled bitmaps and persistent-disabled bitmaps, then
enabled one will be enabled on next Qemu start and disabled will be
disabled.

How to achieve this?

Let's start from qcow2. We need a stored information on "is this
bitmap enabled or not". But we actually have it. Let's look on qcow2
"auto" flag definiton:
1: auto
   The bitmap must reflect all changes of the virtual
   disk by any application that would write to this qcow2
   file (including writes, snapshot switching, etc.). The
   type of this bitmap must be 'dirty tracking bitmap'.

Isn't it a definition of "enabled" bitmap?
And yes, current mapping from qapi flag "autoload" to qcow2 flag
"auto" is not good mapping.

So, it looks ok, to map !BdrvDirtyBitmap.disabled to "auto". If we
have in future some bitmaps in qcow2, which are not enabled and
not disabled (something more complicated), we can introduce new flags
or even new bitmap type.
(from qcow2 spec):
16:type
   This field describes the sort of the bitmap.
   Values:
 1: Dirty tracking bitmap

   Values 0, 2 - 255 are reserved.
So, it looks like we _do not need_ qcow2 format changing for now. It
already maintain enabled (auto=true) and disabled (auto=false) bitmaps
(may be, additional note in spec about mapping of this flag in Qemu
will be not bad)

And actually, we do not have anything about "load this bitmap or not"
in qcow2.  And I do not think that we need. Qemu (and user) should
decide, which bitmaps to load. (and it is obvious, that we must load
"auto" bitmaps, to not break them)

Then about qapi. What will occur, if we store
disabled-persistent-autolading bitmap? It will be enabled on next
Qemu start! And it shows that mapping "autoload"->"auto" is definitely
bad. So, as we do not want information on "load or not the bitmap" in
qcow2 (ok, I don't want, but I think Kevin and Max will agree, as it
keeps qcow2 format more generic and separated from Qemu specifics), we
see again, that "autoload" is useless, dangerous and wrong.

If we agreed, that for now auto = !BdrvDirtyBitmap.disabled and
"autoload" is deprecated, we need to decide, which disabled bitmaps we
want to load.

The simplest way to solve the problem is to load all bitmaps, mapping
BdrvDirtyBitmap.disabled = !auto. In future, if we need, we'll be able
to introduce some cmd-line flags to select disabled bitmaps for
loading or separate qmp-command to load them (and do not load them on
start).

Or we can go another way, and continue loading all disabled bitmaps,
but in "lazy mode", so bitmap is not actually loaded, only its name
and some metadata. And we can actually load it, if user enables or
exports it. It looks very interesting approach, as we do not lose RAM
on (possibly) a lot of not needed bitmaps, but we can manage them (for
example, remove).

Any way, loading all bitmaps looks like a good start.
 qemu-doc.texi|  7 +++
 qapi/block-core.json |  6 +++---
 block/qcow2.h|  2 +-
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c | 18 --
 block/qcow2-bitmap.c | 12 

Re: [Qemu-block] [PATCH v3 24/39] qcow2: Update discard_single_l2() to support L2 slices

[Alberto Garcia]

On Thu 01 Feb 2018 08:07:15 PM CET, Max Reitz  wrote:
> On 2018-01-26 15:59, Alberto Garcia wrote:
>> discard_single_l2() limits the number of clusters to be discarded to
>> the amount that fits inside an L2 table. Since we'll be loading L2
>> slices instead of full tables we need to update that limit.
>
> H, maybe rename the function to discard_l2_slice() or
> discard_in_l2_slice() or discard_all_in_l2_slice() or
> discard_single_l2_slice()?

Good idea, I think I that will change that (also the related comment in
qcow2_cluster_discard()).

Berto

Re: [Qemu-block] [PATCH v3 31/39] qcow2: Update qcow2_truncate() to support L2 slices

[Alberto Garcia]

On Thu 01 Feb 2018 08:46:46 PM CET, Max Reitz  wrote:
>> @@ -3261,8 +3261,9 @@ static int qcow2_truncate(BlockDriverState *bs, 
>> int64_t offset,
>>  guest_offset = old_length;
>>  while (nb_new_data_clusters) {
>>  int64_t guest_cluster = guest_offset >> s->cluster_bits;
>> -int64_t nb_clusters = MIN(nb_new_data_clusters,
>> -  s->l2_size - guest_cluster % 
>> s->l2_size);
>> +int64_t nb_clusters = MIN(
>> +nb_new_data_clusters,
>> +s->l2_slice_size - guest_cluster % s->l2_slice_size);
>
> An alternative would be the
> "s->l2_slice_size - offset_to_l2_slice_index(s, guest_offset)" we
> basically have elsewhere, but that's longer and doesn't really matter:

It's a bit longer but it looks better and we can get rid of
guest_cluster, so I think I'll change it.

Berto

Re: [Qemu-block] [PATCH v2] block: maintain persistent disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

02.02.2018 18:23, Max Reitz wrote:

On 2018-02-02 16:18, Vladimir Sementsov-Ogievskiy wrote:

31.01.2018 22:04, Max Reitz wrote:

On 2018-01-29 19:43, Max Reitz wrote:

On 2018-01-22 11:41, Vladimir Sementsov-Ogievskiy wrote:

To maintain load/store disabled bitmap there is new approach:

   - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
   - store enabled bitmaps as "auto" to qcow2
   - store disabled bitmaps without "auto" flag to qcow2
   - on qcow2 open load "auto" bitmaps as enabled and others
     as disabled (except in_use bitmaps)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---

Thanks, looks very reasonable.  Applied to my block branch:

https://github.com/XanClic/qemu/commits/block

...aaand I've only just now seen that iotest 176 will need to be fixed
along with this, so I'm going to unqueue this patch for now.

ohh, sorry for that. Will resend today.


And when I'm already at it: Should we add deprecation information to
qemu-doc.texi?

didn't find anything in qemu-doc.texi about dirty bitmaps, so I think, no.

I mean in the "Deprecated features" appendix.  I think whenever we
deprecate something, it should be noted there (as far as I've understood).


Ok, understand, will add. (hmm no section for QMP in this appendix, I'll 
be the first =(




(Also on the Wiki, I think.)


nothing about autoload and persistent parameters in the wiki ( 
https://wiki.qemu.org/Features/IncrementalBackup , nothing else ? ). 
Hovewer, it is good idea to update material in wiki, I'll think about it.





Is there a way to generate some documentation files from qapi comments?
Where is it?

There is docs/interop/qemu-qmp-qapi.texi in the build directory.

Max




--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v3 17/39] qcow2: Update l2_allocate() to support L2 slices

[Alberto Garcia]

On Thu 01 Feb 2018 07:22:16 PM CET, Max Reitz wrote:
> On 2018-02-01 16:43, Alberto Garcia wrote:
>> On Thu 01 Feb 2018 04:23:09 PM CET, Anton Nefedov wrote:
> However, I'm wondering whether this is the best approach.  The old
> L2 table is probably not going to be used after this function, so
> we're basically polluting the cache here.  That was bad enough so
> far, but now that actually means wasting multiple cache entries on
> it.
>
> Sure, the code is simpler this way.  But maybe it would still be
> better to manually copy the data over from the old offset...  (As
> long as it's not much more complicated.)

 You mean bypassing the cache altogether?

  qcow2_cache_flush(bs, s->l2_table_cache);
  new_table = g_malloc(s->cluster_size);
  if (old_l2_offset & L1E_OFFSET_MASK) {
  bdrv_pread(bs->file, old_l2_offset, new_table, s->cluster_size);
  } else {
  memset(new_table, 0, s->cluster_size);
  }
  bdrv_pwrite(bs->file, new_l2_offset, new_table, s->cluster_size);
  g_free(new_table);

 ??
>>>
>>> (I know it's a draft so you probably just skipped that but just in
>>> case) It seems ok to bypass the cache read - perhaps even a flush is
>>> not necessary: old_l2_offset must be read-only and flushed at this
>>> point; I believe new_l2_offset might be cached too, so it needs to be
>>> updated.
>> 
>> One problem I see with this is that while we wouldn't pollute the cache
>> we'd always be reading the table twice from disk in all cases:
>> 
>>  1) Read old table
>>  2) Write new table
>>  3) Read new table (after l2_allocate(), using the cache this time)
>> 
>> We can of course improve it by reading the old table from disk but
>> directly in the cache -so we'd spare step (3)-, but we'd still have to
>> read at least once from disk.
>> 
>> With the old code (especially if slice_size == cluster_size) we don't
>> need to read anything if the L2 table is already cached:
>> 
>>  1) Get empty table from the cache
>>  2) memcpy() the old data
>>  3) Get new table from the cache (after l2_allocate()).
>
> Well, then scratch the bdrv_pwrite() for the new table and keep using
> the cache for that (because that actually sounds useful).
>
> On second thought, though, it's rather probable the old L2 table is
> already in the cache...  Before the guest does a write to a location,
> it is reasonable to assume it has read from there before.
>
> So I guess we could think about adding a parameter to qcow2_cache_put()
> or something to reset the LRU counter because we probably won't need
> that entry anymore.  But not something for this series, of course.

That actually doesn't sound like a bad idea, there are maybe more cases
in which we know we're unlikely to need a cache entry soon, but as you
say let's take a look at it after this series.

Berto

Re: [Qemu-block] [PATCH v3 17/39] qcow2: Update l2_allocate() to support L2 slices

[Alberto Garcia]

On Thu 01 Feb 2018 07:15:23 PM CET, Max Reitz  wrote:
 -static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t 
 **table)
 +static int l2_allocate(BlockDriverState *bs, int l1_index)
  {
  BDRVQcow2State *s = bs->opaque;
  uint64_t old_l2_offset;
 -uint64_t *l2_table = NULL;
 +uint64_t *l2_slice = NULL;
 +unsigned slice, slice_size2, n_slices;
>>>
>>> I'd personally prefer size_t, but oh well.
>> 
>> I don't see any reason not to use int / unsigned. The size of a slice
>> is always <= cluster_size (an int), and both slice and n_slices are
>> smaller than that.
>
> Well, what's the reason to use unsigned? :-)
> The type of the expression used to set slice_size2 simply is size_t.

I tend to choose the type of a variable based on what it's going to
hold, and use int (signed or not) whenever possible.

In this case a normal integer can certainly hold all possible values of
slice_size2. And unsigned because it's never going to be negative. It
could also be signed, it's not going to be any different in practice,
it's just more explicit.

Berto

Re: [Qemu-block] [PATCH v3 21/39] qcow2: Update qcow2_alloc_cluster_link_l2() to support L2 slices

[Alberto Garcia]

On Thu 01 Feb 2018 07:44:56 PM CET, Max Reitz  wrote:
> On 2018-01-26 15:59, Alberto Garcia wrote:
>> There's a loop in this function that iterates over the L2 entries in a
>> table, so now we need to assert that it remains within the limits of
>> an L2 slice.
>> 
>> Apart from that, this function doesn't need any additional changes, so
>> this patch simply updates the variable name from l2_table to l2_slice.
>> 
>> Signed-off-by: Alberto Garcia 
>> Reviewed-by: Eric Blake 
>> ---
>>  block/qcow2-cluster.c | 16 
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> Hm, well, strictly speaking this patch should not be at this point in
> this series -- e.g. handle_alloc() so far only limits its nb_clusters to
> the L2 size, not the L2 slice size.

Yeah, I didn't try to be too strict with this because you can only
change the slice size after everything else is ready.

Berto

Re: [Qemu-block] [PATCH 1/2] qcow2: add overlap check for bitmap directory

[Max Reitz]

On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote:
> 29.01.2018 18:34, Max Reitz wrote:
>> On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block/qcow2.h  |  7 +--
>>>   block/qcow2-refcount.c | 12 
>>>   block/qcow2.c  |  6 ++
>>>   3 files changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 6f0ff15dd0..8f226a3609 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -98,6 +98,7 @@
>>>   #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
>>> "overlap-check.snapshot-table"
>>>   #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
>>>   #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
>>> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
>>> "overlap-check.bitmap-directory"
>>>   #define QCOW2_OPT_CACHE_SIZE "cache-size"
>>>   #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
>>>   #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>>> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap {
>>>   QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>>>   QCOW2_OL_INACTIVE_L1_BITNR    = 6,
>>>   QCOW2_OL_INACTIVE_L2_BITNR    = 7,
>>> +    QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
>>>   -    QCOW2_OL_MAX_BITNR    = 8,
>>> +    QCOW2_OL_MAX_BITNR  = 9,
>>>     QCOW2_OL_NONE   = 0,
>>>   QCOW2_OL_MAIN_HEADER    = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>>> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap {
>>>   /* NOTE: Checking overlaps with inactive L2 tables will result
>>> in bdrv
>>>    * reads. */
>>>   QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>> +    QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
>>>   } QCow2MetadataOverlap;
>>>     /* Perform all overlap checks which can be done in constant time */
>>>   #define QCOW2_OL_CONSTANT \
>>>   (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
>>> QCOW2_OL_REFCOUNT_TABLE | \
>>> - QCOW2_OL_SNAPSHOT_TABLE)
>>> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>>>     /* Perform all overlap checks which don't require disk access */
>>>   #define QCOW2_OL_CACHED \
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 3de1ab51ba..a7a2703f26 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -2585,6 +2585,18 @@ int
>>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t
>>> offset,
>>>   }
>>>   }
>>>   +    if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
>>> +    (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
>>> +    {
>>> +    /* update_ext_header_and_dir_in_place firstly drop autoclear
>>> flag,
>>> + * so it will not fail */
>> That's really not an argument.  bitmap_list_store() has to pass
>> QCOW2_OL_BITMAP_DIRECTORY to @ign anyway.  (Because there is no reason
>> not to.)
> 
> in_place is a reason. When we store directory in_place, it definitely
> overlaps with current directory.

Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is
what that argument is for? :-)

Max

> But this is done with cleared autoclear flag (to make it safe), so we
> will skip this check and will not
> fail.



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3] iotests: Fix CID for VMDK afl image

[Fam Zheng]

This reverts commit 76bf133c4 which updated the reference output, and
fixed the reference image, because the code path we want to exercise is
actually the invalid image size.

The descriptor block in the image, which includes the CID to verify, has been
invalid since the reference image was added. Since commit 9877860e7bd we report
this error earlier than the "file too large", so 059.out mismatches.

The binary change is generated along the operations of:

  $ bunzip2 afl9.vmdk.bz2
  $ qemu-img create -f vmdk fix.vmdk 1G
  $ dd if=afl9.vmdk of=fix.vmdk bs=512 count=1 conv=notrunc
  $ mv fix.vmdk afl9.vmdk
  $ bzip2 afl9.vmdk

Signed-off-by: Fam Zheng 

---

v3: Skip test when ENOMEM. [Max, Eric]

v2: Fix commit message "qcow2 -> vmdk". [Kevin]
Revert 76bf133c4.
---
 tests/qemu-iotests/059 |   5 ++---
 tests/qemu-iotests/059.out |   2 +-
 tests/qemu-iotests/sample_images/afl9.vmdk.bz2 | Bin 178 -> 618 bytes
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 40f89eae18..530bbbe6ce 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -152,9 +152,8 @@ done
 echo
 echo "=== Testing afl image with a very large capacity ==="
 _use_sample_img afl9.vmdk.bz2
-# The sed makes this test pass on machines with little RAM
-# (and also with 32 bit builds)
-_img_info | sed -e 's/Cannot allocate memory/Invalid argument/'
+_img_info | grep -q 'Cannot allocate memory' && _notrun "Insufficent memory, 
skipped test"
+_img_info
 _cleanup_test_img
 
 # success, all done
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 1ac5d56233..f6dce7947c 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2358,5 +2358,5 @@ Offset  Length  Mapped to   File
 0x14000 0x1 0x5 TEST_DIR/t-s003.vmdk
 
 === Testing afl image with a very large capacity ===
-qemu-img: Could not open 'TEST_DIR/afl9.IMGFMT': Could not open 
'TEST_DIR/afl9.IMGFMT': Invalid argument
+qemu-img: Can't get image size 'TEST_DIR/afl9.IMGFMT': File too large
 *** done
diff --git a/tests/qemu-iotests/sample_images/afl9.vmdk.bz2 
b/tests/qemu-iotests/sample_images/afl9.vmdk.bz2
index 
03615d36a12425cf4240bab86f4cfe648db14572..9fcd0af45a815431acf4689e0845ecf2d333cd58
 100644
GIT binary patch
literal 618
zcmV-w0+szjT4*^jL0KkKSvgW7ssIN3|NsBH-Q9UpfAhclU70`s-*NE~5QvC~h=_=Y
zh>D2n*q*=vygR634445h35k;?00h9835kMW4$iPepVE{Bqk)uhJ^wfGLr=)3s
zhM5CR88jLh7)B;cA*K)*6GmuECPU3o4NWG5O#pg>Ak#xY8Z^CrMt}oD38Ns$
z02n}M0LdjZ&}cLPqd+nPKmn$j0iXe(02%-d27nnJriN-uE+X@Bj4BBfd|yV!NB
zwqkL}nW3AI5x^jp=t%^F1pxqp)v#n#)j$zcm1xqv(!$2d*5%vF{5RPWnOV8-^tE<(
zU~%&}Y0uNu*9Wt=yS^8PkC%IG;aD{l#sG`m4Ho*fsHXdM

Re: [Qemu-block] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Eric Blake]

On 02/02/2018 08:06 AM, Edgar Kaziakhmedov wrote:

 However, it would be nice to remove can_write_zeroes_with_unmap from
 BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
 !!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
 think?
>> Actually, I may even just give a shot at writing this alternative patch,
>> to make Kevin's decision easier.
> But actually qcow2 performs some checks for version inside get_info
> callback before setting can_write_zeroes_with_unmap flag,
> so we can't take into account such checks in
> bdrv_can_write_zeroes_with_unmap subroutine. Therefore, I don't think it
> is possible to do it like that.

Here's the patch I proposed (it looks like I forgot to CC you):

https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06471.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Edgar Kaziakhmedov]

On 02/02/2018 05:15 PM, Eric Blake wrote:

On 02/02/2018 08:06 AM, Edgar Kaziakhmedov wrote:


However, it would be nice to remove can_write_zeroes_with_unmap from
BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
!!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
think?

Actually, I may even just give a shot at writing this alternative patch,
to make Kevin's decision easier.

But actually qcow2 performs some checks for version inside get_info
callback before setting can_write_zeroes_with_unmap flag,
so we can't take into account such checks in
bdrv_can_write_zeroes_with_unmap subroutine. Therefore, I don't think it
is possible to do it like that.

Here's the patch I proposed (it looks like I forgot to CC you):

https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06471.html


Yes, it was possible to move check to open, ok, get it.

Re: [Qemu-block] [PATCH 1/2] qcow2: add overlap check for bitmap directory

[Vladimir Sementsov-Ogievskiy]

02.02.2018 16:00, Max Reitz wrote:

On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote:

29.01.2018 18:34, Max Reitz wrote:

On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   block/qcow2.h  |  7 +--
   block/qcow2-refcount.c | 12 
   block/qcow2.c  |  6 ++
   3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 6f0ff15dd0..8f226a3609 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -98,6 +98,7 @@
   #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
"overlap-check.snapshot-table"
   #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
   #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
+#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
"overlap-check.bitmap-directory"
   #define QCOW2_OPT_CACHE_SIZE "cache-size"
   #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
   #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
@@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap {
   QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
   QCOW2_OL_INACTIVE_L1_BITNR    = 6,
   QCOW2_OL_INACTIVE_L2_BITNR    = 7,
+    QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
   -    QCOW2_OL_MAX_BITNR    = 8,
+    QCOW2_OL_MAX_BITNR  = 9,
     QCOW2_OL_NONE   = 0,
   QCOW2_OL_MAIN_HEADER    = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
@@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap {
   /* NOTE: Checking overlaps with inactive L2 tables will result
in bdrv
    * reads. */
   QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+    QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
   } QCow2MetadataOverlap;
     /* Perform all overlap checks which can be done in constant time */
   #define QCOW2_OL_CONSTANT \
   (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
QCOW2_OL_REFCOUNT_TABLE | \
- QCOW2_OL_SNAPSHOT_TABLE)
+ QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
     /* Perform all overlap checks which don't require disk access */
   #define QCOW2_OL_CACHED \
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3de1ab51ba..a7a2703f26 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2585,6 +2585,18 @@ int
qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t
offset,
   }
   }
   +    if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
+    (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
+    {
+    /* update_ext_header_and_dir_in_place firstly drop autoclear
flag,
+ * so it will not fail */

That's really not an argument.  bitmap_list_store() has to pass
QCOW2_OL_BITMAP_DIRECTORY to @ign anyway.  (Because there is no reason
not to.)

in_place is a reason. When we store directory in_place, it definitely
overlaps with current directory.

Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is
what that argument is for? :-)


hmm. but actually, I should not, because of zeroed autoclear flag. So,
do you think, it is better to pass it, anyway?



Max


But this is done with cleared autoclear flag (to make it safe), so we
will skip this check and will not
fail.



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v3] iotests: Fix CID for VMDK afl image

[Max Reitz]

On 2018-02-02 06:23, Fam Zheng wrote:
> This reverts commit 76bf133c4 which updated the reference output, and
> fixed the reference image, because the code path we want to exercise is
> actually the invalid image size.
> 
> The descriptor block in the image, which includes the CID to verify, has been
> invalid since the reference image was added. Since commit 9877860e7bd we 
> report
> this error earlier than the "file too large", so 059.out mismatches.
> 
> The binary change is generated along the operations of:
> 
>   $ bunzip2 afl9.vmdk.bz2
>   $ qemu-img create -f vmdk fix.vmdk 1G
>   $ dd if=afl9.vmdk of=fix.vmdk bs=512 count=1 conv=notrunc
>   $ mv fix.vmdk afl9.vmdk
>   $ bzip2 afl9.vmdk
> 
> Signed-off-by: Fam Zheng 
> 
> ---
> 
> v3: Skip test when ENOMEM. [Max, Eric]
> 
> v2: Fix commit message "qcow2 -> vmdk". [Kevin]
> Revert 76bf133c4.
> ---
>  tests/qemu-iotests/059 |   5 ++---
>  tests/qemu-iotests/059.out |   2 +-
>  tests/qemu-iotests/sample_images/afl9.vmdk.bz2 | Bin 178 -> 618 bytes
>  3 files changed, 3 insertions(+), 4 deletions(-)

Nice, thanks.

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[Vladimir Sementsov-Ogievskiy]

22.01.2018 20:23, John Snow wrote:


On 01/22/2018 07:22 AM, Vladimir Sementsov-Ogievskiy wrote:

22.01.2018 12:20, Vladimir Sementsov-Ogievskiy wrote:

20.01.2018 02:30, John Snow wrote:

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:

Hi all.

There are three qmp commands, needed to implement external backup API.

Using these three commands, client may do all needed bitmap
management by
hand:

on backup start we need to do a transaction:
   {disable old bitmap, create new bitmap}

on backup success:
   drop old bitmap

on backup fail:
   enable old bitmap
   merge new bitmap to old bitmap
   drop new bitmap

v2: fix merge command deadlock
    add new patches: 1 and 6

Vladimir Sementsov-Ogievskiy (6):
    block: maintain persistent disabled bitmaps
    block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
    qapi: add block-dirty-bitmap-enable/disable
    qmp: transaction support for block-dirty-bitmap-enable/disable
    qapi: add block-dirty-bitmap-merge
    qapi: add disabled parameter to block-dirty-bitmap-add

   qapi/block-core.json |  92 ++-
   qapi/transaction.json    |   4 +
   block/qcow2.h    |   2 +-
   include/block/dirty-bitmap.h |   3 +-
   block/dirty-bitmap.c |  42 ++-
   block/qcow2-bitmap.c |  12 +--
   block/qcow2.c    |   2 +-
   blockdev.c   | 169
+--
   8 files changed, 287 insertions(+), 39 deletions(-)


Fails to apply to master (b384cd95) on patch four and five. Only
contextual problems, I've patched it up and I'll review that.

(mirrored here if you want to check my rebase work:
https://github.com/jnsnow/qemu/tree/vlad-review)

Since I was full of such bad and stupid ideas last time, I'd like
someone else to look over this one for design and I'll just review it
for accuracy.

--js

Thank you for review, John!

Ok, so, I'll going to:

- take patch 1 into migration and respin it today (I hope) with test
about qcow2-based bitmap migration disabled.
- separate fixes and refactoring from here (locking + _bitmap_clear
transaction), send them separately
- than, make test for external backup and respin these series with it


changed to:

1. send patch 1/6 separately with the whole reasoning[done], as it
blocks two series, wait for accepting
2. respin postcopy series
3. finish up discussion on bitmap locking under "[PATCH v9 03/13]
block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap"
4. separate fixes and refactoring from here (locking + _bitmap_clear
transaction), send them separately
5. make test for external backup and respin these series with it

2 depends on 1
4 depends on 3
5 depends on 1 and 4


Great, thanks!


Sorry for long delay, I was ill. Now I'm returning to these plans.

--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 1/2] qcow2: add overlap check for bitmap directory

[Vladimir Sementsov-Ogievskiy]

29.01.2018 18:34, Max Reitz wrote:

On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/qcow2.h  |  7 +--
  block/qcow2-refcount.c | 12 
  block/qcow2.c  |  6 ++
  3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 6f0ff15dd0..8f226a3609 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -98,6 +98,7 @@
  #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
  #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
  #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
+#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
  #define QCOW2_OPT_CACHE_SIZE "cache-size"
  #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
  #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
@@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap {
  QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
  QCOW2_OL_INACTIVE_L1_BITNR= 6,
  QCOW2_OL_INACTIVE_L2_BITNR= 7,
+QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
  
-QCOW2_OL_MAX_BITNR= 8,

+QCOW2_OL_MAX_BITNR  = 9,
  
  QCOW2_OL_NONE   = 0,

  QCOW2_OL_MAIN_HEADER= (1 << QCOW2_OL_MAIN_HEADER_BITNR),
@@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap {
  /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
   * reads. */
  QCOW2_OL_INACTIVE_L2= (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
  } QCow2MetadataOverlap;
  
  /* Perform all overlap checks which can be done in constant time */

  #define QCOW2_OL_CONSTANT \
  (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
- QCOW2_OL_SNAPSHOT_TABLE)
+ QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
  
  /* Perform all overlap checks which don't require disk access */

  #define QCOW2_OL_CACHED \
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3de1ab51ba..a7a2703f26 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2585,6 +2585,18 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, 
int ign, int64_t offset,
  }
  }
  
+if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&

+(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
+{
+/* update_ext_header_and_dir_in_place firstly drop autoclear flag,
+ * so it will not fail */

That's really not an argument.  bitmap_list_store() has to pass
QCOW2_OL_BITMAP_DIRECTORY to @ign anyway.  (Because there is no reason
not to.)


in_place is a reason. When we store directory in_place, it definitely 
overlaps with current directory.
But this is done with cleared autoclear flag (to make it safe), so we 
will skip this check and will not

fail.



Max


+if (overlaps_with(s->bitmap_directory_offset,
+  s->bitmap_directory_size))
+{
+return QCOW2_OL_BITMAP_DIRECTORY;
+}
+}
+
  return 0;
  }
  
diff --git a/block/qcow2.c b/block/qcow2.c

index 1914a940e5..8278c0e124 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = {
  .help = "Check for unintended writes into an inactive L2 table",
  },
  {
+.name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
+.type = QEMU_OPT_BOOL,
+.help = "Check for unintended writes into the bitmap directory",
+},
+{
  .name = QCOW2_OPT_CACHE_SIZE,
  .type = QEMU_OPT_SIZE,
  .help = "Maximum combined metadata (L2 tables and refcount blocks) 
"
@@ -690,6 +695,7 @@ static const char 
*overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
  [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
  [QCOW2_OL_INACTIVE_L1_BITNR]= QCOW2_OPT_OVERLAP_INACTIVE_L1,
  [QCOW2_OL_INACTIVE_L2_BITNR]= QCOW2_OPT_OVERLAP_INACTIVE_L2,
+[QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
  };
  
  static void cache_clean_timer_cb(void *opaque)







--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Edgar Kaziakhmedov]

On 01/26/2018 05:28 PM, Eric Blake wrote:

On 01/26/2018 06:39 AM, Edgar Kaziakhmedov wrote:

PIng

So, let me know if I need to make any changes in patch

On 1/18/18 1:09 PM, Paolo Bonzini wrote:

On 18/01/2018 12:51, Edgar Kaziakhmedov wrote:

+static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    if (bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP) {
+    bdi->can_write_zeroes_with_unmap = true;
+    }
+    return 0;
+}
+

Other drivers set the flag always, while NBD only sets it if the server
knows the flag.

Well, other drivers may be able to always implement it (NBD can only
implement it if the server supports WRITE_ZEROES - and I'm even in the
middle of working up an nbdkit patch [1] that makes it easier to write
an NBD server that specifically does not support WRITE_ZEROES to make
code paths like this easier to test)

[1]


I think NBD is more correct, so:

Reviewed-by: Paolo Bonzini 

Agreed; I'm fine queueing this on my NBD queue, except I'd first like to
hear Kevin's opinion:


However, it would be nice to remove can_write_zeroes_with_unmap from
BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
!!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
think?

Actually, I may even just give a shot at writing this alternative patch,
to make Kevin's decision easier.
But actually qcow2 performs some checks for version inside get_info 
callback before setting can_write_zeroes_with_unmap flag,
so we can't take into account such checks in 
bdrv_can_write_zeroes_with_unmap subroutine. Therefore, I don't think it 
is possible to do it like that.