[PATCH qemu 0/2] dump: Only use the makedumpfile flattened format when necessary

[PATCH qemu 0/2] dump: Only use the makedumpfile flattened format when necessary

[Stephen Brennan]

Hello,

The dump-guest-memory monitor command supports kdump-zlib, as well as a
few other compression options for the kdump format. However, the format
it actually outputs is a variation on that format known as the
"flattened format".  The flattened format is essentially a sequence of
records that give instructions on how to create the final output file.
Each record can be viewed as an instruction: seek to the given offset,
and then write a certain number of bytes to the output file.

The "flattened format" exists to serve makedumpfile in very specific
cases, when there's no filesystem to write a dump to. Then, makedumpfile
may output to stdout or a socket. Since these files aren't seekable, and
buffering the full output would exceed the memory available to it,
makedumpfile is forced to use this flattened format instead.

The flattened format is intended to be reassembled after the fact, using
"makedumpfile -R" or "makedumpfile-R.pl", but this is a lengthy process
because it requires copying the entire file. Crash can read the
flattened format, but this requires a lengthy phase in which it
reassembles / indexes the file. Essentially, I can think of no reason
why a user would want to have the flattened format, when the regular
kdump format is available to them.

So therefore, this patch series changes the behavior of the kdump
creation to match the behavior of makedumpfile: when the output file is
seekable, it will write the normal kdump format. When the output file is
not, it will continue to write the flattened format.

While there could be an argument that the flattened format is more
efficient (in time and/or memory) to create, because it doesn't require
seeking, I'm don't believe this to be the case. Makedumpfile itself is
intended to run in very constrained kexec environments, and it always
writes the regular kdump format, unless its output file is not seekable.
However, I would be open to doing some testing if necessary to verify
the performance is similar.

I do understand that this could raise compatibility concerns. From my
perspective, the "regular kdump format" is strictly more compatible to
other tools than the flattened format. And as I said, I can't think of a
use case where a person would *want* the flattened format. But if it's a
problem, then we could implement a new option to enable the new
behavior.

I've gone ahead and tested the changes and confirmed that the resulting
dumps are still readable via crash with an x86_64 guest.

Thanks for your consideration and review,
Stephen

Stephen Brennan (2):
  dump: Pass DumpState to write_ functions
  dump: Only use the makedumpfile flattened format when necessary

 dump/dump.c           | 68 ++++++++++++++++++++++++++++---------------
 include/sysemu/dump.h |  3 +-
 2 files changed, 46 insertions(+), 25 deletions(-)

-- 
2.39.2

[PATCH qemu 1/2] dump: Pass DumpState to write_ functions

[Stephen Brennan]

For the next patch, we need a reference to DumpState when writing data.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 dump/dump.c           | 40 ++++++++++++++++++++--------------------
 include/sysemu/dump.h |  2 +-
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 1f1a6edcab..2708ddc135 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -809,7 +809,7 @@ static void create_vmcore(DumpState *s, Error **errp)
     dump_end(s, errp);
 }
 
-static int write_start_flat_header(int fd)
+static int write_start_flat_header(DumpState *s)
 {
     MakedumpfileHeader *mh;
     int ret = 0;
@@ -824,7 +824,7 @@ static int write_start_flat_header(int fd)
     mh->version = cpu_to_be64(VERSION_FLAT_HEADER);
 
     size_t written_size;
-    written_size = qemu_write_full(fd, mh, MAX_SIZE_MDF_HEADER);
+    written_size = qemu_write_full(s->fd, mh, MAX_SIZE_MDF_HEADER);
     if (written_size != MAX_SIZE_MDF_HEADER) {
         ret = -1;
     }
@@ -833,7 +833,7 @@ static int write_start_flat_header(int fd)
     return ret;
 }
 
-static int write_end_flat_header(int fd)
+static int write_end_flat_header(DumpState *s)
 {
     MakedumpfileDataHeader mdh;
 
@@ -841,7 +841,7 @@ static int write_end_flat_header(int fd)
     mdh.buf_size = END_FLAG_FLAT_HEADER;
 
     size_t written_size;
-    written_size = qemu_write_full(fd, &mdh, sizeof(mdh));
+    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
     if (written_size != sizeof(mdh)) {
         return -1;
     }
@@ -849,7 +849,7 @@ static int write_end_flat_header(int fd)
     return 0;
 }
 
