[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 1/6] block/nvme: don't touch the completion e
Re: [Qemu-block] [PATCH v3 1/6] block/nvme: don't touch the completion entries
Mon, 8 Jul 2019 15:00:53 +0200
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0
On 08.07.19 14:51, Maxim Levitsky wrote:
> On Mon, 2019-07-08 at 14:23 +0200, Max Reitz wrote:
>> On 07.07.19 10:43, Maxim Levitsky wrote:
>>> On Fri, 2019-07-05 at 13:03 +0200, Max Reitz wrote:
>>>> On 03.07.19 17:59, Maxim Levitsky wrote:
>>>>> Completion entries are meant to be only read by the host and written by
>>>>> the device.
>>>>> The driver is supposed to scan the completions from the last point where
>>>>> it left,
>>>>> and until it sees a completion with non flipped phase bit.
>>>> (Disclaimer: This is the first time I read the nvme driver, or really
>>>> something in the nvme spec.)
>>>> Well, no, completion entries are also meant to be initialized by the
>>>> host. To me it looks like this is the place where that happens:
>>>> Everything that has been processed by the device is immediately being
>>>> Maybe we shouldn’t do that here but in nvme_submit_command(). But
>>>> currently we don’t, and I don’t see any other place where we currently
>>>> initialize the CQ entries.
>>> I couldn't find any place in the spec that says that completion entries
>>> should be initialized.
>>> It is probably wise to initialize that area to 0 on driver initialization,
>>> but nothing beyond that.
>> Ah, you’re right, I misread. I didn’t pay as much attention to the
>> “...prior to setting CC.EN to ‘1’” as I should have. Yep, and that is
>> done in nvme_init_queue().
>> OK, I cease my wrongful protest:
>> Reviewed-by: Max Reitz <address@hidden>
> Thank you very much!
> BTW, the qemu driver does allocate zeroed memory (in nvme_init_queue,
> "q->queue = qemu_try_blockalign0(bs, bytes);"
Yes, that’s what I was referring to above. :-)
> Thus I think this is all that is needed in that regard.
> Note that this patch doesn't fix any real bug I know of,
> but just makes the thing right in regard to the spec.
> Also racing with hardware in theory can have various memory ordering bugs,
> although in this case the writes are done in
> entries which controller probably won't touch, but still.
> TL;DR - no need in code which does nothing and might cause issues.
> Do you want me to resend the series or shall I wait till we decide
> what to do with the image creation support? I done fixing all the
> review comments long ago, just didn't want to resend the series.
> Or shall I drop that patch and resend?
I think I won’t apply the image creation patch now, so it’s probably
better to just drop it for now.
> From the urgency standpoint the only patch that really should
> be merged ASAP is the one that adds support for block sizes,
> because without it, the whole thing crashes and burns on 4K
> nvme drives.
By now we’re in softfreeze anyway, so unless write-zeroes/discard
support is important now, it’s difficult to justify taking them for 4.1.
So for me it would be best if you put patches 1 through 3 into a
for-4.1 series and move the rest to 4.2. (I’d probably also split the
creation patch off, because I don’t think I’m going to apply it before
having experimented a bit with blockdev-create for qemu-img.)
If you think write-zeroes/discard support is important for 4.1, feel
free to include them in the for-4.1 series along with an explanation as
to why it’s important.
Description: OpenPGP digital signature
[Qemu-block] [PATCH v3 2/6] block/nvme: fix doorbell stride, Maxim Levitsky, 2019/07/03
[Qemu-block] [PATCH v3 3/6] block/nvme: support larger that 512 bytes sector devices, Maxim Levitsky, 2019/07/03
[Qemu-block] [PATCH v3 5/6] block/nvme: add support for write zeros, Maxim Levitsky, 2019/07/03