Re: [External] [PATCH] hostmem: Add clear option to file backend

[Feiran Zheng]

> On 2 Mar 2023, at 11:54, David Hildenbrand  wrote:
> 
> On 02.03.23 12:48, Feiran Zheng wrote:
>>> On 2 Mar 2023, at 11:39, David Hildenbrand  wrote:
>>> 
>>> On 02.03.23 12:37, Feiran Zheng wrote:
>>>>> On 2 Mar 2023, at 11:31, David Hildenbrand  wrote:
>>>>> 
>>>>> On 02.03.23 12:09, Fam Zheng wrote:
>>>>>> This adds a memset to clear the backing memory. This is useful in the
>>>>>> case of PMEM DAX to drop dirty data, if the backing memory is handed
>>>>>> over from a previous application or firmware which didn't clean up
>>>>>> before exiting.
>>>>> 
>>>>> Why can't the VM manager do that instead? If you have a file that's 
>>>>> certainly easily possible.
>>>> Hi David,
>>>> Technically yes, but I have a simple VM manager here which wants to avoid 
>>>> replicating the same mmap code, such as handling the flags depending on 
>>>> share=on|off,hugepages=on|off. All in all this approach requires the least 
>>>> additional code to achieve it.
>>> 
>>> so ... we're supposed to maintain that code in QEMU instead to make your 
>>> life easier ? :)
>>> 
>>> Sorry, for this particular use case I don't see the big benefit of moving 
>>> that code into QEMU.
>>> 
>> I am posting because this does not only makes my life easier, supposedly it 
>> also make other developers life easier, because the file here can be a char 
>> file and there is no easy way to clear it (/dev/dax1.0) from command line if 
>> you want to invoke a QEMU command directly.
>> Maybe I’m missing a convenient command to clear a DAX char file?
> 
> Can't you simply use dd and read from /dev/zero?
> 

I don’t think it works for dax because the fs driver only implemented mmap, not 
read/write:

# strace -e write dd if=/dev/zero of=/dev/dax1.0 bs=1G count=1
write(1, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
1073741824) = -1 EINVAL (Invalid argument)
write(2, "dd: error writing '/dev/dax1.0':"..., 50dd: error writing 
'/dev/dax1.0': Invalid argument

Fam

Re: [External] [PATCH] hostmem: Add clear option to file backend

[Feiran Zheng]

> On 2 Mar 2023, at 11:44, Daniel P. Berrangé  wrote:
> 
> On Thu, Mar 02, 2023 at 12:31:46PM +0100, David Hildenbrand wrote:
>> On 02.03.23 12:09, Fam Zheng wrote:
>>> This adds a memset to clear the backing memory. This is useful in the
>>> case of PMEM DAX to drop dirty data, if the backing memory is handed
>>> over from a previous application or firmware which didn't clean up
>>> before exiting.
>>> 
>> 
>> Why can't the VM manager do that instead? If you have a file that's
>> certainly easily possible.
> 
> This feels conceptually similar to the case where you expose a host
> block device to the guest. If that block device was previously given
> to a different guest it might still have data in it. Someone needs
> to take responsibility for scrubbing that data. Since that may take
> a non-trivial amount of time, it is typically todo that scrubbing in
> the background after the old VM is gone rather than put it into the
> startup path for a new VM which would delay boot.
> 
> PMEM is blurring the boundary between memory and disk, but the tradeoff
> is not so different. We know that in general merely faulting in guest
> memory is quite time consuming and delays VM startup significantly as
> RAM size increases. Doing the full memset can only be slower still.
> 
> For prealloc we've create complex code to fault in memory across many
> threads and even that's too slow, so we're considering doing it in the
> background as the VM starts up.
> 
> IIUC, this patch just puts the memset in the critical serialized path.
> This will inevitably lead to a demand for improving performance by
> parallelizing across threads, but we know that's too slow already,
> and we cant play the background async game with memset as that's
> actually changunig guest visible contents.
> 
> IOW, for large PMEM sizes, it does look compelling to do the clearing
> of old data in the background outside context of QEMU VM startup to
> avoid delayed startup.
> 
> I can still understand the appeal of a simple flag to set on QEMU from
> a usability POV, but not sure its a good idea to encourage this usage
> by mgmt apps.

I can totally see the reasoning about the latency here, but I’m a little 
dubious if multi-threading for memset can actaully help reduce the start-up 
time; the total cost is going to be bound by memory bandwidth between the CPU 
and memory (even more so if it’s PMEM) which is limited.

Fam

Re: [External] [PATCH] hostmem: Add clear option to file backend

[Feiran Zheng]

> On 2 Mar 2023, at 11:39, David Hildenbrand  wrote:
> 
> On 02.03.23 12:37, Feiran Zheng wrote:
>>> On 2 Mar 2023, at 11:31, David Hildenbrand  wrote:
>>> 
>>> On 02.03.23 12:09, Fam Zheng wrote:
>>>> This adds a memset to clear the backing memory. This is useful in the
>>>> case of PMEM DAX to drop dirty data, if the backing memory is handed
>>>> over from a previous application or firmware which didn't clean up
>>>> before exiting.
>>> 
>>> Why can't the VM manager do that instead? If you have a file that's 
>>> certainly easily possible.
>> Hi David,
>> Technically yes, but I have a simple VM manager here which wants to avoid 
>> replicating the same mmap code, such as handling the flags depending on 
>> share=on|off,hugepages=on|off. All in all this approach requires the least 
>> additional code to achieve it.
> 
> so ... we're supposed to maintain that code in QEMU instead to make your life 
> easier ? :)
> 
> Sorry, for this particular use case I don't see the big benefit of moving 
> that code into QEMU.
> 

