Re: [Qemu-devel] [PATCH] bdrv_flush error handling
On Thu, Feb 21, 2008 at 11:28:23AM -0600, [EMAIL PROTECTED] wrote: > On Thu, Feb 21, 2008 at 05:24:10PM +, Daniel P. Berrange wrote: > > On Thu, Feb 21, 2008 at 12:19:22PM -0500, Ben Taylor wrote: > > > > > Also, in block-raw-posix.c, raw_pwrite et al seem to return -1 on > > > > > error (the return value from write) whereas the other block read/write > > > > > methods return errno values. This is a mistake, surely ? -1 would be > > > > > -EPERM. If any of the callers did anything with these return values > > > > > you'd get incorrect error indications. > > > > > > > > > > > > > > > Finally, it would perhaps be best if the block device emulators wrote > > > > > to the qemu console to complain if they give write errors. Otherwise > > > > > the errno value and other important information will be lost, which > > > > > makes debugging hard. > > > > > > > > If by 'qemu console' you mean stderr, then fine, but please don't > > > > spew log messages to the monitor console, because that'll make it > > > > very hard to interact with reliably from management tools. > > > > > > Would it make sense to have a log messages screen associated with > > > the monitor (like Ctrl-Alt-7) to deal with those sorts of things? > > > > Why invent a new special QEMU log screen, when stderr works just fine. If an > > app wants to capture log messages they just capture stderr and persist it. > > We already capture stderr for exactly this reason in libvirt when managing > > QEMU instances. > > Please excuse my intrusion in this thread, but I'm a user of the new > ncurses user interface. when ssh'd in, running qemu, I don't believe > having messages pop out of stderr and over the current screen contents is > the appropriate behavior, as it sounds to me like it would cause redraw > defects in the normal text console (via ncurses) So just redirect stderr to a logfile, or /dev/null, etc, etc Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
On Thu, Feb 21, 2008 at 05:24:10PM +, Daniel P. Berrange wrote:
> On Thu, Feb 21, 2008 at 12:19:22PM -0500, Ben Taylor wrote:
> >
> > "Daniel P. Berrange" <[EMAIL PROTECTED]> wrote:
> > > On Wed, Feb 20, 2008 at 03:53:46PM +, Ian Jackson wrote:
> > > Content-Description: message body text
> > > > bdrv_flush is declared to return void, but this is wrong because it
> > > > means that the implementations have nowhere to report their errors.
> > > > Indeed, the implementations generally ignore errors.
> > > >
> > > > This patch corrects this by making it return int (implicitly, either 0
> > > > or -errno, as for other similar functions). All of the
> > > > implementations and callers are adjusted too.
> > > >
> > > >
> > > > While looking at this I was surprised to see this:
> > > >
> > > > static void scsi_write_complete(void * opaque, int ret)
> > > > ...
> > > > if (ret) {
> > > > fprintf(stderr, "scsi-disc: IO write error\n");
> > > > exit(1);
> > > > }
> > > >
> > > > Surely that is overkill ?
> > >
> > > Yes, any i/o errors I'd expect to be propagated back up to the guest
> > > OS as the most appropriate IDE / SCSI error code.
> > >
> > > > Also, in block-raw-posix.c, raw_pwrite et al seem to return -1 on
> > > > error (the return value from write) whereas the other block read/write
> > > > methods return errno values. This is a mistake, surely ? -1 would be
> > > > -EPERM. If any of the callers did anything with these return values
> > > > you'd get incorrect error indications.
> > > >
> > > >
> > > > Finally, it would perhaps be best if the block device emulators wrote
> > > > to the qemu console to complain if they give write errors. Otherwise
> > > > the errno value and other important information will be lost, which
> > > > makes debugging hard.
> > >
> > > If by 'qemu console' you mean stderr, then fine, but please don't
> > > spew log messages to the monitor console, because that'll make it
> > > very hard to interact with reliably from management tools.
> >
> > Would it make sense to have a log messages screen associated with
> > the monitor (like Ctrl-Alt-7) to deal with those sorts of things?
>
> Why invent a new special QEMU log screen, when stderr works just fine. If an
> app wants to capture log messages they just capture stderr and persist it.
> We already capture stderr for exactly this reason in libvirt when managing
> QEMU instances.
>
> Dan,
> --
> |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
> |=- Perl modules: http://search.cpan.org/~danberr/ -=|
> |=- Projects: http://freshmeat.net/~danielpb/ -=|
> |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
>
Please excuse my intrusion in this thread, but I'm a user of the new
ncurses user interface. when ssh'd in, running qemu, I don't believe
having messages pop out of stderr and over the current screen contents is
the appropriate behavior, as it sounds to me like it would cause redraw
defects in the normal text console (via ncurses)
Julia Longtin <[EMAIL PROTECTED]>
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
On Thu, Feb 21, 2008 at 12:19:22PM -0500, Ben Taylor wrote:
>
> "Daniel P. Berrange" <[EMAIL PROTECTED]> wrote:
> > On Wed, Feb 20, 2008 at 03:53:46PM +, Ian Jackson wrote:
> > Content-Description: message body text
> > > bdrv_flush is declared to return void, but this is wrong because it
> > > means that the implementations have nowhere to report their errors.
> > > Indeed, the implementations generally ignore errors.
> > >
> > > This patch corrects this by making it return int (implicitly, either 0
> > > or -errno, as for other similar functions). All of the
> > > implementations and callers are adjusted too.
> > >
> > >
> > > While looking at this I was surprised to see this:
> > >
> > > static void scsi_write_complete(void * opaque, int ret)
> > > ...
> > > if (ret) {
> > > fprintf(stderr, "scsi-disc: IO write error\n");
> > > exit(1);
> > > }
> > >
> > > Surely that is overkill ?
> >
> > Yes, any i/o errors I'd expect to be propagated back up to the guest
> > OS as the most appropriate IDE / SCSI error code.
> >
> > > Also, in block-raw-posix.c, raw_pwrite et al seem to return -1 on
> > > error (the return value from write) whereas the other block read/write
> > > methods return errno values. This is a mistake, surely ? -1 would be
> > > -EPERM. If any of the callers did anything with these return values
> > > you'd get incorrect error indications.
> > >
> > >
> > > Finally, it would perhaps be best if the block device emulators wrote
> > > to the qemu console to complain if they give write errors. Otherwise
> > > the errno value and other important information will be lost, which
> > > makes debugging hard.
> >
> > If by 'qemu console' you mean stderr, then fine, but please don't
> > spew log messages to the monitor console, because that'll make it
> > very hard to interact with reliably from management tools.
>
> Would it make sense to have a log messages screen associated with
> the monitor (like Ctrl-Alt-7) to deal with those sorts of things?
Why invent a new special QEMU log screen, when stderr works just fine. If an
app wants to capture log messages they just capture stderr and persist it.
We already capture stderr for exactly this reason in libvirt when managing
QEMU instances.
Dan,
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
"Daniel P. Berrange" <[EMAIL PROTECTED]> wrote:
> On Wed, Feb 20, 2008 at 03:53:46PM +, Ian Jackson wrote:
> Content-Description: message body text
> > bdrv_flush is declared to return void, but this is wrong because it
> > means that the implementations have nowhere to report their errors.
> > Indeed, the implementations generally ignore errors.
> >
> > This patch corrects this by making it return int (implicitly, either 0
> > or -errno, as for other similar functions). All of the
> > implementations and callers are adjusted too.
> >
> >
> > While looking at this I was surprised to see this:
> >
> > static void scsi_write_complete(void * opaque, int ret)
> > ...
> > if (ret) {
> > fprintf(stderr, "scsi-disc: IO write error\n");
> > exit(1);
> > }
> >
> > Surely that is overkill ?
>
> Yes, any i/o errors I'd expect to be propagated back up to the guest
> OS as the most appropriate IDE / SCSI error code.
>
> > Also, in block-raw-posix.c, raw_pwrite et al seem to return -1 on
> > error (the return value from write) whereas the other block read/write
> > methods return errno values. This is a mistake, surely ? -1 would be
> > -EPERM. If any of the callers did anything with these return values
> > you'd get incorrect error indications.
> >
> >
> > Finally, it would perhaps be best if the block device emulators wrote
> > to the qemu console to complain if they give write errors. Otherwise
> > the errno value and other important information will be lost, which
> > makes debugging hard.
>
> If by 'qemu console' you mean stderr, then fine, but please don't
> spew log messages to the monitor console, because that'll make it
> very hard to interact with reliably from management tools.
Would it make sense to have a log messages screen associated with
the monitor (like Ctrl-Alt-7) to deal with those sorts of things?
Ben
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Sometimes the guest intentionally seeks the error. Example? TrueCrypt 5.0 supports encryption of the full system disk. To get the real size of the disk, the truecrypt driver queries the number of blocks of the disk, but then tries to read after the last block, until it gets an error. Qemu hangs in this operation. So, for me, the blockdriver has to give the errors back to the guest (in this case, reading behind eof). In no cases, qemu should die because of that. Tom PS: I had to patch truecrypt because of this qemu feature.
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
On Wednesday 20 February 2008 01:01:33 pm Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> > On Wed, Feb 20, 2008 at 04:31:56PM +, Jamie Lokier wrote:
> >> Ian Jackson wrote:
> >>> Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error
handling"):
> >>>> Disk full is a fundamentally unfriendly situation to be in. There is
> >>>> no good answer. Reporting errors back to the host has its own set of
> >>>> problems. Many guest OS effectively just lock up when this occurs.
> >>>
> >>> I think that's fine, surely ? A locked up guest isn't very nice but
> >>> it's better than a guest shot dead.
> >>
> >> Well, a guest which receives an IDE write error might do things like
> >> mark parts of the device bad, to avoiding writing to those parts. If
> >> the guest is running software RAID, for example, it will radically
> >> change its behaviour in response to those errors.
> >>
> >> Sometimes that's what you want, but sometimes it is really unwanted.
> >> If the host runs out of disk space, ideally you might want to suspend
> >> the guest until you can free up host disk space (or move to another
> >> host), then resume the guest, perhaps manually.
> >>
> >> Is savevm-upon-disk-full a realistic prospect?
> >
> > In the 'out of disk space' scenario you wouldn't need to save the guest -
> > merely stop its CPU. This gives the host admin the opportunity to hot-add
> > storage to the host & then resume execution, or to kill the VM, or to
> > free enough space to save the VM to disk / live migrate it to another
> > host.
>
> I agree. Stopping the CPUs and spitting out a big fat warning message
> would be the best thing to do. For the average user, this would give
> the opportunity to free up some space if possible.
agreed.
>
> Regards,
>
> Anthony Liguori
>
> > Shooting it dead on any I/O error doesn't give the host admin any choices
> > at all
> >
> > Dan.
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Daniel P. Berrange wrote:
On Wed, Feb 20, 2008 at 04:31:56PM +, Jamie Lokier wrote:
Ian Jackson wrote:
Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
Disk full is a fundamentally unfriendly situation to be in. There is no good
answer. Reporting errors back to the host has its own set of problems. Many
guest OS effectively just lock up when this occurs.
I think that's fine, surely ? A locked up guest isn't very nice but
it's better than a guest shot dead.
Well, a guest which receives an IDE write error might do things like
mark parts of the device bad, to avoiding writing to those parts. If
the guest is running software RAID, for example, it will radically
change its behaviour in response to those errors.
Sometimes that's what you want, but sometimes it is really unwanted.
If the host runs out of disk space, ideally you might want to suspend
the guest until you can free up host disk space (or move to another
host), then resume the guest, perhaps manually.
Is savevm-upon-disk-full a realistic prospect?
In the 'out of disk space' scenario you wouldn't need to save the guest - merely
stop its CPU. This gives the host admin the opportunity to hot-add storage
to the host & then resume execution, or to kill the VM, or to free enough
space to save the VM to disk / live migrate it to another host.
I agree. Stopping the CPUs and spitting out a big fat warning message
would be the best thing to do. For the average user, this would give
the opportunity to free up some space if possible.
Regards,
Anthony Liguori
Shooting it dead on any I/O error doesn't give the host admin any choices
at all
Dan.
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
On Wed, Feb 20, 2008 at 04:31:56PM +, Jamie Lokier wrote:
> Ian Jackson wrote:
> > Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> > > Disk full is a fundamentally unfriendly situation to be in. There is no
> > > good
> > > answer. Reporting errors back to the host has its own set of problems.
> > > Many
> > > guest OS effectively just lock up when this occurs.
> >
> > I think that's fine, surely ? A locked up guest isn't very nice but
> > it's better than a guest shot dead.
>
> Well, a guest which receives an IDE write error might do things like
> mark parts of the device bad, to avoiding writing to those parts. If
> the guest is running software RAID, for example, it will radically
> change its behaviour in response to those errors.
>
> Sometimes that's what you want, but sometimes it is really unwanted.
> If the host runs out of disk space, ideally you might want to suspend
> the guest until you can free up host disk space (or move to another
> host), then resume the guest, perhaps manually.
>
> Is savevm-upon-disk-full a realistic prospect?
In the 'out of disk space' scenario you wouldn't need to save the guest - merely
stop its CPU. This gives the host admin the opportunity to hot-add storage
to the host & then resume execution, or to kill the VM, or to free enough
space to save the VM to disk / live migrate it to another host.
Shooting it dead on any I/O error doesn't give the host admin any choices
at all
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Paul Brook wrote: > > Is savevm-upon-disk-full a realistic prospect? > > Not particularly useful. If you're run out of disk space, chances are that > savevm will also fail. I'm imagining (a) that the savevm space is preallocated, or (b) is on a different disk. -- Jamie
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
> Is savevm-upon-disk-full a realistic prospect? Not particularly useful. If you're run out of disk space, chances are that savevm will also fail. Paul
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Ian Jackson wrote:
> Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> > Disk full is a fundamentally unfriendly situation to be in. There is no
> > good
> > answer. Reporting errors back to the host has its own set of problems. Many
> > guest OS effectively just lock up when this occurs.
>
> I think that's fine, surely ? A locked up guest isn't very nice but
> it's better than a guest shot dead.
Well, a guest which receives an IDE write error might do things like
mark parts of the device bad, to avoiding writing to those parts. If
the guest is running software RAID, for example, it will radically
change its behaviour in response to those errors.
Sometimes that's what you want, but sometimes it is really unwanted.
If the host runs out of disk space, ideally you might want to suspend
the guest until you can free up host disk space (or move to another
host), then resume the guest, perhaps manually.
Is savevm-upon-disk-full a realistic prospect?
-- Jamie
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> Disk full is a fundamentally unfriendly situation to be in. There is no good
> answer. Reporting errors back to the host has its own set of problems. Many
> guest OS effectively just lock up when this occurs.
I think that's fine, surely ? A locked up guest isn't very nice but
it's better than a guest shot dead.
Ian.
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
> Write errors for non-raw formats can easily be caused by a disk full > situation on the host. Killing the guest hard is unfriendly for that > situation. Disk full is a fundamentally unfriendly situation to be in. There is no good answer. Reporting errors back to the host has its own set of problems. Many guest OS effectively just lock up when this occurs. Paul
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Avi Kivity writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> For non-raw formats, you can pass through errors on data, but it is
> impossible to recover on metadata errors, so dying on I/O error should
> be fine.
You mean metadata write errors I assume. I don't see why a metadata
read error is any worse than any other read error.
The guest will often prefer to be told that the volume was broken and
then to be denied the ability to continue accessing it. Who knows
what the guest thinks of the device ? Perhaps it's an unimportant
peripheral, or simulated network block device, used only for data
interchange.
Write errors for non-raw formats can easily be caused by a disk full
situation on the host. Killing the guest hard is unfriendly for that
situation.
Ian.
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Paul Brook wrote: Finally, it would perhaps be best if the block device emulators wrote to the qemu console to complain if they give write errors. Otherwise the errno value and other important information will be lost, which makes debugging hard. If by 'qemu console' you mean stderr, then fine, but please don't spew log messages to the monitor console, because that'll make it very hard to interact with reliably from management tools. Actually I think a better default would be for qemu to die right there and then. If the host is getting IO errors then something is badly wrong, and you're probably screwed anyway. If you're passing through a real disk or volume, this denies the guest the possibility of recover y (for example, if it is running a RAID setup over multiple volumes, or if you are testing the guest's ability to recover from errors). For non-raw formats, you can pass through errors on data, but it is impossible to recover on metadata errors, so dying on I/O error should be fine. -- Any sufficiently difficult bug is indistinguishable from a feature.
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
> > Finally, it would perhaps be best if the block device emulators wrote > > to the qemu console to complain if they give write errors. Otherwise > > the errno value and other important information will be lost, which > > makes debugging hard. > > If by 'qemu console' you mean stderr, then fine, but please don't > spew log messages to the monitor console, because that'll make it > very hard to interact with reliably from management tools. Actually I think a better default would be for qemu to die right there and then. If the host is getting IO errors then something is badly wrong, and you're probably screwed anyway. Paul
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error
handling"):
> On Wed, Feb 20, 2008 at 03:53:46PM +, Ian Jackson wrote:
> > Finally, it would perhaps be best if the block device emulators wrote
> > to the qemu console to complain if they give write errors. Otherwise
> > the errno value and other important information will be lost, which
> > makes debugging hard.
>
> If by 'qemu console' you mean stderr, then fine, but please don't
> spew log messages to the monitor console, because that'll make it
> very hard to interact with reliably from management tools.
Is it permitted, then, to generally print to qemu's stderr from inside
a device model ? This seems to be done quite a bit during
initialisation, and quite a bit in various rather less common kinds of
device, but not routinely in the main emulations.
Perhaps it would be better to invent a `logprintf' function to make it
easier to redirect these kind of messages later if that turns out to
be desirable ?
Ian.
Re: [Qemu-devel] [PATCH] bdrv_flush error handling
On Wed, Feb 20, 2008 at 03:53:46PM +, Ian Jackson wrote:
Content-Description: message body text
> bdrv_flush is declared to return void, but this is wrong because it
> means that the implementations have nowhere to report their errors.
> Indeed, the implementations generally ignore errors.
>
> This patch corrects this by making it return int (implicitly, either 0
> or -errno, as for other similar functions). All of the
> implementations and callers are adjusted too.
>
>
> While looking at this I was surprised to see this:
>
> static void scsi_write_complete(void * opaque, int ret)
> ...
> if (ret) {
> fprintf(stderr, "scsi-disc: IO write error\n");
> exit(1);
> }
>
> Surely that is overkill ?
Yes, any i/o errors I'd expect to be propagated back up to the guest
OS as the most appropriate IDE / SCSI error code.
> Also, in block-raw-posix.c, raw_pwrite et al seem to return -1 on
> error (the return value from write) whereas the other block read/write
> methods return errno values. This is a mistake, surely ? -1 would be
> -EPERM. If any of the callers did anything with these return values
> you'd get incorrect error indications.
>
>
> Finally, it would perhaps be best if the block device emulators wrote
> to the qemu console to complain if they give write errors. Otherwise
> the errno value and other important information will be lost, which
> makes debugging hard.
If by 'qemu console' you mean stderr, then fine, but please don't
spew log messages to the monitor console, because that'll make it
very hard to interact with reliably from management tools.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
