[Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

[Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

[Daniel P. Berrange]

When using a memory-backend object with prealloc turned on, QEMU
will memset() the first byte in every memory page to zero. While
this might have been acceptable for memory backends associated
with RAM, this corrupts application data for NVDIMMs.

Instead of setting every page to zero, read the current byte
value and then just write that same value back, so we are not
corrupting the original data. Directly write the value instead
of memset()ing it, since there's no benefit to memset for a
single byte write.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---

Changed in v3:

 - Mark the target of the write as volatile, instead of the intermedia
   variable (Andrea)

 util/oslib-posix.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f631464..544fb05 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -355,7 +355,20 @@ void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp)
 
         /* MAP_POPULATE silently ignores failures */
         for (i = 0; i < numpages; i++) {
-            memset(area + (hpagesize * i), 0, 1);
+            /*
+             * Read & write back the same value, so we don't
+             * corrupt existing user/app data that might be
+             * stored.
+             *
+             * 'volatile' to stop compiler optimizing this away
+             * to a no-op
+             *
+             * TODO: get a better solution from kernel so we
+             * don't need to write at all so we don't cause
+             * wear on the storage backing the region...
+             */
+            char *page = area + (hpagesize * i);
+            *(volatile char *)page = *page;
         }
     }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20170303113255.28262-1-berrange@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
4210940 os: don't corrupt pre-existing memory-backend data with prealloc

=== OUTPUT BEGIN ===
Checking PATCH 1/1: os: don't corrupt pre-existing memory-backend data with prealloc...
ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#43: FILE: util/oslib-posix.c:371:
+            *(volatile char *)page = *page;

total: 1 errors, 0 warnings, 21 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

[Andrea Arcangeli]

On Fri, Mar 03, 2017 at 11:32:55AM +0000, Daniel P. Berrange wrote:
> When using a memory-backend object with prealloc turned on, QEMU
> will memset() the first byte in every memory page to zero. While
> this might have been acceptable for memory backends associated
> with RAM, this corrupts application data for NVDIMMs.
> 
> Instead of setting every page to zero, read the current byte
> value and then just write that same value back, so we are not
> corrupting the original data. Directly write the value instead
> of memset()ing it, since there's no benefit to memset for a
> single byte write.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> 
> Changed in v3:

Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>

Thanks,
Andrea

Re: [Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

On Fri, Mar 03, 2017 at 11:32:55AM +0000, Daniel P. Berrange wrote:
> When using a memory-backend object with prealloc turned on, QEMU
> will memset() the first byte in every memory page to zero. While
> this might have been acceptable for memory backends associated
> with RAM, this corrupts application data for NVDIMMs.
> 
> Instead of setting every page to zero, read the current byte
> value and then just write that same value back, so we are not
> corrupting the original data. Directly write the value instead
> of memset()ing it, since there's no benefit to memset for a
> single byte write.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> 
> Changed in v3:
> 
>  - Mark the target of the write as volatile, instead of the intermedia
>    variable (Andrea)
> 
>  util/oslib-posix.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On Fri, Mar 03, 2017 at 11:32:55AM +0000, Daniel P. Berrange wrote:
> When using a memory-backend object with prealloc turned on, QEMU
> will memset() the first byte in every memory page to zero. While
> this might have been acceptable for memory backends associated
> with RAM, this corrupts application data for NVDIMMs.
> 
> Instead of setting every page to zero, read the current byte
> value and then just write that same value back, so we are not
> corrupting the original data. Directly write the value instead
> of memset()ing it, since there's no benefit to memset for a
> single byte write.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> 
> Changed in v3:
> 
>  - Mark the target of the write as volatile, instead of the intermedia
>    variable (Andrea)
> 
>  util/oslib-posix.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)

There are no official maintainers for this source file.  I want to make
sure this patch goes into QEMU 2.9, so...

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

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3] os: don't corrupt pre-existing memory-backend data with prealloc

[Paolo Bonzini]

On 10/03/2017 05:30, Stefan Hajnoczi wrote:
> On Fri, Mar 03, 2017 at 11:32:55AM +0000, Daniel P. Berrange wrote:
>> When using a memory-backend object with prealloc turned on, QEMU
>> will memset() the first byte in every memory page to zero. While
>> this might have been acceptable for memory backends associated
>> with RAM, this corrupts application data for NVDIMMs.
>>
>> Instead of setting every page to zero, read the current byte
>> value and then just write that same value back, so we are not
>> corrupting the original data. Directly write the value instead
>> of memset()ing it, since there's no benefit to memset for a
>> single byte write.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>
>> Changed in v3:
>>
>>  - Mark the target of the write as volatile, instead of the intermedia
>>    variable (Andrea)
>>
>>  util/oslib-posix.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> There are no official maintainers for this source file.  I want to make
> sure this patch goes into QEMU 2.9, so...
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block

Yeah, I was leaving it to you because it's related to NVDIMM.  Thanks,

Paolo