qemu-commits
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-commits] [qemu/qemu] 680503: qapi-block-core: Clean up blockdev-sn


From: Richard Henderson
Subject: [Qemu-commits] [qemu/qemu] 680503: qapi-block-core: Clean up blockdev-snapshot-intern...
Date: Tue, 06 Aug 2024 02:48:40 -0700

  Branch: refs/heads/staging
  Home:   https://github.com/qemu/qemu
  Commit: 68050300b256bd7639876afe9516d7c553645f58
      
https://github.com/qemu/qemu/commit/68050300b256bd7639876afe9516d7c553645f58
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M qapi/block-core.json

  Log Message:
  -----------
  qapi-block-core: Clean up blockdev-snapshot-internal-sync doc

BlockdevSnapshotInternal is the arguments type of command
blockdev-snapshot-internal-sync.  Its doc comment contains this note:

    # .. note:: In a transaction, if @name is empty or any snapshot matching
    #    @name exists, the operation will fail.  Only some image formats
    #    support it; for example, qcow2, and rbd.

"In a transaction" is misleading, and "if @name is empty or any
snapshot matching @name exists, the operation will fail" is redundant
with the command's Errors documentation.  Drop.

The remainder is fine.  Move it to the command's doc comment, where it
is more prominently visible, with a slight rephrasing for clarity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240718123609.3063055-1-armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 43fbd3757f2b447bc36b5537d4f77b7d8d91825b
      
https://github.com/qemu/qemu/commit/43fbd3757f2b447bc36b5537d4f77b7d8d91825b
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/block-copy.c

  Log Message:
  -----------
  block-copy: Fix missing graph lock

The graph lock needs to be held when calling bdrv_co_pdiscard(). Fix
block_copy_task_entry() to take it for the call.

WITH_GRAPH_RDLOCK_GUARD() was implemented in a weak way because of
limitations in clang's Thread Safety Analysis at the time, so that it
only asserts that the lock is held (which allows calling functions that
require the lock), but we never deal with the unlocking (so even after
the scope of the guard, the compiler assumes that the lock is still
held). This is why the compiler didn't catch this locking error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240627181245.281403-2-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: fefb705bbd44fb7b0d954ecdc4e74b4c58941434
      
https://github.com/qemu/qemu/commit/fefb705bbd44fb7b0d954ecdc4e74b4c58941434
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M include/block/graph-lock.h
    M meson.build

  Log Message:
  -----------
  block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked

Upstream clang 18 (and backports to clang 17 in Fedora and RHEL)
implemented support for __attribute__((cleanup())) in its Thread Safety
Analysis, so we can now actually have a proper implementation of
WITH_GRAPH_RDLOCK_GUARD() that understands when we acquire and when we
release the lock.

-Wthread-safety is now only enabled if the compiler is new enough to
understand this pattern. In theory, we could have used some #ifdefs to
keep the existing basic checks on old compilers, but as long as someone
runs a newer compiler (and our CI does), we will catch locking problems,
so it's probably not worth keeping multiple implementations for this.

The implementation can't use g_autoptr any more because the glib macros
define wrapper functions that don't have the right TSA attributes, so
the compiler would complain about them. Just use the cleanup attribute
directly instead.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240627181245.281403-3-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: bd1b989be7a009aabbf5a2001784eb4796a1eaee
      
https://github.com/qemu/qemu/commit/bd1b989be7a009aabbf5a2001784eb4796a1eaee
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-disk: Use positive return value for status in dma_readv/writev

In some error cases, scsi_block_sgio_complete() never calls the passed
callback, but directly completes the request. This leads to bugs because
its error paths are not exact copies of what the callback would normally
do.

In preparation to fix this, allow passing positive return values to the
callbacks that represent the status code that should be used to complete
the request.

scsi_handle_rw_error() already handles positive values for its ret
parameter because scsi_block_sgio_complete() calls directly into it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-2-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 796b6e7e664cd0ee9777480a62cac6f3afb53140
      
https://github.com/qemu/qemu/commit/796b6e7e664cd0ee9777480a62cac6f3afb53140
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-block: Don't skip callback for sgio error status/driver_status

