Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gulam Mohamed <gulam.mohamed@oracle.com>
To:     Keith Busch <kbusch@kernel.org>
CC:     "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "philipp.reisner@linbit.com" <philipp.reisner@linbit.com>,
        "lars.ellenberg@linbit.com" <lars.ellenberg@linbit.com>,
        "christoph.boehmwalder@linbit.com" <christoph.boehmwalder@linbit.com>,
        "minchan@kernel.org" <minchan@kernel.org>,
        "ngupta@vflare.org" <ngupta@vflare.org>,
        "senozhatsky@chromium.org" <senozhatsky@chromium.org>,
        "colyli@suse.de" <colyli@suse.de>,
        "kent.overstreet@gmail.com" <kent.overstreet@gmail.com>,
        "agk@redhat.com" <agk@redhat.com>,
        "snitzer@kernel.org" <snitzer@kernel.org>,
        "dm-devel@redhat.com" <dm-devel@redhat.com>,
        "song@kernel.org" <song@kernel.org>,
        "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
        "vishal.l.verma@intel.com" <vishal.l.verma@intel.com>,
        "dave.jiang@intel.com" <dave.jiang@intel.com>,
        "ira.weiny@intel.com" <ira.weiny@intel.com>,
        Junxiao Bi <junxiao.bi@oracle.com>,
        Martin Petersen <martin.petersen@oracle.com>,
        "kch@nvidia.com" <kch@nvidia.com>,
        "drbd-dev@lists.linbit.com" <drbd-dev@lists.linbit.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-bcache@vger.kernel.org" <linux-bcache@vger.kernel.org>,
        "linux-raid@vger.kernel.org" <linux-raid@vger.kernel.org>,
        "nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>,
        Konrad Wilk <konrad.wilk@oracle.com>,
        Joe Jin <joe.jin@oracle.com>,
        Rajesh Sivaramasubramaniom 
        <rajesh.sivaramasubramaniom@oracle.com>,
        Shminderjit Singh <shminderjit.singh@oracle.com>
Subject: RE: [PATCH for-6.2/block V3 2/2] block: Change the granularity of io
 ticks from ms to ns
Thread-Topic: [PATCH for-6.2/block V3 2/2] block: Change the granularity of io
 ticks from ms to ns
Thread-Index: AQHZFPFuNnmU8fHcpkiSBSHlMBjHga54k/MAgAL7k+A=
Date:   Fri, 23 Dec 2022 14:47:06 +0000
Message-ID: <CO1PR10MB4563F566452B9D3035A82EE298E99@CO1PR10MB4563.namprd10.prod.outlook.com>
References: <20221221040506.1174644-1-gulam.mohamed@oracle.com>
 <20221221040506.1174644-2-gulam.mohamed@oracle.com>
 <Y6M9rJbw3ZMvOeDr@kbusch-mbp.dhcp.thefacebook.com>
