diff mbox series

iomap: standardize tracepoint formatting and storage

Message ID 20210822023223.GY12640@magnolia (mailing list archive)
State Deferred, archived
Headers show
Series iomap: standardize tracepoint formatting and storage | expand

Commit Message

Darrick J. Wong Aug. 22, 2021, 2:32 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Print all the offset, pos, and length quantities in hexadecimal.  While
we're at it, update the types of the tracepoint structure fields to
match the types of the values being recorded in them.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/iomap/trace.h |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Matthew Wilcox Aug. 22, 2021, 3:30 a.m. UTC | #1 
On Sat, Aug 21, 2021 at 07:32:23PM -0700, Darrick J. Wong wrote:
> @@ -58,8 +58,7 @@ DECLARE_EVENT_CLASS(iomap_range_class,
>  		__entry->offset = off;
>  		__entry->length = len;
>  	),
> -	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset %lx "
> -		  "length %x",
> +	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset 0x%llx length 0x%llx",

%#llx is one character shorter than 0x%llx ;-)
Darrick J. Wong Aug. 22, 2021, 5:20 p.m. UTC | #2 
On Sun, Aug 22, 2021 at 04:30:24AM +0100, Matthew Wilcox wrote:
> On Sat, Aug 21, 2021 at 07:32:23PM -0700, Darrick J. Wong wrote:
> > @@ -58,8 +58,7 @@ DECLARE_EVENT_CLASS(iomap_range_class,
> >  		__entry->offset = off;
> >  		__entry->length = len;
> >  	),
> > -	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset %lx "
> > -		  "length %x",
> > +	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset 0x%llx length 0x%llx",
> 
> %#llx is one character shorter than 0x%llx ;-)

...and since you declined to add even a conditional review tag, this
patch will now sit in limbo for 2-3 days while I agonize over whether 1)
I should send a v2 patch with this minor stylistic thing changed and
hope that nobody objects to the v2, or worse, tells me I should go back
to v1; 2) hope that someone else won't be as picky and send a review; 3)
prepare myself for the inevitable DM that's coming about how I'm too
soft on reviewers; 4) wait the other inevitable DM about how I really
could exercise maintainer's prerogative to ignore the trivial style
complaints; and 5) brace for the other inevitable DM about how I really
shouldn't abandon the review process to ram code into the kernel just
because of stylistic complaints.

Yeah, I'm not going to abandon the established public process.
Requiring RVB even for maintainer patches is a useful check against
maintainers committing garbage to the repo.

But.  All five of these things keep happening every kernel cycle, and
it's dragging down my mental health.  To say nothing of the worldwide
disease, political unrest, and hostile weather.

**This** is the reason why I want to quit and never come back.

I enjoy every part of the process from requirements to implementation.
I enjoy discussing the design of algorithms, and I find it satisfying
even to rip things out to try other suggestions.

I don't enjoy revving patchsets repeatedly to try to find the magic
combination of printf formatting strings, comment massage, and helper
function placement golf that will get every patch through review.  This
is why I'm constantly stressed out about not being able to keep up with
all the demands being placed on me.  Experts' time should be spent on
solving tough problems, debugging the really nasty bugs, and coaching
less familiar people as they get up to speed.  Not this.

So I'm putting everyone on notice: If you care about stylistic problems
(and while I'm on my soapbox: cleaning up other parts of the codebase
that a patch touches only briefly) so strongly, then change my patch and
send it back to me.  Or change the snippets you want in the reply, and
state that you ack/review the patch on condition that the snippets are
applied.

Once I'm done with 5.15 merge patches, I would like to have a broader
discussion about xfs/iomap review policies, and have requested a session
at LPC for this purpose.

(And in case it wasn't clear: If you think a piece of code is so poorly
fitted to a problem that you want me to go back to the drawing board, I
am still completely happy to do that.)

