On Mon, May 15, 2023 at 10:55:01AM +0800, haowenchao (C) wrote:
> On 2023/5/13 23:55, Ming Lei wrote:
> > Hello Guys,
> >
> > scsi_device->iorequest_cnt/iodone_cnt/ioerr_cnt are only inc/dec,
> > and exported to userspace via sysfs in kernel, and not see any kernel
> > consumers, so the exported counters are only for userspace, just be curious
> > which/how applications consume them?
> >
>
> These counters are used to checking the disk health status in a certain scenario
> for us.
All the three counters are increased only, and you could use some simple bpf code
to retrieve such info easily. It adds such overhead for everyone, who may not use
this counters at all, maybe 99% of people don't use it.
>
> > These counters not only adds cost in fast path, but also causes kernel panic,
> > especially the "atomic_inc(&cmd->device->iorequest_cnt)" in
> > scsi_queue_rq(), because cmd->device may be freed after returning
> > from scsi_dispatch_cmd(), which is introduced by:
> >
> > cfee29ffb45b ("scsi: core: Do not increase scsi_device's iorequest_cnt if dispatch failed")
> >
> > If there aren't explicit applications which depend on these counters,
> > I'd suggest to kill the three since all are not in stable ABI. Otherwise
> > I think we need to revert cfee29ffb45b.
> >
> >
>
> Sorry for introduce this bug.
>
> We would check these counters after iodone_cnt equal to iorequest_cnt. So
> cfee29ffb45b is aimed to fix the issue of iorequest_cnt is increased for
> multiple times if scsi_dispatch_cmd() failed.
>
> Maybe we should revert cfee29ffb45b and fix the original issue with following
> changes:
Yeah, it can be fixed in this way, can you cook one such fix?
Longterm, maybe we can mark these sysfs interface as obsolete and let
existed users switch to ebpf, and finally remove them.
thanks,
Ming
