Re: [dm-devel] [dm-6.4 PATCH 5/8] dm bufio: improve concurrent IO performance

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Mar 23 2023 at  3:51P -0400,
Joe Thornber <thornber@redhat.com> wrote:

> Mike,
> 
> I'm Nacking this patch here.  You've taken the comment from my latest
> patch, and attached it to something else.  I don't know if you've taken an
> older patch and updated it, or taken the latest patch and downgraded it.

I lean on git to tell me if code is identical.

Since we're now clearly keeping score here. I used git to compare what
I had here (with various small whitespace tweaks and other nits):
https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=2023-03-01-thin-concurrency-8

with your -8 code:
https://github.com/jthornber/linux/tree/2023-03-21-thin-concurrency-8

I had to deal with the problem that you didn't use my tree you rebased
so I had to reconcile not losing my handful of fluff.

I leaned on the fact that your -7 and -8 were identical in code:
git diff ejt/2023-02-28-thin-concurrency-7 ejt/2023-03-21-thin-concurrency-8 -- drivers/md/dm-bufio.c

I could've just generated my fluff and folded it into your new clean
-8.  But I went the other way around, instead I picked up your other
incremental changes from -7 and folded them into what I had. No real
reason, that's just how I did it.

> For instance the latest patch uses the name 'struct dm_buffer_cache', but
> it looks like you've renamed to 'struct buffer_cache' and then posted
> another patch in the series that renames back to dm_buffer_cache.

Right, I was cherry-picking your incremantal changes your staged in -7
before your clean patch rollup in your -8. How the struct rename
occurred to the code didn't much matter to me, so long as the change
was made.  But yeah I should've just folded it.  Not a big deal.

> I also see the comment change:
> 
> original "A refcount_t for hold_count does not work." which has changed to
> "NOTE: attempting to use refcount_t instead of atomic_t caused crashes!".
> Talk about over dramatisation (If the hold_count is incorrect we detect and
> BUG).

Yes, but I was aiming to leave breadcrumbs if/when someone might think
to retry using refcount_t to see why it "does not work". I was trying
to save future me or you or someone else some time. By making it
clear: there will be dragons.

But in doing so I unwittingly already unleashed different dragons ;)

> I can't stand by my code if I don't know what's being sent up in my name.
>
> Please use the patchset from this branch:
> https://github.com/jthornber/linux/tree/2023-03-21-thin-concurrency-8

We've resolved this now interactively in chat, but I had to reply on
dm-devel to let others know.  Again, on a code level what I posted to
dm-devel reflects what was staged in 2023-03-21-thin-concurrency-8:

For me this yields my fluff I was referring to above:
git diff ejt/2023-03-21-thin-concurrency-8 dm/dm-6.4 -- drivers/md/dm-bufio.c

Biggest difference being dm_buffer struct member reordering (which I
had discussed with you).

For the benefit of others, I since dropped patch 1 and patch 2
(max_discard_granularity and bio-prison-v1 patch) but pulled them to
the HEAD of this new development branch (that has extra debugging):
https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=dm-6.4

I'll go back to bed now, hopefully you'll have this discard issue
fixed in a couple hours when I return (as we discussed). ;)

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel