[dm-devel] [PATCH 0/4] dm: zoned block device fixes

[Damien Le Moal]

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 at redhat.com> 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