| Message ID | 20230717163855.7383-3-stephen.s.brennan@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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.. --- > 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 <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
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
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.
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
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.
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
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
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
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 :|
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;
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(-)