--D
Christoph Hellwig Aug. 23, 2021, 7:03 a.m. UTC | #3 
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Christoph Hellwig Aug. 23, 2021, 7:05 a.m. UTC | #4 
On Sun, Aug 22, 2021 at 04:30:24AM +0100, Matthew Wilcox wrote:
> On Sat, Aug 21, 2021 at 07:32:23PM -0700, Darrick J. Wong wrote:
> > @@ -58,8 +58,7 @@ DECLARE_EVENT_CLASS(iomap_range_class,
> >  		__entry->offset = off;
> >  		__entry->length = len;
> >  	),
> > -	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset %lx "
> > -		  "length %x",
> > +	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset 0x%llx length 0x%llx",
> 
> %#llx is one character shorter than 0x%llx ;-)

But at least for me a little harder to read.
Dave Chinner Aug. 23, 2021, 10:08 p.m. UTC | #5 
On Sat, Aug 21, 2021 at 07:32:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Print all the offset, pos, and length quantities in hexadecimal.  While
> we're at it, update the types of the tracepoint structure fields to
> match the types of the values being recorded in them.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/iomap/trace.h |   19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Looks good. That will help when mixed with XFS tracepoints :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff mbox series

Patch

diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h
index f1519f9a1403..48cff616ef81 100644
--- a/fs/iomap/trace.h
+++ b/fs/iomap/trace.h
@@ -42,14 +42,14 @@  DEFINE_READPAGE_EVENT(iomap_readpage);
 DEFINE_READPAGE_EVENT(iomap_readahead);
 
 DECLARE_EVENT_CLASS(iomap_range_class,
-	TP_PROTO(struct inode *inode, unsigned long off, unsigned int len),
+	TP_PROTO(struct inode *inode, loff_t off, u64 len),
 	TP_ARGS(inode, off, len),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
 		__field(u64, ino)
 		__field(loff_t, size)
-		__field(unsigned long, offset)
-		__field(unsigned int, length)
+		__field(loff_t, offset)
+		__field(u64, length)
 	),
 	TP_fast_assign(
 		__entry->dev = inode->i_sb->s_dev;
@@ -58,8 +58,7 @@  DECLARE_EVENT_CLASS(iomap_range_class,
 		__entry->offset = off;
 		__entry->length = len;
 	),
-	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset %lx "
-		  "length %x",
+	TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset 0x%llx length 0x%llx",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __entry->size,
@@ -69,7 +68,7 @@  DECLARE_EVENT_CLASS(iomap_range_class,
 
 #define DEFINE_RANGE_EVENT(name)		\
 DEFINE_EVENT(iomap_range_class, name,	\
-	TP_PROTO(struct inode *inode, unsigned long off, unsigned int len),\
+	TP_PROTO(struct inode *inode, loff_t off, u64 len),\
 	TP_ARGS(inode, off, len))
 DEFINE_RANGE_EVENT(iomap_writepage);
 DEFINE_RANGE_EVENT(iomap_releasepage);
@@ -122,8 +121,8 @@  DECLARE_EVENT_CLASS(iomap_class,
 		__entry->flags = iomap->flags;
 		__entry->bdev = iomap->bdev ? iomap->bdev->bd_dev : 0;
 	),
-	TP_printk("dev %d:%d ino 0x%llx bdev %d:%d addr %lld offset %lld "
-		  "length %llu type %s flags %s",
+	TP_printk("dev %d:%d ino 0x%llx bdev %d:%d addr 0x%llx offset 0x%llx "
+		  "length 0x%llx type %s flags %s",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  MAJOR(__entry->bdev), MINOR(__entry->bdev),
@@ -149,7 +148,7 @@  TRACE_EVENT(iomap_iter,
 		__field(dev_t, dev)
 		__field(u64, ino)
 		__field(loff_t, pos)
-		__field(loff_t, length)
+		__field(u64, length)
 		__field(unsigned int, flags)
 		__field(const void *, ops)
 		__field(unsigned long, caller)
@@ -163,7 +162,7 @@  TRACE_EVENT(iomap_iter,
 		__entry->ops = ops;
 		__entry->caller = caller;
 	),
-	TP_printk("dev %d:%d ino 0x%llx pos %lld length %lld flags %s (0x%x) ops %ps caller %pS",
+	TP_printk("dev %d:%d ino 0x%llx pos 0x%llx length 0x%llx flags %s (0x%x) ops %ps caller %pS",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		   __entry->ino,
 		   __entry->pos,