In-Reply-To: <Y6M9rJbw3ZMvOeDr@kbusch-mbp.dhcp.thefacebook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?EW5Uo8B53+tpfuETDtluY5h7lhLKksJdv4OKn/KdEn31sEKnSHzCaOliZJ5z?=
 =?us-ascii?Q?GDaf+cxQbgbRKW0P4+YDTL2YO4RnBNBdnqqlLez54r8shU/MEfKVtJGwBhWv?=
 =?us-ascii?Q?SyZnWR1jlPNdwFpdeOs5HRii7n/eKiosLyKZMM2bGVytnkde22WjLpT7d2cw?=
 =?us-ascii?Q?IEdv3slZm/vHiB8bvcjBXFpUuuIhFmpKJiHgiIrCkW+NfIoNJOdKOdZ2DVgD?=
 =?us-ascii?Q?YqCGu7IHxNJ5zTUjtTUcFoRQrh/OwmMov/HpvECbahPuQU3poRpxauAa5uLr?=
 =?us-ascii?Q?9MDZ6neMMMo5RvpZlurNMImG3ZpMJEoxovY+w8kwtk67C/0b2AeP140R8t2n?=
 =?us-ascii?Q?ibvO54TRLnqD55Ih1GAZykwvQb23/XoA6t68PbOVfitUuGDFq1Gp58Wsmwjq?=
 =?us-ascii?Q?uC2ZIf1BC76uKWyqJvZoe7O0Yj2CMHJcDmwOhfhTO79GNT00uPda9or5Bxan?=
 =?us-ascii?Q?enTyCcUzfUwUC13/xkS6t2P44RD2snGV4+YKwgLVjA/DaubEgQq348/F8Jm6?=
 =?us-ascii?Q?N73OyYcKD9mEt/FGJRmgmrp3mGylN78FY6obG1/NE5FZOkdkJLubeMQkv4pF?=
 =?us-ascii?Q?+z+K6Q52TfEvRzP5fO4K2m2uOTz/jrplV7t0PlOqpt/CRU+EpN382XiqpBT9?=
 =?us-ascii?Q?Xl0lOSPqGzMQL2U+9XPBeUi7fcHh00szN4UcOfHbxCxhYGvOpMxPWMlFw4rh?=
 =?us-ascii?Q?zTGVh+OkffCb8UHrXxdsFQA8RsBwl2D++/KKB4V+kjJM+G6f0TPuArmIyeu+?=
 =?us-ascii?Q?pmIgQPCYwc4EANqrSo6VohlxDx0CSuj/Lxwr6wFd6G8oaNHGJkIdT7vVLRHg?=
 =?us-ascii?Q?KItKFjmgl3tekBbj/Ro3zsRrTijaFCuBcwB8tWEWugLjg6xiPZZbu34tGmvX?=
 =?us-ascii?Q?eAjtkiEk4nm8S+NbaKv0qtptfKLb9j6+ZdiOyh0UmdBV47N8ARvQF/0A3roa?=
 =?us-ascii?Q?i+tGYt6hmZ0UH8SCH26pyII4ErCmhkT1j/U4iyn8XC6FoW5mkwYo20MEtb23?=
 =?us-ascii?Q?Ld8sib2aD3Z4tKju41WAy/VYfC+lG0vtGr9mCDXCXnAyYWCyFP+Sci1Of+ji?=
 =?us-ascii?Q?T1yD43JkVIUFRQKEyaixTZnGJERAHx1PR5tGF3p+u8Oj1oGNzsjBAmmVZ0uS?=
 =?us-ascii?Q?lbR5LjBOWRQTRocZP1aVydF+18Q0RwDt/P9g45cMXZ1ehXtFUvx7PkihhJW7?=
 =?us-ascii?Q?FjW4neXn75fqG+jqZTj0PBJTEDIMD5cVar7lD/tbhwbpBoo/hX9CSFFd8A5H?=
 =?us-ascii?Q?u5REeZQmlSLavxm6LKEGaliXhWksUmN9ynJWoeYDdoJIyMcYDq3kjLHmgDR2?=
 =?us-ascii?Q?tzFKcpQpOsMk3INi58lDPD59JzUi9boGcKBxjh/QtDfWEiWUy+aaO2BA8AmM?=
 =?us-ascii?Q?i8D68HqKJx72yhM5Y9s322zflFY+IB1HC1WiPKI9e5AghTMIH6bXsmynXVON?=
 =?us-ascii?Q?smmZ7Wb9ySeWT6rJxHpb2PMZwZ/Dtg0nEMEJQVf/8yPjfxVe+KsWeUoRp4jp?=
 =?us-ascii?Q?L1LvAgEGOfzm9aKWlDXQxKj6Sep8RibMNVti6dHdV6B98eOpK6j1aEDArrrU?=
 =?us-ascii?Q?9mBmEWXmTtFF2P3oc8MeSnAYxRlwHnaMa0OwH5pY?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4563.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c3ebebc7-35b1-4a4d-12ec-08dae4f49030
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Dec 2022 14:47:06.8413
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: cubjR2yZnnjMVb3TlKXHaQlyLPbKgL8H5kHLjCl0RUo9JnGqp62srFlYb2rx/fK+2WPxRg1tRJlwLOGe5WFsSWiFWSP5t3TosrIlMnA+61U=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5795
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-12-23_06,2022-12-23_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 phishscore=0 bulkscore=0 malwarescore=0 spamscore=0 adultscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2212230124
X-Proofpoint-GUID: ezNCfCW5MSRLe2i75-RAc-Yvrd9fkFj-
X-Proofpoint-ORIG-GUID: ezNCfCW5MSRLe2i75-RAc-Yvrd9fkFj-
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Keith,

   Thanks for reviewing this request. Can you please see my inline comments=
?

Regards,
Gulam Mohamed.

-----Original Message-----
From: Keith Busch <kbusch@kernel.org>=20
Sent: Wednesday, December 21, 2022 10:39 PM
To: Gulam Mohamed <gulam.mohamed@oracle.com>
Cc: linux-block@vger.kernel.org; axboe@kernel.dk; philipp.reisner@linbit.co=
m; lars.ellenberg@linbit.com; christoph.boehmwalder@linbit.com; minchan@ker=
nel.org; ngupta@vflare.org; senozhatsky@chromium.org; colyli@suse.de; kent.=
overstreet@gmail.com; agk@redhat.com; snitzer@kernel.org; dm-devel@redhat.c=
om; song@kernel.org; dan.j.williams@intel.com; vishal.l.verma@intel.com; da=
ve.jiang@intel.com; ira.weiny@intel.com; Junxiao Bi <junxiao.bi@oracle.com>=
; Martin Petersen <martin.petersen@oracle.com>; kch@nvidia.com; drbd-dev@li=
sts.linbit.com; linux-kernel@vger.kernel.org; linux-bcache@vger.kernel.org;=
 linux-raid@vger.kernel.org; nvdimm@lists.linux.dev; Konrad Wilk <konrad.wi=
