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

Marc-André Lureau <[email protected]> writes:
> Hi
>
> On Mon, Jul 17, 2023 at 8:45 PM Stephen Brennan <
> [email protected]> 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 <[email protected]>
>>
>
> Reviewed-by: Marc-André Lureau <[email protected]>
> Tested-by: Marc-André Lureau <[email protected]>
>
> 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