iomap: Fix writepage tracepoint pgoff

Commit Message

Matthew Wilcox March 4, 2020, 2:22 p.m. UTC

From: Matthew Wilcox (Oracle) <[e-mail]>

page_offset() confusingly returns the number of bytes from the
beginning of the file and not the pgoff, which the tracepoint claims
to be returning.  We're already returning the number of bytes from the
beginning of the file in the 'offset' parameter, so correct the pgoff
to be what was apparently intended.

Fixes: 0b1b213fcf3a ("xfs: event tracing support")
Signed-off-by: Matthew Wilcox (Oracle) <[e-mail]>

Comments

Christoph Hellwig March 4, 2020, 3:25 p.m. UTC | #1

On Wed, Mar 04, 2020 at 06:22:59AM -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox (Oracle) <[e-mail]>
> 
> page_offset() confusingly returns the number of bytes from the
> beginning of the file and not the pgoff, which the tracepoint claims
> to be returning.  We're already returning the number of bytes from the
> beginning of the file in the 'offset' parameter, so correct the pgoff
> to be what was apparently intended.
> 
> Fixes: 0b1b213fcf3a ("xfs: event tracing support")
> Signed-off-by: Matthew Wilcox (Oracle) <[e-mail]>

I wonder if tracing the byte offset and just changing the name
might be more useful.  But I agree that we should fix it one way or
another.

Matthew Wilcox March 4, 2020, 3:34 p.m. UTC | #2

On Wed, Mar 04, 2020 at 07:25:15AM -0800, Christoph Hellwig wrote:
> On Wed, Mar 04, 2020 at 06:22:59AM -0800, Matthew Wilcox wrote:
> > From: Matthew Wilcox (Oracle) <[e-mail]>
> > 
> > page_offset() confusingly returns the number of bytes from the
> > beginning of the file and not the pgoff, which the tracepoint claims
> > to be returning.  We're already returning the number of bytes from the
> > beginning of the file in the 'offset' parameter, so correct the pgoff
> > to be what was apparently intended.
> > 
> > Fixes: 0b1b213fcf3a ("xfs: event tracing support")
> > Signed-off-by: Matthew Wilcox (Oracle) <[e-mail]>
> 
> I wonder if tracing the byte offset and just changing the name
> might be more useful.  But I agree that we should fix it one way or
> another.

I covered that -- "We're already returning the number of bytes from the
beginning of the file in the 'offset' parameter, so correct the pgoff
to be what was apparently intended."

I mean, we could just delete the pgoff instead.  Apparently nobody's
using it, or they would surely have noticed.

Christoph Hellwig March 4, 2020, 3:36 p.m. UTC | #3

On Wed, Mar 04, 2020 at 07:34:00AM -0800, Matthew Wilcox wrote:
> I covered that -- "We're already returning the number of bytes from the
> beginning of the file in the 'offset' parameter, so correct the pgoff
> to be what was apparently intended."
> 
> I mean, we could just delete the pgoff instead.  Apparently nobody's
> using it, or they would surely have noticed.

Let's just kill it.

Patch

diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h
index d6ba705f938a..ebc89ec5e6c7 100644
--- a/fs/iomap/trace.h
+++ b/fs/iomap/trace.h
@@ -56,7 +56,7 @@  DECLARE_EVENT_CLASS(iomap_page_class,
 	TP_fast_assign(
 		__entry->dev = inode->i_sb->s_dev;
 		__entry->ino = inode->i_ino;
-		__entry->pgoff = page_offset(page);
+		__entry->pgoff = page->index;
 		__entry->size = i_size_read(inode);
 		__entry->offset = off;
 		__entry->length = len;