Re: [dm-devel] [PATCH v2 00/39] Add the dm-vdo deduplication and compression device mapper target.

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Jul 28 2023 at  4:28P -0400,
Ken Raeburn <raeburn@redhat.com> wrote:

> 
> Mike Snitzer <snitzer@kernel.org> writes:
> > Thanks for the extra context, but a _big_ elephant in the room for
> > this line of discussion is that: the Linux workqueue code has
> > basically always been only available for use by GPL'd code.  Given
> > VDO's historic non-GPL origins, it seems _to me_ that an alternative
> > to Linux's workqueues had to be created to allow VDO to drive its
> > work.  While understandable, I gave guidance 6 years ago that VDO
> > engineering should work to definitively reconcile if using Linux
> > workqueues viable now that VDO has been GPL'd.
> 
> Yes, initially that was a significant reason.
> 
> More recently, when we've tried switching, the performance loss made it
> appear not worth the change. Especially since we also needed to ship a
> usable version at the same time.
> 
> > But it appears there wasn't much in the way of serious effort put to
> > completely converting to using Linux workqueues. That is a problem
> > because all of the work item strategy deployed by VDO is quite
> > bespoke.  I don't think the code lends itself to being properly
> > maintained by more than a 1 or 2 engineers (if we're lucky at this
> > point).
> 
> By "work item strategy" are you referring to the lower level handling of
> queueing and executing the work items? Because I've done that. Well, the
> first 90%, by making the VDO work queues function as a shim on top of
> the kernel ones instead of creating their own threads. It would also
> need the kernel workqueues modified to support the SYSFS and ORDERED
> options together, because on NUMA systems the VDO performance really
> tanks without tweaking CPU affinity, and one or two other small
> additions. If we were to actually commit to that version there might be
> additional work like tweaking some data structures and eliding some shim
> functions if appropriate, but given the performance loss, we decided to
> stop there.

There needs to be a comprehensive audit of the locking and the
granularity of work.  The model VDO uses already requires that
anything that needs a continuation is assigned to the same thread
right?  Matt said that there is additional locking in the rare case
that another thread needs read access to an object.

Determining how best to initiate the work VDO requires (and provide
mutual exclusion that still allows concurrency is the goal). Having a
deep look at this is needed.
 
> Or do you mean the use of executing all actions affecting a data
> structure in a single thread/queue via message passing to serialize
> access to data structures instead of having a thread serially lock,
> modify, and unlock the various different data structures on behalf of a
> single I/O request, while another thread does the same for another I/O
> request? The model we use can certainly make things more difficult to
> follow. It reads like continuation-passing style code, not the
> straight-line code many of us are more accustomed to.
> 
> "Converting to using Linux workqueues" really doesn't say the latter to
> me, it says the former. But I thought I'd already mentioned I'd tried
> the former out. (Perhaps not very clearly?)

The implicit locking of the VDO thread assignment model needs to be
factored out.  If 'use_vdo_wq' is true then the locking operations are
a noop. But if Linux workqueues are used then appropriate locking
needed.

FYI, dm-cache-target.c uses a struct continuation to queue a sequence
of work.  Can VDO translate its ~12 stages of work into locking a vio
and using continuations to progress through the stages?  The locking
shouldn't be overbearing since VDO is already taking steps to isolate
the work to particular threads.

Also, just so you're aware DM core now provides helpers to shard a
data structures locking (used by dm-bufio and dm-bio-prison-v1).  See
dm_hash_locks_index() and dm_num_hash_locks().

> > I would like to see a patch crafted that allows branching between the
> > use of Linux and VDO workqueues. Initially have a dm-vdo modparam
> > (e.g. use_vdo_wq or vice-versa: use_linux_wq).  And have a wrapping
> > interface and associated data struct(s) that can bridge between work
> > being driven/coordinated by either (depending on disposition of
> > modparam).
> 
> If we're talking about the lower level handling, I don't think it would
> be terribly hard.
> 
> > This work isn't trivial, I get that. But it serves to clearly showcase
> > shortcomings, areas for improvement, while pivoting to more standard
> > Linux interfaces that really should've been used from VDO's inception.
> >
> > Is this work that you feel you could focus on with urgency?
> >
> > Thanks,
> > Mike
> 
> I think so, once we're clear on exactly what we're talking about...

I'm talking about a comprehensive audit of how work is performed.  And
backfilling proper locking by factoring out adequate protection that
allows conditional use of locking (e.g. IFF using linux workqueues).

In the end, using either VDO workqueue or Linux workqueues must pass
all VDO tests.

Mike

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