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 >>>> re-initialized. >>>> >>>> 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. >>> >>> Hi! >>> 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 <mre...@redhat.com> >> >>> > > 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. Max
signature.asc
Description: OpenPGP digital signature