Re: [PATCH 1/6] loop: clean up blkcg association

From: Ming Lei <ming.lei@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: "Jens Axboe" <axboe@kernel.dk>,
	linux-block@vger.kernel.org, "Michal Koutný" <mkoutny@suse.com>,
	"Dan Schatzberg" <schatzberg.dan@gmail.com>
Subject: Re: [PATCH 1/6] loop: clean up blkcg association
Date: Thu, 8 Jul 2021 15:20:04 +0800	[thread overview]
Message-ID: <YOanJGsEGiG3BKgh@T590> (raw)
In-Reply-To: <20210706055109.GF17027@lst.de>

On Tue, Jul 06, 2021 at 07:51:09AM +0200, Christoph Hellwig wrote:
> Did I mention that all this loop blkcg integration is complete and utter
> garbage and should have never been merged?

Sorry, I didn't see your comment on that, and loop blkcg patches have been
merged to linus tree. IMO, this approach fixes real issue wrt. cgroup on
loop.

> 
> 
> >  	struct list_head *cmd_list;
> > +	struct cgroup_subsys_state *blkcg_css = NULL;
> > +#ifdef CONFIG_BLK_CGROUP
> > +	struct request *rq = blk_mq_rq_from_pdu(cmd);
> > +
> > +	if (rq->bio && rq->bio->bi_blkg)
> > +		blkcg_css = &bio_blkcg(rq->bio)->css;
> > +#endif
> 
> This kind of junk has no business in a driver.  The blkcg code
> need to provide a helper for retreiving the blkcg_css from a request,
> including a stub for the the !CONFIG_BLK_CGROUP case.

Fine, it should be cleaner to move the above code into one helper.

> 
> >  		cur_worker = container_of(*node, struct loop_worker, rb_node);
> > -		if (cur_worker->blkcg_css == cmd->blkcg_css) {
> > +		if (cur_worker->blkcg_css == blkcg_css) {
> >  			worker = cur_worker;
> >  			break;
> > -		} else if ((long)cur_worker->blkcg_css < (long)cmd->blkcg_css) {
> > +		} else if ((long)cur_worker->blkcg_css < (long)blkcg_css) {
> 
> No need for an else after a break.  And more importantly no need to cast
> a pointer to compare it to another pointer of the same type.

I believe that change doesn't belong to this patch, also the rbtree is killed
in patch 5/6.

> 
> > +	struct mem_cgroup *old_memcg = NULL;
> > +	struct cgroup_subsys_state *memcg_css = NULL;
> > +
> > +	kthread_associate_blkcg(worker->blkcg_css);
> > +#ifdef CONFIG_MEMCG
> > +	memcg_css = cgroup_get_e_css(worker->blkcg_css->cgroup,
> > +			&memory_cgrp_subsys);
> > +#endif
> > +	if (memcg_css)
> > +		old_memcg = set_active_memcg(
> > +				mem_cgroup_from_css(memcg_css));
> > +
> 
> This kind of crap also has absolutely no business in a block driver
> and the memcg code should provide a proper helper.

Except for loop, cgroup_get_e_css() is only used by cgwb, and this use
case is very special, in which the associated memcg has to be set as active,
so memcg has to be visible here.

Thanks,
Ming