Instead of calling into scsi_handle_rw_error() directly from
scsi_block_sgio_complete() and skipping the normal callback, go through
the normal cleanup path by calling the callback with a positive error
value.

The important difference here is not only that the code path is cleaner,
but that the callbacks set r->req.aiocb = NULL. If we skip setting this
and the error action is BLOCK_ERROR_ACTION_STOP, resuming the VM runs
into an assertion failure in scsi_read_data() or scsi_write_data()
because the dangling aiocb pointer is unexpected.

Fixes: a108557bbf ("scsi: inline sg_io_sense_from_errno() into the callers.")
Buglink: https://issues.redhat.com/browse/RHEL-50000
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-3-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: ab44a129ebd2b07c312378736dc44c9b0f6bafd3
      
https://github.com/qemu/qemu/commit/ab44a129ebd2b07c312378736dc44c9b0f6bafd3
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-disk: Add warning comments that host_status errors take a shortcut

scsi_block_sgio_complete() has surprising behaviour in that there are
error cases in which it directly completes the request and never calls
the passed callback. In the current state of the code, this doesn't seem
to result in bugs, but with future code changes, we must be careful to
never rely on the callback doing some cleanup until this code smell is
fixed. For now, just add warnings to make people aware of the trap.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-4-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 789990ac12e0438842bf3429735ebe58921b71d9
      
https://github.com/qemu/qemu/commit/789990ac12e0438842bf3429735ebe58921b71d9
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-disk: Always report RESERVATION_CONFLICT to guest

In the case of scsi-block, RESERVATION_CONFLICT is not a backend error,
but indicates that the guest tried to make a request that it isn't
allowed to execute. Pass the error to the guest so that it can decide
what to do with it.

Without this, if we stop the VM in response to a RESERVATION_CONFLICT
(as is the default policy in management software such as oVirt or
KubeVirt), it can happen that the VM cannot be resumed any more because
every attempt to resume it immediately runs into the same error and
stops the VM again.

One case that expects RESERVATION_CONFLICT errors to be visible in the
guest is running the validation tests in Windows 2019's Failover Cluster
Manager, which intentionally tries to execute invalid requests to see if
they are properly rejected.

Buglink: https://issues.redhat.com/browse/RHEL-50000
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-5-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 3735ee3e399ed7badc477ce7ddcca4b893696c40
      
https://github.com/qemu/qemu/commit/3735ee3e399ed7badc477ce7ddcca4b893696c40
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix bug in writing to middle of file

Before this commit, the behavior when calling `commit_one_file` for
example with `offset=0x2000` (second cluster), what will happen is that
we won't fetch the next cluster from the fat, and instead use the first
cluster for the read operation.

This is due to off-by-one error here, where `i=0x2000 !< offset=0x2000`,
thus not fetching the next cluster.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<b97c1e1f1bc2f776061ae914f95d799d124fcd73.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: d0acb810d1f240328ee78f12e68cc4d7cc234718
      
https://github.com/qemu/qemu/commit/d0acb810d1f240328ee78f12e68cc4d7cc234718
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix usage of `info.file.offset`

The field is marked as "the offset in the file (in clusters)", but it
was being used like this
`cluster_size*(nums)+mapping->info.file.offset`, which is incorrect.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<72f19a7903886dda1aa78bcae0e17702ee939262.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 5c51a494f34f0b8eaf9db97897a202f6931d43b9
      
https://github.com/qemu/qemu/commit/5c51a494f34f0b8eaf9db97897a202f6931d43b9
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix wrong checks for cluster mappings invariant

How this `abort` was intended to check for was:
- if the `mapping->first_mapping_index` is not the same as
  `first_mapping_index`, which **should** happen only in one case,
  when we are handling the first mapping, in that case
  `mapping->first_mapping_index == -1`, in all other cases, the other
  mappings after the first should have the condition `true`.
- From above, we know that this is the first mapping, so if the offset
  is not `0`, then abort, since this is an invalid state.

The issue was that `first_mapping_index` is not set if we are
checking from the middle, the variable `first_mapping_index` is
only set if we passed through the check `cluster_was_modified` with the
first mapping, and in the same function call we checked the other
mappings.

