From: Dave Chinner <david@fromorbit.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Andrey Albershteyn <aalbersh@redhat.com>,
fsverity@lists.linux.dev, linux-xfs@vger.kernel.org,
linux-fsdevel@vger.kernel.org, chandan.babu@oracle.com,
djwong@kernel.org
Subject: Re: [PATCH v4 09/25] fsverity: add tracepoints
Date: Mon, 26 Feb 2024 13:24:33 +1100 [thread overview]
Message-ID: <Zdv2YZP0sIzDA5UT@dread.disaster.area> (raw)
In-Reply-To: <20240223182735.GD1112@sol.localdomain>
On Fri, Feb 23, 2024 at 10:27:35AM -0800, Eric Biggers wrote:
> On Fri, Feb 23, 2024 at 02:23:52PM +0100, Andrey Albershteyn wrote:
> > On 2024-02-22 21:31:56, Eric Biggers wrote:
> > > On Mon, Feb 12, 2024 at 05:58:06PM +0100, Andrey Albershteyn wrote:
> > > > fs-verity previously had debug printk but it was removed. This patch
> > > > adds trace points to the same places where printk were used (with a
> > > > few additional ones).
> > >
> > > Are all of these actually useful? There's a maintenance cost to adding all of
> > > these.
> > >
> >
> > Well, they were useful for me while testing/working on this
> > patchset. Especially combining -e xfs -e fsverity was quite good for
> > checking correctness and debugging with xfstests tests. They're
> > probably could be handy if something breaks.
> >
> > Or you mean if each of them is useful? The ones which I added to
> > signature verification probably aren't as useful as other; my
> > intention adding them was to also cover these code paths.
>
> Well, I'll have to maintain all of these, including reviewing them, keeping them
> working as code gets refactored, and fixing any bugs that exist or may get
> introduced later in them. They also increase the icache footprint of the code.
> I'd like to make sure that it will be worthwhile. The pr_debug messages that I
> had put in fs/verity/ originally were slightly useful when writing fs/verity/
> originally, but after that I never really used them. Instead I found they
> actually made patching fs/verity/ a bit harder, since I had to make sure to keep
> all the pr_debug statements updated as code changed around them.
pr_debug is largely useless outside of code development purposes.
The value in tracepoints is that they are available for diagnosing
problems on production systems and should be thought of as such.
Yes, you can also use them to debug development code, but in that
environment they are no substitute for custom trace_printk() debug
output.
However, when you have extensive tracepoints coverage, the amount of
custom trace_printk() stuff you need to add to a kernel to debug an
issue ends up being limited, because most of the key state and
object changes in the code are already covered by tracepoints.
> Maybe I am an outlier and other people really do like having these tracepoints
> around. But I'd like to see a bit more feedback along those lines first. If we
> could keep them to a more minimal set, that would also be helpful.
For people who are used to subsystems with extensive tracepoint
coverage (like XFS), the lack of tracepoints in all the surrounding
code is jarring. It makes the rest of the system feel like a black
hole where detailed runtime introspection is almost completely
impossible without a *lot* of work.
Extensive tracepoints help everyone in the production support
and diagnosis chain understand what is going on by providing easy to
access runtime introspection for the code. i.e. they provide
benefit to far more people than just the one kernel developer who
enables pr_debug on the subsystem when developing new code...
-Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2024-02-26 2:24 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-12 16:57 [PATCH v4 00/25] fs-verity support for XFS Andrey Albershteyn
2024-02-12 16:57 ` [PATCH v4 01/25] fsverity: remove hash page spin lock Andrey Albershteyn
2024-02-12 16:57 ` [PATCH v4 02/25] xfs: add parent pointer support to attribute code Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 03/25] xfs: define parent pointer ondisk extended attribute format Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 04/25] xfs: add parent pointer validator functions Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 05/25] fs: add FS_XFLAG_VERITY for verity files Andrey Albershteyn
2024-02-23 4:23 ` Eric Biggers
2024-02-23 12:55 ` Andrey Albershteyn
2024-02-23 17:59 ` Eric Biggers
2024-02-12 16:58 ` [PATCH v4 06/25] fsverity: pass log_blocksize to end_enable_verity() Andrey Albershteyn
2024-02-15 21:45 ` Dave Chinner
2024-02-16 16:18 ` Andrey Albershteyn
2024-02-23 4:26 ` Eric Biggers
2024-02-23 13:02 ` Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 07/25] fsverity: support block-based Merkle tree caching Andrey Albershteyn
2024-02-23 5:24 ` Eric Biggers
2024-02-23 16:02 ` Andrey Albershteyn
2024-02-23 18:07 ` Eric Biggers
2024-02-24 14:10 ` Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 08/25] fsverity: calculate readahead in bytes instead of pages Andrey Albershteyn
2024-02-23 5:29 ` Eric Biggers
2024-02-12 16:58 ` [PATCH v4 09/25] fsverity: add tracepoints Andrey Albershteyn
2024-02-23 5:31 ` Eric Biggers
2024-02-23 13:23 ` Andrey Albershteyn
2024-02-23 18:27 ` Eric Biggers
2024-02-26 2:24 ` Dave Chinner [this message]
2024-02-12 16:58 ` [PATCH v4 10/25] iomap: integrate fsverity verification into iomap's read path Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 11/25] xfs: add XBF_VERITY_SEEN xfs_buf flag Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 12/25] xfs: add XFS_DA_OP_BUFFER to make xfs_attr_get() return buffer Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 13/25] xfs: introduce workqueue for post read IO work Andrey Albershteyn
2024-02-15 22:11 ` Dave Chinner
2024-02-16 16:29 ` Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 14/25] xfs: add attribute type for fs-verity Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 15/25] xfs: make xfs_buf_get() to take XBF_* flags Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 16/25] xfs: add XBF_DOUBLE_ALLOC to increase size of the buffer Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 17/25] xfs: add fs-verity ro-compat flag Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 18/25] xfs: add inode on-disk VERITY flag Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 19/25] xfs: initialize fs-verity on file open and cleanup on inode destruction Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 20/25] xfs: don't allow to enable DAX on fs-verity sealsed inode Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 21/25] xfs: disable direct read path for fs-verity files Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 22/25] xfs: add fs-verity support Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 23/25] xfs: make scrub aware of verity dinode flag Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 24/25] xfs: add fs-verity ioctls Andrey Albershteyn
2024-02-12 16:58 ` [PATCH v4 25/25] xfs: enable ro-compat fs-verity flag Andrey Albershteyn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zdv2YZP0sIzDA5UT@dread.disaster.area \
--to=david@fromorbit.com \
--cc=aalbersh@redhat.com \
--cc=chandan.babu@oracle.com \
--cc=djwong@kernel.org \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).