mbox series

[0/3] Fix blkparse and iowatcher for kernels >= 4.14

Message ID 20200506133933.4773-1-jack@suse.cz (mailing list archive)
Headers show
Series Fix blkparse and iowatcher for kernels >= 4.14 | expand

Message

Jan Kara May 6, 2020, 1:39 p.m. UTC
I was investigating a performance issue with BFQ IO scheduler and I was
pondering why I'm not seeing informational messages from BFQ. After quite
some debugging I have found out that commit 35fe6d763229 "block: use
standard blktrace API to output cgroup info for debug notes" broke standard
blktrace API - namely the informational messages logged by bfq_log_bfqq()
are no longer displayed by blkparse(8) tool. This is because these messages
have now __BLK_TA_CGROUP bit set and that breaks flags checking in
blkparse(1) and iowatcher(1). This series fixes both tools to be able to
cope with events with __BLK_TA_CGROUP flag set.

								Honza

Comments

Paolo Valente May 6, 2020, 1:50 p.m. UTC | #1
Does this fix make process names (with PIDS) be shown too in bfq log messages?

> Il giorno 6 mag 2020, alle ore 15:39, Jan Kara <jack@suse.cz> ha scritto:
> 
> I was investigating a performance issue with BFQ IO scheduler and I was
> pondering why I'm not seeing informational messages from BFQ. After quite
> some debugging I have found out that commit 35fe6d763229 "block: use
> standard blktrace API to output cgroup info for debug notes" broke standard
> blktrace API - namely the informational messages logged by bfq_log_bfqq()
> are no longer displayed by blkparse(8) tool. This is because these messages
> have now __BLK_TA_CGROUP bit set and that breaks flags checking in
> blkparse(1) and iowatcher(1). This series fixes both tools to be able to
> cope with events with __BLK_TA_CGROUP flag set.
> 
> 								Honza
Jan Kara May 6, 2020, 2:12 p.m. UTC | #2
On Wed 06-05-20 15:50:30, Paolo Valente wrote:
> Does this fix make process names (with PIDS) be shown too in bfq log messages?

No, I didn't change how that's handled. Also AFAICS the kernel does not add
PID information to BLK_TN_MESSAGE events (which is how bfq log messages are
passed) so blkparse has nothing to display even if it wanted.

So poor man's solution is to derive pid from the BFQ queue name and then
lookup process name from some other event. Nicer solution would be to
modify blktrace handling in the kernel to add PID information to such
events as well so we get similar info as when using tracing infrastructure
directly.

								Honza

> > Il giorno 6 mag 2020, alle ore 15:39, Jan Kara <jack@suse.cz> ha scritto:
> > 
> > I was investigating a performance issue with BFQ IO scheduler and I was
> > pondering why I'm not seeing informational messages from BFQ. After quite
> > some debugging I have found out that commit 35fe6d763229 "block: use
> > standard blktrace API to output cgroup info for debug notes" broke standard
> > blktrace API - namely the informational messages logged by bfq_log_bfqq()
> > are no longer displayed by blkparse(8) tool. This is because these messages
> > have now __BLK_TA_CGROUP bit set and that breaks flags checking in
> > blkparse(1) and iowatcher(1). This series fixes both tools to be able to
> > cope with events with __BLK_TA_CGROUP flag set.
> > 
> > 								Honza
>
Christoph Hellwig May 6, 2020, 2:42 p.m. UTC | #3
On Wed, May 06, 2020 at 03:39:30PM +0200, Jan Kara wrote:
> I was investigating a performance issue with BFQ IO scheduler and I was
> pondering why I'm not seeing informational messages from BFQ. After quite
> some debugging I have found out that commit 35fe6d763229 "block: use
> standard blktrace API to output cgroup info for debug notes" broke standard
> blktrace API - namely the informational messages logged by bfq_log_bfqq()
> are no longer displayed by blkparse(8) tool. This is because these messages
> have now __BLK_TA_CGROUP bit set and that breaks flags checking in
> blkparse(1) and iowatcher(1). This series fixes both tools to be able to
> cope with events with __BLK_TA_CGROUP flag set.

I'd much rather revert a kernel change that breaks frequently used
userspace.
Jan Kara May 7, 2020, 9:49 a.m. UTC | #4
On Wed 06-05-20 07:42:51, Christoph Hellwig wrote:
> On Wed, May 06, 2020 at 03:39:30PM +0200, Jan Kara wrote:
> > I was investigating a performance issue with BFQ IO scheduler and I was
> > pondering why I'm not seeing informational messages from BFQ. After quite
> > some debugging I have found out that commit 35fe6d763229 "block: use
> > standard blktrace API to output cgroup info for debug notes" broke standard
> > blktrace API - namely the informational messages logged by bfq_log_bfqq()
> > are no longer displayed by blkparse(8) tool. This is because these messages
> > have now __BLK_TA_CGROUP bit set and that breaks flags checking in
> > blkparse(1) and iowatcher(1). This series fixes both tools to be able to
> > cope with events with __BLK_TA_CGROUP flag set.
> 
> I'd much rather revert a kernel change that breaks frequently used
> userspace.

We've discussed it [1]. The commit was merged in 4.14 which shows that the
breakage is not that severe as nobody noticed until now. Actually I did
some more digging in history now and until commit 743210386c "cgroup: use
cgrp->kn->id as the cgroup ID" from last November the breakage was only
visible if you had CONFIG_BLK_CGROUP enabled and had bio with bi_blkcg set
or trace note messages for non-trivial cgroups (effectively only BFQ
informational messages). After that commit trace note messages are broken
whenever you have CONFIG_BLK_CGROUP. So the reason why nobody noticed is
probably because until 5.5 the breakage was actually difficult to hit.

So I'm undecided whether at this point it's better to revert the original
commit (as likely there is other tooling that depends on this info), just
fix the fact that since commit 743210386c all trace note messages will have
non-empty cgroup info, or just fixup blkparse and move on...

								Honza

[1] https://lore.kernel.org/r/20200430114711.GA6576@quack2.suse.cz
Jens Axboe May 20, 2020, 1:38 p.m. UTC | #5
On 5/6/20 7:39 AM, Jan Kara wrote:
> I was investigating a performance issue with BFQ IO scheduler and I was
> pondering why I'm not seeing informational messages from BFQ. After quite
> some debugging I have found out that commit 35fe6d763229 "block: use
> standard blktrace API to output cgroup info for debug notes" broke standard
> blktrace API - namely the informational messages logged by bfq_log_bfqq()
> are no longer displayed by blkparse(8) tool. This is because these messages
> have now __BLK_TA_CGROUP bit set and that breaks flags checking in
> blkparse(1) and iowatcher(1). This series fixes both tools to be able to
> cope with events with __BLK_TA_CGROUP flag set.

Applied, thanks.