- User Since
- Jun 3 2014, 6:27 PM (215 w, 4 d)
Tue, Jul 17
The general motivation is good for me. Very alike approach is used in isp(4) driver, may be just for some additional reasons. But unless I missed something, I think this patch of yours is incomplete -- it implements the translation table for initiator role, but not for target. If the HBA is capable of playing both roles same time, there will be a mess in CAM and CTL. isp(4) driver originally also had the same issue, but I fixed it there few years ago, and now target and initiator roles may successfully coexist.
Mon, Jul 16
Mon, Jul 9
I have no objections.
Fri, Jul 6
Thank you! Looks good to me, aside of mentioned by Rod err()/errx() issues. All perror()'s should probably turn into err() instead of errx().
Tue, Jul 3
On a quick look I spotted number of style issues you may fix.
Mon, Jul 2
Fri, Jun 29
Jun 14 2018
Jun 12 2018
I like the space cleanup, thanks, and SPDX part is also fine. But I also agree that mixing together unrelated changes is not a very good idea. While in this particular case I would personally not create much churn about it (much worse when formatting changes are mixed with code changes, which is not really a case here), once other people complained, I think Marcelo you could faster do the requested separation rather then argue indefinitely from weak position.
Jun 8 2018
Jun 6 2018
Looks good to me. Thanks you. I agree with avg@ that forward declaration of the function of few lines is not very useful.
May 31 2018
Ah, sorry, I've mixed biodone() with bufdone(). Then it is less bad idea.
I can't say that I like it, but it makes sense to me.
May 29 2018
May 26 2018
I don't think it is a very good idea to merge two pretty much independent ZoL commits into one FreeBSD commit. It could have sense if the first one would be significantly broken and second fixed the issue, but priority upgrade implemented in second commit is only a performance optimization, mostly unrelated to scrub even, in which case having it separate would make commit history cleaner for later comparison.
May 25 2018
Makes sense to me.
May 24 2018
As I have told, I have no objections. I still don't like the global counters, since they mean we can never remove respective lock or at least atomics, but for now I can live with that. It is better then broken code.
May 23 2018
May 22 2018
Looks good to me. The only rough edge I see is that outstanding_cmds is used only for the newly added assertion, that is why it was so broken before this and nobody cared.
May 21 2018
Great to see it! I did only first on-surface look so far, so comments are mostly cosmetic.
May 17 2018
It is OK to me. I still don't think protection against GEOM bugs belongs here, but so be it, since we any way have and acquire lock to protect that.
May 16 2018
I agree with John that this is better then nothing, but the proper way I think would be: first suspend third-party devices, then stop clock, and then suspend timer devices, resume -- in opposite order.
May 14 2018
The motivation for postponing the calibration was to not spend whole second on boot in cases when LAPIC timer is not used. Though that should be much less frequent lately.
May 9 2018
May 7 2018
Something made me to comment out hpet_disable() call on suspend 8 years ago. Wish I remember what was that.
May 5 2018
May 4 2018
Some more man/help tunings and so be it.
It looks good enough for me. I see some rough edges in details of SCSI interface implementation (for example, there seem to be a race window between regular and task management requests processing, submitted via different queues), but those may be addressed/improved later.
This looks like a hack, but I have no real objections.
May 2 2018
Apr 30 2018
Apr 25 2018
Looks mostly fine aside of commented places.
Apr 19 2018
Apr 18 2018
Apr 17 2018
Apr 16 2018
Apr 13 2018
Apr 11 2018
Is this API and device classification exist somewhere else, or being just designed? Differentiation between STRIPE, MIRROR and RAID sound somewhat odd to me, same as between SPINNING and CDROM. Besides it duplicates "GEOM::rotation_rate" attribute, we already have and use in some places (though obviously it is not passed through UFS).
Apr 9 2018
I am not closely familiar with WITNESS, so just a feeling: the long lists of blessed locks and their combinations promises high chances for them to be forgotten on following ZFS updates. At very list it would be good to document how those new mechanisms should be used.