On Thu, Feb 20, 2020 at 11:51:51AM -0500, Dan Schatzberg wrote:
> Existing uses of loop device may have multiple cgroups reading/writing
> to the same device. Simply charging resources for I/O to the backing
> file could result in priority inversion where one cgroup gets
> synchronously blocked, holding up all other I/O to the loop device.
>
> In order to avoid this priority inversion, we use a single workqueue
> where each work item is a "struct loop_worker" which contains a queue of
> struct loop_cmds to issue. The loop device maintains a tree mapping blk
> css_id -> loop_worker. This allows each cgroup to independently make
> forward progress issuing I/O to the backing file.
>
> There is also a single queue for I/O associated with the rootcg which
> can be used in cases of extreme memory shortage where we cannot allocate
> a loop_worker.
>
> The locking for the tree and queues is fairly heavy handed - we acquire
> the per-loop-device spinlock any time either is accessed. The existing
> implementation serializes all I/O through a single thread anyways, so I
> don't believe this is any worse.
>
> Signed-off-by: Dan Schatzberg <schatzberg.dan@xxxxxxxxx>
FWIW, this looks good to me, please feel free to include:
Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>
I have only some minor style nitpicks (along with the other email I
sent earlier on this patch), that would be nice to get fixed:
> +static void loop_queue_work(struct loop_device *lo, struct loop_cmd *cmd)
> +{
> + struct rb_node **node = &(lo->worker_tree.rb_node), *parent = NULL;
> + struct loop_worker *cur_worker, *worker = NULL;
> + struct work_struct *work;
> + struct list_head *cmd_list;
> +
> + spin_lock_irq(&lo->lo_lock);
> +
> + if (!cmd->css)
> + goto queue_work;
> +
> + node = &(lo->worker_tree.rb_node);
-> and . are > &, the parentheses aren't necessary.
> + while (*node) {
> + parent = *node;
> + cur_worker = container_of(*node, struct loop_worker, rb_node);
> + if ((long)cur_worker->css == (long)cmd->css) {
The casts aren't necessary, but they made me doubt myself and look up
the types. I wouldn't add them just to be symmetrical with the other
arm of the branch.
> + worker = cur_worker;
> + break;
> + } else if ((long)cur_worker->css < (long)cmd->css) {
> + node = &((*node)->rb_left);
> + } else {
> + node = &((*node)->rb_right);
The outer parentheses aren't necessary.
> + }
> + }
> + if (worker)
> + goto queue_work;
> +
> + worker = kzalloc(sizeof(struct loop_worker),
> + GFP_NOWAIT | __GFP_NOWARN);
This fits on an 80 character line.