I am posting because this does not only makes my life easier, supposedly it 
also make other developers life easier, because the file here can be a char 
file and there is no easy way to clear it (/dev/dax1.0) from command line if 
you want to invoke a QEMU command directly.

Maybe I’m missing a convenient command to clear a DAX char file?

Surely it doesn’t make a big difference either way and I am not worried of 
maintaining the code outside QEMU, but I just think this flag is a nice thing 
in QEMU anyway.



Fam

> -- 
> Thanks,
> 
> David / dhildenb
> 

Re: [External] [PATCH] hostmem: Add clear option to file backend

[Feiran Zheng]

> On 2 Mar 2023, at 11:31, David Hildenbrand  wrote:
> 
> On 02.03.23 12:09, Fam Zheng wrote:
>> This adds a memset to clear the backing memory. This is useful in the
>> case of PMEM DAX to drop dirty data, if the backing memory is handed
>> over from a previous application or firmware which didn't clean up
>> before exiting.
> 
> Why can't the VM manager do that instead? If you have a file that's certainly 
> easily possible.


Hi David,

Technically yes, but I have a simple VM manager here which wants to avoid 
replicating the same mmap code, such as handling the flags depending on 
share=on|off,hugepages=on|off. All in all this approach requires the least 
additional code to achieve it.

Thanks,
Fam

> 
> -- 
> Thanks,
> 
> David / dhildenb
> 

[Qemu-devel] [PATCH] qemu-iotest: test 005, don't run on raw

[Feiran Zheng]

Patch on qemu-iotest.

005, test of creating 5TB images, not practical on raw format, so not run on it.

Signed-off-by: Fam Zheng famc...@gmail.com
---
 005 |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/005 b/005
index 74537db..e086b6f 100755
--- a/005
+++ b/005
@@ -46,7 +46,7 @@ _supported_proto generic
 _supported_os Linux

 # vpc is limited to 127GB, so we can't test it here
-if [ $IMGFMT = vpc ]; then
+if [ $IMGFMT = vpc ] || [ $IMGFMT = raw ]; then
 _notrun image format $IMGFMT does not support large image sizes
 fi

Re: [Qemu-devel] [PATCH] qemu-iotest: test 005, don't run on raw

[Feiran Zheng]

Does this mean one must have that large fs space?

