Mike, Thank you very much for the very detailed review and all the cleanup. On 6/2/17 09:36, Mike Snitzer wrote: > On Wed, May 31 2017 at 10:39am -0400, > Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > >> FYI: my review of dm-zoned will be focused on DM target correctness >> (suspend/resume quirks, no allocations in the IO path that aren't backed >> by a mempool, coding style nits, etc). I don't know enough about zoned >> block devices to weigh-in on those details. Ultimately I'll be >> deferring to you, others on your team, and others in the community that >> are more invested in zoned block devices to steer and stabilize this >> target. >> >> Anyway, hopefully my review will be fairly quick and I can get dm-zoned >> staged for 4.13 by end of day tomorrow. > > I made a go of it but I'm getting hung up on quite a lot of code that > doesn't conform to, what I'd like to think is, the cleaner nature of how > DM targets that are split across multiple files should be. > > You basically slammed everything into 'struct dmz_target' and passed dmz > everywhere. I tried to split out a 'struct dmz_metadata' (and got quite > far!) but finally gave up because affecting that churn was killing me > slowly. Anyway, here is where I left off: > https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=dm-zoned > > In hindsight, maybe I should've just responded with the laundry list of > things I saw so that you could fix them. But if you see changes that > you like in that branch feel free to pull them in to a new version of > dm-zoned that you resubmit. > > As for splitting out a 'struct dmz_metadata'.. I'd really prefer _some_ > separation but there is little point with doing so if we're going to > just half-ass it and add in a back-pointer to the 'struct dmz_target' to > access certain members. I was left unhappy with my attempt.. again, was > a shit-show of churn. I continued and finished the separation. There is no back-pointer anymore. All functions in dm-zoned-metadata.c take the metadata struct pointer as argument. Other functions in dm-zoned-target.c and dm-zoned-reclaim.c take the dmz_target pointer as argument. To do this, I had to add a "struct dmz_dev" to store the static information about the device being used (bdev, name, capacity, number of zones, zone size). This does cleanup further the initialization and tear-down path. I also moved around and renamed some functions to further cleanup the code and make it easier to read. At this point, I think it would be easy to also separate all fields needed for reclaim from the dmz_target structure. But I have not pushed changes this far as the amount of data needed for reclaim is rather small. > I think this target needs a more critical eye on the various places IO > is being submitted and where allocations are occuring. I allowed myself > to get hung up on code movement when I should've focused on more > constructive design choices you made. I did address some of the "FIXME" notes you added. The main one is the BIO cloning in the I/O path. I removed most of that and added a .end_io method for completion processing. The only place were I do not see how to remove the call to bio_clone() is during read BIO processing: since a read BIO may end up being split between buffer zone, sequential zone and simple buffer zero-out, fragmentation of the read BIO is sometimes necessary and so need a clone. There is one "FIXME" that I did not address: the allocation of metadata blocks on cache miss. This is in the I/O path, but called only from the context of the chunk workers, so a different context than the BIO submit one. I do not see a problem with this. Please let me know if you would prefer another solution. I am running tests against the new version created with all these changes. If everything goes well, I will send it out tomorrow. Best regards. -- Damien Le Moal, Western Digital -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
