[Qemu-devel] Clean Block Driver Shutdown

[Qemu-devel] Clean Block Driver Shutdown

[Peter Lieven]

Hi,

I noticed that Qemu quits at several points with an exit() if the supplied parameters in the commandline are incorrect.
This at some stages happens after there have already been connections to storage backends established.
These connections are not cleanly shut down in this case. For posix file backends that doesn't matter, but for other
backends this leads to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to tcp reset.

I wonder what is the best way to fix this. A simply call to bdrv_close_all() in an atexit handler seems to work.
But is this a good solution? Maybe register this handler only until the VM starts.
Or do we need an atexit handler in each block driver that requires a clean shutdown?

Thanks,
Peter

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Kevin Wolf]

Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
> I noticed that Qemu quits at several points with an exit() if the
> supplied parameters in the commandline are incorrect. This at some
> stages happens after there have already been connections to storage
> backends established.

Maybe we need to come to the conclusion that exit() is always wrong,
even during the initialisation.

> These connections are not cleanly shut down in this case. For posix
> file backends that doesn't matter, but for other backends this leads
> to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
> tcp reset.
> 
> I wonder what is the best way to fix this. A simply call to
> bdrv_close_all() in an atexit handler seems to work.  But is this a
> good solution? Maybe register this handler only until the VM starts.
> Or do we need an atexit handler in each block driver that requires a
> clean shutdown?

No, definitely not code in every single block driver. We need to make
sure to properly clean up what has been started.

An atexit handler is probably relatively easy. I think it would be
cleaner to have proper error paths even in main(), like in every other
function. I'm not sure if this would be reasonably easy to achieve,
though.

Kevin

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Peter Lieven]

Am 17.10.2017 um 13:46 schrieb Kevin Wolf:
> Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
>> I noticed that Qemu quits at several points with an exit() if the
>> supplied parameters in the commandline are incorrect. This at some
>> stages happens after there have already been connections to storage
>> backends established.
> Maybe we need to come to the conclusion that exit() is always wrong,
> even during the initialisation.
>
>> These connections are not cleanly shut down in this case. For posix
>> file backends that doesn't matter, but for other backends this leads
>> to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
>> tcp reset.
>>
>> I wonder what is the best way to fix this. A simply call to
>> bdrv_close_all() in an atexit handler seems to work.  But is this a
>> good solution? Maybe register this handler only until the VM starts.
>> Or do we need an atexit handler in each block driver that requires a
>> clean shutdown?
> No, definitely not code in every single block driver. We need to make
> sure to properly clean up what has been started.
>
> An atexit handler is probably relatively easy. I think it would be
> cleaner to have proper error paths even in main(), like in every other
> function. I'm not sure if this would be reasonably easy to achieve,
> though.

A clean error path would obviously be a good idea. However,
the following seems to work:

diff --git a/vl.c b/vl.c
index 0723835..bed512c 100644
--- a/vl.c
+++ b/vl.c
@@ -3197,6 +3197,7 @@ int main(int argc, char **argv, char **envp)
      nb_nics = 0;

      bdrv_init_with_whitelist();
+    atexit(bdrv_close_all);

      autostart = 1;

Peter

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Stefan Hajnoczi]

On Tue, Oct 17, 2017 at 01:46:25PM +0200, Kevin Wolf wrote:
> Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
> > I noticed that Qemu quits at several points with an exit() if the
> > supplied parameters in the commandline are incorrect. This at some
> > stages happens after there have already been connections to storage
> > backends established.
> 
> Maybe we need to come to the conclusion that exit() is always wrong,
> even during the initialisation.
> 
> > These connections are not cleanly shut down in this case. For posix
> > file backends that doesn't matter, but for other backends this leads
> > to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
> > tcp reset.
> > 
> > I wonder what is the best way to fix this. A simply call to
> > bdrv_close_all() in an atexit handler seems to work.  But is this a
> > good solution? Maybe register this handler only until the VM starts.
> > Or do we need an atexit handler in each block driver that requires a
> > clean shutdown?
> 
> No, definitely not code in every single block driver. We need to make
> sure to properly clean up what has been started.
> 
> An atexit handler is probably relatively easy. I think it would be
> cleaner to have proper error paths even in main(), like in every other
> function. I'm not sure if this would be reasonably easy to achieve,
> though.

