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;
From:   Niklas Cassel <Niklas.Cassel@wdc.com>
To:     Bart Van Assche <bvanassche@acm.org>
CC:     Jens Axboe <axboe@kernel.dk>,
        Damien Le Moal <Damien.LeMoal@wdc.com>,
        Paolo Valente <paolo.valente@linaro.org>,
        Ming Lei <ming.lei@redhat.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] Revert "mq-deadline: Fix request accounting"
Thread-Topic: [PATCH 2/2] Revert "mq-deadline: Fix request accounting"
Thread-Index: AQHXo/O2L+LBuS/YJECb1WaT3ju4YKuYrlyAgAAUmwCAAAxMgIABOjaA
Date:   Wed, 8 Sep 2021 11:57:26 +0000
Message-ID: <YTilJQ4sQ3dSnev+@x1-carbon>
References: <20210907142145.112096-1-Niklas.Cassel@wdc.com>
 <20210907142145.112096-3-Niklas.Cassel@wdc.com>
 <fdd5ab2a-af90-69bd-2d37-c5060ce68de4@acm.org> <YTeTQGrw41k08hgf@x1-carbon>
 <55fca3f4-4ed0-fd56-3069-c0ab343b2aed@acm.org>
In-Reply-To: <55fca3f4-4ed0-fd56-3069-c0ab343b2aed@acm.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?BWF61Ug+K2Fs/TKFPo1qhm/d9ncyMyPyXVLymkgN4buOIfMuVkOrMh2Sw0aH?=
 =?us-ascii?Q?XgHGmJXjfM0DdOm3CXkWH1dn+pkyPzpLxGa2vV5wXn51eqxlqPpM+8yk96c8?=
 =?us-ascii?Q?GgX8hCGbPFRXgn2XzVDospLQ2P5sMPHmunVJtWLGJiqTa5bmB2WKxTXoLSHU?=
 =?us-ascii?Q?raXMv+9e+mpNvMi76/sG6U2Q+xgP1DRmm5lyGZ0XLLtMUyUYzQLmxA6JrKf2?=
 =?us-ascii?Q?LcZIuhEWA91GRbGSEbXfxH5zjBWjjR/9ViMHF2tP/7Qd22k6bMJ4EH/TbWA+?=
 =?us-ascii?Q?VakWRIZhn0ZA7zM6Ho4y14WSDLaXXU/Y70TOKTgaULOQtK8VdYFnRQg8TBy7?=
 =?us-ascii?Q?B9Dvxk1wWJ666PBMU5203gJWnvV3NKrjmrh0WpibuWZ7g+XXcWBpmNj95St2?=
 =?us-ascii?Q?kvYQONMLLVyLiwiC3YyC2oq0Zidtfzh+uPVr0EX98Mv++gOG/THajAh3TsUO?=
 =?us-ascii?Q?676gtCZxTnOVkN8GbFwrNhAO73AhbiNyYz3YhGRQevpe+BRs1asgdKFKeItH?=
 =?us-ascii?Q?nWGLd3pDtgzy/By/JOG6YnFSgVNJBhdzFVIEzr7JJOoTHFnzqZNI73OFoOIL?=
 =?us-ascii?Q?ulNqcWK2gAiQRMLcKhX/AzM9SGU93klryUpWCVGszoFyXPlQqexIuR57jfCA?=
 =?us-ascii?Q?c1OhHJUSZGmva2mr87ep1vYZIXkrrACVChKj+B3E2WXBZN2DA3M3kqvhK6Sx?=
 =?us-ascii?Q?jQIzsKoZst514d1g9823nOIbTi6MuuyXhSRkrjzaF/UjykZNAqHqt2jP/Xn9?=
 =?us-ascii?Q?xCcdmsXZv6/ZcJaVArr+NHHllOvhhrbRQSafRIvLb3Ks8RamREFPOjPM8pXE?=
 =?us-ascii?Q?dKaD8bH8r9mi6BkXgl02lk2znDBxqrbKFG9OAm2zDtCo407u382dcZtyN/Qd?=
 =?us-ascii?Q?lZ+WP7UCIx487yUG01szjpqNVp3jgbYdFzS1/yd3tJF+0XU8IN2hjMWwWMhy?=
 =?us-ascii?Q?hRQoi1NUlMsrJCw7ME9b9rVpBzMpsIetATO7Q7WWSSzk6TQXpu74/PPOtubP?=
 =?us-ascii?Q?9ey2IxqaZvhtTbgiDmWAwafgEZms6hJqCwnRF2chOkeL3AYfKv47MENP6F3X?=
 =?us-ascii?Q?B7GpDSNTIcyxx5bN1x3+1rXgd6VC/mxE3D4sd1njYmY6Rg6d2+pIBOEByw+n?=
 =?us-ascii?Q?CJCTuMPH4AYl2eZKsGz0gLml6sxz8q7TxNS/mgeKKUV3SqJnEGUGJfy7ImMe?=
 =?us-ascii?Q?8HZLvlGXRjH+/As9zLmn/gpT6+VJwRnNGCRrIfbT3ickkM46iDVUAQtBLV5a?=
 =?us-ascii?Q?LCDR1IYvG6m0RMY81CLyonhZwgN7unvn9YZ5wgQHoUDz0Uv7B1Uea33/xKBd?=
 =?us-ascii?Q?FEhHHhieQfywNLA/7zRWHjo+hPzaA7cBEtyTdxXL0v1toA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <054C2ED7753FB343932B98198E7C8AA0@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR04MB7158.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 983fe865-eead-4ead-e5c9-08d972bfd3a3
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Sep 2021 11:57:26.3345
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: xnaEbzSvShrfn69dFfe7/IQKHtVvMIMyI33HecRMg/9GIuDO21XKBMQrTRnWOfLI2J3Ozh76i4bk6r88LH8mOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR04MB7383
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Sep 07, 2021 at 10:12:49AM -0700, Bart Van Assche wrote:
> On 9/7/21 9:28 AM, Niklas Cassel wrote:
> > On Tue, Sep 07, 2021 at 08:15:03AM -0700, Bart Van Assche wrote:
> > > On 9/7/21 7:21 AM, Niklas Cassel wrote:
> > > > blk-mq will no longer call the I/O scheduler .finish_request() call=
back
> > > > for requests that were never inserted to the I/O scheduler.
> > >=20
> > > I do not agree. Even with patch 1/2 from this series applied, finish_=
request()
> > > will still be called for requests inserted by blk_insert_cloned_reque=
st()
> > > although these requests are never inserted to the I/O scheduler.
> >=20
> > Looking at blk_mq_free_request(),
> > e->type->ops.finish_request() will only be called if RQF_ELVPRIV
> > is set.
> >=20
> > blk_insert_cloned_request() doesn't seem to allocate a request
> > itself, but instead takes an already cloned request.
> >=20
> > So I guess it depends on how the supplied request was cloned.
> >=20
> > I would assume if the original request doesn't have RQF_ELVPRIV set,
> > then neither will the cloned request?
> >=20
> > I tried to look at blk_rq_prep_clone(), which seems to be a common
> > cloning function, but I don't see req->rq_flags being copied
> > (except for RQF_SPECIAL_PAYLOAD).
> >=20
> > Anyway, I don't see how .finish_request() will be called in relation
> > to blk_insert_cloned_request(). Could you please help me out and
> > give me an example of a call chain where this can happen?
>=20
> Hi Niklas,
>=20
> This is a bit outside my area of expertise. Anyway: map_request() calls
> .clone_and_map_rq(). At least multipath_clone_and_map() calls
> blk_get_request(). I think this shows that blk_insert_cloned_request()
> may insert an entirely new request. Is my understanding correct that
> blk_mq_rq_ctx_init() will set RQF_ELVPRIV for the cloned request if a
> scheduler is associated with the request queue associated with the
> cloned request?
>=20
> Bart.

