Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`
On Mon, Jun 10, 2024 at 06:49:43PM +0200, Kevin Wolf wrote:
> Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben:
> > 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.
> >
> > Additionally, removed the `abort` when `first_mapping_index` does not
> > match, as this matches the case when adding new clusters for files, and
> > its inevitable that we reach this condition when doing that if the
> > clusters are not after one another, so there is no reason to `abort`
> > here, execution continues and the new clusters are written to disk
> > correctly.
> >
> > Signed-off-by: Amjad Alsharafi
>
> Can you help me understand how first_mapping_index really works?
>
> It seems to me that you get a chain of mappings for each file on the FAT
> filesystem, which are just the contiguous areas in it, and
> first_mapping_index refers to the mapping at the start of the file. But
> for much of the time, it actually doesn't seem to be set at all, so you
> have mapping->first_mapping_index == -1. Do you understand the rules
> around when it's set and when it isn't?
Yeah. So `first_mapping_index` is the index of the first mapping, each
mapping is a group of clusters that are contiguous in the file.
Its mostly `-1` because the first mapping will have the value set as
`-1` and not its own index, this value will only be set when the file
contain more than one mapping, and this will only happen when you add
clusters to a file that are not contiguous with the existing clusters.
And actually, thanks to that I noticed another bug not fixed in PATCH 3,
We are doing this check
`s->current_mapping->first_mapping_index != mapping->first_mapping_index`
to know if we should switch to the new mapping or not.
If we were reading from the first mapping (`first_mapping_index == -1`)
and we jumped to the second mapping (`first_mapping_index == n`), we
will catch this condition and switch to the new mapping.
But if the file has more than 2 mappings, and we jumped to the 3rd
mapping, we will not catch this since (`first_mapping_index == n`) for
both of them haha. I think a better check is to check the `mapping`
pointer directly. (I'll add it also in the next series together with a
test for it.)
>
> > block/vvfat.c | 12 +++-
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 19da009a5b..f0642ac3e4 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -1408,7 +1408,9 @@ read_cluster_directory:
> >
> > assert(s->current_fd);
> >
> > -
> > offset=s->cluster_size*(cluster_num-s->current_mapping->begin)+s->current_mapping->info.file.offset;
> > +offset = s->cluster_size *
> > +((cluster_num - s->current_mapping->begin)
> > ++ s->current_mapping->info.file.offset);
> > if(lseek(s->current_fd, offset, SEEK_SET)!=offset)
> > return -3;
> > s->cluster=s->cluster_buffer;
> > @@ -1929,8 +1931,9 @@ get_cluster_count_for_direntry(BDRVVVFATState* s,
> > direntry_t* direntry, const ch
> > (mapping->mode & MODE_DIRECTORY) == 0) {
> >
> > /* was modified in qcow */
> > -if (offset != mapping->info.file.offset +
> > s->cluster_size
> > -* (cluster_num - mapping->begin)) {
> > +if (offset != s->cluster_size
> > +* ((cluster_num - mapping->begin)
> > ++ mapping->info.file.offset)) {
> > /* offset of this cluster in file chain has
> > changed */
> > abort();
> > copy_it = 1;
> > @@ -1944,7 +1947,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s,
> > direntry_t* direntry, const ch
> >
> > if (mapping->first_mapping_index != first_mapping_index
> > && mapping->info.file.offset > 0) {
> > -abort();
> > copy_it = 1;
> > }
>
> I'm unsure which case this represents. If first_mapping_index refers to
> the mapping of the first cluster in the file, does this mean we got a
> mapping for a different file here? Or is the comparison between -1 and a
> real value?
Now that I think more about it, I think this `abort` is actually
correct, the issue though is that the handling around this code is not.
What this `abort` actually does is that it checks.
- 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
Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`
Am 11.06.2024 um 14:31 hat Amjad Alsharafi geschrieben:
> On Mon, Jun 10, 2024 at 06:49:43PM +0200, Kevin Wolf wrote:
> > Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben:
> > > 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.
> > >
> > > Additionally, removed the `abort` when `first_mapping_index` does not
> > > match, as this matches the case when adding new clusters for files, and
> > > its inevitable that we reach this condition when doing that if the
> > > clusters are not after one another, so there is no reason to `abort`
> > > here, execution continues and the new clusters are written to disk
> > > correctly.
> > >
> > > Signed-off-by: Amjad Alsharafi
> >
> > Can you help me understand how first_mapping_index really works?
> >
> > It seems to me that you get a chain of mappings for each file on the FAT
> > filesystem, which are just the contiguous areas in it, and
> > first_mapping_index refers to the mapping at the start of the file. But
> > for much of the time, it actually doesn't seem to be set at all, so you
> > have mapping->first_mapping_index == -1. Do you understand the rules
> > around when it's set and when it isn't?
>
> Yeah. So `first_mapping_index` is the index of the first mapping, each
> mapping is a group of clusters that are contiguous in the file.
> Its mostly `-1` because the first mapping will have the value set as
> `-1` and not its own index, this value will only be set when the file
> contain more than one mapping, and this will only happen when you add
> clusters to a file that are not contiguous with the existing clusters.
Ah, that makes some sense. Not sure if it's optimal, but it's a rule I
can work with. So just to confirm, this is the invariant that we think
should always hold true, right?
assert((mapping->mode & MODE_DIRECTORY) ||
!mapping->info.file.offset ||
mapping->first_mapping_index > 0);
> And actually, thanks to that I noticed another bug not fixed in PATCH 3,
> We are doing this check
> `s->current_mapping->first_mapping_index != mapping->first_mapping_index`
> to know if we should switch to the new mapping or not.
> If we were reading from the first mapping (`first_mapping_index == -1`)
> and we jumped to the second mapping (`first_mapping_index == n`), we
> will catch this condition and switch to the new mapping.
>
> But if the file has more than 2 mappings, and we jumped to the 3rd
> mapping, we will not catch this since (`first_mapping_index == n`) for
> both of them haha. I think a better check is to check the `mapping`
> pointer directly. (I'll add it also in the next series together with a
> test for it.)
This comparison is exactly what confused me. I didn't realise that the
first mapping in the chain has a different value here, so I thought this
must mean that we're looking at a different file now - but of course I
couldn't see a reason for that because we're iterating through a single
file in this function.
But even now that I know that the condition triggers when switching from
the first to the second mapping, it doesn't make sense to me. We don't
have to copy things around just because a file is non-contiguous.
What we want to catch is if the order of mappings has changed compared
to the old state. Do we need a linked list, maybe a prev_mapping_index,
instead of first_mapping_index so that we can compare if it is still the
same as before?
Or actually, I suppose that's the first block with an abort() in the
code, just that it doesn't compare mappings, but their offsets.
> >
> > > block/vvfat.c | 12 +++-
> > > 1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/block/vvfat.c b/block/vvfat.c
> > > index 19da009a5b..f0642ac3e4 100644
> > > --- a/block/vvfat.c
> > > +++ b/block/vvfat.c
> > > @@ -1408,7 +1408,9 @@ read_cluster_directory:
> > >
> > > assert(s->current_fd);
> > >
> > > -
> > > offset=s->cluster_size*(cluster_num-s->current_mapping->begin)+s->current_mapping->info.file.offset;
> > > +offset = s->cluster_size *
> > > +((cluster_num - s->current_mapping->begin)
> > > ++ s->current_mapping->info.file.offset);
> > > if(lseek(s->current_fd, offset, SEEK_SET)!=offset)
> > > return -3;
> > > s->cluster=s->cluster_buffer;
> > > @@ -1929,8 +1931,9 @@ get_cluster_count_for_direntry(BDRVVVFATState* s,
> > > direntry_t* direntry, const ch
> > > (mapping->mode & MODE_DIRECTORY) == 0) {
> > >
> > > /* was modified in qcow */
> > > -if (offset != mapping->info.file.offset +
> > > s->cluster_size
> > > -* (cluster_num - mapping->begin)) {
> > > +if (offset != s->cluster_size
> > > +* ((cluster_num - mapping->begin)
> > > +
Re: Re: [PATCH v5 00/10] Support persistent reservation operations
On Mon, Jun 10, 2024 at 07:55:20PM -0700, 卢长奇 wrote: > Hi, > > Sorry, I explained it in patch2 and forgot to reply your email. > > The existing PRManager only works with local scsi devices. This series > will completely decouple devices and drivers. The device can not only be > scsi, but also other devices such as nvme. The same is true for the > driver, which is completely unrestricted. > > And block/file-posix.c can implement the new block driver, and > pr_manager can be executed after splicing ioctl commands in these > drivers. This will be implemented in subsequent patches. Thanks for explaining! Stefan > > On 2024/6/11 01:18, Stefan Hajnoczi wrote: > > On Thu, Jun 06, 2024 at 08:24:34PM +0800, Changqi Lu wrote: > >> Hi, > >> > >> patchv5 has been modified. > >> > >> Sincerely hope that everyone can help review the > >> code and provide some suggestions. > >> > >> v4->v5: > >> - Fixed a memory leak bug at hw/nvme/ctrl.c. > >> > >> v3->v4: > >> - At the nvme layer, the two patches of enabling the ONCS > >> function and enabling rescap are combined into one. > >> - At the nvme layer, add helper functions for pr capacity > >> conversion between the block layer and the nvme layer. > >> > >> v2->v3: > >> In v2 Persist Through Power Loss(PTPL) is enable default. > >> In v3 PTPL is supported, which is passed as a parameter. > >> > >> v1->v2: > >> - Add sg_persist --report-capabilities for SCSI protocol and enable > >> oncs and rescap for NVMe protocol. > >> - Add persistent reservation capabilities constants and helper functions > for > >> SCSI and NVMe protocol. > >> - Add comments for necessary APIs. > >> > >> v1: > >> - Add seven APIs about persistent reservation command for block layer. > >> These APIs including reading keys, reading reservations, registering, > >> reserving, releasing, clearing and preempting. > >> - Add the necessary pr-related operation APIs for both the > >> SCSI protocol and NVMe protocol at the device layer. > >> - Add scsi driver at the driver layer to verify the functions > > > > My question from v1 is unanswered: > > > > What is the relationship to the existing PRManager functionality > > (docs/interop/pr-helper.rst) where block/file-posix.c interprets SCSI > > ioctls and sends persistent reservation requests to an external helper > > process? > > > > I wonder if block/file-posix.c can implement the new block driver > > callbacks using pr_mgr (while keeping the existing scsi-generic > > support). > > > > Thanks, > > Stefan > > > >> > >> > >> Changqi Lu (10): > >> block: add persistent reservation in/out api > >> block/raw: add persistent reservation in/out driver > >> scsi/constant: add persistent reservation in/out protocol constants > >> scsi/util: add helper functions for persistent reservation types > >> conversion > >> hw/scsi: add persistent reservation in/out api for scsi device > >> block/nvme: add reservation command protocol constants > >> hw/nvme: add helper functions for converting reservation types > >> hw/nvme: enable ONCS and rescap function > >> hw/nvme: add reservation protocal command > >> block/iscsi: add persistent reservation in/out driver > >> > >> block/block-backend.c | 397 ++ > >> block/io.c | 163 +++ > >> block/iscsi.c | 443 ++ > >> block/raw-format.c | 56 > >> hw/nvme/ctrl.c | 326 +- > >> hw/nvme/ns.c | 5 + > >> hw/nvme/nvme.h | 84 ++ > >> hw/scsi/scsi-disk.c | 352 > >> include/block/block-common.h | 40 +++ > >> include/block/block-io.h | 20 ++ > >> include/block/block_int-common.h | 84 ++ > >> include/block/nvme.h | 98 +++ > >> include/scsi/constants.h | 52 > >> include/scsi/utils.h | 8 + > >> include/sysemu/block-backend-io.h | 24 ++ > >> scsi/utils.c | 81 ++ > >> 16 files changed, 2231 insertions(+), 2 deletions(-) > >> > >> -- > >> 2.20.1 > >> signature.asc Description: PGP signature
Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`
On Tue, Jun 11, 2024 at 04:30:53PM +0200, Kevin Wolf wrote: > Am 11.06.2024 um 14:31 hat Amjad Alsharafi geschrieben: > > On Mon, Jun 10, 2024 at 06:49:43PM +0200, Kevin Wolf wrote: > > > Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben: > > > > 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. > > > > > > > > Additionally, removed the `abort` when `first_mapping_index` does not > > > > match, as this matches the case when adding new clusters for files, and > > > > its inevitable that we reach this condition when doing that if the > > > > clusters are not after one another, so there is no reason to `abort` > > > > here, execution continues and the new clusters are written to disk > > > > correctly. > > > > > > > > Signed-off-by: Amjad Alsharafi > > > > > > Can you help me understand how first_mapping_index really works? > > > > > > It seems to me that you get a chain of mappings for each file on the FAT > > > filesystem, which are just the contiguous areas in it, and > > > first_mapping_index refers to the mapping at the start of the file. But > > > for much of the time, it actually doesn't seem to be set at all, so you > > > have mapping->first_mapping_index == -1. Do you understand the rules > > > around when it's set and when it isn't? > > > > Yeah. So `first_mapping_index` is the index of the first mapping, each > > mapping is a group of clusters that are contiguous in the file. > > Its mostly `-1` because the first mapping will have the value set as > > `-1` and not its own index, this value will only be set when the file > > contain more than one mapping, and this will only happen when you add > > clusters to a file that are not contiguous with the existing clusters. > > Ah, that makes some sense. Not sure if it's optimal, but it's a rule I > can work with. So just to confirm, this is the invariant that we think > should always hold true, right? > > assert((mapping->mode & MODE_DIRECTORY) || >!mapping->info.file.offset || >mapping->first_mapping_index > 0); > Yes. We can add this into `get_cluster_count_for_direntry` loop. I'm thinking of also converting those `abort` into `assert`, since the line `copy_it = 1;` was confusing me, since it was after the `abort`. > > And actually, thanks to that I noticed another bug not fixed in PATCH 3, > > We are doing this check > > `s->current_mapping->first_mapping_index != mapping->first_mapping_index` > > to know if we should switch to the new mapping or not. > > If we were reading from the first mapping (`first_mapping_index == -1`) > > and we jumped to the second mapping (`first_mapping_index == n`), we > > will catch this condition and switch to the new mapping. > > > > But if the file has more than 2 mappings, and we jumped to the 3rd > > mapping, we will not catch this since (`first_mapping_index == n`) for > > both of them haha. I think a better check is to check the `mapping` > > pointer directly. (I'll add it also in the next series together with a > > test for it.) > > This comparison is exactly what confused me. I didn't realise that the > first mapping in the chain has a different value here, so I thought this > must mean that we're looking at a different file now - but of course I > couldn't see a reason for that because we're iterating through a single > file in this function. > > But even now that I know that the condition triggers when switching from > the first to the second mapping, it doesn't make sense to me. We don't > have to copy things around just because a file is non-contiguous. > > What we want to catch is if the order of mappings has changed compared > to the old state. Do we need a linked list, maybe a prev_mapping_index, > instead of first_mapping_index so that we can compare if it is still the > same as before? I think this would be the better design (tbh, that's what I thought `first_mapping_index` would do), though not sure if other components depend so much into the current design that it would be hard to change. I'll try to implement this `prev_mapping_index` and see how it goes. > > Or actually, I suppose that's the first block with an abort() in the > code, just that it doesn't compare mappings, but their offsets. I think, I'm still confused on the whole logic there, the function `get_cluster_count_for_direntry` is a mess, and it doesn't just *get* the cluster count, it also schedule writeouts and may copy clusters around. > > > > > > > > block/vvfat.c | 12 +++- > > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/block/vvfat.c b/block/vvfat.c > > > > index 19da009a5b..f0642ac3e4 100644 > > > > --- a/block/vvfat.c > > > > +++ b/block/vvfat.c > > > > @@ -1408,7 +1408,9 @@ read_cluster_directory: > > > > > > > > assert(s->current_fd); > > > > > > > > - > > > > offset=s->cluste
[PULL 2/8] Revert "monitor: use aio_co_reschedule_self()"
From: Stefan Hajnoczi
Commit 1f25c172f837 ("monitor: use aio_co_reschedule_self()") was a code
cleanup that uses aio_co_reschedule_self() instead of open coding
coroutine rescheduling.
Bug RHEL-34618 was reported and Kevin Wolf identified
the root cause. I missed that aio_co_reschedule_self() ->
qemu_get_current_aio_context() only knows about
qemu_aio_context/IOThread AioContexts and not about iohandler_ctx. It
does not function correctly when going back from the iohandler_ctx to
qemu_aio_context.
Go back to open coding the AioContext transitions to avoid this bug.
This reverts commit 1f25c172f83704e350c0829438d832384084a74d.
Cc: qemu-sta...@nongnu.org
Buglink: https://issues.redhat.com/browse/RHEL-34618
Signed-off-by: Stefan Hajnoczi
Message-ID: <20240506190622.56095-2-stefa...@redhat.com>
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
qapi/qmp-dispatch.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index f3488afeef..176b549473 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -212,7 +212,8 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList
*cmds, QObject *requ
* executing the command handler so that it can make progress if it
* involves an AIO_WAIT_WHILE().
*/
-aio_co_reschedule_self(qemu_get_aio_context());
+aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
+qemu_coroutine_yield();
}
monitor_set_cur(qemu_coroutine_self(), cur_mon);
@@ -226,7 +227,9 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList
*cmds, QObject *requ
* Move back to iohandler_ctx so that nested event loops for
* qemu_aio_context don't start new monitor commands.
*/
-aio_co_reschedule_self(iohandler_get_aio_context());
+aio_co_schedule(iohandler_get_aio_context(),
+qemu_coroutine_self());
+qemu_coroutine_yield();
}
} else {
/*
--
2.45.2
[PULL 7/8] block/crypto: create ciphers on demand
From: Stefan Hajnoczi
Ciphers are pre-allocated by qcrypto_block_init_cipher() depending on
the given number of threads. The -device
virtio-blk-pci,iothread-vq-mapping= feature allows users to assign
multiple IOThreads to a virtio-blk device, but the association between
the virtio-blk device and the block driver happens after the block
driver is already open.
When the number of threads given to qcrypto_block_init_cipher() is
smaller than the actual number of threads at runtime, the
block->n_free_ciphers > 0 assertion in qcrypto_block_pop_cipher() can
fail.
Get rid of qcrypto_block_init_cipher() n_thread's argument and allocate
ciphers on demand.
Reported-by: Qing Wang
Buglink: https://issues.redhat.com/browse/RHEL-36159
Signed-off-by: Stefan Hajnoczi
Message-ID: <20240527155851.892885-2-stefa...@redhat.com>
Reviewed-by: Kevin Wolf
Acked-by: Daniel P. Berrangé
Signed-off-by: Kevin Wolf
---
crypto/blockpriv.h | 12 +++--
crypto/block-luks.c | 3 +-
crypto/block-qcow.c | 2 +-
crypto/block.c | 111 ++--
4 files changed, 78 insertions(+), 50 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 836f3b4726..4bf6043d5d 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -32,8 +32,14 @@ struct QCryptoBlock {
const QCryptoBlockDriver *driver;
void *opaque;
-QCryptoCipher **ciphers;
-size_t n_ciphers;
+/* Cipher parameters */
+QCryptoCipherAlgorithm alg;
+QCryptoCipherMode mode;
+uint8_t *key;
+size_t nkey;
+
+QCryptoCipher **free_ciphers;
+size_t max_free_ciphers;
size_t n_free_ciphers;
QCryptoIVGen *ivgen;
QemuMutex mutex;
@@ -130,7 +136,7 @@ int qcrypto_block_init_cipher(QCryptoBlock *block,
QCryptoCipherAlgorithm alg,
QCryptoCipherMode mode,
const uint8_t *key, size_t nkey,
- size_t n_threads, Error **errp);
+ Error **errp);
void qcrypto_block_free_cipher(QCryptoBlock *block);
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 3ee928fb5a..3357852c0a 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1262,7 +1262,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
luks->cipher_mode,
masterkey,
luks->header.master_key_len,
- n_threads,
errp) < 0) {
goto fail;
}
@@ -1456,7 +1455,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Setup the block device payload encryption objects */
if (qcrypto_block_init_cipher(block, luks_opts.cipher_alg,
luks_opts.cipher_mode, masterkey,
- luks->header.master_key_len, 1, errp) < 0) {
+ luks->header.master_key_len, errp) < 0) {
goto error;
}
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 4d7cf36a8f..02305058e3 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -75,7 +75,7 @@ qcrypto_block_qcow_init(QCryptoBlock *block,
ret = qcrypto_block_init_cipher(block, QCRYPTO_CIPHER_ALG_AES_128,
QCRYPTO_CIPHER_MODE_CBC,
keybuf, G_N_ELEMENTS(keybuf),
-n_threads, errp);
+errp);
if (ret < 0) {
ret = -ENOTSUP;
goto fail;
diff --git a/crypto/block.c b/crypto/block.c
index 506ea1d1a3..ba6d1cebc7 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -20,6 +20,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qemu/lockable.h"
#include "blockpriv.h"
#include "block-qcow.h"
#include "block-luks.h"
@@ -57,6 +58,8 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions
*options,
{
QCryptoBlock *block = g_new0(QCryptoBlock, 1);
+qemu_mutex_init(&block->mutex);
+
block->format = options->format;
if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -76,8 +79,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions
*options,
return NULL;
}
-qemu_mutex_init(&block->mutex);
-
return block;
}
@@ -92,6 +93,8 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions
*options,
{
QCryptoBlock *block = g_new0(QCryptoBlock, 1);
+qemu_mutex_init(&block->mutex);
+
block->format = options->format;
if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -111,8 +114,6 @@ QCryptoBlock
*qcrypto_block_create(QCryptoBlockCreateOptions *options,
return NULL;
}
-qemu_mutex_init(&block->mutex);
-
return block;
}
@@ -227,37 +228,42 @@ QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock
*block)
* This func
[PULL 3/8] aio: warn about iohandler_ctx special casing
From: Stefan Hajnoczi The main loop has two AioContexts: qemu_aio_context and iohandler_ctx. The main loop runs them both, but nested aio_poll() calls on qemu_aio_context exclude iohandler_ctx. Which one should qemu_get_current_aio_context() return when called from the main loop? Document that it's always qemu_aio_context. This has subtle effects on functions that use qemu_get_current_aio_context(). For example, aio_co_reschedule_self() does not work when moving from iohandler_ctx to qemu_aio_context because qemu_get_current_aio_context() does not differentiate these two AioContexts. Document this in order to reduce the chance of future bugs. Signed-off-by: Stefan Hajnoczi Message-ID: <20240506190622.56095-3-stefa...@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- include/block/aio.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/include/block/aio.h b/include/block/aio.h index 8378553eb9..4ee81936ed 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -629,6 +629,9 @@ void aio_co_schedule(AioContext *ctx, Coroutine *co); * * Move the currently running coroutine to new_ctx. If the coroutine is already * running in new_ctx, do nothing. + * + * Note that this function cannot reschedule from iohandler_ctx to + * qemu_aio_context. */ void coroutine_fn aio_co_reschedule_self(AioContext *new_ctx); @@ -661,6 +664,9 @@ void aio_co_enter(AioContext *ctx, Coroutine *co); * If called from an IOThread this will be the IOThread's AioContext. If * called from the main thread or with the "big QEMU lock" taken it * will be the main loop AioContext. + * + * Note that the return value is never the main loop's iohandler_ctx and the + * return value is the main loop AioContext instead. */ AioContext *qemu_get_current_aio_context(void); -- 2.45.2
[PULL 1/8] block: drop force_dup parameter of raw_reconfigure_getfd()
From: "Denis V. Lunev via"
Since commit 72373e40fbc, this parameter is always passed as 'false'
from the caller.
Signed-off-by: Denis V. Lunev
CC: Andrey Zhadchenko
CC: Kevin Wolf
CC: Hanna Reitz
Message-ID: <20240430170213.148558-1-...@openvz.org>
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
block/file-posix.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..5c46938936 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1039,8 +1039,7 @@ static int fcntl_setfl(int fd, int flag)
}
static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
- int *open_flags, uint64_t perm, bool
force_dup,
- Error **errp)
+ int *open_flags, uint64_t perm, Error **errp)
{
BDRVRawState *s = bs->opaque;
int fd = -1;
@@ -1068,7 +1067,7 @@ static int raw_reconfigure_getfd(BlockDriverState *bs,
int flags,
assert((s->open_flags & O_ASYNC) == 0);
#endif
-if (!force_dup && *open_flags == s->open_flags) {
+if (*open_flags == s->open_flags) {
/* We're lucky, the existing fd is fine */
return s->fd;
}
@@ -3748,8 +3747,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t
perm, uint64_t shared,
int ret;
/* We may need a new fd if auto-read-only switches the mode */
-ret = raw_reconfigure_getfd(bs, input_flags, &open_flags, perm,
-false, errp);
+ret = raw_reconfigure_getfd(bs, input_flags, &open_flags, perm, errp);
if (ret < 0) {
return ret;
} else if (ret != s->fd) {
--
2.45.2
[PULL 6/8] linux-aio: add IO_CMD_FDSYNC command support
From: Prasad Pandit
Libaio defines IO_CMD_FDSYNC command to sync all outstanding
asynchronous I/O operations, by flushing out file data to the
disk storage. Enable linux-aio to submit such aio request.
When using aio=native without fdsync() support, QEMU creates
pthreads, and destroying these pthreads results in TLB flushes.
In a real-time guest environment, TLB flushes cause a latency
spike. This patch helps to avoid such spikes.
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Prasad Pandit
Message-ID: <20240425070412.37248-1-ppan...@redhat.com>
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
include/block/raw-aio.h | 1 +
block/file-posix.c | 9 +
block/linux-aio.c | 21 -
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 20e000b8ef..626706827f 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -60,6 +60,7 @@ void laio_cleanup(LinuxAioState *s);
int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov,
int type, uint64_t dev_max_batch);
+bool laio_has_fdsync(int);
void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
#endif
diff --git a/block/file-posix.c b/block/file-posix.c
index 5c46938936..be25e35ff6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -159,6 +159,7 @@ typedef struct BDRVRawState {
bool has_discard:1;
bool has_write_zeroes:1;
bool use_linux_aio:1;
+bool has_laio_fdsync:1;
bool use_linux_io_uring:1;
int page_cache_inconsistent; /* errno from fdatasync failure */
bool has_fallocate;
@@ -718,6 +719,9 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
ret = -EINVAL;
goto fail;
}
+if (s->use_linux_aio) {
+s->has_laio_fdsync = laio_has_fdsync(s->fd);
+}
#else
if (s->use_linux_aio) {
error_setg(errp, "aio=native was specified, but is not supported "
@@ -2598,6 +2602,11 @@ static int coroutine_fn
raw_co_flush_to_disk(BlockDriverState *bs)
if (raw_check_linux_io_uring(s)) {
return luring_co_submit(bs, s->fd, 0, NULL, QEMU_AIO_FLUSH);
}
+#endif
+#ifdef CONFIG_LINUX_AIO
+if (s->has_laio_fdsync && raw_check_linux_aio(s)) {
+return laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0);
+}
#endif
return raw_thread_pool_submit(handle_aiocb_flush, &acb);
}
diff --git a/block/linux-aio.c b/block/linux-aio.c
index ec05d946f3..e3b5ec9aba 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -384,6 +384,9 @@ static int laio_do_submit(int fd, struct qemu_laiocb
*laiocb, off_t offset,
case QEMU_AIO_READ:
io_prep_preadv(iocbs, fd, qiov->iov, qiov->niov, offset);
break;
+case QEMU_AIO_FLUSH:
+io_prep_fdsync(iocbs, fd);
+break;
/* Currently Linux kernel does not support other operations */
default:
fprintf(stderr, "%s: invalid AIO request type 0x%x.\n",
@@ -412,7 +415,7 @@ int coroutine_fn laio_co_submit(int fd, uint64_t offset,
QEMUIOVector *qiov,
AioContext *ctx = qemu_get_current_aio_context();
struct qemu_laiocb laiocb = {
.co = qemu_coroutine_self(),
-.nbytes = qiov->size,
+.nbytes = qiov ? qiov->size : 0,
.ctx= aio_get_linux_aio(ctx),
.ret= -EINPROGRESS,
.is_read= (type == QEMU_AIO_READ),
@@ -486,3 +489,19 @@ void laio_cleanup(LinuxAioState *s)
}
g_free(s);
}
+
+bool laio_has_fdsync(int fd)
+{
+struct iocb cb;
+struct iocb *cbs[] = {&cb, NULL};
+
+io_context_t ctx = 0;
+io_setup(1, &ctx);
+
+/* check if host kernel supports IO_CMD_FDSYNC */
+io_prep_fdsync(&cb, fd);
+int ret = io_submit(ctx, 1, cbs);
+
+io_destroy(ctx);
+return (ret == -EINVAL) ? false : true;
+}
--
2.45.2
[PULL 8/8] crypto/block: drop qcrypto_block_open() n_threads argument
From: Stefan Hajnoczi
The n_threads argument is no longer used since the previous commit.
Remove it.
Signed-off-by: Stefan Hajnoczi
Message-ID: <20240527155851.892885-3-stefa...@redhat.com>
Reviewed-by: Kevin Wolf
Acked-by: Daniel P. Berrangé
Signed-off-by: Kevin Wolf
---
crypto/blockpriv.h | 1 -
include/crypto/block.h | 2 --
block/crypto.c | 1 -
block/qcow.c | 2 +-
block/qcow2.c | 5 ++---
crypto/block-luks.c| 1 -
crypto/block-qcow.c| 6 ++
crypto/block.c | 3 +--
tests/unit/test-crypto-block.c | 4
9 files changed, 6 insertions(+), 19 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 4bf6043d5d..b8f77cb5eb 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -59,7 +59,6 @@ struct QCryptoBlockDriver {
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
-size_t n_threads,
Error **errp);
int (*create)(QCryptoBlock *block,
diff --git a/include/crypto/block.h b/include/crypto/block.h
index 92e823c9f2..5b5d039800 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -76,7 +76,6 @@ typedef enum {
* @readfunc: callback for reading data from the volume
* @opaque: data to pass to @readfunc
* @flags: bitmask of QCryptoBlockOpenFlags values
- * @n_threads: allow concurrent I/O from up to @n_threads threads
* @errp: pointer to a NULL-initialized error object
*
* Create a new block encryption object for an existing
@@ -113,7 +112,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions
*options,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
- size_t n_threads,
Error **errp);
typedef enum {
diff --git a/block/crypto.c b/block/crypto.c
index 21eed909c1..4eed3ffa6a 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -363,7 +363,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat
format,
block_crypto_read_func,
bs,
cflags,
- 1,
errp);
if (!crypto->block) {
diff --git a/block/qcow.c b/block/qcow.c
index ca8e1d5ec8..c2f89db055 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -211,7 +211,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options,
int flags,
cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
}
s->crypto = qcrypto_block_open(crypto_opts, "encrypt.",
- NULL, NULL, cflags, 1, errp);
+ NULL, NULL, cflags, errp);
if (!s->crypto) {
ret = -EINVAL;
goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 956128b409..10883a2494 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -321,7 +321,7 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t
start_offset,
}
s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
qcow2_crypto_hdr_read_func,
- bs, cflags, QCOW2_MAX_THREADS,
errp);
+ bs, cflags, errp);
if (!s->crypto) {
return -EINVAL;
}
@@ -1701,8 +1701,7 @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int
flags,
cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
}
s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
- NULL, NULL, cflags,
- QCOW2_MAX_THREADS, errp);
+ NULL, NULL, cflags, errp);
if (!s->crypto) {
ret = -EINVAL;
goto fail;
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 3357852c0a..5b777c15d3 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1189,7 +1189,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
-size_t n_threads,
Error **errp)
{
QCryptoBlockLUKS *luks = NULL;
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 02305058e3..42e9556e42 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -44,7 +44,6 @@ qcrypto_block_qcow_has_format(const uint8_t *buf
G_GNUC_UNUSED,
static int
qcrypto_block_qcow_init(QCryptoBlock *block,
const char *keysecret,
-size
[PULL 4/8] qemu-io: add cvtnum() error handling for zone commands
From: Stefan Hajnoczi
cvtnum() parses positive int64_t values and returns a negative errno on
failure. Print errors and return early when cvtnum() fails.
While we're at it, also reject nr_zones values greater or equal to 2^32
since they cannot be represented.
Reported-by: Peter Maydell
Cc: Sam Li
Signed-off-by: Stefan Hajnoczi
Message-ID: <20240507180558.377233-1-stefa...@redhat.com>
Reviewed-by: Sam Li
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
qemu-io-cmds.c | 48 +++-
1 file changed, 47 insertions(+), 1 deletion(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index f5d7202a13..e2fab57183 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1739,12 +1739,26 @@ static int zone_report_f(BlockBackend *blk, int argc,
char **argv)
{
int ret;
int64_t offset;
+int64_t val;
unsigned int nr_zones;
++optind;
offset = cvtnum(argv[optind]);
+if (offset < 0) {
+print_cvtnum_err(offset, argv[optind]);
+return offset;
+}
++optind;
-nr_zones = cvtnum(argv[optind]);
+val = cvtnum(argv[optind]);
+if (val < 0) {
+print_cvtnum_err(val, argv[optind]);
+return val;
+}
+if (val > UINT_MAX) {
+printf("Number of zones must be less than 2^32\n");
+return -ERANGE;
+}
+nr_zones = val;
g_autofree BlockZoneDescriptor *zones = NULL;
zones = g_new(BlockZoneDescriptor, nr_zones);
@@ -1780,8 +1794,16 @@ static int zone_open_f(BlockBackend *blk, int argc, char
**argv)
int64_t offset, len;
++optind;
offset = cvtnum(argv[optind]);
+if (offset < 0) {
+print_cvtnum_err(offset, argv[optind]);
+return offset;
+}
++optind;
len = cvtnum(argv[optind]);
+if (len < 0) {
+print_cvtnum_err(len, argv[optind]);
+return len;
+}
ret = blk_zone_mgmt(blk, BLK_ZO_OPEN, offset, len);
if (ret < 0) {
printf("zone open failed: %s\n", strerror(-ret));
@@ -1805,8 +1827,16 @@ static int zone_close_f(BlockBackend *blk, int argc,
char **argv)
int64_t offset, len;
++optind;
offset = cvtnum(argv[optind]);
+if (offset < 0) {
+print_cvtnum_err(offset, argv[optind]);
+return offset;
+}
++optind;
len = cvtnum(argv[optind]);
+if (len < 0) {
+print_cvtnum_err(len, argv[optind]);
+return len;
+}
ret = blk_zone_mgmt(blk, BLK_ZO_CLOSE, offset, len);
if (ret < 0) {
printf("zone close failed: %s\n", strerror(-ret));
@@ -1830,8 +1860,16 @@ static int zone_finish_f(BlockBackend *blk, int argc,
char **argv)
int64_t offset, len;
++optind;
offset = cvtnum(argv[optind]);
+if (offset < 0) {
+print_cvtnum_err(offset, argv[optind]);
+return offset;
+}
++optind;
len = cvtnum(argv[optind]);
+if (len < 0) {
+print_cvtnum_err(len, argv[optind]);
+return len;
+}
ret = blk_zone_mgmt(blk, BLK_ZO_FINISH, offset, len);
if (ret < 0) {
printf("zone finish failed: %s\n", strerror(-ret));
@@ -1855,8 +1893,16 @@ static int zone_reset_f(BlockBackend *blk, int argc,
char **argv)
int64_t offset, len;
++optind;
offset = cvtnum(argv[optind]);
+if (offset < 0) {
+print_cvtnum_err(offset, argv[optind]);
+return offset;
+}
++optind;
len = cvtnum(argv[optind]);
+if (len < 0) {
+print_cvtnum_err(len, argv[optind]);
+return len;
+}
ret = blk_zone_mgmt(blk, BLK_ZO_RESET, offset, len);
if (ret < 0) {
printf("zone reset failed: %s\n", strerror(-ret));
--
2.45.2
[PULL 0/8] Block layer patches
The following changes since commit 80e8f0602168f451a93e71cbb1d59e93d745e62e: Merge tag 'bsd-user-misc-2024q2-pull-request' of gitlab.com:bsdimp/qemu into staging (2024-06-09 11:21:55 -0700) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 3ab0f063e58ed9224237d69c4211ca83335164c4: crypto/block: drop qcrypto_block_open() n_threads argument (2024-06-10 11:05:43 +0200) Block layer patches - crypto: Fix crash when used with multiqueue devices - linux-aio: add IO_CMD_FDSYNC command support - copy-before-write: Avoid integer overflows for timeout > 4s - Fix crash with QMP block_resize and iothreads - qemu-io: add cvtnum() error handling for zone commands - Code cleanup Denis V. Lunev via (1): block: drop force_dup parameter of raw_reconfigure_getfd() Fiona Ebner (1): block/copy-before-write: use uint64_t for timeout in nanoseconds Prasad J Pandit (1): linux-aio: add IO_CMD_FDSYNC command support Stefan Hajnoczi (5): Revert "monitor: use aio_co_reschedule_self()" aio: warn about iohandler_ctx special casing qemu-io: add cvtnum() error handling for zone commands block/crypto: create ciphers on demand crypto/block: drop qcrypto_block_open() n_threads argument crypto/blockpriv.h | 13 +++-- include/block/aio.h| 6 +++ include/block/raw-aio.h| 1 + include/crypto/block.h | 2 - block/copy-before-write.c | 2 +- block/crypto.c | 1 - block/file-posix.c | 17 -- block/linux-aio.c | 21 +++- block/qcow.c | 2 +- block/qcow2.c | 5 +- crypto/block-luks.c| 4 +- crypto/block-qcow.c| 8 ++- crypto/block.c | 114 - qapi/qmp-dispatch.c| 7 ++- qemu-io-cmds.c | 48 - tests/unit/test-crypto-block.c | 4 -- 16 files changed, 176 insertions(+), 79 deletions(-)
[PULL 5/8] block/copy-before-write: use uint64_t for timeout in nanoseconds
From: Fiona Ebner
rather than the uint32_t for which the maximum is slightly more than 4
seconds and larger values would overflow. The QAPI interface allows
specifying the number of seconds, so only values 0 to 4 are safe right
now, other values lead to a much lower timeout than a user expects.
The block_copy() call where this is used already takes a uint64_t for
the timeout, so no change required there.
Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option")
Reported-by: Friedrich Weber
Signed-off-by: Fiona Ebner
Message-ID: <20240429141934.442154-1-f.eb...@proxmox.com>
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
block/copy-before-write.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index cd65524e26..853e01a1eb 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -43,7 +43,7 @@ typedef struct BDRVCopyBeforeWriteState {
BlockCopyState *bcs;
BdrvChild *target;
OnCbwError on_cbw_error;
-uint32_t cbw_timeout_ns;
+uint64_t cbw_timeout_ns;
bool discard_source;
/*
--
2.45.2
Re: [PATCH v4 5/5] iotests: add backup-discard-source
Am 13.03.2024 um 16:28 hat Vladimir Sementsov-Ogievskiy geschrieben: > Add test for a new backup option: discard-source. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Fiona Ebner > Tested-by: Fiona Ebner This test fails for me, and it already does so after this commit that introduced it. I haven't checked what get_actual_size(), but I'm running on XFS, so its preallocation could be causing this. We generally avoid checking the number of allocated blocks in image files for this reason. Kevin backup-discard-source fail [19:45:49] [19:45:50] 0.8s failed, exit status 1 --- /home/kwolf/source/qemu/tests/qemu-iotests/tests/backup-discard-source.out +++ /home/kwolf/source/qemu/build-clang/scratch/qcow2-file-backup-discard-source/backup-discard-source.out.bad @@ -1,5 +1,14 @@ -.. +F. +== +FAIL: test_discard_cbw (__main__.TestBackup.test_discard_cbw) +1. do backup(discard_source=True), which should inform +-- +Traceback (most recent call last): + File "/home/kwolf/source/qemu/tests/qemu-iotests/tests/backup-discard-source", line 147, in test_discard_cbw +self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024) +AssertionError: 1249280 not less than 524288 + -- Ran 2 tests -OK +FAILED (failures=1) Failures: backup-discard-source Failed 1 of 1 iotests
Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`
Am 11.06.2024 um 18:22 hat Amjad Alsharafi geschrieben: > On Tue, Jun 11, 2024 at 04:30:53PM +0200, Kevin Wolf wrote: > > Am 11.06.2024 um 14:31 hat Amjad Alsharafi geschrieben: > > > On Mon, Jun 10, 2024 at 06:49:43PM +0200, Kevin Wolf wrote: > > > > Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben: > > > > > 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. > > > > > > > > > > Additionally, removed the `abort` when `first_mapping_index` does not > > > > > match, as this matches the case when adding new clusters for files, > > > > > and > > > > > its inevitable that we reach this condition when doing that if the > > > > > clusters are not after one another, so there is no reason to `abort` > > > > > here, execution continues and the new clusters are written to disk > > > > > correctly. > > > > > > > > > > Signed-off-by: Amjad Alsharafi > > > > > > > > Can you help me understand how first_mapping_index really works? > > > > > > > > It seems to me that you get a chain of mappings for each file on the FAT > > > > filesystem, which are just the contiguous areas in it, and > > > > first_mapping_index refers to the mapping at the start of the file. But > > > > for much of the time, it actually doesn't seem to be set at all, so you > > > > have mapping->first_mapping_index == -1. Do you understand the rules > > > > around when it's set and when it isn't? > > > > > > Yeah. So `first_mapping_index` is the index of the first mapping, each > > > mapping is a group of clusters that are contiguous in the file. > > > Its mostly `-1` because the first mapping will have the value set as > > > `-1` and not its own index, this value will only be set when the file > > > contain more than one mapping, and this will only happen when you add > > > clusters to a file that are not contiguous with the existing clusters. > > > > Ah, that makes some sense. Not sure if it's optimal, but it's a rule I > > can work with. So just to confirm, this is the invariant that we think > > should always hold true, right? > > > > assert((mapping->mode & MODE_DIRECTORY) || > >!mapping->info.file.offset || > >mapping->first_mapping_index > 0); > > > > Yes. > > We can add this into `get_cluster_count_for_direntry` loop. Maybe even find_mapping_for_cluster() because we think it should apply always? It's called by get_cluster_count_for_direntry(), but also by other functions. Either way, I think this should be a separate patch. > I'm thinking of also converting those `abort` into `assert`, since > the line `copy_it = 1;` was confusing me, since it was after the `abort`. I agree for the abort() that you removed, but I'm not sure about the other one. I have a feeling the copy_it = 1 might actually be correct there (if the copying logic is implemented correctly; I didn't check that). > > > And actually, thanks to that I noticed another bug not fixed in PATCH 3, > > > We are doing this check > > > `s->current_mapping->first_mapping_index != mapping->first_mapping_index` > > > to know if we should switch to the new mapping or not. > > > If we were reading from the first mapping (`first_mapping_index == -1`) > > > and we jumped to the second mapping (`first_mapping_index == n`), we > > > will catch this condition and switch to the new mapping. > > > > > > But if the file has more than 2 mappings, and we jumped to the 3rd > > > mapping, we will not catch this since (`first_mapping_index == n`) for > > > both of them haha. I think a better check is to check the `mapping` > > > pointer directly. (I'll add it also in the next series together with a > > > test for it.) > > > > This comparison is exactly what confused me. I didn't realise that the > > first mapping in the chain has a different value here, so I thought this > > must mean that we're looking at a different file now - but of course I > > couldn't see a reason for that because we're iterating through a single > > file in this function. > > > > But even now that I know that the condition triggers when switching from > > the first to the second mapping, it doesn't make sense to me. We don't > > have to copy things around just because a file is non-contiguous. > > > > What we want to catch is if the order of mappings has changed compared > > to the old state. Do we need a linked list, maybe a prev_mapping_index, > > instead of first_mapping_index so that we can compare if it is still the > > same as before? > > I think this would be the better design (tbh, that's what I thought > `first_mapping_index` would do), though not sure if other components > depend so much into the current design that it would be hard to change. > > I'll try to implement this `prev_mapping_index` and see how it goes. Let's try not to do too much at once. We know that vvfat is a mess, nobody fully understands it, and the write support is
Re: [PATCH v4 0/4] hw/nvme: FDP and SR-IOV enhancements
On May 29 21:42, Minwoo Im wrote: > Hello, > > This is v4 patchset to increase number of virtual functions for NVMe SR-IOV. > Please consider the following change notes per version. > > This patchset has been tested with the following simple script more than > 127 VFs. > > -device nvme-subsys,id=subsys0 \ > -device ioh3420,id=rp2,multifunction=on,chassis=12 \ > -device > nvme,serial=foo,id=nvme0,bus=rp2,subsys=subsys0,mdts=9,msix_qsize=130,max_ioqpairs=260,sriov_max_vfs=129,sriov_vq_flexible=258,sriov_vi_flexible=129 > \ > > $ cat nvme-enable-vfs.sh > #!/bin/bash > > nr_vfs=129 > > for (( i=1; i<=$nr_vfs; i++ )) > do > nvme virt-mgmt /dev/nvme0 -c $i -r 0 -a 8 -n 2 > nvme virt-mgmt /dev/nvme0 -c $i -r 1 -a 8 -n 1 > done > > bdf=":01:00.0" > sysfs="/sys/bus/pci/devices/$bdf" > nvme="/sys/bus/pci/drivers/nvme" > > echo 0 > $sysfs/sriov_drivers_autoprobe > echo $nr_vfs > $sysfs/sriov_numvfs > > for (( i=1; i<=$nr_vfs; i++ )) > do > nvme virt-mgmt /dev/nvme0 -c $i -a 9 > > echo "nvme" > $sysfs/virtfn$(($i-1))/driver_override > bdf="$(basename $(readlink $sysfs/virtfn$(($i-1" > echo $bdf > $nvme/bind > done > > Thanks, > > v4: > - Rebased on the latest master. > - Update n->params.sriov_max_vfs to uint16_t as per spec. > > v3: > - Replace [3/4] patch with one allocating a dyanmic array of secondary >controller list rather than a static array with a fixed size of >maximum number of VF to support (Suggested by Klaus). > v2: > - Added [2/4] commit to fix crash due to entry overflow > > Minwoo Im (4): > hw/nvme: add Identify Endurance Group List > hw/nvme: separate identify data for sec. ctrl list > hw/nvme: Allocate sec-ctrl-list as a dynamic array > hw/nvme: Expand VI/VQ resource to uint32 > > hw/nvme/ctrl.c | 59 +++- > hw/nvme/nvme.h | 19 +++--- > hw/nvme/subsys.c | 10 +--- > include/block/nvme.h | 1 + > 4 files changed, 54 insertions(+), 35 deletions(-) > > -- > 2.34.1 > Looks good Minwoo! Grabbing for nvme-next. Reviewed-by: Klaus Jensen signature.asc Description: PGP signature
Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
Hello Alberto, On Mon, 10 Jun 2024 17:47, Alberto Garcia wrote: This tool converts a disk image to qcow2, writing the result directly to stdout. This can be used for example to send the generated file over the network. This is equivalent to using qemu-img to convert a file to qcow2 and then writing the result to stdout, with the difference that this tool does not need to create this temporary qcow2 file and therefore does not need any additional disk space. Can you expand on this a little bit? Would modifying qemu-img to write to stdout if given, say, - instead of a file output path be enough to make this script unnecessary? The input file is read twice. The first pass is used to determine which clusters contain non-zero data and that information is used to create the qcow2 header, refcount table and blocks, and L1 and L2 tables. After all that metadata is created then the second pass is used to write the guest data. By default qcow2-to-stdout.py expects the input to be a raw file, but if qemu-storage-daemon is available then it can also be used to read images in other formats. Alternatively the user can also run qemu-ndb or qemu-storage-daemon manually instead. Signed-off-by: Alberto Garcia Signed-off-by: Madeeha Javed ---
