Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 28 Jun 2021 10:17:18 -0400
From:   Dan Schatzberg <schatzberg.dan@gmail.com>
To:     Michal =?iso-8859-1?Q?Koutn=FD?= <mkoutny@suse.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "open list:BLOCK LAYER" <linux-block@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:CONTROL GROUP (CGROUP)" <cgroups@vger.kernel.org>,
        "open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH 3/3] loop: Charge i/o to mem and blk cg
Message-ID: <YNnZ7hIRIk9dJDry@dschatzberg-fedora-PC0Y6AEN>
References: <20210610173944.1203706-1-schatzberg.dan@gmail.com>
 <20210610173944.1203706-4-schatzberg.dan@gmail.com>
 <YNXvr81YFzbaTxCb@blackbook>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YNXvr81YFzbaTxCb@blackbook>
Precedence: bulk

Hi Michal,

On Fri, Jun 25, 2021 at 05:01:03PM +0200, Michal Koutn? wrote:
> Hi.
> 
> On Thu, Jun 10, 2021 at 10:39:44AM -0700, Dan Schatzberg <schatzberg.dan@gmail.com> wrote:
> > The current code only associates with the existing blkcg when aio is
> > used to access the backing file. This patch covers all types of i/o to
> > the backing file and also associates the memcg so if the backing file is
> > on tmpfs, memory is charged appropriately.
> > 
> > This patch also exports cgroup_get_e_css and int_active_memcg so it
> > can be used by the loop module.
> 
> Wouldn't it be clearer to export (not explicitly inlined anymore)
> set_active_memcg() instead of the int_active_memcg that's rather an
> implementation detail?

Agreed that exporting int_active_memcg is an implementation detail,
but would this prevent set_active_memcg from being inlined? Is that
desireable?

> 
> > @@ -2111,13 +2112,18 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
> >  	}
> >  
> >  	/* always use the first bio's css */
> > +	cmd->blkcg_css = NULL;
> > +	cmd->memcg_css = NULL;
> >  #ifdef CONFIG_BLK_CGROUP
> > -	if (cmd->use_aio && rq->bio && rq->bio->bi_blkg) {
> > -		cmd->css = &bio_blkcg(rq->bio)->css;
> > -		css_get(cmd->css);
> > -	} else
> > +	if (rq->bio && rq->bio->bi_blkg) {
> > +		cmd->blkcg_css = &bio_blkcg(rq->bio)->css;
> > +#ifdef CONFIG_MEMCG
> > +		cmd->memcg_css =
> > +			cgroup_get_e_css(cmd->blkcg_css->cgroup,
> > +					&memory_cgrp_subsys);
> > +#endif
> > +	}
> >  #endif
> > -		cmd->css = NULL;
> >  	loop_queue_work(lo, cmd);
> 
> I see you dropped the cmd->blkcg_css reference (while rq is handled). Is
> it intentional?

Yes it is intentional. All requests (not just aio) go through the loop
worker which grabs the blkcg reference in loop_queue_work() on
construction. So I believe grabbing a reference per request is
unnecessary.