On Fri, May 27, 2011 at 6:47 PM, Christoph Hellwig h...@lst.de wrote:
 On Fri, May 27, 2011 at 06:45:03PM +0800, Feiran Zheng wrote:
 Patch on qemu-iotest.

 005, test of creating 5TB images, not practical on raw format, so not run on 
 it.

 It's perfectly fine on raw, just try it.





-- 
Best regards!
Fam Zheng

[Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error

[Feiran Zheng]

On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
stefano.stabell...@eu.citrix.com wrote:
 On Sun, 27 Mar 2011, Feiran Zheng wrote:
 Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
 won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
 the blkdev-inflight list and a leak.

 Signed-off-by: Feiran Zheng famc...@gmail.com
 ---
  hw/xen_disk.c |   22 +-
  1 files changed, 17 insertions(+), 5 deletions(-)

 diff --git a/hw/xen_disk.c b/hw/xen_disk.c
 index 445bf03..7940fab 100644
 --- a/hw/xen_disk.c
 +++ b/hw/xen_disk.c
 @@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
      int i, rc, len = 0;
      off_t pos;

 -    if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
 -     goto err;
 +    if (ioreq-req.nr_segments) {
 +     if (ioreq_map(ioreq) == -1)
 +         goto err_no_map;
 +    }
      if (ioreq-presync)
       bdrv_flush(blkdev-bs);


 this change is just to make the code clearer and easier to read, right?

I think so.



 @@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
      return 0;

  err:
 +    ioreq_unmap(ioreq);
 +err_no_map:
 +    ioreq_finish(ioreq);
      ioreq-status = BLKIF_RSP_ERROR;
      return -1;
  }
 @@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
  {
      struct XenBlkDev *blkdev = ioreq-blkdev;

 -    if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
 -     goto err;
 +    if (ioreq-req.nr_segments) {
 +     if (ioreq_map(ioreq) == -1)
 +         goto err_no_map;
 +    }

      ioreq-aio_inflight++;
      if (ioreq-presync)
 @@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
      qemu_aio_complete(ioreq, 0);

      return 0;
 +
 +err_no_map:
 +    ioreq_finish(ioreq);
 +    ioreq-status = BLKIF_RSP_ERROR;
 +    return -1;


 why aren't you calling ioreq_unmap before ioreq_finish, like in the
 previous case?



It seems not a must but if call qemu_aio_complete, the error info can
be printed, I thought it may be helpful.


-- 
Best regards!
Fam Zheng

[Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error

[Feiran Zheng]

Sorry for the confusing, I'll clean up it.

On Tue, Mar 29, 2011 at 2:16 AM, Stefano Stabellini
stefano.stabell...@eu.citrix.com wrote:
 On Mon, 28 Mar 2011, Feiran Zheng wrote:
 On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
 stefano.stabell...@eu.citrix.com wrote:
  On Sun, 27 Mar 2011, Feiran Zheng wrote:
  Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
  won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
  the blkdev-inflight list and a leak.
 
  Signed-off-by: Feiran Zheng famc...@gmail.com
  ---
   hw/xen_disk.c |   22 +-
   1 files changed, 17 insertions(+), 5 deletions(-)
 
  diff --git a/hw/xen_disk.c b/hw/xen_disk.c
  index 445bf03..7940fab 100644
  --- a/hw/xen_disk.c
  +++ b/hw/xen_disk.c
  @@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
       int i, rc, len = 0;
       off_t pos;
 
  -    if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
  -     goto err;
  +    if (ioreq-req.nr_segments) {
  +     if (ioreq_map(ioreq) == -1)
  +         goto err_no_map;
  +    }
       if (ioreq-presync)
        bdrv_flush(blkdev-bs);
 
 
  this change is just to make the code clearer and easier to read, right?

 I think so.

 
 
  @@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
       return 0;
 
   err:
  +    ioreq_unmap(ioreq);
  +err_no_map:
  +    ioreq_finish(ioreq);
       ioreq-status = BLKIF_RSP_ERROR;
       return -1;
   }
  @@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
   {
       struct XenBlkDev *blkdev = ioreq-blkdev;
 
  -    if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
  -     goto err;
  +    if (ioreq-req.nr_segments) {
  +     if (ioreq_map(ioreq) == -1)
  +         goto err_no_map;
  +    }
 
       ioreq-aio_inflight++;
       if (ioreq-presync)
  @@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
       qemu_aio_complete(ioreq, 0);
 
       return 0;
  +
  +err_no_map:
  +    ioreq_finish(ioreq);
  +    ioreq-status = BLKIF_RSP_ERROR;
  +    return -1;
 
 
  why aren't you calling ioreq_unmap before ioreq_finish, like in the
  previous case?
 
 
 
 It seems not a must but if call qemu_aio_complete, the error info can
 be printed, I thought it may be helpful.

 I am not sure I understand what you mean here because in case of error
 we don't call qemu_aio_complete at all in the err_no_map code path.



-- 
Best regards!
Fam Zheng

[Qemu-devel] [PATCH V2] hw/xen_disk: ioreq not finished on error

[Feiran Zheng]

Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
the blkdev-inflight list and a leak.

Signed-off-by: Feiran Zheng famc...@gmail.com
---
 hw/xen_disk.c |   10 --
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 445bf03..558bf8a 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -310,7 +310,7 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
 off_t pos;

 if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
-   goto err;
+   goto err_no_map;
 if (ioreq-presync)
bdrv_flush(blkdev-bs);

@@ -364,6 +364,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
 return 0;

 err:
+ioreq_unmap(ioreq);
+err_no_map:
+ioreq_finish(ioreq);
 ioreq-status = BLKIF_RSP_ERROR;
 return -1;
 }
@@ -393,7 +396,7 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 struct XenBlkDev *blkdev = ioreq-blkdev;

 if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
-   goto err;
+   goto err_no_map;

 ioreq-aio_inflight++;
 if (ioreq-presync)
@@ -427,6 +430,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 return 0;

 err:
+ioreq_unmap(ioreq);
+err_no_map:
+ioreq_finish(ioreq);
 ioreq-status = BLKIF_RSP_ERROR;
 return -1;
 }