One approach is to go into the loop even if `cluster_was_modified`
is not true so that we will be able to set `first_mapping_index` for the
first mapping, but since `first_mapping_index` is only used here,
another approach is to just check manually for the
`mapping->first_mapping_index != -1` since we know that this is the
value for the only entry where `offset == 0` (i.e. first mapping).

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<b0fbca3ee208c565885838f6a7deeaeb23f4f9c2.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: bd887223bcc6b5d7817c7e335534efea3c4f7ce0
      
https://github.com/qemu/qemu/commit/bd887223bcc6b5d7817c7e335534efea3c4f7ce0
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix reading files with non-continuous clusters

When reading with `read_cluster` we get the `mapping` with
`find_mapping_for_cluster` and then we call `open_file` for this
mapping.
The issue appear when its the same file, but a second cluster that is
not immediately after it, imagine clusters `500 -> 503`, this will give
us 2 mappings one has the range `500..501` and another `503..504`, both
point to the same file, but different offsets.

When we don't open the file since the path is the same, we won't assign
`s->current_mapping` and thus accessing way out of bound of the file.

>From our example above, after `open_file` (that didn't open anything) we
will get the offset into the file with
`s->cluster_size*(cluster_num-s->current_mapping->begin)`, which will
give us `0x2000 * (504-500)`, which is out of bound for this mapping and
will produce some issues.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Message-ID: 
<1f3ea115779abab62ba32c788073cdc99f9ad5dd.1721470238.git.amjadsharafi10@gmail.com>
[kwolf: Simplified the patch based on Amjad's analysis and input]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 045678cecdd59f7b2a6e5edd775c5013ade2086a
      
https://github.com/qemu/qemu/commit/045678cecdd59f7b2a6e5edd775c5013ade2086a
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M tests/qemu-iotests/check
    A tests/qemu-iotests/fat16.py
    M tests/qemu-iotests/testenv.py
    A tests/qemu-iotests/tests/vvfat
    A tests/qemu-iotests/tests/vvfat.out

  Log Message:
  -----------
  iotests: Add `vvfat` tests

Added several tests to verify the implementation of the vvfat driver.

We needed a way to interact with it, so created a basic `fat16.py` driver
that handled writing correct sectors for us.

Added `vvfat` to the non-generic formats, as its not a normal image format.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<bb8149c945301aefbdf470a0924c07f69f9c087d.1721470238.git.amjadsharafi10@gmail.com>
[kwolf: Made mypy and pylint happy to unbreak 297]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 91f16b6cdccd2942ae09d155cf3f79e6bbb485d5
      
https://github.com/qemu/qemu/commit/91f16b6cdccd2942ae09d155cf3f79e6bbb485d5
  Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M tests/qemu-iotests/024
    M tests/qemu-iotests/024.out

  Log Message:
  -----------
  iotests/024: exclude 'backing file format' field from the output

Apparently 'qemu-img info' doesn't report the backing file format field
for qed (as it does for qcow2):

$ qemu-img create -f qed base.qed 1M && qemu-img create -f qed -b base.qed -F 
qed top.qed 1M
$ qemu-img create -f qcow2 base.qcow2 1M && qemu-img create -f qcow2 -b 
base.qcow2 -F qcow2 top.qcow2 1M
$ qemu-img info top.qed | grep 'backing file format'
$ qemu-img info top.qcow2 | grep 'backing file format'
backing file format: qcow2

This leads to the 024 test failure with -qed.  Let's just filter the
field out and exclude it from the output.

This is a fixup for the commit f93e65ee51 ("iotests/{024, 271}: add
testcases for qemu-img rebase").

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Message-ID: <20240730094701.790624-1-andrey.drobyshev@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: bf188fa666036ea1c22364ed89a8a6a43c3b97f1
      
https://github.com/qemu/qemu/commit/bf188fa666036ea1c22364ed89a8a6a43c3b97f1
  Author: Richard Henderson <richard.henderson@linaro.org>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/block-copy.c
    M block/vvfat.c
    M hw/scsi/scsi-disk.c
    M include/block/graph-lock.h
    M meson.build
    M qapi/block-core.json
    M tests/qemu-iotests/024
    M tests/qemu-iotests/024.out
    M tests/qemu-iotests/check
    A tests/qemu-iotests/fat16.py
    M tests/qemu-iotests/testenv.py
    A tests/qemu-iotests/tests/vvfat
    A tests/qemu-iotests/tests/vvfat.out

  Log Message:
  -----------
  Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging

Block layer patches

- scsi-block: Fix error handling with r/werror=stop
- Depend on newer clang for TSA, make WITH_GRAPH_RDLOCK_GUARD() fully
  checked, fix block-copy to add missing lock
- vvfat: Fix write bugs for large files and add iotests
- Clean up blockdev-snapshot-internal-sync doc
- Fix iotests 024 for qed

# -----BEGIN PGP SIGNATURE-----
#
# iQJFBAABCAAvFiEE3D3rFZqa+V09dFb+fwmycsiPL9YFAmax2CQRHGt3b2xmQHJl
# ZGhhdC5jb20ACgkQfwmycsiPL9aGBg/+NoMVYQOnU9UmKXYaiEnBwUdQzeNnHS+i
# XKxT+OXGvRJEl2tUvl0dtr13gJTMMQ/f5MCwvm920cN2DXtXJ96rWoXDoptFwbpk
# sKkSIUG//3rkpMD6/Yry6hoORPGUXLfB5+ObY9beuFriuh6EPvmKqk5ga/zBF0wb
# lyVfIo6W769PPFIcmIbRJxDed7lcbKMPWKjXPSD4mxjZHGtsKgwMO7B6aut2hC+n
# L9a9cT968s4i6NmLDxI412X/hUrdnxBDWMTN4BSItzSbNiS/PHWxqPlCNwnGsL1V
# MpLOnY57atWOOJneRaF9z6+3ah281/Xa6vHvMho4aCB4KveUyKGrQPow/G+GuOgO
# 08dfblqsdDmzypJ9mBq+0PAY1OtkJZzrOEzJYbQRcmYiSUMqRtGVerLrXNHllh5z
# ZHSSK4O85gf6K+HfCSEwFfqzwjlf3vwn9eITdblQ1oGtdcyzc0CPVKu9hOLNujuh
# C5ErmWYjSO24dRrtwRxzN1CXVuYZ/0J4M1P7moP2v5HTZhu2uyW1mCJqNjT1kLWb
# EWEz5Ot5RVNty1W7QMakKIPkDRChBRsYVHOkAfT2zeLUj08PykjTOtGX0k7b3KrS
# R5xlVBP2leskwlNWimKshK6Bt20PkLcluH9YYUDwPygrUx9Tx8REt2ACtFmxsAUg
# vbibvnWI+oo=
# =gHWX
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue 06 Aug 2024 06:00:36 PM AEST
# gpg:                using RSA key DC3DEB159A9AF95D3D7456FE7F09B272C88F2FD6
# gpg:                issuer "kwolf@redhat.com"
# gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>" [full]

* tag 'for-upstream' of https://repo.or.cz/qemu/kevin:
  iotests/024: exclude 'backing file format' field from the output
  iotests: Add `vvfat` tests
  vvfat: Fix reading files with non-continuous clusters
  vvfat: Fix wrong checks for cluster mappings invariant
  vvfat: Fix usage of `info.file.offset`
  vvfat: Fix bug in writing to middle of file
  scsi-disk: Always report RESERVATION_CONFLICT to guest
  scsi-disk: Add warning comments that host_status errors take a shortcut
  scsi-block: Don't skip callback for sgio error status/driver_status
  scsi-disk: Use positive return value for status in dma_readv/writev
  block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked
  block-copy: Fix missing graph lock
  qapi-block-core: Clean up blockdev-snapshot-internal-sync doc

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


Compare: https://github.com/qemu/qemu/compare/c659b7b3b492...bf188fa66603

To unsubscribe from these emails, change your notification settings at 
https://github.com/qemu/qemu/settings/notifications



reply via email to

[Prev in Thread] Current Thread [Next in Thread]