Re: [Qemu-devel] [PATCH 1/5] ide/atapi: make PIO read requests async

[Peter Lieven]

Am 09.10.2015 um 10:21 schrieb Kevin Wolf:
> Am 08.10.2015 um 18:44 hat John Snow geschrieben:
>> On 10/08/2015 08:06 AM, Peter Lieven wrote:
>>> Hi all,
>>>
>>> short summary from my side. The whole thing seems to get complicated,
>>> let me explain why:
>>>
>>> 1) During review I found that the code in ide_atapi_cmd_reply_end can't
>>> work correctly if the
>>> byte_count_limit is not a divider or a multiple of cd_sector_size. The
>>> reason is that as soon
>>> as we load the next sector we start at io_buffer offset 0 overwriting
>>> whatever is left in there
>>> for transfer. We also reset the io_buffer_index to 0 which means if we
>>> continue with the
>>> elementary transfer we always transfer a whole sector (of corrupt data)
>>> regardless if we
>>> are allowed to transfer that much data. Before we consider fixing this I
>>> wonder if it
>>> is legal at all to have an unaligned byte_count_limit. It obviously has
>>> never caused trouble in
>>> practice so maybe its not happening in real life.
>>>
>> I had overlooked that part. Good catch. I do suspect that in practice
>> nobody will be asking for bizarre values.
>>
>> There's no rule against an unaligned byte_count_limit as far as I have
>> read, but suspect nobody would have a reason to use it in practice.
> If we know that our code is technically wrong, "nobody uses it" is not a
> good reason not to fix it. Because sooner or later someone will use it.

I found that I just misinterpreted the code. I think its correct altough
its not nice.

>
>>> 2) I found that whatever cool optimization I put in to buffer multiple
>>> sectors at once I end
>>> up with code that breaks migration because older versions would either
>>> not fill the io_buffer
>>> as expected or we introduce variables that older versions do not
>>> understand. This will
>>> lead to problems if we migrate in the middle of a transfer.
>>>
>> Ech. This sounds like a bit of a problem. I'll need to think about this
>> one...
> Sounds like a textbook example for subsections to me?

I wonder if we need this at all. Its just PIO. However, Windows and Linux
fall back to PIO if I disconnect the NFS Server for some time. With the
latest version of my patch series this works fine now and even works
fine after restoring NFS connectivy. This was not properly working
because of the glitch that John found.

I have an optimization in mind that might work and will disable the need
for the sync requests while keeping migration possible.
I hope to be able to complete this by Monday and send out a V2 then.

If someone wants to have a look at what I currently have (especially
the cancelable requests part) its here:
https://github.com/plieven/qemu/commits/atapi_async_V2

Thanks,
Peter