lk@oracle.com>; Joe Jin <joe.jin@oracle.com>; Rajesh Sivaramasubramaniom <r=
ajesh.sivaramasubramaniom@oracle.com>; Shminderjit Singh <shminderjit.singh=
@oracle.com>
Subject: Re: [PATCH for-6.2/block V3 2/2] block: Change the granularity of =
io ticks from ms to ns

On Wed, Dec 21, 2022 at 04:05:06AM +0000, Gulam Mohamed wrote:
> +u64  blk_get_iostat_ticks(struct request_queue *q) {
> +       return (blk_queue_precise_io_stat(q) ? ktime_get_ns() :=20
> +jiffies); } EXPORT_SYMBOL_GPL(blk_get_iostat_ticks);
> +
>  void update_io_ticks(struct block_device *part, u64 now, bool end)  {
>  	u64 stamp;
> @@ -968,20 +980,26 @@ EXPORT_SYMBOL(bdev_start_io_acct);
>  u64 bio_start_io_acct(struct bio *bio)  {
>  	return bdev_start_io_acct(bio->bi_bdev, bio_sectors(bio),
> -				  bio_op(bio), jiffies);
> +				  bio_op(bio),
> +				  blk_get_iostat_ticks(bio->bi_bdev->bd_queue));
>  }
>  EXPORT_SYMBOL_GPL(bio_start_io_acct);
> =20
>  void bdev_end_io_acct(struct block_device *bdev, enum req_op op,
>  		      u64 start_time)
>  {
> +	u64 now;
> +	u64 duration;
> +	struct request_queue *q =3D bdev_get_queue(bdev);
>  	const int sgrp =3D op_stat_group(op);
> -	u64 now =3D READ_ONCE(jiffies);
> -	u64 duration =3D (unsigned long)now -(unsigned long) start_time;
> +	now =3D blk_get_iostat_ticks(q);;

I don't think you can rely on the blk_queue_precise_io_stat() flag in the c=
ompletion side. The user can toggle this with data in flight, which means t=
he completion may use different tick units than the start. I think you'll n=
eed to add a flag to the request in the start accounting side to know which=
 method to use for the completion.

[GULAM]: As per my understanding, this may work for a single request_queue =
implemetation. But this request based accounting, as per my understanding, =
may be an issue with the Multi-QUEUE as there is a separate queue for each =
CPU and the time-stamp being recorded for the block device is a global one.=
 Also, the issue you mentioned about the start and end accounting may updat=
e the ticks in different=20
units for the inflight IOs, may be just for a while. So, even if it works f=
or MQ, I am trying to understand how much is it feasible to do this request=
-based change for an issue which may be there for just a moment?
So, can you please correct me if I am wrong and explore more on your sugges=
tion so that I can understand properly?

> @@ -951,6 +951,7 @@ ssize_t part_stat_show(struct device *dev,
>  	struct request_queue *q =3D bdev_get_queue(bdev);
>  	struct disk_stats stat;
>  	unsigned int inflight;
> +	u64 stat_ioticks;
> =20
>  	if (queue_is_mq(q))
>  		inflight =3D blk_mq_in_flight(q, bdev); @@ -959,10 +960,13 @@ ssize_t=
=20
> part_stat_show(struct device *dev,
> =20
>  	if (inflight) {
>  		part_stat_lock();
> -		update_io_ticks(bdev, jiffies, true);
> +		update_io_ticks(bdev, blk_get_iostat_ticks(q), true);
>  		part_stat_unlock();
>  	}
>  	part_stat_read_all(bdev, &stat);
> +	stat_ioticks =3D (blk_queue_precise_io_stat(q) ?
> +				div_u64(stat.io_ticks, NSEC_PER_MSEC) :
> +				jiffies_to_msecs(stat.io_ticks));


With the user able to change the precision at will, I think these io_ticks =
need to have a consistent unit size. Mixing jiffies and nsecs is going to c=
reate garbage stats output. Could existing io_ticks using jiffies be conver=
ted to jiffies_to_nsecs() so that you only have one unit to consider?
[GULAM]: I am not sure if this will work as we just multiply with 1000000 t=
o convert jiffies to nano-seconds.