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

Daniel P. Berrangé <[email protected]> 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
>> <[email protected]> wrote:
>> >
>> > Hi
>> >
>> > On Wed, Aug 23, 2023 at 4:31 AM Stephen Brennan
>> > <[email protected]> wrote:
>> > >
>> > > Stephen Brennan <[email protected]> writes:
>> > > > Marc-André Lureau <[email protected]> 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 :|