I agree that converting from exit(3) to real error handling is cleanest.
Doing so would also be a good opportunity to consolidate ad-hoc
fprintf(stderr) and error_report() calls.

Stefan

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Tue, Oct 17, 2017 at 01:46:25PM +0200, Kevin Wolf wrote:
>> Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
>> > I noticed that Qemu quits at several points with an exit() if the
>> > supplied parameters in the commandline are incorrect. This at some
>> > stages happens after there have already been connections to storage
>> > backends established.
>> 
>> Maybe we need to come to the conclusion that exit() is always wrong,
>> even during the initialisation.
>> 
>> > These connections are not cleanly shut down in this case. For posix
>> > file backends that doesn't matter, but for other backends this leads
>> > to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
>> > tcp reset.
>> > 
>> > I wonder what is the best way to fix this. A simply call to
>> > bdrv_close_all() in an atexit handler seems to work.  But is this a
>> > good solution? Maybe register this handler only until the VM starts.
>> > Or do we need an atexit handler in each block driver that requires a
>> > clean shutdown?
>> 
>> No, definitely not code in every single block driver. We need to make
>> sure to properly clean up what has been started.
>> 
>> An atexit handler is probably relatively easy. I think it would be
>> cleaner to have proper error paths even in main(), like in every other
>> function. I'm not sure if this would be reasonably easy to achieve,
>> though.
>
> I agree that converting from exit(3) to real error handling is cleanest.
> Doing so would also be a good opportunity to consolidate ad-hoc
> fprintf(stderr) and error_report() calls.

error_report() & exit() assume a certain context.  They're good enough
when the assumption obviously holds.

We also use them in code that can run when the assumption doesn't hold.
Sometimes because the code acquired new users, sometimes because the
code was always wrong.  Regardless, these are bugs in need of fixing.

We also use them in code that currently happens to run only when the
assumption holds.  Trap for the unwary, cleanup can make sense, but is
hardly a priority.

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Peter Lieven]

Am 07.11.2017 um 11:22 schrieb Markus Armbruster:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Tue, Oct 17, 2017 at 01:46:25PM +0200, Kevin Wolf wrote:
>>> Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
>>>> I noticed that Qemu quits at several points with an exit() if the
>>>> supplied parameters in the commandline are incorrect. This at some
>>>> stages happens after there have already been connections to storage
>>>> backends established.
>>> Maybe we need to come to the conclusion that exit() is always wrong,
>>> even during the initialisation.
>>>
>>>> These connections are not cleanly shut down in this case. For posix
>>>> file backends that doesn't matter, but for other backends this leads
>>>> to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
>>>> tcp reset.
>>>>
>>>> I wonder what is the best way to fix this. A simply call to
>>>> bdrv_close_all() in an atexit handler seems to work.  But is this a
>>>> good solution? Maybe register this handler only until the VM starts.
>>>> Or do we need an atexit handler in each block driver that requires a
>>>> clean shutdown?
>>> No, definitely not code in every single block driver. We need to make
>>> sure to properly clean up what has been started.
>>>
>>> An atexit handler is probably relatively easy. I think it would be
>>> cleaner to have proper error paths even in main(), like in every other
>>> function. I'm not sure if this would be reasonably easy to achieve,
>>> though.
>> I agree that converting from exit(3) to real error handling is cleanest.
>> Doing so would also be a good opportunity to consolidate ad-hoc
>> fprintf(stderr) and error_report() calls.
> error_report() & exit() assume a certain context.  They're good enough
> when the assumption obviously holds.
>
> We also use them in code that can run when the assumption doesn't hold.
> Sometimes because the code acquired new users, sometimes because the
> code was always wrong.  Regardless, these are bugs in need of fixing.
>
> We also use them in code that currently happens to run only when the
> assumption holds.  Trap for the unwary, cleanup can make sense, but is
> hardly a priority.


Of course, no priority, but how would you then handle block drivers
that need a clean shutdown? Register atexit handlers for them?

Peter

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Markus Armbruster]

Peter Lieven <pl@kamp.de> writes:

> Am 07.11.2017 um 11:22 schrieb Markus Armbruster:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>
>>> On Tue, Oct 17, 2017 at 01:46:25PM +0200, Kevin Wolf wrote:
>>>> Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
>>>>> I noticed that Qemu quits at several points with an exit() if the
>>>>> supplied parameters in the commandline are incorrect. This at some
>>>>> stages happens after there have already been connections to storage
>>>>> backends established.
>>>> Maybe we need to come to the conclusion that exit() is always wrong,
>>>> even during the initialisation.
>>>>
>>>>> These connections are not cleanly shut down in this case. For posix
>>>>> file backends that doesn't matter, but for other backends this leads
>>>>> to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
>>>>> tcp reset.
>>>>>
>>>>> I wonder what is the best way to fix this. A simply call to
>>>>> bdrv_close_all() in an atexit handler seems to work.  But is this a
>>>>> good solution? Maybe register this handler only until the VM starts.
>>>>> Or do we need an atexit handler in each block driver that requires a
>>>>> clean shutdown?
>>>> No, definitely not code in every single block driver. We need to make
>>>> sure to properly clean up what has been started.
>>>>
>>>> An atexit handler is probably relatively easy. I think it would be
>>>> cleaner to have proper error paths even in main(), like in every other
>>>> function. I'm not sure if this would be reasonably easy to achieve,
>>>> though.
>>> I agree that converting from exit(3) to real error handling is cleanest.
>>> Doing so would also be a good opportunity to consolidate ad-hoc
>>> fprintf(stderr) and error_report() calls.
>> error_report() & exit() assume a certain context.  They're good enough
>> when the assumption obviously holds.
>>
>> We also use them in code that can run when the assumption doesn't hold.
>> Sometimes because the code acquired new users, sometimes because the
>> code was always wrong.  Regardless, these are bugs in need of fixing.
>>
>> We also use them in code that currently happens to run only when the
>> assumption holds.  Trap for the unwary, cleanup can make sense, but is
>> hardly a priority.
>
>
> Of course, no priority, but how would you then handle block drivers
> that need a clean shutdown? Register atexit handlers for them?

Cleanup for cleanup's sake is not a priority.  But that's not why you're
considering it: you could use it to fix a bug (failure to finalize
certain resources on certain errors), so you don't have to atexit().
Judgement call.

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Peter Lieven]

Am 07.11.2017 um 12:02 schrieb Markus Armbruster:
> Peter Lieven <pl@kamp.de> writes:
>
>> Am 07.11.2017 um 11:22 schrieb Markus Armbruster:
>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>
>>>> On Tue, Oct 17, 2017 at 01:46:25PM +0200, Kevin Wolf wrote:
>>>>> Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
>>>>>> I noticed that Qemu quits at several points with an exit() if the
>>>>>> supplied parameters in the commandline are incorrect. This at some
>>>>>> stages happens after there have already been connections to storage
>>>>>> backends established.
>>>>> Maybe we need to come to the conclusion that exit() is always wrong,
>>>>> even during the initialisation.
>>>>>
>>>>>> These connections are not cleanly shut down in this case. For posix
>>>>>> file backends that doesn't matter, but for other backends this leads
>>>>>> to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
>>>>>> tcp reset.
>>>>>>
>>>>>> I wonder what is the best way to fix this. A simply call to
>>>>>> bdrv_close_all() in an atexit handler seems to work.  But is this a
>>>>>> good solution? Maybe register this handler only until the VM starts.
>>>>>> Or do we need an atexit handler in each block driver that requires a
>>>>>> clean shutdown?
>>>>> No, definitely not code in every single block driver. We need to make
>>>>> sure to properly clean up what has been started.
>>>>>
>>>>> An atexit handler is probably relatively easy. I think it would be
>>>>> cleaner to have proper error paths even in main(), like in every other
>>>>> function. I'm not sure if this would be reasonably easy to achieve,
>>>>> though.
>>>> I agree that converting from exit(3) to real error handling is cleanest.
>>>> Doing so would also be a good opportunity to consolidate ad-hoc
>>>> fprintf(stderr) and error_report() calls.
>>> error_report() & exit() assume a certain context.  They're good enough
>>> when the assumption obviously holds.
>>>
>>> We also use them in code that can run when the assumption doesn't hold.
>>> Sometimes because the code acquired new users, sometimes because the
>>> code was always wrong.  Regardless, these are bugs in need of fixing.
>>>
>>> We also use them in code that currently happens to run only when the
>>> assumption holds.  Trap for the unwary, cleanup can make sense, but is
>>> hardly a priority.
>>
>> Of course, no priority, but how would you then handle block drivers
>> that need a clean shutdown? Register atexit handlers for them?
> Cleanup for cleanup's sake is not a priority.  But that's not why you're
> considering it: you could use it to fix a bug (failure to finalize
> certain resources on certain errors), so you don't have to atexit().
> Judgement call.