Thank you Bart.


dm-rq.c:map_request() calls .clone_and_map_rq()

one example of a .clone_and_map_rq() implementation is
multipath_clone_and_map().

multipath_clone_and_map(), to get a clone simply does blk_get_request(),
which does blk_mq_alloc_request(), which calls __blk_mq_alloc_request(),
which calls blk_mq_rq_ctx_init(), which will set RQF_ELVPRIV, as long as
the request is not a flush or a passthrough request.

dm-rq.c:dm_dispatch_clone_request() calls blk_insert_cloned_request(),
which will bypass the I/O scheduler.

So, a request cloned by e.g. drivers/md/dm-mpath.c will
bypass the I/O scheduler, but can still have the RQF_ELVPRIV flag set.


This just tells me that if someone wants to clean up this mess,
we have to do something similar to what I proposed in my initial RFC:
https://lore.kernel.org/linux-block/20210827124100.98112-2-Niklas.Cassel@wd=
c.com/

i.e., set RQF_ELVPRIV flag immediately before calling
e->type->ops.insert_requests(), and only then.

It seems logical to simply set the flag before actually inserting the reque=
st
to the scheduler, rather than finding all the corner cases where we don't.

Anyway, I'll leave this potential cleanup to people more familiar with the
blk-mq code for now.


Kind regards,
Niklas=