On Thu, Jan 27, 2022 at 10:18 AM Yang Shi <shy828301@xxxxxxxxx> wrote: > > On Thu, Jan 27, 2022 at 1:53 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > > > On Wed, Jan 26, 2022 at 10:51:53AM -0800, Yang Shi wrote: > > > + __entry->dev = rq->q->disk ? disk_devt(rq->q->disk) : 0; > > > + __assign_str(name, rq->q->disk ? rq->q->disk->disk_name : "?"); > > > > None f the other tracepoints has the disk name, why does this one need > > it? And if you add it please avoid the overly long line. > > I guess the disk name was added to ease some handling in userspace > tools. But if all other tracepoints don't have disk name shown, I > think I'd better follow the convention. I did overlook this when I > ported this patch. Will remove it. > > > > > > + __entry->sector = blk_rq_pos(rq); > > > + __entry->nr_sector = nr_bytes >> 9; > > > + __entry->error = blk_status_to_errno(error); > > > > This still converts the block status to an errno. > > I may misunderstand your comments. I just followed what > block_rq_complete tracepoint does. Or the linux-block community is > converting all tracepoints to show blk status code instead of > conventional errno? > > And the userspace tool doesn't know blk status code and still expects > the conventional errno. For example, rasdaemon reads block_rq_complete > events now and have the below: > > static const struct { > int error; > const char *name; > } blk_errors[] = { > { -EOPNOTSUPP, "operation not supported error" }, > { -ETIMEDOUT, "timeout error" }, > { -ENOSPC, "critical space allocation error" }, > { -ENOLINK, "recoverable transport error" }, > { -EREMOTEIO, "critical target error" }, > { -EBADE, "critical nexus error" }, > { -ENODATA, "critical medium error" }, > { -EILSEQ, "protection error" }, > { -ENOMEM, "kernel resource error" }, > { -EBUSY, "device resource error" }, > { -EAGAIN, "nonblocking retry error" }, > { -EREMCHG, "dm internal retry error" }, > { -EIO, "I/O error" }, > }; Gently ping. Should I print blk status code instead of errno for this trace point? But I really don't get why. And block_rq_complete tracepoint does: TP_fast_assign( __entry->dev = rq->q->disk ? disk_devt(rq->q->disk) : 0; __entry->sector = blk_rq_pos(rq); __entry->nr_sector = nr_bytes >> 9; __entry->error = blk_status_to_errno(error); <=== convert blk status code to errno blk_fill_rwbs(__entry->rwbs, rq->cmd_flags); __get_str(cmd)[0] = '\0'; ), > > This patch aims to add block_rq_err tracepoint to replace > block_rq_complete in rasdaemon.