Let me describe my motivation by a simple cmdline mounting an iSCSI target.

If I mount an iSCSI target and later in cmdline processing an error occurs. The target

is not cleanly unmounted which generates an error on the target.

Cmdline:

x86_64-softmmu/qemu-system-x86_64 -m 1024 -monitor stdio -drive if=none,format=raw,file=iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-1e542510a-c2c000000f551e51-test/0,id=test -device xxx
QEMU 2.9.0 monitor - type 'help' for more information
(qemu) qemu-system-x86_64: -device xxx: 'xxx' is not a valid device model name

What should happen here is that all block devices that have been opened up to that point have to properly closed. In case of a posix file the OS cleans up the FD, but
for other drivers like iSCSI, NFS, RBD etc. this is not the case.

Peter

Re: [Qemu-devel] [Qemu-block] Clean Block Driver Shutdown

[Kevin Wolf]

Am 07.11.2017 um 11:48 hat Peter Lieven geschrieben:
> Am 07.11.2017 um 11:22 schrieb Markus Armbruster:
> > Stefan Hajnoczi <stefanha@gmail.com> writes:
> > 
> > > On Tue, Oct 17, 2017 at 01:46:25PM +0200, Kevin Wolf wrote:
> > > > Am 17.10.2017 um 12:33 hat Peter Lieven geschrieben:
> > > > > I noticed that Qemu quits at several points with an exit() if the
> > > > > supplied parameters in the commandline are incorrect. This at some
> > > > > stages happens after there have already been connections to storage
> > > > > backends established.
> > > > Maybe we need to come to the conclusion that exit() is always wrong,
> > > > even during the initialisation.
> > > > 
> > > > > These connections are not cleanly shut down in this case. For posix
> > > > > file backends that doesn't matter, but for other backends this leads
> > > > > to errors. E.g. iSCSI Targets log an aborted iSCSI connection due to
> > > > > tcp reset.
> > > > > 
> > > > > I wonder what is the best way to fix this. A simply call to
> > > > > bdrv_close_all() in an atexit handler seems to work.  But is this a
> > > > > good solution? Maybe register this handler only until the VM starts.
> > > > > Or do we need an atexit handler in each block driver that requires a
> > > > > clean shutdown?
> > > > No, definitely not code in every single block driver. We need to make
> > > > sure to properly clean up what has been started.
> > > > 
> > > > An atexit handler is probably relatively easy. I think it would be
> > > > cleaner to have proper error paths even in main(), like in every other
> > > > function. I'm not sure if this would be reasonably easy to achieve,
> > > > though.
> > > I agree that converting from exit(3) to real error handling is cleanest.
> > > Doing so would also be a good opportunity to consolidate ad-hoc
> > > fprintf(stderr) and error_report() calls.
> > error_report() & exit() assume a certain context.  They're good enough
> > when the assumption obviously holds.
> > 
> > We also use them in code that can run when the assumption doesn't hold.
> > Sometimes because the code acquired new users, sometimes because the
> > code was always wrong.  Regardless, these are bugs in need of fixing.
> > 
> > We also use them in code that currently happens to run only when the
> > assumption holds.  Trap for the unwary, cleanup can make sense, but is
> > hardly a priority.
> 
> Of course, no priority, but how would you then handle block drivers
> that need a clean shutdown? Register atexit handlers for them?

These are cases where the assumption doesn't hold, so they might in fact
be a priority.

I'm not sure if there are any cases of the assumption obviously holding
apart from the first two calls in main(), but at least we know that the
assumption obviously breaks after creating block devices starting in
vl.c:4654, which at least includes the rest of main() itself as well as
the creation of the machine and the device models.

Kevin