[Qemu-devel] [PATCH]hw/xen_disk: ioreq not finished on error

[Feiran Zheng]

Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
the blkdev-inflight list and a leak.

Signed-off-by: Feiran Zheng famc...@gmail.com
---
 hw/xen_disk.c |   22 +-
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 445bf03..7940fab 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
 int i, rc, len = 0;
 off_t pos;

-if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
-   goto err;
+if (ioreq-req.nr_segments) {
+   if (ioreq_map(ioreq) == -1)
+   goto err_no_map;
+}
 if (ioreq-presync)
bdrv_flush(blkdev-bs);

@@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
 return 0;

 err:
+ioreq_unmap(ioreq);
+err_no_map:
+ioreq_finish(ioreq);
 ioreq-status = BLKIF_RSP_ERROR;
 return -1;
 }
@@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 {
 struct XenBlkDev *blkdev = ioreq-blkdev;

-if (ioreq-req.nr_segments  ioreq_map(ioreq) == -1)
-   goto err;
+if (ioreq-req.nr_segments) {
+   if (ioreq_map(ioreq) == -1)
+   goto err_no_map;
+}

 ioreq-aio_inflight++;
 if (ioreq-presync)
@@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 qemu_aio_complete(ioreq, 0);

 return 0;
+
+err_no_map:
+ioreq_finish(ioreq);
+ioreq-status = BLKIF_RSP_ERROR;
+return -1;

 err:
-ioreq-status = BLKIF_RSP_ERROR;
+qemu_aio_complete(ioreq, -1);
 return -1;
 }

Re: [Qemu-devel] qemu not starting

[Feiran Zheng]

Did you install from src? Try add --enable-sdl when configure and run
qemu with -vga std.

On Mon, Mar 28, 2011 at 12:15 PM, chandra shekar
chandrashekar...@gmail.com wrote:
 hi friends after installing qemu when i try to start qemu then i get an
 error as Could not open SDL display
 plz someone help,thanks

 --
 chandra





-- 
Best regards!
Fam Zheng