-static int write_buffer(int fd, off_t offset, const void *buf, size_t size)
+static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size)
 {
     size_t written_size;
     MakedumpfileDataHeader mdh;
@@ -857,12 +857,12 @@ static int write_buffer(int fd, off_t offset, const void *buf, size_t size)
     mdh.offset = cpu_to_be64(offset);
     mdh.buf_size = cpu_to_be64(size);
 
-    written_size = qemu_write_full(fd, &mdh, sizeof(mdh));
+    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
     if (written_size != sizeof(mdh)) {
         return -1;
     }
 
-    written_size = qemu_write_full(fd, buf, size);
+    written_size = qemu_write_full(s->fd, buf, size);
     if (written_size != size) {
         return -1;
     }
@@ -982,7 +982,7 @@ static void create_header32(DumpState *s, Error **errp)
 #endif
     dh->status = cpu_to_dump32(s, status);
 
-    if (write_buffer(s->fd, 0, dh, size) < 0) {
+    if (write_buffer(s, 0, dh, size) < 0) {
         error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
@@ -1012,7 +1012,7 @@ static void create_header32(DumpState *s, Error **errp)
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump32(s, s->note_size);
 
-    if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
+    if (write_buffer(s, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
         error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
@@ -1027,7 +1027,7 @@ static void create_header32(DumpState *s, Error **errp)
     if (*errp) {
         goto out;
     }
-    if (write_buffer(s->fd, offset_note, s->note_buf,
+    if (write_buffer(s, offset_note, s->note_buf,
                      s->note_size) < 0) {
         error_setg(errp, "dump: failed to write notes");
         goto out;
@@ -1093,7 +1093,7 @@ static void create_header64(DumpState *s, Error **errp)
 #endif
     dh->status = cpu_to_dump32(s, status);
 
-    if (write_buffer(s->fd, 0, dh, size) < 0) {
+    if (write_buffer(s, 0, dh, size) < 0) {
         error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
@@ -1123,7 +1123,7 @@ static void create_header64(DumpState *s, Error **errp)
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump64(s, s->note_size);
 
-    if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
+    if (write_buffer(s, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
         error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
@@ -1139,7 +1139,7 @@ static void create_header64(DumpState *s, Error **errp)
         goto out;
     }
 
-    if (write_buffer(s->fd, offset_note, s->note_buf,
+    if (write_buffer(s, offset_note, s->note_buf,
                      s->note_size) < 0) {
         error_setg(errp, "dump: failed to write notes");
         goto out;
@@ -1204,7 +1204,7 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
     while (old_offset < new_offset) {
         /* calculate the offset and write dump_bitmap */
         offset_bitmap1 = s->offset_dump_bitmap + old_offset;
-        if (write_buffer(s->fd, offset_bitmap1, buf,
+        if (write_buffer(s, offset_bitmap1, buf,
                          bitmap_bufsize) < 0) {
             return -1;
         }
@@ -1212,7 +1212,7 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
         /* dump level 1 is chosen, so 1st and 2nd bitmap are same */
         offset_bitmap2 = s->offset_dump_bitmap + s->len_dump_bitmap +
                          old_offset;
-        if (write_buffer(s->fd, offset_bitmap2, buf,
+        if (write_buffer(s, offset_bitmap2, buf,
                          bitmap_bufsize) < 0) {
             return -1;
         }
@@ -1380,7 +1380,7 @@ out:
 static void prepare_data_cache(DataCache *data_cache, DumpState *s,
                                off_t offset)
 {
-    data_cache->fd = s->fd;
+    data_cache->state = s;
     data_cache->data_size = 0;
     data_cache->buf_size = 4 * dump_bitmap_get_bufsize(s);
     data_cache->buf = g_malloc0(data_cache->buf_size);
@@ -1399,11 +1399,11 @@ static int write_cache(DataCache *dc, const void *buf, size_t size,
     /*
      * if flag_sync is set, synchronize data in dc->buf into vmcore.
      * otherwise check if the space is enough for caching data in buf, if not,
-     * write the data in dc->buf to dc->fd and reset dc->buf
+     * write the data in dc->buf to dc->state->fd and reset dc->buf
      */
     if ((!flag_sync && dc->data_size + size > dc->buf_size) ||
         (flag_sync && dc->data_size > 0)) {
-        if (write_buffer(dc->fd, dc->offset, dc->buf, dc->data_size) < 0) {
+        if (write_buffer(dc->state, dc->offset, dc->buf, dc->data_size) < 0) {
             return -1;
         }
 
@@ -1644,7 +1644,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
      *  +------------------------------------------+
      */
 
-    ret = write_start_flat_header(s->fd);
+    ret = write_start_flat_header(s);
     if (ret < 0) {
         error_setg(errp, "dump: failed to write start flat header");
         return;
@@ -1665,7 +1665,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
         return;
     }
 
-    ret = write_end_flat_header(s->fd);
+    ret = write_end_flat_header(s);
     if (ret < 0) {
         error_setg(errp, "dump: failed to write end flat header");
         return;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7008d43d04..e27af8fb34 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -137,7 +137,7 @@ typedef struct QEMU_PACKED KdumpSubHeader64 {
 } KdumpSubHeader64;
 
 typedef struct DataCache {
-    int fd;             /* fd of the file where to write the cached data */
+    DumpState *state;   /* dump state related to this data */
     uint8_t *buf;       /* buffer for cached data */
     size_t buf_size;    /* size of the buf */
     size_t data_size;   /* size of cached data in buf */
-- 
2.39.2

[PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Stephen Brennan]

The flattened format is used by makedumpfile only when it is outputting
a vmcore to a file which is not seekable. The flattened format functions
essentially as a set of instructions of the form "seek to the given
offset, then write the given bytes out".

The flattened format can be reconstructed using makedumpfile -R, or
makedumpfile-R.pl, but it is a slow process beacuse it requires copying
the entire vmcore. The flattened format can also be directly read by
crash, but still, it requires a lengthy reassembly phase.

To sum up, the flattened format is not an ideal one: it should only be
used on files which are actually not seekable. This is the exact
strategy which makedumpfile uses, as seen in the implementation of
"write_buffer()" in makedumpfile [1].

So, update the "dump-guest-memory" monitor command implementation so
that it will directly do the seeks and writes, in the same strategy as
makedumpfile. However, if the file is not seekable, then we can fall
back to the flattened format.

[1]: https://github.com/makedumpfile/makedumpfile/blob/f23bb943568188a2746dbf9b6692668f5a2ac3b6/makedumpfile.c#L5008-L5040

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 dump/dump.c           | 30 +++++++++++++++++++++++++-----
 include/sysemu/dump.h |  1 +
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 2708ddc135..384d275e39 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -813,6 +813,13 @@ static int write_start_flat_header(DumpState *s)
 {
     MakedumpfileHeader *mh;
     int ret = 0;
+    loff_t offset = lseek(s->fd, 0, SEEK_CUR);
+
+    /* If the file is seekable, don't output flattened header */
+    if (offset == 0) {
+        s->seekable = true;
+        return 0;
+    }
 
     QEMU_BUILD_BUG_ON(sizeof *mh > MAX_SIZE_MDF_HEADER);
     mh = g_malloc0(MAX_SIZE_MDF_HEADER);
@@ -837,6 +844,10 @@ static int write_end_flat_header(DumpState *s)
 {
     MakedumpfileDataHeader mdh;
 
+    if (s->seekable) {
+        return 0;
+    }
+
     mdh.offset = END_FLAG_FLAT_HEADER;
     mdh.buf_size = END_FLAG_FLAT_HEADER;
 
@@ -853,13 +864,21 @@ static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size
 {
     size_t written_size;
     MakedumpfileDataHeader mdh;
+    loff_t seek_loc;
 
-    mdh.offset = cpu_to_be64(offset);
-    mdh.buf_size = cpu_to_be64(size);
+    if (s->seekable) {
+        seek_loc = lseek(s->fd, offset, SEEK_SET);
+        if (seek_loc == (off_t) -1) {
+            return -1;
+        }
+    } else {
+        mdh.offset = cpu_to_be64(offset);
+        mdh.buf_size = cpu_to_be64(size);
 
-    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
-    if (written_size != sizeof(mdh)) {
-        return -1;
+        written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
+        if (written_size != sizeof(mdh)) {
+            return -1;
+        }
     }
 
     written_size = qemu_write_full(s->fd, buf, size);
@@ -1786,6 +1805,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     s->has_format = has_format;
     s->format = format;
     s->written_size = 0;
+    s->seekable = false;
 
     /* kdump-compressed is conflict with paging and filter */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index e27af8fb34..ab703c3a5e 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -157,6 +157,7 @@ typedef struct DumpState {
     MemoryMappingList list;
     bool resume;
     bool detached;
+    bool seekable;
     hwaddr memory_offset;
     int fd;
 
-- 
2.39.2

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Stephen Brennan]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Mon, Jul 17, 2023 at 8:45 PM Stephen Brennan <
> stephen.s.brennan@oracle.com> wrote:
>
>> The flattened format is used by makedumpfile only when it is outputting
>> a vmcore to a file which is not seekable. The flattened format functions
>> essentially as a set of instructions of the form "seek to the given
>> offset, then write the given bytes out".
>>
>> The flattened format can be reconstructed using makedumpfile -R, or
>> makedumpfile-R.pl, but it is a slow process beacuse it requires copying
>> the entire vmcore. The flattened format can also be directly read by
>> crash, but still, it requires a lengthy reassembly phase.
>>
>> To sum up, the flattened format is not an ideal one: it should only be
>> used on files which are actually not seekable. This is the exact
>> strategy which makedumpfile uses, as seen in the implementation of
>> "write_buffer()" in makedumpfile [1].
>>
>> So, update the "dump-guest-memory" monitor command implementation so
>> that it will directly do the seeks and writes, in the same strategy as
>> makedumpfile. However, if the file is not seekable, then we can fall
>> back to the flattened format.
>>
>> [1]:
>> https://github.com/makedumpfile/makedumpfile/blob/f23bb943568188a2746dbf9b6692668f5a2ac3b6/makedumpfile.c#L5008-L5040
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Tested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> I am a bit reluctant to change the dump format by default. But since the
> flatten format is more "complicated" than the "normal" format, perhaps we
> can assume all users will handle it.
>
> The change is probably late for 8.1 though..

Thank you for your review and testing!

I totally understand the concern about making the change by default. I
do believe that nobody should notice too much because the normal format
should be easier to work with, and more broadly compatible. I don't know
of any tool which deals with the flattened format that can't handle the
normal format, except for "makedumpfile -R" itself.

If it's a blocker, I can go ahead and add a new flag to the command to
enable the behavior. However there are a few good justifications not to
add a new flag. I think it's going to be difficult to explain the
difference between the two formats in documentation, as the
implementation of the formats is a bit "into the weeds". The libvirt API
also specifies each format separately (kdump-zlib, kdump-lzo,
kdump-snappy) and so adding several new options there would be
unfortunate for end-users as well.

At the end of the day, it's your judgment call, and I'll implement it
how you prefer.

Thanks,
Stephen

>>  dump/dump.c           | 30 +++++++++++++++++++++++++-----
>>  include/sysemu/dump.h |  1 +
>>  2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 2708ddc135..384d275e39 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -813,6 +813,13 @@ static int write_start_flat_header(DumpState *s)
>>  {
>>      MakedumpfileHeader *mh;
>>      int ret = 0;
>> +    loff_t offset = lseek(s->fd, 0, SEEK_CUR);
>> +
>> +    /* If the file is seekable, don't output flattened header */
>> +    if (offset == 0) {
>> +        s->seekable = true;
>> +        return 0;
>> +    }
>>
>>      QEMU_BUILD_BUG_ON(sizeof *mh > MAX_SIZE_MDF_HEADER);
>>      mh = g_malloc0(MAX_SIZE_MDF_HEADER);
>> @@ -837,6 +844,10 @@ static int write_end_flat_header(DumpState *s)
>>  {
>>      MakedumpfileDataHeader mdh;
>>
>> +    if (s->seekable) {
>> +        return 0;
>> +    }
>> +
>>      mdh.offset = END_FLAG_FLAT_HEADER;
>>      mdh.buf_size = END_FLAG_FLAT_HEADER;
>>
>> @@ -853,13 +864,21 @@ static int write_buffer(DumpState *s, off_t offset,
>> const void *buf, size_t size
>>  {
>>      size_t written_size;
>>      MakedumpfileDataHeader mdh;
>> +    loff_t seek_loc;
>>
>> -    mdh.offset = cpu_to_be64(offset);
>> -    mdh.buf_size = cpu_to_be64(size);
>> +    if (s->seekable) {
>> +        seek_loc = lseek(s->fd, offset, SEEK_SET);
>> +        if (seek_loc == (off_t) -1) {
>> +            return -1;
>> +        }
>> +    } else {
>> +        mdh.offset = cpu_to_be64(offset);
>> +        mdh.buf_size = cpu_to_be64(size);
>>
>> -    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
>> -    if (written_size != sizeof(mdh)) {
>> -        return -1;
>> +        written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
>> +        if (written_size != sizeof(mdh)) {
>> +            return -1;
>> +        }
>>      }
>>
>>      written_size = qemu_write_full(s->fd, buf, size);
>> @@ -1786,6 +1805,7 @@ static void dump_init(DumpState *s, int fd, bool
>> has_format,
>>      s->has_format = has_format;
>>      s->format = format;
>>      s->written_size = 0;
>> +    s->seekable = false;
>>
>>      /* kdump-compressed is conflict with paging and filter */
>>      if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index e27af8fb34..ab703c3a5e 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -157,6 +157,7 @@ typedef struct DumpState {
>>      MemoryMappingList list;
>>      bool resume;
>>      bool detached;
>> +    bool seekable;
>>      hwaddr memory_offset;
>>      int fd;
>>
>> --
>> 2.39.2
>>
>>
>>
>
> -- 
> Marc-André Lureau

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Stephen Brennan]

Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> I am a bit reluctant to change the dump format by default. But since the
>> flatten format is more "complicated" than the "normal" format, perhaps we
>> can assume all users will handle it.
>>
>> The change is probably late for 8.1 though..
>
> Thank you for your review and testing!
>
> I totally understand the concern about making the change by default. I
> do believe that nobody should notice too much because the normal format
> should be easier to work with, and more broadly compatible. I don't know
> of any tool which deals with the flattened format that can't handle the
> normal format, except for "makedumpfile -R" itself.
>
> If it's a blocker, I can go ahead and add a new flag to the command to
> enable the behavior. However there are a few good justifications not to
> add a new flag. I think it's going to be difficult to explain the
> difference between the two formats in documentation, as the
> implementation of the formats is a bit "into the weeds". The libvirt API
> also specifies each format separately (kdump-zlib, kdump-lzo,
> kdump-snappy) and so adding several new options there would be
> unfortunate for end-users as well.
>
> At the end of the day, it's your judgment call, and I'll implement it
> how you prefer.

I just wanted to check back on this to clarify the next step. Are you
satisfied with this and waiting to apply it until the right time? Or
would you prefer me to send a new version making this opt-in?

Thanks,
Stephen

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Marc-André Lureau]

Hi

On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> > Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >> I am a bit reluctant to change the dump format by default. But since the
> >> flatten format is more "complicated" than the "normal" format, perhaps we
> >> can assume all users will handle it.
> >>
> >> The change is probably late for 8.1 though..
> >
> > Thank you for your review and testing!
> >
> > I totally understand the concern about making the change by default. I
> > do believe that nobody should notice too much because the normal format
> > should be easier to work with, and more broadly compatible. I don't know
> > of any tool which deals with the flattened format that can't handle the
> > normal format, except for "makedumpfile -R" itself.
> >
> > If it's a blocker, I can go ahead and add a new flag to the command to
> > enable the behavior. However there are a few good justifications not to
> > add a new flag. I think it's going to be difficult to explain the
> > difference between the two formats in documentation, as the
> > implementation of the formats is a bit "into the weeds". The libvirt API
> > also specifies each format separately (kdump-zlib, kdump-lzo,
> > kdump-snappy) and so adding several new options there would be
> > unfortunate for end-users as well.
> >
> > At the end of the day, it's your judgment call, and I'll implement it
> > how you prefer.
>
> I just wanted to check back on this to clarify the next step. Are you
> satisfied with this and waiting to apply it until the right time? Or
> would you prefer me to send a new version making this opt-in?
>

Nobody seems to raise concerns. If it would be just me, I would change
it. But we should rather be safe, so let's make it this opt-in please.



-- 
Marc-André Lureau

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Marc-André Lureau]

Hi

On Wed, Aug 23, 2023 at 2:03 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
> >
> > Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> > > Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> > >> I am a bit reluctant to change the dump format by default. But since the
> > >> flatten format is more "complicated" than the "normal" format, perhaps we
> > >> can assume all users will handle it.
> > >>
> > >> The change is probably late for 8.1 though..
> > >
> > > Thank you for your review and testing!
> > >
> > > I totally understand the concern about making the change by default. I
> > > do believe that nobody should notice too much because the normal format
> > > should be easier to work with, and more broadly compatible. I don't know
> > > of any tool which deals with the flattened format that can't handle the
> > > normal format, except for "makedumpfile -R" itself.
> > >
> > > If it's a blocker, I can go ahead and add a new flag to the command to
> > > enable the behavior. However there are a few good justifications not to
> > > add a new flag. I think it's going to be difficult to explain the
> > > difference between the two formats in documentation, as the
> > > implementation of the formats is a bit "into the weeds". The libvirt API
> > > also specifies each format separately (kdump-zlib, kdump-lzo,
> > > kdump-snappy) and so adding several new options there would be
> > > unfortunate for end-users as well.
> > >
> > > At the end of the day, it's your judgment call, and I'll implement it
> > > how you prefer.
> >
> > I just wanted to check back on this to clarify the next step. Are you
> > satisfied with this and waiting to apply it until the right time? Or
> > would you prefer me to send a new version making this opt-in?
> >
>
> Nobody seems to raise concerns. If it would be just me, I would change
> it. But we should rather be safe, so let's make it this opt-in please.
>
>

Alex, Daniel, Thomas .. Do you have an opinion on changing the dump format?

Stephen, do you have a new version of the patches to make it opt-in?

thanks



-- 
Marc-André Lureau

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Omar Sandoval]

On Tue, Sep 12, 2023 at 10:34:04AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Aug 23, 2023 at 2:03 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
> > <stephen.s.brennan@oracle.com> wrote:
> > >
> > > Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> > > > Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> > > >> I am a bit reluctant to change the dump format by default. But since the
> > > >> flatten format is more "complicated" than the "normal" format, perhaps we
> > > >> can assume all users will handle it.
> > > >>
> > > >> The change is probably late for 8.1 though..
> > > >
> > > > Thank you for your review and testing!
> > > >
> > > > I totally understand the concern about making the change by default. I
> > > > do believe that nobody should notice too much because the normal format
> > > > should be easier to work with, and more broadly compatible. I don't know
> > > > of any tool which deals with the flattened format that can't handle the
> > > > normal format, except for "makedumpfile -R" itself.
> > > >
> > > > If it's a blocker, I can go ahead and add a new flag to the command to
> > > > enable the behavior. However there are a few good justifications not to
> > > > add a new flag. I think it's going to be difficult to explain the
> > > > difference between the two formats in documentation, as the
> > > > implementation of the formats is a bit "into the weeds". The libvirt API
> > > > also specifies each format separately (kdump-zlib, kdump-lzo,
> > > > kdump-snappy) and so adding several new options there would be
> > > > unfortunate for end-users as well.
> > > >
> > > > At the end of the day, it's your judgment call, and I'll implement it
> > > > how you prefer.
> > >
> > > I just wanted to check back on this to clarify the next step. Are you
> > > satisfied with this and waiting to apply it until the right time? Or
> > > would you prefer me to send a new version making this opt-in?
> > >
> >
> > Nobody seems to raise concerns. If it would be just me, I would change
> > it. But we should rather be safe, so let's make it this opt-in please.
> >
> >
> 
> Alex, Daniel, Thomas .. Do you have an opinion on changing the dump format?

Chiming in as a user here, but I'd much prefer for the normal format to
be the default. The last time that I tried dump-guest-memory with the
kdump format, it took me awhile to figure out what format it was
actually using since libkdumpfile didn't understand it.

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Thomas Huth]

On Tue, 2023-09-12 at 10:34 +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Aug 23, 2023 at 2:03 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> > 
> > Hi
> > 
> > On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
> > <stephen.s.brennan@oracle.com> wrote:
> > > 
> > > Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> > > > Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> > > > > I am a bit reluctant to change the dump format by default.
> > > > > But since the
> > > > > flatten format is more "complicated" than the "normal"
> > > > > format, perhaps we
> > > > > can assume all users will handle it.
> > > > > 
> > > > > The change is probably late for 8.1 though..
> > > > 
> > > > Thank you for your review and testing!
> > > > 
> > > > I totally understand the concern about making the change by
> > > > default. I
> > > > do believe that nobody should notice too much because the
> > > > normal format
> > > > should be easier to work with, and more broadly compatible. I
> > > > don't know
> > > > of any tool which deals with the flattened format that can't
> > > > handle the
> > > > normal format, except for "makedumpfile -R" itself.
> > > > 
> > > > If it's a blocker, I can go ahead and add a new flag to the
> > > > command to
> > > > enable the behavior. However there are a few good
> > > > justifications not to
> > > > add a new flag. I think it's going to be difficult to explain
> > > > the
> > > > difference between the two formats in documentation, as the
> > > > implementation of the formats is a bit "into the weeds". The
> > > > libvirt API
> > > > also specifies each format separately (kdump-zlib, kdump-lzo,
> > > > kdump-snappy) and so adding several new options there would be
> > > > unfortunate for end-users as well.
> > > > 
> > > > At the end of the day, it's your judgment call, and I'll
> > > > implement it
> > > > how you prefer.
> > > 
> > > I just wanted to check back on this to clarify the next step. Are
> > > you
> > > satisfied with this and waiting to apply it until the right time?
> > > Or
> > > would you prefer me to send a new version making this opt-in?
> > > 
> > 
> > Nobody seems to raise concerns. If it would be just me, I would
> > change
> > it. But we should rather be safe, so let's make it this opt-in
> > please.
> > 
> > 
> 
> Alex, Daniel, Thomas .. Do you have an opinion on changing the dump
> format?

I just double-checked with Janosch Frank, and from s390x perspective
we're mostly interested in the ELF dump format, so from the s390x side
of view: ¯\_(ツ)_/¯

 Thomas

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Daniel P. Berrangé]

On Tue, Sep 12, 2023 at 10:34:04AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Aug 23, 2023 at 2:03 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
> > <stephen.s.brennan@oracle.com> wrote:
> > >
> > > Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> > > > Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> > > >> I am a bit reluctant to change the dump format by default. But since the
> > > >> flatten format is more "complicated" than the "normal" format, perhaps we
> > > >> can assume all users will handle it.
> > > >>
> > > >> The change is probably late for 8.1 though..
> > > >
> > > > Thank you for your review and testing!
> > > >
> > > > I totally understand the concern about making the change by default. I
> > > > do believe that nobody should notice too much because the normal format
> > > > should be easier to work with, and more broadly compatible. I don't know
> > > > of any tool which deals with the flattened format that can't handle the
> > > > normal format, except for "makedumpfile -R" itself.
> > > >
> > > > If it's a blocker, I can go ahead and add a new flag to the command to
> > > > enable the behavior. However there are a few good justifications not to
> > > > add a new flag. I think it's going to be difficult to explain the
> > > > difference between the two formats in documentation, as the
> > > > implementation of the formats is a bit "into the weeds". The libvirt API
> > > > also specifies each format separately (kdump-zlib, kdump-lzo,
> > > > kdump-snappy) and so adding several new options there would be
> > > > unfortunate for end-users as well.
> > > >
> > > > At the end of the day, it's your judgment call, and I'll implement it
> > > > how you prefer.
> > >
> > > I just wanted to check back on this to clarify the next step. Are you
> > > satisfied with this and waiting to apply it until the right time? Or
> > > would you prefer me to send a new version making this opt-in?
> > >
> >
> > Nobody seems to raise concerns. If it would be just me, I would change
> > it. But we should rather be safe, so let's make it this opt-in please.
> 
> Alex, Daniel, Thomas .. Do you have an opinion on changing the dump format?

I think it is a bad idea for the command to output data in a different
format, silently switching based on whether it is passed a pipe or and
file FD.

Libvirt may pass either a plain file FD or a pipe FD, depending on
whether the user set the VIR_DUMP_BYPASS_CACHE API flag. IMHO it
is unacceptable for that to result in a change of data format as a
silent side effect.

Looking back at the history, the patches originally did NOT use the
flatten makedumpfile format, but to ensure it was able to work with
non-seekable files, it jumped through hoops to write to a temporary
file first process it and then write to the real file. Switching to
makedumpfile format was to enable it to avoid temp files and always
be compatible with non-seekable FDs.

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 qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Stephen Brennan]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Aug 23, 2023 at 2:03 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>>
>> Hi
>>
>> On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
>> <stephen.s.brennan@oracle.com> wrote:
>> >
>> > Stephen Brennan <stephen.s.brennan@oracle.com> writes:
>> > > Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> > >> I am a bit reluctant to change the dump format by default. But since the
>> > >> flatten format is more "complicated" than the "normal" format, perhaps we
>> > >> can assume all users will handle it.
>> > >>
>> > >> The change is probably late for 8.1 though..
>> > >
>> > > Thank you for your review and testing!
>> > >
>> > > I totally understand the concern about making the change by default. I
>> > > do believe that nobody should notice too much because the normal format
>> > > should be easier to work with, and more broadly compatible. I don't know
>> > > of any tool which deals with the flattened format that can't handle the
>> > > normal format, except for "makedumpfile -R" itself.
>> > >
>> > > If it's a blocker, I can go ahead and add a new flag to the command to
>> > > enable the behavior. However there are a few good justifications not to
>> > > add a new flag. I think it's going to be difficult to explain the
>> > > difference between the two formats in documentation, as the
>> > > implementation of the formats is a bit "into the weeds". The libvirt API
>> > > also specifies each format separately (kdump-zlib, kdump-lzo,
>> > > kdump-snappy) and so adding several new options there would be
>> > > unfortunate for end-users as well.
>> > >
>> > > At the end of the day, it's your judgment call, and I'll implement it
>> > > how you prefer.
>> >
>> > I just wanted to check back on this to clarify the next step. Are you
>> > satisfied with this and waiting to apply it until the right time? Or
>> > would you prefer me to send a new version making this opt-in?
>> >
>>
>> Nobody seems to raise concerns. If it would be just me, I would change
>> it. But we should rather be safe, so let's make it this opt-in please.
>>
>>
>
> Alex, Daniel, Thomas .. Do you have an opinion on changing the dump format?
>
> Stephen, do you have a new version of the patches to make it opt-in?

Thanks for the reminder, I did have a start on the patches, but they're
not quite ready to share just yet. I'm learning about the QMP command
JSON schemas, which I'll need to update. I ended up receiving a new work
laptop which disrupted my work. I'll try to get a new patch series out
during my Wednesday (US Pacific time) workday.

Thanks,
Stephen

Re: [PATCH qemu 2/2] dump: Only use the makedumpfile flattened format when necessary

[Stephen Brennan]

Daniel P. Berrangé <berrange@redhat.com> writes:
> On Tue, Sep 12, 2023 at 10:34:04AM +0400, Marc-André Lureau wrote:
>> Hi
>> 
>> On Wed, Aug 23, 2023 at 2:03 PM Marc-André Lureau
>> <marcandre.lureau@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
>> > <stephen.s.brennan@oracle.com> wrote:
>> > >
>> > > Stephen Brennan <stephen.s.brennan@oracle.com> writes:
>> > > > Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> > > >> I am a bit reluctant to change the dump format by default. But since the
>> > > >> flatten format is more "complicated" than the "normal" format, perhaps we
>> > > >> can assume all users will handle it.
>> > > >>
>> > > >> The change is probably late for 8.1 though..
>> > > >
>> > > > Thank you for your review and testing!
>> > > >
>> > > > I totally understand the concern about making the change by default. I
>> > > > do believe that nobody should notice too much because the normal format
>> > > > should be easier to work with, and more broadly compatible. I don't know
>> > > > of any tool which deals with the flattened format that can't handle the
>> > > > normal format, except for "makedumpfile -R" itself.
>> > > >
>> > > > If it's a blocker, I can go ahead and add a new flag to the command to
>> > > > enable the behavior. However there are a few good justifications not to
>> > > > add a new flag. I think it's going to be difficult to explain the
>> > > > difference between the two formats in documentation, as the
>> > > > implementation of the formats is a bit "into the weeds". The libvirt API
>> > > > also specifies each format separately (kdump-zlib, kdump-lzo,
>> > > > kdump-snappy) and so adding several new options there would be
>> > > > unfortunate for end-users as well.
>> > > >
>> > > > At the end of the day, it's your judgment call, and I'll implement it
>> > > > how you prefer.
>> > >
>> > > I just wanted to check back on this to clarify the next step. Are you
>> > > satisfied with this and waiting to apply it until the right time? Or
>> > > would you prefer me to send a new version making this opt-in?
>> > >
>> >
>> > Nobody seems to raise concerns. If it would be just me, I would change
>> > it. But we should rather be safe, so let's make it this opt-in please.
>> 
>> Alex, Daniel, Thomas .. Do you have an opinion on changing the dump format?
>
> I think it is a bad idea for the command to output data in a different
> format, silently switching based on whether it is passed a pipe or and
> file FD.

You say that, but this exactly describes the default behavior of
makedumpfile. That may not make it *good* design, but it's at least a
point in favor of consistency =)

> Libvirt may pass either a plain file FD or a pipe FD, depending on
> whether the user set the VIR_DUMP_BYPASS_CACHE API flag. IMHO it
> is unacceptable for that to result in a change of data format as a
> silent side effect.
>
> Looking back at the history, the patches originally did NOT use the
> flatten makedumpfile format, but to ensure it was able to work with
> non-seekable files, it jumped through hoops to write to a temporary
> file first process it and then write to the real file. Switching to
> makedumpfile format was to enable it to avoid temp files and always
> be compatible with non-seekable FDs.

Thanks for taking a look at the history, this is something I should have
done. The patches need some historical footing for context. My next
series will include better investigation on the history.

That said, the flattened format is quite a compromise for users, given
that it is far less compatible, and users are given absolutely no
warning that the advertised "kdump" format is actually a format in need
of repair by way of "makedumpfile -R". As it is, we are sacrificing the
common case (writing a broadly compatible, non-flattened core dump) on
the altar of what seems like a special case (using pipes to avoid the
page cache).

I don't have the experience with the Qemu project to say what is the
best for the project, so I leave it in your capable hands. And
ultimately, relitigating the past is irrelevant. There are existing
users who will be broken if we silently change the format based on
seekability, so I can see why it is a non-starter. It's just frustrating
that now, users will need to opt-in to enable the broadly compatible
option which should have been the default in the first place.

Rgeardless, it sounds like the way forward will be to use a flag to opt
into the "non-flattened" format behavior. I'll try to get that out
tomorrow!

Thanks,
Stephen

> 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 :|