[PATCH] hw:w25p80: Add STATE_STANDBY to handle incorrect command

[Dan Zhang]

---
 hw/block/m25p80.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index b6bd430a99..3bb0466dca 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -423,6 +423,7 @@ typedef enum {
 STATE_COLLECTING_DATA,
 STATE_COLLECTING_VAR_LEN_DATA,
 STATE_READING_DATA,
+STATE_STANDBY,
 } CMDState;
 
 typedef enum {
@@ -1218,6 +1219,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 || !s->write_enable) {
 qemu_log_mask(LOG_GUEST_ERROR,
   "M25P80: Status register write is disabled!\n");
+   qemu_log_mask(LOG_GUEST_ERROR,
+  "M25P80: switch to standby, re-aseert CS to 
reactivate \n");
+   s->state = STATE_STANDBY;
 break;
 }
 
@@ -1472,6 +1476,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, 
uint32_t tx)
   s->cur_addr, (uint8_t)tx);
 
 switch (s->state) {
+case STATE_STANDBY:
+   r = 0x; /* StandBy state SO shall be HiZ */
+   break;
 
 case STATE_PAGE_PROGRAM:
 trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
-- 
2.34.3

Re: [PATCH 0/2] linux-aio: fix unbalanced plugged counter in laio_io_unplug()

[Stefan Hajnoczi]

On Thu, Jun 09, 2022 at 05:47:10PM +0100, Stefan Hajnoczi wrote:
> An unlucky I/O pattern can result in stalled Linux AIO requests when the
> plugged counter becomes unbalanced. See Patch 1 for details.
> 
> Patch 2 adds a comment to explain why the laio_io_unplug() even checks max
> batch in the first place.
> 
> Stefan Hajnoczi (2):
>   linux-aio: fix unbalanced plugged counter in laio_io_unplug()
>   linux-aio: explain why max batch is checked in laio_io_unplug()
> 
>  block/linux-aio.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> -- 
> 2.36.1
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [PATCH] hw:w25p80: Add STATE_STANDBY to handle incorrect command

[Dan Zhang]

Hi Cedric,

I am sorry that accidently submit a pre-view code change as a patch using the
git-sendmail. 
I originally mean to copy the following code in email reply and let
commnity get better understand my proposal.

Let me submit a formal patch in seperate thread. And will remove the
code using this STATE_STANDBY state, as those code shall be in @iris WP#
patch.

BRs
Dan

On Tue, Jun 14, 2022 at 09:02:46AM -0700, Dan Zhang wrote:
> ---
>  hw/block/m25p80.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index b6bd430a99..3bb0466dca 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -423,6 +423,7 @@ typedef enum {
>  STATE_COLLECTING_DATA,
>  STATE_COLLECTING_VAR_LEN_DATA,
>  STATE_READING_DATA,
> +STATE_STANDBY,
>  } CMDState;
>  
>  typedef enum {
> @@ -1218,6 +1219,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  || !s->write_enable) {
>  qemu_log_mask(LOG_GUEST_ERROR,
>"M25P80: Status register write is disabled!\n");
> + qemu_log_mask(LOG_GUEST_ERROR,
> +  "M25P80: switch to standby, re-aseert CS to 
> reactivate \n");
> + s->state = STATE_STANDBY;
>  break;
>  }
>  
> @@ -1472,6 +1476,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, 
> uint32_t tx)
>s->cur_addr, (uint8_t)tx);
>  
>  switch (s->state) {
> +case STATE_STANDBY:
> + r = 0x; /* StandBy state SO shall be HiZ */
> + break;
>  
>  case STATE_PAGE_PROGRAM:
>  trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
> -- 
> 2.34.3
> 

Re: [PULL 00/10] Block jobs & NBD patches

[Richard Henderson]

On 6/14/22 03:29, Vladimir Sementsov-Ogievskiy wrote:

The following changes since commit debd0753663bc89c86f5462a53268f2e3f680f60:

   Merge tag 'pull-testing-next-140622-1' of https://github.com/stsquad/qemu 
into staging (2022-06-13 21:10:57 -0700)

are available in the Git repository at:

   https://gitlab.com/vsementsov/qemu.git tags/pull-block-2022-06-14

for you to fetch changes up to 5aef6747a250f545ff53ba7e1a3ed7a3d166011a:

   MAINTAINERS: update Vladimir's address and repositories (2022-06-14 12:51:48 
+0300)


Block jobs & NBD patches

