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.