- add new options for copy-before-write filter
- new trace points for NBD
- prefer unsigned type for some 'in_flight' fields
- update my addresses in MAINTAINERS (already in Stefan's tree, but
   I think it's OK to send it with this PULL)


Note also, that I've recently updated my pgp key with new address and
new expire time.
Updated key is here: 
https://keys.openpgp.org/search?q=vsementsov%40yandex-team.ru


This introduces or exposes new timeouts:

https://gitlab.com/qemu-project/qemu/-/pipelines/563590515/failures


r~

[PATCH] hw:m25p80: Add STATE_STANDBY command state

[Dan Zhang]

HW normally will switch it to stand by mode when receive incorrect
command.
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
```
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
```
Add STATE_STANDBY CMDState and let the device ignore all input and keep
SO as HIZ (output 1)

Signed-off-by: Dan Zhang 
---
A usage of this new state can be aborting in HPM checking 
or unknown command code received.

 hw/block/m25p80.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index b6bd430a99..9f89773b11 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -423,6 +423,7 @@ typedef enum {
 STATE_COLLECTING_DATA,
 STATE_COLLECTING_VAR_LEN_DATA,
 STATE_READING_DATA,
+STATE_STANDBY,
 } CMDState;
 
 typedef enum {
@@ -1472,6 +1473,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, 
uint32_t tx)
   s->cur_addr, (uint8_t)tx);
 
 switch (s->state) {
+case STATE_STANDBY:
+r = 0x; /* StandBy state SO shall be HiZ */
+break;
 
 case STATE_PAGE_PROGRAM:
 trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
-- 
2.34.3

Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection

[Dan Zhang]

On Mon, Jun 13, 2022 at 10:45:34PM -0700, Dan Zhang wrote:
> Just find out how to use mutt to reply all in the thread.
> repeat the previous comments. Add STATE_HIZ to handle decode_new_command
> aborting gracefully. 
> 
> On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
> > 
> > 
> > > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias 
> > >  wrote:
> > > 
> > > Hi Iris,
> > > 
> > > Looks good some, a couple of comments below.
> > > 
> > > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> > >> From: Iris Chen 
> > >> 
> > >> Signed-off-by: Iris Chen 
> > >> ---
> > >> Addressed all comments from V1. The biggest change: removed 
> > >> object_class_property_add.
> > >> 
> > >> hw/block/m25p80.c | 37 +++
> > >> tests/qtest/aspeed_smc-test.c |  2 ++
> > >> 2 files changed, 39 insertions(+)
> > >> 
> > >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > >> index 81ba3da4df..1a20bd55d4 100644
> > >> --- a/hw/block/m25p80.c
> > >> +++ b/hw/block/m25p80.c
> > >> @@ -27,12 +27,14 @@
> > >> #include "hw/qdev-properties.h"
> > >> #include "hw/qdev-properties-system.h"
> > >> #include "hw/ssi/ssi.h"
> > >> +#include "hw/irq.h"
> > >> #include "migration/vmstate.h"
> > >> #include "qemu/bitops.h"
> > >> #include "qemu/log.h"
> > >> #include "qemu/module.h"
> > >> #include "qemu/error-report.h"
> > >> #include "qapi/error.h"
> > >> +#include "qapi/visitor.h"
> > >> #include "trace.h"
> > >> #include "qom/object.h"
> > >> 
> > >> @@ -472,11 +474,13 @@ struct Flash {
> > >> uint8_t spansion_cr2v;
> > >> uint8_t spansion_cr3v;
> > >> uint8_t spansion_cr4v;
> > >> +bool wp_level;
> > >> bool write_enable;
> > >> bool four_bytes_address_mode;
> > >> bool reset_enable;
> > >> bool quad_enable;
> > >> bool aai_enable;
> > >> +bool status_register_write_disabled;
> > >> uint8_t ear;
> > >> 
> > >> int64_t dirty_page;
> > >> @@ -723,6 +727,21 @@ static void complete_collecting_data(Flash *s)
> > >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> > >> break;
> > >> case WRSR:
> > >> +/*
> > >> + * If WP# is low and status_register_write_disabled is high,
> > >> + * status register writes are disabled.
> > >> + * This is also called "hardware protected mode" (HPM). All 
> > >> other
> > >> + * combinations of the two states are called "software 
> > >> protected mode"
> > >> + * (SPM), and status register writes are permitted.
> > >> + */
> > >> +if ((s->wp_level == 0 && s->status_register_write_disabled)
> > >> +|| !s->write_enable) {
> > > 
> > > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > > command, otherwise the state machinery will not advance to this function
> > > (meaning that above check for !s->write_enable will never hit as far as I 
> > > can
> > > tell). A suggestion is to move the check for wp_level and
> > > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
> > 
> > Oh good catch! Yes actually, in our fork, we also removed the write_enable
> > guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> > or both checks in complete_collecting_data.
> > 
> > I think we had some difficulty deciding whether to block command decoding,
> > or to decode and ignore the command if restrictions are enabled.
> > 
> > The reason being that, in the qtest, the WRSR command code gets ignored, and
> > then the subsequent write data gets interpreted as some random command code.
> > We had elected to decode and ignore the command, but I think the
> > datasheet actually describes that the command won’t be decoded successfully,
> > so you’re probably right, we should put this logic in decode_new_cmd.
> > 
> > Most likely, the qtest will also need to be modified to reset the transfer
> > state machine after a blocked write command. I can’t remember if
> > exiting and re-entering user mode is sufficient for that, but something
> > like that is probably possible.
> > 
> > Thanks for catching this!
> > Peter
> > 
> 
> I am proposing add a CMDState: STATE_HIZ to handle command decode fail
> situation. When decode_new_command need abort the decoding and ignore
> following
> on input bytes of this transaction, set the state to STATE_HIZ.
> And m25p80_transfer8() will ignore all the following on byte when in
> this state.
> 
> This is to simulating the real device operation behavior
> i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
> ```
> 2. When an incorrect command is written to this device, it enters
> standby mode and stays in standby mode until the next CS# falling edge.
> In standby mode, This device's SO pin should be High-Z.
> ``` 
If don't want to consider WRSR command when HPM activated is "incorrect
command" and enter into standby mode, then according to data sheet in WRSR 
section
```
The WRSR instruction cannot be executed

Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection

[Dan Zhang]

Just find out how to use mutt to reply all in the thread.
repeat the previous comments. Add STATE_HIZ to handle decode_new_command
aborting gracefully. 

On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
> 
> 
> > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias  
> > wrote:
> > 
> > Hi Iris,
> > 
> > Looks good some, a couple of comments below.
> > 
> > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> >> From: Iris Chen 
> >> 
> >> Signed-off-by: Iris Chen 
> >> ---
> >> Addressed all comments from V1. The biggest change: removed 
> >> object_class_property_add.
> >> 
> >> hw/block/m25p80.c | 37 +++
> >> tests/qtest/aspeed_smc-test.c |  2 ++
> >> 2 files changed, 39 insertions(+)
> >> 
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 81ba3da4df..1a20bd55d4 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -27,12 +27,14 @@
> >> #include "hw/qdev-properties.h"
> >> #include "hw/qdev-properties-system.h"
> >> #include "hw/ssi/ssi.h"
> >> +#include "hw/irq.h"
> >> #include "migration/vmstate.h"
> >> #include "qemu/bitops.h"
> >> #include "qemu/log.h"
> >> #include "qemu/module.h"
> >> #include "qemu/error-report.h"
> >> #include "qapi/error.h"
> >> +#include "qapi/visitor.h"
> >> #include "trace.h"
> >> #include "qom/object.h"
> >> 
> >> @@ -472,11 +474,13 @@ struct Flash {
> >> uint8_t spansion_cr2v;
> >> uint8_t spansion_cr3v;
> >> uint8_t spansion_cr4v;
> >> +bool wp_level;
> >> bool write_enable;
> >> bool four_bytes_address_mode;
> >> bool reset_enable;
> >> bool quad_enable;
> >> bool aai_enable;
> >> +bool status_register_write_disabled;
> >> uint8_t ear;
> >> 
> >> int64_t dirty_page;
> >> @@ -723,6 +727,21 @@ static void complete_collecting_data(Flash *s)
> >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >> break;
> >> case WRSR:
> >> +/*
> >> + * If WP# is low and status_register_write_disabled is high,
> >> + * status register writes are disabled.
> >> + * This is also called "hardware protected mode" (HPM). All other
> >> + * combinations of the two states are called "software protected 
> >> mode"
> >> + * (SPM), and status register writes are permitted.
> >> + */
> >> +if ((s->wp_level == 0 && s->status_register_write_disabled)
> >> +|| !s->write_enable) {
> > 
> > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > command, otherwise the state machinery will not advance to this function
> > (meaning that above check for !s->write_enable will never hit as far as I 
> > can
> > tell). A suggestion is to move the check for wp_level and
> > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
> 
> Oh good catch! Yes actually, in our fork, we also removed the write_enable
> guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> or both checks in complete_collecting_data.
> 
> I think we had some difficulty deciding whether to block command decoding,
> or to decode and ignore the command if restrictions are enabled.
> 
> The reason being that, in the qtest, the WRSR command code gets ignored, and
> then the subsequent write data gets interpreted as some random command code.
> We had elected to decode and ignore the command, but I think the
> datasheet actually describes that the command won’t be decoded successfully,
> so you’re probably right, we should put this logic in decode_new_cmd.
> 
> Most likely, the qtest will also need to be modified to reset the transfer
> state machine after a blocked write command. I can’t remember if
> exiting and re-entering user mode is sufficient for that, but something
> like that is probably possible.
> 
> Thanks for catching this!
> Peter
> 

I am proposing add a CMDState: STATE_HIZ to handle command decode fail
situation. When decode_new_command need abort the decoding and ignore
following
on input bytes of this transaction, set the state to STATE_HIZ.
And m25p80_transfer8() will ignore all the following on byte when in
this state.

This is to simulating the real device operation behavior
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
```
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
``` 
BRs
Dan Zhang
> > 
> >> +qemu_log_mask(LOG_GUEST_ERROR,
> >> +  "M25P80: Status register write is disabled!\n");
> >> +break;
> >> +}
> >> +s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> >> +
> >> switch (get_man(s)) {
> >> case MAN_SPANSION:
> >> s->quad_enable = !!(s->data[1] & 0x02);
> >> @@ -1195,6 +1214,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> >> 
> >> case 

Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection

[Cédric Le Goater]

Hello Dan

On 6/14/22 07:45, Dan Zhang wrote:

Just find out how to use mutt to reply all in the thread.
repeat the previous comments. Add STATE_HIZ to handle decode_new_command
aborting gracefully.

On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:




On Jun 9, 2022, at 12:22 PM, Francisco Iglesias  
wrote:

Hi Iris,

Looks good some, a couple of comments below.

On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:

From: Iris Chen 

Signed-off-by: Iris Chen 
---
Addressed all comments from V1. The biggest change: removed 
object_class_property_add.

hw/block/m25p80.c | 37 +++
tests/qtest/aspeed_smc-test.c |  2 ++
2 files changed, 39 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 81ba3da4df..1a20bd55d4 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -27,12 +27,14 @@
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
#include "hw/ssi/ssi.h"
+#include "hw/irq.h"
#include "migration/vmstate.h"
#include "qemu/bitops.h"
#include "qemu/log.h"
#include "qemu/module.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "qapi/visitor.h"
#include "trace.h"
#include "qom/object.h"

@@ -472,11 +474,13 @@ struct Flash {
 uint8_t spansion_cr2v;
 uint8_t spansion_cr3v;
 uint8_t spansion_cr4v;
+bool wp_level;
 bool write_enable;
 bool four_bytes_address_mode;
 bool reset_enable;
 bool quad_enable;
 bool aai_enable;
+bool status_register_write_disabled;
 uint8_t ear;

 int64_t dirty_page;
@@ -723,6 +727,21 @@ static void complete_collecting_data(Flash *s)
 flash_erase(s, s->cur_addr, s->cmd_in_progress);
 break;
 case WRSR:
+/*
+ * If WP# is low and status_register_write_disabled is high,
+ * status register writes are disabled.
+ * This is also called "hardware protected mode" (HPM). All other
+ * combinations of the two states are called "software protected mode"
+ * (SPM), and status register writes are permitted.
+ */
+if ((s->wp_level == 0 && s->status_register_write_disabled)
+|| !s->write_enable) {


'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
command, otherwise the state machinery will not advance to this function
(meaning that above check for !s->write_enable will never hit as far as I can
tell). A suggestion is to move the check for wp_level and
status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.


Oh good catch! Yes actually, in our fork, we also removed the write_enable
guard in decode_new_cmd. We either need both checks in decode_new_cmd,
or both checks in complete_collecting_data.

I think we had some difficulty deciding whether to block command decoding,
or to decode and ignore the command if restrictions are enabled.

The reason being that, in the qtest, the WRSR command code gets ignored, and
then the subsequent write data gets interpreted as some random command code.
We had elected to decode and ignore the command, but I think the
datasheet actually describes that the command won’t be decoded successfully,
so you’re probably right, we should put this logic in decode_new_cmd.

Most likely, the qtest will also need to be modified to reset the transfer
state machine after a blocked write command. I can’t remember if
exiting and re-entering user mode is sufficient for that, but something
like that is probably possible.

Thanks for catching this!
Peter



I am proposing add a CMDState: STATE_HIZ to handle command decode fail
situation. When decode_new_command need abort the decoding and ignore
following
on input bytes of this transaction, set the state to STATE_HIZ.
And m25p80_transfer8() will ignore all the following on byte when in
this state.

This is to simulating the real device operation behavior
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
```
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
```


Could you please send a patch ?

Thanks,

C.


BRs
Dan Zhang



+qemu_log_mask(LOG_GUEST_ERROR,
+  "M25P80: Status register write is disabled!\n");
+break;
+}
+s->status_register_write_disabled = extract32(s->data[0], 7, 1);
+
 switch (get_man(s)) {
 case MAN_SPANSION:
 s->quad_enable = !!(s->data[1] & 0x02);
@@ -1195,6 +1214,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)

 case RDSR:
 s->data[0] = (!!s->write_enable) << 1;
+s->data[0] |= (!!s->status_register_write_disabled) << 7;
+
 if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
 s->data[0] |= (!!s->quad_enable) << 6;
 }
@@ -1484,6 +1505,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, 
uint32_t tx)

Re: [PATCH v2 1/1] nbd: trace long NBD operations

[Vladimir Sementsov-Ogievskiy]

On 5/30/22 13:39, Denis V. Lunev wrote:

At the moment there are 2 sources of lengthy operations if configured:
* open connection, which could retry inside and
* reconnect of already opened connection
These operations could be quite lengthy and cumbersome to catch thus
it would be quite natural to add trace points for them.

This patch is based on the original downstream work made by Vladimir.

Signed-off-by: Denis V. Lunev
CC: Eric Blake
CC: Vladimir Sementsov-Ogievskiy
CC: Kevin Wolf
CC: Hanna Reitz
CC: Paolo Bonzini


Thanks, applied to my block branch at 
https://gitlab.com/vsementsov/qemu/-/commits/block

--
Best regards,
Vladimir

Re: [PATCH 1/1] block: use 'unsigned' for in_flight field on driver state

[Vladimir Sementsov-Ogievskiy]

On 5/30/22 13:39, Denis V. Lunev wrote:

This patch makes in_flight field 'unsigned' for BDRVNBDState and
MirrorBlockJob. This matches the definition of this field on BDS
and is generically correct - we should never get negative value here.

Signed-off-by: Denis V. Lunev
CC: John Snow
CC: Vladimir Sementsov-Ogievskiy
CC: Kevin Wolf
CC: Hanna Reitz
CC: Eric Blake



Thanks, applied to my block branch at 
https://gitlab.com/vsementsov/qemu/-/commits/block

--
Best regards,
Vladimir

Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Daniel P . Berrangé]

On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> On 14/06/2022 03.50, John Snow wrote:
> > In certain container environments we may not have FUSE at all, so skip
> > the test in this circumstance too.
> > 
> > Signed-off-by: John Snow 
> > ---
> >   tests/qemu-iotests/108 | 6 ++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > index 9e923d6a59f..e401c5e9933 100755
> > --- a/tests/qemu-iotests/108
> > +++ b/tests/qemu-iotests/108
> > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> >   else
> >   loopdev=false
> > +# Check for fuse support in the host environment:
> > +lsmod | grep fuse &>/dev/null;
> 
> That doesn't work if fuse has been linked statically into the kernel. Would
> it make sense to test for /sys/fs/fuse instead?
> 
> (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> so it might not matter too much)

But more importantly 'lsmod' may not be installed in our container
images. So checking /sys/fs/fuse avoids introducing a dep on the
'kmod' package.

> 
> > +if [[ $? -ne 0 ]]; then
> 
> I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> anyway, it likely doesn't matter.

Or

if  test $? != 0 ; then

> 
> > +_notrun 'No Passwordless sudo nor FUSE kernel module'
> > +fi
> > +
> >   # QSD --export fuse will either yield "Parameter 'id' is missing"
> >   # or "Invalid parameter 'fuse'", depending on whether there is
> >   # FUSE support or not.
> 
>  Thomas
> 

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 1/5] tests/qemu-iotests: hotfix for 307, 223 output

[Daniel P . Berrangé]

On Mon, Jun 13, 2022 at 09:50:40PM -0400, John Snow wrote:
> Fixes: 58a6fdcc

CC'ing Eric given the above commit hash

> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/223.out | 4 ++--
>  tests/qemu-iotests/307.out | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

Tested-by: Daniel P. Berrangé 
Reviewed-by: Daniel P. Berrangé 

> 
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index 06479415312..26fb347c5da 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -93,7 +93,7 @@ exports available: 3
>   export: 'n2'
>description: some text
>size:  4194304
> -  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> +  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
>min block: 1
>opt block: 4096
>max block: 33554432
> @@ -212,7 +212,7 @@ exports available: 3
>   export: 'n2'
>description: some text
>size:  4194304
> -  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> +  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
>min block: 1
>opt block: 4096
>max block: 33554432
> diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
> index ec8d2be0e0a..390f05d1b78 100644
> --- a/tests/qemu-iotests/307.out
> +++ b/tests/qemu-iotests/307.out
> @@ -83,7 +83,7 @@ exports available: 2
>   export: 'export1'
>description: This is the writable second export
>size:  67108864
> -  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> +  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
>min block: XXX
>opt block: XXX
>max block: XXX
> @@ -109,7 +109,7 @@ exports available: 1
>   export: 'export1'
>description: This is the writable second export
>size:  67108864
> -  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
> +  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
>min block: XXX
>opt block: XXX
>max block: XXX
> -- 
> 2.34.3
> 

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] tests/vm: switch CentOS 8 to CentOS 8 Stream

[Daniel P . Berrangé]

On Mon, Jun 13, 2022 at 09:50:43PM -0400, John Snow wrote:
> The old CentOS image didn't work anymore because it was already EOL at
> the beginning of 2022.
> 
> Signed-off-by: John Snow 
> ---
>  tests/vm/centos | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/vm/centos b/tests/vm/centos
> index be4f6ff2f14..f5bbdecf62d 100755
> --- a/tests/vm/centos
> +++ b/tests/vm/centos
> @@ -1,8 +1,8 @@
>  #!/usr/bin/env python3
>  #
> -# CentOS image
> +# CentOS 8 Stream image
>  #
> -# Copyright 2018 Red Hat Inc.
> +# Copyright 2018, 2022 Red Hat Inc.
>  #
>  # Authors:
>  #  Fam Zheng 
> @@ -18,7 +18,7 @@ import basevm
>  import time
>  
>  class CentosVM(basevm.BaseVM):
> -name = "centos"
> +name = "centos8s"


What's the effect of this ?  It feels a little odd to set name to 'centos8s'
here but have this file still called just 'centos' - I assume the 'name'
variable was intended to always match the filename

>  arch = "x86_64"
>  BUILD_SCRIPT = """
>  set -e;
> @@ -32,7 +32,7 @@ class CentosVM(basevm.BaseVM):
>  """
>  
>  def build_image(self, img):
> -cimg = 
> self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2";)
> +cimg = 
> self._download_with_cache("https://cloud.centos.org/centos/8-stream/x86_64/images/CentOS-Stream-GenericCloud-8-20220125.1.x86_64.qcow2";)
>  img_tmp = img + ".tmp"
>  subprocess.check_call(['cp', '-f', cimg, img_tmp])
>  self.exec_qemu_img("resize", img_tmp, "50G")


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[PULL 02/10] block/copy-before-write: add on-cbw-error open parameter

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

Currently, behavior on copy-before-write operation failure is simple:
report error to the guest.

Let's implement alternative behavior: break the whole copy-before-write
process (and corresponding backup job or NBD client) but keep guest
working. It's needed if we consider guest stability as more important.

The realisation is simple: on copy-before-write failure we set
s->snapshot_ret and continue guest operations. s->snapshot_ret being
set will lead to all further snapshot API requests. Note that all
in-flight snapshot-API requests may still success: we do wait for them
on BREAK_SNAPSHOT-failure path in cbw_do_copy_before_write().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Hanna Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-before-write.c | 32 ++--
 qapi/block-core.json  | 25 -
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index e29c46cd7a..c8a11a09d2 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -41,6 +41,7 @@
 typedef struct BDRVCopyBeforeWriteState {
 BlockCopyState *bcs;
 BdrvChild *target;
+OnCbwError on_cbw_error;
 
 /*
  * @lock: protects access to @access_bitmap, @done_bitmap and
@@ -65,6 +66,14 @@ typedef struct BDRVCopyBeforeWriteState {
  * node. These areas must not be rewritten by guest.
  */
 BlockReqList frozen_read_reqs;
+
+/*
+ * @snapshot_error is normally zero. But on first copy-before-write failure
+ * when @on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT, @snapshot_error takes
+ * value of this error (<0). After that all in-flight and further
+ * snapshot-API requests will fail with that error.
+ */
+int snapshot_error;
 } BDRVCopyBeforeWriteState;
 
 static coroutine_fn int cbw_co_preadv(
@@ -95,16 +104,27 @@ static coroutine_fn int 
cbw_do_copy_before_write(BlockDriverState *bs,
 return 0;
 }
 
+if (s->snapshot_error) {
+return 0;
+}
+
 off = QEMU_ALIGN_DOWN(offset, cluster_size);
 end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
 
 ret = block_copy(s->bcs, off, end - off, true);
-if (ret < 0) {
+if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
 return ret;
 }
 
 WITH_QEMU_LOCK_GUARD(&s->lock) {
-bdrv_set_dirty_bitmap(s->done_bitmap, off, end - off);
+if (ret < 0) {
+assert(s->on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT);
+if (!s->snapshot_error) {
+s->snapshot_error = ret;
+}
+} else {
+bdrv_set_dirty_bitmap(s->done_bitmap, off, end - off);
+}
 reqlist_wait_all(&s->frozen_read_reqs, off, end - off, &s->lock);
 }
 
@@ -176,6 +196,11 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState 
*bs,
 
 QEMU_LOCK_GUARD(&s->lock);
 
+if (s->snapshot_error) {
+g_free(req);
+return NULL;
+}
+
 if (bdrv_dirty_bitmap_next_zero(s->access_bitmap, offset, bytes) != -1) {
 g_free(req);
 return NULL;
@@ -351,6 +376,7 @@ static BlockdevOptions *cbw_parse_options(QDict *options, 
Error **errp)
  * object for original options.
  */
 qdict_extract_subqdict(options, NULL, "bitmap");
+qdict_del(options, "on-cbw-error");
 
 out:
 visit_free(v);
@@ -395,6 +421,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, 
int flags,
 return -EINVAL;
 }
 }
+s->on_cbw_error = opts->has_on_cbw_error ? opts->on_cbw_error :
+ON_CBW_ERROR_BREAK_GUEST_WRITE;
 
 bs->total_sectors = bs->file->bs->total_sectors;
 bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f0383c7925..4abf26b42d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4155,6 +4155,25 @@
   'base': 'BlockdevOptionsGenericFormat',
   'data': { '*bottom': 'str' } }
 
+##
+# @OnCbwError:
+#
+# An enumeration of possible behaviors for copy-before-write operation
+# failures.
+#
+# @break-guest-write: report the error to the guest. This way, the guest
+# will not be able to overwrite areas that cannot be
+# backed up, so the backup process remains valid.
+#
+# @break-snapshot: continue guest write. Doing so will make the provided
+#  snapshot state invalid and any backup or export
+#  process based on it will finally fail.
+#
+# Since: 7.1
+##
+{ 'enum': 'OnCbwError',
+  'data': [ 'break-guest-write', 'break-snapshot' ] }
+
 ##
 # @BlockdevOptionsCbw:
 #
@@ -4176,11 +4195,15 @@
 #  modifications (or removing) of specified bitmap doesn't
 #  influence the filter. (Since 7.0)
 #
+# @on-cbw-error: Behavior on failure of copy-before-write operation.
+#Default is @break-guest-write. (Sin

[PULL 01/10] block/copy-before-write: refactor option parsing

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

We are going to add one more option of enum type. Let's refactor option
parsing so that we can simply work with BlockdevOptionsCbw object.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Hanna Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-before-write.c | 56 ---
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index a8a06fdc09..e29c46cd7a 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/qmp/qjson.h"
 
 #include "sysemu/block-backend.h"
 #include "qemu/cutils.h"
@@ -328,46 +329,34 @@ static void cbw_child_perm(BlockDriverState *bs, 
BdrvChild *c,
 }
 }
 
-static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
-Error **errp)
+static BlockdevOptions *cbw_parse_options(QDict *options, Error **errp)
 {
-QDict *bitmap_qdict = NULL;
-BlockDirtyBitmap *bmp_param = NULL;
+BlockdevOptions *opts = NULL;
 Visitor *v = NULL;
-bool ret = false;
 
-*bitmap = NULL;
+qdict_put_str(options, "driver", "copy-before-write");
 
-qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
-if (!qdict_size(bitmap_qdict)) {
-ret = true;
-goto out;
-}
-
-v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
+v = qobject_input_visitor_new_flat_confused(options, errp);
 if (!v) {
 goto out;
 }
 
-visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp);
-if (!bmp_param) {
+visit_type_BlockdevOptions(v, NULL, &opts, errp);
+if (!opts) {
 goto out;
 }
 
-*bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
-errp);
-if (!*bitmap) {
-goto out;
-}
-
-ret = true;
+/*
+ * Delete options which we are going to parse through BlockdevOptions
+ * object for original options.
+ */
+qdict_extract_subqdict(options, NULL, "bitmap");
 
 out:
-qapi_free_BlockDirtyBitmap(bmp_param);
 visit_free(v);
-qobject_unref(bitmap_qdict);
+qdict_del(options, "driver");
 
-return ret;
+return opts;
 }
 
 static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
@@ -376,6 +365,15 @@ static int cbw_open(BlockDriverState *bs, QDict *options, 
int flags,
 BDRVCopyBeforeWriteState *s = bs->opaque;
 BdrvDirtyBitmap *bitmap = NULL;
 int64_t cluster_size;
+g_autoptr(BlockdevOptions) full_opts = NULL;
+BlockdevOptionsCbw *opts;
+
+full_opts = cbw_parse_options(options, errp);
+if (!full_opts) {
+return -EINVAL;
+}
+assert(full_opts->driver == BLOCKDEV_DRIVER_COPY_BEFORE_WRITE);
+opts = &full_opts->u.copy_before_write;
 
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -390,8 +388,12 @@ static int cbw_open(BlockDriverState *bs, QDict *options, 
int flags,
 return -EINVAL;
 }
 
-if (!cbw_parse_bitmap_option(options, &bitmap, errp)) {
-return -EINVAL;
+if (opts->has_bitmap) {
+bitmap = block_dirty_bitmap_lookup(opts->bitmap->node,
+   opts->bitmap->name, NULL, errp);
+if (!bitmap) {
+return -EINVAL;
+}
 }
 
 bs->total_sectors = bs->file->bs->total_sectors;
-- 
2.25.1

[PULL 00/10] Block jobs & NBD patches

[Vladimir Sementsov-Ogievskiy]

The following changes since commit debd0753663bc89c86f5462a53268f2e3f680f60:

  Merge tag 'pull-testing-next-140622-1' of https://github.com/stsquad/qemu 
into staging (2022-06-13 21:10:57 -0700)

are available in the Git repository at:

  https://gitlab.com/vsementsov/qemu.git tags/pull-block-2022-06-14

for you to fetch changes up to 5aef6747a250f545ff53ba7e1a3ed7a3d166011a:

  MAINTAINERS: update Vladimir's address and repositories (2022-06-14 12:51:48 
+0300)


Block jobs & NBD patches

- add new options for copy-before-write filter
- new trace points for NBD
- prefer unsigned type for some 'in_flight' fields
- update my addresses in MAINTAINERS (already in Stefan's tree, but
  I think it's OK to send it with this PULL)


Note also, that I've recently updated my pgp key with new address and
new expire time.
Updated key is here: 
https://keys.openpgp.org/search?q=vsementsov%40yandex-team.ru



Denis V. Lunev (2):
  nbd: trace long NBD operations
  block: use 'unsigned' for in_flight field on driver state

Vladimir Sementsov-Ogievskiy (8):
  block/copy-before-write: refactor option parsing
  block/copy-before-write: add on-cbw-error open parameter
  iotests: add copy-before-write: on-cbw-error tests
  util: add qemu-co-timeout
  block/block-copy: block_copy(): add timeout_ns parameter
  block/copy-before-write: implement cbw-timeout option
  iotests: copy-before-write: add cases for cbw-timeout option
  MAINTAINERS: update Vladimir's address and repositories

 MAINTAINERS   |  22 +-
 block/block-copy.c|  33 ++-
 block/copy-before-write.c | 111 ++---
 block/mirror.c|   2 +-
 block/nbd.c   |   8 +-
 block/trace-events|   2 +
 include/block/block-copy.h|   4 +-
 include/qemu/coroutine.h  |  13 ++
 nbd/client-connection.c   |   2 +
 nbd/trace-events  |   3 +
 qapi/block-core.json  |  31 ++-
 tests/qemu-iotests/pylintrc   |   5 +
 tests/qemu-iotests/tests/copy-before-write| 213 ++
 .../qemu-iotests/tests/copy-before-write.out  |   5 +
 util/meson.build  |   1 +
 util/qemu-co-timeout.c|  89 
 16 files changed, 492 insertions(+), 52 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/copy-before-write
 create mode 100644 tests/qemu-iotests/tests/copy-before-write.out
 create mode 100644 util/qemu-co-timeout.c

-- 
2.25.1

[PULL 04/10] util: add qemu-co-timeout

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

Add new API, to make a time limited call of the coroutine.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Hanna Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/qemu/coroutine.h | 13 ++
 util/meson.build |  1 +
 util/qemu-co-timeout.c   | 89 
 3 files changed, 103 insertions(+)
 create mode 100644 util/qemu-co-timeout.c

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index d1548d5b11..08c5bb3c76 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -331,6 +331,19 @@ static inline void coroutine_fn 
qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
 qemu_co_sleep_ns_wakeable(&w, type, ns);
 }
 
+typedef void CleanupFunc(void *opaque);
+/**
+ * Run entry in a coroutine and start timer. Wait for entry to finish or for
+ * timer to elapse, what happen first. If entry finished, return 0, if timer
+ * elapsed earlier, return -ETIMEDOUT.
+ *
+ * Be careful, entry execution is not canceled, user should handle it somehow.
+ * If @clean is provided, it's called after coroutine finish if timeout
+ * happened.
+ */
+int coroutine_fn qemu_co_timeout(CoroutineEntry *entry, void *opaque,
+ uint64_t timeout_ns, CleanupFunc clean);
+
 /**
  * Wake a coroutine if it is sleeping in qemu_co_sleep_ns. The timer will be
  * deleted. @sleep_state must be the variable whose address was given to
diff --git a/util/meson.build b/util/meson.build
index 8f16018cd4..9abd2f5bcc 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -85,6 +85,7 @@ if have_block
   util_ss.add(files('block-helpers.c'))
   util_ss.add(files('qemu-coroutine-sleep.c'))
   util_ss.add(files('qemu-co-shared-resource.c'))
+  util_ss.add(files('qemu-co-timeout.c'))
   util_ss.add(files('thread-pool.c', 'qemu-timer.c'))
   util_ss.add(files('readline.c'))
   util_ss.add(files('throttle.c'))
diff --git a/util/qemu-co-timeout.c b/util/qemu-co-timeout.c
new file mode 100644
index 00..00cd335649
--- /dev/null
+++ b/util/qemu-co-timeout.c
@@ -0,0 +1,89 @@
+/*
+ * Helper functionality for distributing a fixed total amount of
+ * an abstract resource among multiple coroutines.
+ *
+ * Copyright (c) 2022 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/coroutine.h"
+#include "block/aio.h"
+
+typedef struct QemuCoTimeoutState {
+CoroutineEntry *entry;
+void *opaque;
+QemuCoSleep sleep_state;
+bool marker;
+CleanupFunc *clean;
+} QemuCoTimeoutState;
+
+static void coroutine_fn qemu_co_timeout_entry(void *opaque)
+{
+QemuCoTimeoutState *s = opaque;
+
+s->entry(s->opaque);
+
+if (s->marker) {
+assert(!s->sleep_state.to_wake);
+/* .marker set by qemu_co_timeout, it have been failed */
+if (s->clean) {
+s->clean(s->opaque);
+}
+g_free(s);
+} else {
+s->marker = true;
+qemu_co_sleep_wake(&s->sleep_state);
+}
+}
+
+int coroutine_fn qemu_co_timeout(CoroutineEntry *entry, void *opaque,
+ uint64_t timeout_ns, CleanupFunc clean)
+{
+QemuCoTimeoutState *s;
+Coroutine *co;
+
+if (timeout_ns == 0) {
+entry(opaque);
+return 0;
+}
+
+s = g_new(QemuCoTimeoutState, 1);
+*s = (QemuCoTimeoutState) {
+.entry = entry,
+.opaque = opaque,
+.clean = clean
+};
+
+co = qemu_coroutine_create(qemu_co_timeout_entry, s);
+
+aio_co_enter(qemu_get_current_aio_context(), co);
+qemu_co_sleep_ns_wakeable(&s->sleep_state, QEMU_CLOCK_REALTIME, 
timeout_ns);
+
+if (s->marker) {
+/* .marker set by qemu_co_timeout_entry, success */
+g_free(s);
+return 0;
+}
+
+/* Don't free s, as we can't cancel qemu_co_timeout_entry execution */
+s->marker = true;
+return -E

[PULL 07/10] iotests: copy-before-write: add cases for cbw-timeout option

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

Add two simple test-cases: timeout failure with
break-snapshot-on-cbw-error behavior and similar with
break-guest-write-on-cbw-error behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Hanna Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/copy-before-write| 81 +++
 .../qemu-iotests/tests/copy-before-write.out  |  4 +-
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/tests/copy-before-write 
b/tests/qemu-iotests/tests/copy-before-write
index 6c7638965e..f01f26f01c 100755
--- a/tests/qemu-iotests/tests/copy-before-write
+++ b/tests/qemu-iotests/tests/copy-before-write
@@ -126,6 +126,87 @@ read 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 """)
 
+def do_cbw_timeout(self, on_cbw_error):
+result = self.vm.qmp('object-add', {
+'qom-type': 'throttle-group',
+'id': 'group0',
+'limits': {'bps-write': 300 * 1024}
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-add', {
+'node-name': 'cbw',
+'driver': 'copy-before-write',
+'on-cbw-error': on_cbw_error,
+'cbw-timeout': 1,
+'file': {
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': source_img,
+}
+},
+'target': {
+'driver': 'throttle',
+'throttle-group': 'group0',
+'file': {
+'driver': 'qcow2',
+'file': {
+'driver': 'file',
+'filename': temp_img
+}
+}
+}
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-add', {
+'node-name': 'access',
+'driver': 'snapshot-access',
+'file': 'cbw'
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io cbw "write 0 512K"')
+self.assert_qmp(result, 'return', '')
+
+# We need second write to trigger throttling
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io cbw "write 512K 512K"')
+self.assert_qmp(result, 'return', '')
+
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io access "read 0 1M"')
+self.assert_qmp(result, 'return', '')
+
+self.vm.shutdown()
+log = self.vm.get_log()
+log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
+log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
+log = iotests.filter_qemu_io(log)
+return log
+
+def test_timeout_break_guest(self):
+log = self.do_cbw_timeout('break-guest-write')
+self.assertEqual(log, """\
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Connection timed out
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+""")
+
+def test_timeout_break_snapshot(self):
+log = self.do_cbw_timeout('break-snapshot')
+self.assertEqual(log, """\
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read failed: Permission denied
+""")
+
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2'],
diff --git a/tests/qemu-iotests/tests/copy-before-write.out 
b/tests/qemu-iotests/tests/copy-before-write.out
index fbc63e62f8..89968f35d7 100644
--- a/tests/qemu-iotests/tests/copy-before-write.out
+++ b/tests/qemu-iotests/tests/copy-before-write.out
@@ -1,5 +1,5 @@
-..
+
 --
-Ran 2 tests
+Ran 4 tests
 
 OK
-- 
2.25.1

[PULL 03/10] iotests: add copy-before-write: on-cbw-error tests

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

Add tests for new option of copy-before-write filter: on-cbw-error.

Note that we use QEMUMachine instead of VM class, because in further
commit we'll want to use throttling which doesn't work with -accel
qtest used by VM.

We also touch pylintrc to not break iotest 297.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Hanna Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/pylintrc   |   5 +
 tests/qemu-iotests/tests/copy-before-write| 132 ++
 .../qemu-iotests/tests/copy-before-write.out  |   5 +
 3 files changed, 142 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/copy-before-write
 create mode 100644 tests/qemu-iotests/tests/copy-before-write.out

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 32ab77b8bb..f4f823a991 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -51,3 +51,8 @@ notes=FIXME,
 
 # Maximum number of characters on a single line.
 max-line-length=79
+
+
+[SIMILARITIES]
+
+min-similarity-lines=6
diff --git a/tests/qemu-iotests/tests/copy-before-write 
b/tests/qemu-iotests/tests/copy-before-write
new file mode 100755
index 00..6c7638965e
--- /dev/null
+++ b/tests/qemu-iotests/tests/copy-before-write
@@ -0,0 +1,132 @@
+#!/usr/bin/env python3
+# group: auto backup
+#
+# Copyright (c) 2022 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import re
+
+from qemu.machine import QEMUMachine
+
+import iotests
+from iotests import qemu_img_create, qemu_io
+
+
+temp_img = os.path.join(iotests.test_dir, 'temp')
+source_img = os.path.join(iotests.test_dir, 'source')
+size = '1M'
+
+
+class TestCbwError(iotests.QMPTestCase):
+def tearDown(self):
+self.vm.shutdown()
+os.remove(temp_img)
+os.remove(source_img)
+
+def setUp(self):
+qemu_img_create('-f', iotests.imgfmt, source_img, size)
+qemu_img_create('-f', iotests.imgfmt, temp_img, size)
+qemu_io('-c', 'write 0 1M', source_img)
+
+self.vm = QEMUMachine(iotests.qemu_prog)
+self.vm.launch()
+
+def do_cbw_error(self, on_cbw_error):
+result = self.vm.qmp('blockdev-add', {
+'node-name': 'cbw',
+'driver': 'copy-before-write',
+'on-cbw-error': on_cbw_error,
+'file': {
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': source_img,
+}
+},
+'target': {
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'blkdebug',
+'image': {
+'driver': 'file',
+'filename': temp_img
+},
+'inject-error': [
+{
+'event': 'write_aio',
+'errno': 5,
+'immediately': False,
+'once': True
+}
+]
+}
+}
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-add', {
+'node-name': 'access',
+'driver': 'snapshot-access',
+'file': 'cbw'
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io cbw "write 0 1M"')
+self.assert_qmp(result, 'return', '')
+
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io access "read 0 1M"')
+self.assert_qmp(result, 'return', '')
+
+self.vm.shutdown()
+log = self.vm.get_log()
+log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
+log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
+log = iotests.filter_qemu_io(log)
+return log
+
+def test_break_snapshot_on_cbw_error(self):
+"""break-snapshot behavior:
+Guest write succeed, but further snapshot-read fails, as snapshot is
+broken.
+"""
+log = self.do_cbw_error('break-snapshot')
+
+self.assertEqual(log, """\
+wrote 1048576/10485

[PULL 09/10] block: use 'unsigned' for in_flight field on driver state

[Vladimir Sementsov-Ogievskiy]

From: "Denis V. Lunev" 

This patch makes in_flight field 'unsigned' for BDRVNBDState and
MirrorBlockJob. This matches the definition of this field on BDS
and is generically correct - we should never get negative value here.

Signed-off-by: Denis V. Lunev 
CC: John Snow 
CC: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Hanna Reitz 
CC: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/mirror.c | 2 +-
 block/nbd.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index d8ecb9efa2..3c4ab1159d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -73,7 +73,7 @@ typedef struct MirrorBlockJob {
 
 uint64_t last_pause_ns;
 unsigned long *in_flight_bitmap;
-int in_flight;
+unsigned in_flight;
 int64_t bytes_in_flight;
 QTAILQ_HEAD(, MirrorOp) ops_in_flight;
 int ret;
diff --git a/block/nbd.c b/block/nbd.c
index bc8f128087..19e773d602 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -77,7 +77,7 @@ typedef struct BDRVNBDState {
 QemuMutex requests_lock;
 NBDClientState state;
 CoQueue free_sema;
-int in_flight;
+unsigned in_flight;
 NBDClientRequest requests[MAX_NBD_REQUESTS];
 QEMUTimer *reconnect_delay_timer;
 
-- 
2.25.1

[PULL 08/10] nbd: trace long NBD operations

[Vladimir Sementsov-Ogievskiy]

From: "Denis V. Lunev" 

At the moment there are 2 sources of lengthy operations if configured:
* open connection, which could retry inside and
* reconnect of already opened connection
These operations could be quite lengthy and cumbersome to catch thus
it would be quite natural to add trace points for them.

This patch is based on the original downstream work made by Vladimir.

Signed-off-by: Denis V. Lunev 
CC: Eric Blake 
CC: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Hanna Reitz 
CC: Paolo Bonzini 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c | 6 +-
 block/trace-events  | 2 ++
 nbd/client-connection.c | 2 ++
 nbd/trace-events| 3 +++
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c
index 6085ab1d2c..bc8f128087 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -371,6 +371,7 @@ static bool nbd_client_connecting(BDRVNBDState *s)
 /* Called with s->requests_lock taken.  */
 static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
 {
+int ret;
 bool blocking = s->state == NBD_CLIENT_CONNECTING_WAIT;
 
 /*
@@ -380,6 +381,8 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState 
*s)
 assert(nbd_client_connecting(s));
 assert(s->in_flight == 1);
 
+trace_nbd_reconnect_attempt(s->bs->in_flight);
+
 if (blocking && !s->reconnect_delay_timer) {
 /*
  * It's the first reconnect attempt after switching to
@@ -401,7 +404,8 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState 
*s)
 }
 
 qemu_mutex_unlock(&s->requests_lock);
-nbd_co_do_establish_connection(s->bs, blocking, NULL);
+ret = nbd_co_do_establish_connection(s->bs, blocking, NULL);
+trace_nbd_reconnect_attempt_result(ret, s->bs->in_flight);
 qemu_mutex_lock(&s->requests_lock);
 
 /*
diff --git a/block/trace-events b/block/trace-events
index 549090d453..48dbf10c66 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -172,6 +172,8 @@ nbd_read_reply_entry_fail(int ret, const char *err) "ret = 
%d, err: %s"
 nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t 
flags, uint16_t type, const char *name, int ret, const char *err) "Request 
failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags 
= 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"
 nbd_client_handshake(const char *export_name) "export '%s'"
 nbd_client_handshake_success(const char *export_name) "export '%s'"
+nbd_reconnect_attempt(unsigned in_flight) "in_flight %u"
+nbd_reconnect_attempt_result(int ret, unsigned in_flight) "ret %d in_flight %u"
 
 # ssh.c
 ssh_restart_coroutine(void *co) "co=%p"
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 2a632931c3..0c5f917efa 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "trace.h"
 
 #include "block/nbd.h"
 
@@ -210,6 +211,7 @@ static void *connect_thread_func(void *opaque)
 object_unref(OBJECT(conn->sioc));
 conn->sioc = NULL;
 if (conn->do_retry && !conn->detached) {
+trace_nbd_connect_thread_sleep(timeout);
 qemu_mutex_unlock(&conn->mutex);
 
 sleep(timeout);
diff --git a/nbd/trace-events b/nbd/trace-events
index c4919a2dd5..b7032ca277 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -73,3 +73,6 @@ nbd_co_receive_request_decode_type(uint64_t handle, uint16_t 
type, const char *n
 nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) 
"Payload received: handle = %" PRIu64 ", len = %" PRIu32
 nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t len, 
uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" 
PRIx64 ", len=0x%" PRIx32 ", align=0x%" PRIx32
 nbd_trip(void) "Reading request"
+
+# client-connection.c
+nbd_connect_thread_sleep(uint64_t timeout) "timeout %" PRIu64
-- 
2.25.1

[PULL 10/10] MAINTAINERS: update Vladimir's address and repositories

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 MAINTAINERS | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0df25ed4b0..9e37bfe279 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2538,7 +2538,7 @@ F: scsi/*
 
 Block Jobs
 M: John Snow 
-M: Vladimir Sementsov-Ogievskiy 
+M: Vladimir Sementsov-Ogievskiy 
 L: qemu-block@nongnu.org
 S: Supported
 F: blockjob.c
@@ -2563,7 +2563,7 @@ F: block/aio_task.c
 F: util/qemu-co-shared-resource.c
 F: include/qemu/co-shared-resource.h
 T: git https://gitlab.com/jsnow/qemu.git jobs
-T: git https://src.openvz.org/scm/~vsementsov/qemu.git jobs
+T: git https://gitlab.com/vsementsov/qemu.git block
 
 Block QAPI, monitor, command line
 M: Markus Armbruster 
@@ -2584,7 +2584,7 @@ F: include/hw/cxl/
 
 Dirty Bitmaps
 M: Eric Blake 
-M: Vladimir Sementsov-Ogievskiy 
+M: Vladimir Sementsov-Ogievskiy 
 R: John Snow 
 L: qemu-block@nongnu.org
 S: Supported
@@ -2598,6 +2598,7 @@ F: util/hbitmap.c
 F: tests/unit/test-hbitmap.c
 F: docs/interop/bitmaps.rst
 T: git https://repo.or.cz/qemu/ericb.git bitmaps
+T: git https://gitlab.com/vsementsov/qemu.git block
 
 Character device backends
 M: Marc-André Lureau 
@@ -2808,16 +2809,17 @@ F: scripts/*.py
 F: tests/*.py
 
 Benchmark util
-M: Vladimir Sementsov-Ogievskiy 
+M: Vladimir Sementsov-Ogievskiy 
 S: Maintained
 F: scripts/simplebench/
-T: git https://src.openvz.org/scm/~vsementsov/qemu.git simplebench
+T: git https://gitlab.com/vsementsov/qemu.git simplebench
 
 Transactions helper
-M: Vladimir Sementsov-Ogievskiy 
+M: Vladimir Sementsov-Ogievskiy 
 S: Maintained
 F: include/qemu/transactions.h
 F: util/transactions.c
+T: git https://gitlab.com/vsementsov/qemu.git block
 
 QAPI
 M: Markus Armbruster 
@@ -3394,7 +3396,7 @@ F: block/iscsi-opts.c
 
 Network Block Device (NBD)
 M: Eric Blake 
-M: Vladimir Sementsov-Ogievskiy 
+M: Vladimir Sementsov-Ogievskiy 
 L: qemu-block@nongnu.org
 S: Maintained
 F: block/nbd*
@@ -3406,7 +3408,7 @@ F: docs/interop/nbd.txt
 F: docs/tools/qemu-nbd.rst
 F: tests/qemu-iotests/tests/*nbd*
 T: git https://repo.or.cz/qemu/ericb.git nbd
-T: git https://src.openvz.org/scm/~vsementsov/qemu.git nbd
+T: git https://gitlab.com/vsementsov/qemu.git block
 
 NFS
 M: Peter Lieven 
@@ -3491,13 +3493,13 @@ F: block/dmg.c
 parallels
 M: Stefan Hajnoczi 
 M: Denis V. Lunev 
-M: Vladimir Sementsov-Ogievskiy 
+M: Vladimir Sementsov-Ogievskiy 
 L: qemu-block@nongnu.org
 S: Supported
 F: block/parallels.c
 F: block/parallels-ext.c
 F: docs/interop/parallels.txt
-T: git https://src.openvz.org/scm/~vsementsov/qemu.git parallels
+T: git https://gitlab.com/vsementsov/qemu.git block
 
 qed
 M: Stefan Hajnoczi 
-- 
2.25.1

[PULL 05/10] block/block-copy: block_copy(): add timeout_ns parameter

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

Add possibility to limit block_copy() call in time. To be used in the
next commit.

As timed-out block_copy() call will continue in background anyway (we
can't immediately cancel IO operation), it's important also give user a
possibility to pass a callback, to do some additional actions on
block-copy call finish.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Hanna Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/block-copy.c | 33 ++---
 block/copy-before-write.c  |  2 +-
 include/block/block-copy.h |  4 +++-
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index ec46775ea5..bb947afdda 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -883,23 +883,42 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
 return ret;
 }
 
+static void coroutine_fn block_copy_async_co_entry(void *opaque)
+{
+block_copy_common(opaque);
+}
+
 int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
-bool ignore_ratelimit)
+bool ignore_ratelimit, uint64_t timeout_ns,
+BlockCopyAsyncCallbackFunc cb,
+void *cb_opaque)
 {
-BlockCopyCallState call_state = {
+int ret;
+BlockCopyCallState *call_state = g_new(BlockCopyCallState, 1);
+
+*call_state = (BlockCopyCallState) {
 .s = s,
 .offset = start,
 .bytes = bytes,
 .ignore_ratelimit = ignore_ratelimit,
 .max_workers = BLOCK_COPY_MAX_WORKERS,
+.cb = cb,
+.cb_opaque = cb_opaque,
 };
 
-return block_copy_common(&call_state);
-}
+ret = qemu_co_timeout(block_copy_async_co_entry, call_state, timeout_ns,
+  g_free);
+if (ret < 0) {
+assert(ret == -ETIMEDOUT);
+block_copy_call_cancel(call_state);
+/* call_state will be freed by running coroutine. */
+return ret;
+}
 
-static void coroutine_fn block_copy_async_co_entry(void *opaque)
-{
-block_copy_common(opaque);
+ret = call_state->ret;
+g_free(call_state);
+
+return ret;
 }
 
 BlockCopyCallState *block_copy_async(BlockCopyState *s,
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index c8a11a09d2..fc13c7cd44 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -111,7 +111,7 @@ static coroutine_fn int 
cbw_do_copy_before_write(BlockDriverState *bs,
 off = QEMU_ALIGN_DOWN(offset, cluster_size);
 end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
 
-ret = block_copy(s->bcs, off, end - off, true);
+ret = block_copy(s->bcs, off, end - off, true, 0, NULL, NULL);
 if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
 return ret;
 }
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 68bbd344b2..ba0b425d78 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -40,7 +40,9 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s,
  int64_t offset, int64_t *count);
 
 int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
-bool ignore_ratelimit);
+bool ignore_ratelimit, uint64_t timeout_ns,
+BlockCopyAsyncCallbackFunc cb,
+void *cb_opaque);
 
 /*
  * Run block-copy in a coroutine, create corresponding BlockCopyCallState
-- 
2.25.1

[PATCH 0/2] Two sets of trivials

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

I've sent the 3 char set last month, but have updated
it a little; I cleaned up a comment style that was already
broken so checkpatch is happy.

The 'namesapce' is a new patch; it's amazing how many places
make the same typo!

Dave

Dr. David Alan Gilbert (2):
  Trivial: 3 char repeat typos
  trivial typos: namesapce

 hw/9pfs/9p-xattr-user.c  | 8 
 hw/acpi/nvdimm.c | 2 +-
 hw/intc/openpic.c| 2 +-
 hw/net/imx_fec.c | 2 +-
 hw/nvme/ctrl.c   | 2 +-
 hw/pci/pcie_aer.c| 2 +-
 hw/pci/shpc.c| 3 ++-
 hw/ppc/spapr_caps.c  | 2 +-
 hw/scsi/spapr_vscsi.c| 2 +-
 qapi/net.json| 2 +-
 tools/virtiofsd/passthrough_ll.c | 2 +-
 ui/input.c   | 2 +-
 12 files changed, 16 insertions(+), 15 deletions(-)

-- 
2.36.1

[PATCH 1/2] Trivial: 3 char repeat typos

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Inspired by Julia Lawall's fixing of Linux
kernel comments, I looked at qemu, although I did it manually.

Signed-off-by: Dr. David Alan Gilbert 
---
 hw/intc/openpic.c| 2 +-
 hw/net/imx_fec.c | 2 +-
 hw/pci/pcie_aer.c| 2 +-
 hw/pci/shpc.c| 3 ++-
 hw/ppc/spapr_caps.c  | 2 +-
 hw/scsi/spapr_vscsi.c| 2 +-
 qapi/net.json| 2 +-
 tools/virtiofsd/passthrough_ll.c | 2 +-
 ui/input.c   | 2 +-
 9 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 49504e740f..b0787e8ee7 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -729,7 +729,7 @@ static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t 
val, bool enabled)
 }
 
 /*
- * Returns the currrent tccr value, i.e., timer value (in clocks) with
+ * Returns the current tccr value, i.e., timer value (in clocks) with
  * appropriate TOG.
  */
 static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 0db9aaf76a..8c11b237de 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -438,7 +438,7 @@ static void imx_eth_update(IMXFECState *s)
  *   assignment fail.
  *
  * To ensure that all versions of Linux work, generate ENET_INT_MAC
- * interrrupts on both interrupt lines. This should be changed if and when
+ * interrupts on both interrupt lines. This should be changed if and when
  * qemu supports IOMUX.
  */
 if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] &
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 92bd0530dd..eff62f3945 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -323,7 +323,7 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const 
PCIEAERMsg *msg)
  */
 }
 
-/* Errro Message Received: Root Error Status register */
+/* Error Message Received: Root Error Status register */
 switch (msg->severity) {
 case PCI_ERR_ROOT_CMD_COR_EN:
 if (root_status & PCI_ERR_ROOT_COR_RCV) {
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index f822f18b98..e71f3a7483 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -480,7 +480,8 @@ static const MemoryRegionOps shpc_mmio_ops = {
 .endianness = DEVICE_LITTLE_ENDIAN,
 .valid = {
 /* SHPC ECN requires dword accesses, but the original 1.0 spec doesn't.
- * It's easier to suppport all sizes than worry about it. */
+ * It's easier to support all sizes than worry about it.
+ */
 .min_access_size = 1,
 .max_access_size = 4,
 },
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 655ab856a0..b4283055c1 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -553,7 +553,7 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, 
uint8_t val,
  * instruction is a harmless no-op.  It won't correctly
  * implement the cache count flush *but* if we have
  * count-cache-disabled in the host, that flush is
- * unnnecessary.  So, specifically allow this case.  This
+ * unnecessary.  So, specifically allow this case.  This
  * allows us to have better performance on POWER9 DD2.3,
  * while still working on POWER9 DD2.2 and POWER8 host
  * cpus.
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index a07a8e1523..e320ccaa23 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -1013,7 +1013,7 @@ static int vscsi_send_capabilities(VSCSIState *s, 
vscsi_req *req)
 }
 
 /*
- * Current implementation does not suppport any migration or
+ * Current implementation does not support any migration or
  * reservation capabilities. Construct the response telling the
  * guest not to use them.
  */
diff --git a/qapi/net.json b/qapi/net.json
index d6f7cfd4d6..9af11e9a3b 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -298,7 +298,7 @@
 #
 # @udp: use the udp version of l2tpv3 encapsulation
 #
-# @cookie64: use 64 bit coookies
+# @cookie64: use 64 bit cookies
 #
 # @counter: have sequence counter
 #
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index b15c631ca5..7a73dfcce9 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2319,7 +2319,7 @@ static int do_lo_create(fuse_req_t req, struct lo_inode 
*parent_inode,
  * If security.selinux has not been remapped and selinux is enabled,
  * use fscreate to set context before file creation. If not, use
  * tmpfile method for regular files. Otherwise fallback to
- * non-atomic method of file creation and xattr settting.
+ * non-atomic method of file creation and xattr setting.
  */
 if (!mapped_name && lo->use_fscreate) {
 err = do_create_secctx_fscreate(req, parent_inode, name, mode, fi,
diff --git a/ui/i

[PULL 06/10] block/copy-before-write: implement cbw-timeout option

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

In some scenarios, when copy-before-write operations lasts too long
time, it's better to cancel it.

Most useful would be to use the new option together with
on-cbw-error=break-snapshot: this way if cbw operation takes too long
time we'll just cancel backup process but do not disturb the guest too
much.

Note the tricky point of realization: we keep additional point in
bs->in_flight during block_copy operation even if it's timed-out.
Background "cancelled" block_copy operations will finish at some point
and will want to access state. We should care to not free the state in
.bdrv_close() earlier.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Hanna Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-before-write.c | 23 ++-
 qapi/block-core.json  |  8 +++-
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index fc13c7cd44..1bc2e7f9ba 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -42,6 +42,7 @@ typedef struct BDRVCopyBeforeWriteState {
 BlockCopyState *bcs;
 BdrvChild *target;
 OnCbwError on_cbw_error;
+uint32_t cbw_timeout_ns;
 
 /*
  * @lock: protects access to @access_bitmap, @done_bitmap and
@@ -83,6 +84,14 @@ static coroutine_fn int cbw_co_preadv(
 return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
 }
 
+static void block_copy_cb(void *opaque)
+{
+BlockDriverState *bs = opaque;
+
+bs->in_flight--;
+aio_wait_kick();
+}
+
 /*
  * Do copy-before-write operation.
  *
@@ -111,7 +120,16 @@ static coroutine_fn int 
cbw_do_copy_before_write(BlockDriverState *bs,
 off = QEMU_ALIGN_DOWN(offset, cluster_size);
 end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
 
-ret = block_copy(s->bcs, off, end - off, true, 0, NULL, NULL);
+/*
+ * Increase in_flight, so that in case of timed-out block-copy, the
+ * remaining background block_copy() request (which can't be immediately
+ * cancelled by timeout) is presented in bs->in_flight. This way we are
+ * sure that on bs close() we'll previously wait for all timed-out but yet
+ * running block_copy calls.
+ */
+bs->in_flight++;
+ret = block_copy(s->bcs, off, end - off, true, s->cbw_timeout_ns,
+ block_copy_cb, bs);
 if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
 return ret;
 }
@@ -377,6 +395,7 @@ static BlockdevOptions *cbw_parse_options(QDict *options, 
Error **errp)
  */
 qdict_extract_subqdict(options, NULL, "bitmap");
 qdict_del(options, "on-cbw-error");
+qdict_del(options, "cbw-timeout");
 
 out:
 visit_free(v);
@@ -423,6 +442,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 s->on_cbw_error = opts->has_on_cbw_error ? opts->on_cbw_error :
 ON_CBW_ERROR_BREAK_GUEST_WRITE;
+s->cbw_timeout_ns = opts->has_cbw_timeout ?
+opts->cbw_timeout * NANOSECONDS_PER_SECOND : 0;
 
 bs->total_sectors = bs->file->bs->total_sectors;
 bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4abf26b42d..9fc06e7862 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4198,12 +4198,18 @@
 # @on-cbw-error: Behavior on failure of copy-before-write operation.
 #Default is @break-guest-write. (Since 7.1)
 #
+# @cbw-timeout: Zero means no limit. Non-zero sets the timeout in seconds
+#   for copy-before-write operation. When a timeout occurs,
+#   the respective copy-before-write operation will fail, and
+#   the @on-cbw-error parameter will decide how this failure
+#   is handled. Default 0. (Since 7.1)
+#
 # Since: 6.2
 ##
 { 'struct': 'BlockdevOptionsCbw',
   'base': 'BlockdevOptionsGenericFormat',
   'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
-'*on-cbw-error': 'OnCbwError' } }
+'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
 
 ##
 # @BlockdevOptions:
-- 
2.25.1

[PATCH 2/2] trivial typos: namesapce

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

'namespace' is misspelled in a bunch of places.

Signed-off-by: Dr. David Alan Gilbert 
---
 hw/9pfs/9p-xattr-user.c | 8 
 hw/acpi/nvdimm.c| 2 +-
 hw/nvme/ctrl.c  | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index f2ae9582e6..535677ed60 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -27,7 +27,7 @@ static ssize_t mp_user_getxattr(FsContext *ctx, const char 
*path,
 {
 if (strncmp(name, "user.virtfs.", 12) == 0) {
 /*
- * Don't allow fetch of user.virtfs namesapce
+ * Don't allow fetch of user.virtfs namespace
  * in case of mapped security
  */
 errno = ENOATTR;
@@ -49,7 +49,7 @@ static ssize_t mp_user_listxattr(FsContext *ctx, const char 
*path,
 name_size -= 12;
 } else {
 /*
- * Don't allow fetch of user.virtfs namesapce
+ * Don't allow fetch of user.virtfs namespace
  * in case of mapped security
  */
 return 0;
@@ -74,7 +74,7 @@ static int mp_user_setxattr(FsContext *ctx, const char *path, 
const char *name,
 {
 if (strncmp(name, "user.virtfs.", 12) == 0) {
 /*
- * Don't allow fetch of user.virtfs namesapce
+ * Don't allow fetch of user.virtfs namespace
  * in case of mapped security
  */
 errno = EACCES;
@@ -88,7 +88,7 @@ static int mp_user_removexattr(FsContext *ctx,
 {
 if (strncmp(name, "user.virtfs.", 12) == 0) {
 /*
- * Don't allow fetch of user.virtfs namesapce
+ * Don't allow fetch of user.virtfs namespace
  * in case of mapped security
  */
 errno = EACCES;
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 0d43da19ea..5f85b16327 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -476,7 +476,7 @@ struct NvdimmFuncGetLabelDataOut {
 /* the size of buffer filled by QEMU. */
 uint32_t len;
 uint32_t func_ret_status; /* return status code. */
-uint8_t out_buf[]; /* the data got via Get Namesapce Label function. */
+uint8_t out_buf[]; /* the data got via Get Namespace Label function. */
 } QEMU_PACKED;
 typedef struct NvdimmFuncGetLabelDataOut NvdimmFuncGetLabelDataOut;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > NVDIMM_DSM_MEMORY_SIZE);
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 1e6e0fcad9..770a38381a 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -71,7 +71,7 @@
  *   the SUBNQN field in the controller will report the NQN of the subsystem
  *   device. This also enables multi controller capability represented in
  *   Identify Controller data structure in CMIC (Controller Multi-path I/O and
- *   Namesapce Sharing Capabilities).
+ *   Namespace Sharing Capabilities).
  *
  * - `aerl`
  *   The Asynchronous Event Request Limit (AERL). Indicates the maximum number
-- 
2.36.1

Re: [External] [PATCH v13 3/8] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[chuang xu]

On 2022/5/13 下午2:28, Leonardo Bras wrote:

@@ -557,15 +578,31 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
  memcpy(CMSG_DATA(cmsg), fds, fdsize);
  }
  
+#ifdef QEMU_MSG_ZEROCOPY

+if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
+sflags = MSG_ZEROCOPY;
+}
+#endif
+
   retry:
-ret = sendmsg(sioc->fd, &msg, 0);
+ret = sendmsg(sioc->fd, &msg, sflags);
  if (ret <= 0) {
-if (errno == EAGAIN) {
+switch (errno) {
+case EAGAIN:
  return QIO_CHANNEL_ERR_BLOCK;
-}
-if (errno == EINTR) {
+case EINTR:
  goto retry;
+#ifdef QEMU_MSG_ZEROCOPY
+case ENOBUFS:
+if (sflags & MSG_ZEROCOPY) {
+error_setg_errno(errp, errno,
+ "Process can't lock enough memory for using 
MSG_ZEROCOPY");
+return -1;
+}
+break;
+#endif
  }
+
  error_setg_errno(errp, errno,
   "Unable to write to socket");
  return -1;


Hi, Leo.

There are some other questions I would like to discuss with you.

I tested the multifd zero_copy migration and found that sometimes even 
if max locked memory of qemu was set to 16GB（much greater than 
`MULTIFD_PACKET_SIZE`）, the error "Process can't lock enough memory for 
using MSG_ZEROCOPY" would still be reported.


I noticed that the 
doc(https://www.kernel.org/doc/html/v5.12/networking/msg_zerocopy.html) 
says "A zerocopy failure will return -1 with errno ENOBUFS. This happens 
if the socket option was not set, _the socket exceeds its optmem limit_ 
or the user exceeds its ulimit on locked pages."


I also found that the RFC(https://lwn.net/Articles/715279/) says _"__The 
change to allocate notification skbuffs from optmem requires__ensuring 
that net.core.optmem is at least a few 100KB."_


On my host,  optmem was initially set to 20KB, I tried to change it to 
100KB (echo 102400 > /proc/sys/net/core/optmem_max) as the RFC says.Then 
I tested the multifd zero_copy migration repeatedly，and the error 
disappeared.


So when sendmsg returns -1 with errno ENOBUFS, should we distinguish 
between error ''socket exceeds optmem limit" and error "user exceeds 
ulimit on locked pages"? Or is there any better way to avoid this problem?


Best Regards,

chuang xu

Re: [PATCH 1/2] Trivial: 3 char repeat typos

[Daniel Henrique Barboza]

On 6/14/22 07:40, Dr. David Alan Gilbert (git) wrote:

From: "Dr. David Alan Gilbert" 

Inspired by Julia Lawall's fixing of Linux
kernel comments, I looked at qemu, although I did it manually.

Signed-off-by: Dr. David Alan Gilbert 
---


Reviewed-by: Daniel Henrique Barboza 


  hw/intc/openpic.c| 2 +-
  hw/net/imx_fec.c | 2 +-
  hw/pci/pcie_aer.c| 2 +-
  hw/pci/shpc.c| 3 ++-
  hw/ppc/spapr_caps.c  | 2 +-
  hw/scsi/spapr_vscsi.c| 2 +-
  qapi/net.json| 2 +-
  tools/virtiofsd/passthrough_ll.c | 2 +-
  ui/input.c   | 2 +-
  9 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 49504e740f..b0787e8ee7 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -729,7 +729,7 @@ static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t 
val, bool enabled)
  }
  
  /*

- * Returns the currrent tccr value, i.e., timer value (in clocks) with
+ * Returns the current tccr value, i.e., timer value (in clocks) with
   * appropriate TOG.
   */
  static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 0db9aaf76a..8c11b237de 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -438,7 +438,7 @@ static void imx_eth_update(IMXFECState *s)
   *   assignment fail.
   *
   * To ensure that all versions of Linux work, generate ENET_INT_MAC
- * interrrupts on both interrupt lines. This should be changed if and when
+ * interrupts on both interrupt lines. This should be changed if and when
   * qemu supports IOMUX.
   */
  if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] &
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 92bd0530dd..eff62f3945 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -323,7 +323,7 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const 
PCIEAERMsg *msg)
   */
  }
  
-/* Errro Message Received: Root Error Status register */

+/* Error Message Received: Root Error Status register */
  switch (msg->severity) {
  case PCI_ERR_ROOT_CMD_COR_EN:
  if (root_status & PCI_ERR_ROOT_COR_RCV) {
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index f822f18b98..e71f3a7483 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -480,7 +480,8 @@ static const MemoryRegionOps shpc_mmio_ops = {
  .endianness = DEVICE_LITTLE_ENDIAN,
  .valid = {
  /* SHPC ECN requires dword accesses, but the original 1.0 spec 
doesn't.
- * It's easier to suppport all sizes than worry about it. */
+ * It's easier to support all sizes than worry about it.
+ */
  .min_access_size = 1,
  .max_access_size = 4,
  },
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 655ab856a0..b4283055c1 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -553,7 +553,7 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, 
uint8_t val,
   * instruction is a harmless no-op.  It won't correctly
   * implement the cache count flush *but* if we have
   * count-cache-disabled in the host, that flush is
- * unnnecessary.  So, specifically allow this case.  This
+ * unnecessary.  So, specifically allow this case.  This
   * allows us to have better performance on POWER9 DD2.3,
   * while still working on POWER9 DD2.2 and POWER8 host
   * cpus.
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index a07a8e1523..e320ccaa23 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -1013,7 +1013,7 @@ static int vscsi_send_capabilities(VSCSIState *s, 
vscsi_req *req)
  }
  
  /*

- * Current implementation does not suppport any migration or
+ * Current implementation does not support any migration or
   * reservation capabilities. Construct the response telling the
   * guest not to use them.
   */
diff --git a/qapi/net.json b/qapi/net.json
index d6f7cfd4d6..9af11e9a3b 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -298,7 +298,7 @@
  #
  # @udp: use the udp version of l2tpv3 encapsulation
  #
-# @cookie64: use 64 bit coookies
+# @cookie64: use 64 bit cookies
  #
  # @counter: have sequence counter
  #
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index b15c631ca5..7a73dfcce9 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2319,7 +2319,7 @@ static int do_lo_create(fuse_req_t req, struct lo_inode 
*parent_inode,
   * If security.selinux has not been remapped and selinux is enabled,
   * use fscreate to set context before file creation. If not, use
   * tmpfile method for regular files. Otherwise fallback to
- * non-atomic method of file creation and xattr settting.
+ * non-atomic method of file creation and xattr setting.

Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[John Snow]

On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé  wrote:
>
> On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> > On 14/06/2022 03.50, John Snow wrote:
> > > In certain container environments we may not have FUSE at all, so skip
> > > the test in this circumstance too.
> > >
> > > Signed-off-by: John Snow 
> > > ---
> > >   tests/qemu-iotests/108 | 6 ++
> > >   1 file changed, 6 insertions(+)
> > >
> > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > > index 9e923d6a59f..e401c5e9933 100755
> > > --- a/tests/qemu-iotests/108
> > > +++ b/tests/qemu-iotests/108
> > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> > >   else
> > >   loopdev=false
> > > +# Check for fuse support in the host environment:
> > > +lsmod | grep fuse &>/dev/null;
> >
> > That doesn't work if fuse has been linked statically into the kernel. Would
> > it make sense to test for /sys/fs/fuse instead?
> >
> > (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> > so it might not matter too much)
>
> But more importantly 'lsmod' may not be installed in our container
> images. So checking /sys/fs/fuse avoids introducing a dep on the
> 'kmod' package.
>
> >
> > > +if [[ $? -ne 0 ]]; then
> >
> > I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> > anyway, it likely doesn't matter.
>
> Or
>
> if  test $? != 0 ; then
>
> >
> > > +_notrun 'No Passwordless sudo nor FUSE kernel module'
> > > +fi
> > > +
> > >   # QSD --export fuse will either yield "Parameter 'id' is missing"
> > >   # or "Invalid parameter 'fuse'", depending on whether there is
> > >   # FUSE support or not.
> >

Good suggestions, thanks!

--js

Re: [PATCH 3/5] tests/vm: use 'cp' instead of 'ln' for temporary vm images

[John Snow]

On Tue, Jun 14, 2022 at 12:40 AM Thomas Huth  wrote:
>
> On 14/06/2022 03.50, John Snow wrote:
> > If the initial setup fails, you've permanently altered the state of the
> > downloaded image in an unknowable way. Use 'cp' like our other test
> > setup scripts do.
> >
> > Signed-off-by: John Snow 
> > ---
> >   tests/vm/centos | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/vm/centos b/tests/vm/centos
> > index 5c7bc1c1a9a..be4f6ff2f14 100755
> > --- a/tests/vm/centos
> > +++ b/tests/vm/centos
> > @@ -34,7 +34,7 @@ class CentosVM(basevm.BaseVM):
> >   def build_image(self, img):
> >   cimg = 
> > self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2";)
> >   img_tmp = img + ".tmp"
> > -subprocess.check_call(["ln", "-f", cimg, img_tmp])
> > +subprocess.check_call(['cp', '-f', cimg, img_tmp])
>
> I wonder whether it would make sense to use "qemu-img create -b" instead to
> save some disk space?
>
> Anyway, your patch is certainly already an improvement, so:
>
> Reviewed-by: Thomas Huth 

I wondered the same, but decided to keep a smaller series this time
around. VM tests already use a lot of space, so I doubt this is adding
new constraints that didn't exist before. A more rigorous overhaul may
be in order, but not right now. (It looks like the config file stuff
to override defaults is not necessarily rigorously respected by the
different installer recipes.)

I think the caching of the fully set-up image needs work, too. In
practice we leave the image sitting around, but we seem to always
rebuild it no matter what, so it's not that useful. There's a few
things that can be done here to drastically speed up some things,
but... later.

--js

Re: [PATCH 4/5] tests/vm: switch CentOS 8 to CentOS 8 Stream

[John Snow]

On Tue, Jun 14, 2022 at 5:09 AM Daniel P. Berrangé  wrote:
>
> On Mon, Jun 13, 2022 at 09:50:43PM -0400, John Snow wrote:
> > The old CentOS image didn't work anymore because it was already EOL at
> > the beginning of 2022.
> >
> > Signed-off-by: John Snow 
> > ---
> >  tests/vm/centos | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/vm/centos b/tests/vm/centos
> > index be4f6ff2f14..f5bbdecf62d 100755
> > --- a/tests/vm/centos
> > +++ b/tests/vm/centos
> > @@ -1,8 +1,8 @@
> >  #!/usr/bin/env python3
> >  #
> > -# CentOS image
> > +# CentOS 8 Stream image
> >  #
> > -# Copyright 2018 Red Hat Inc.
> > +# Copyright 2018, 2022 Red Hat Inc.
> >  #
> >  # Authors:
> >  #  Fam Zheng 
> > @@ -18,7 +18,7 @@ import basevm
> >  import time
> >
> >  class CentosVM(basevm.BaseVM):
> > -name = "centos"
> > +name = "centos8s"
>
>
> What's the effect of this ?  It feels a little odd to set name to 'centos8s'
> here but have this file still called just 'centos' - I assume the 'name'
> variable was intended to always match the filename
>

Changes the logfile names in ~/.cache/qemu-vm, changes the hostname
config in gen_cloud_init_iso(), not much else.

You're right, though, I shouldn't change it in one place but not the
other ... I'll just leave it as "centos". I felt compelled briefly to
indicate it was "the newer, different CentOS" but with the old one
being EOL I suppose it's easy enough to infer.

--js