On Thu, Jun 20, 2024 at 06:50:46AM +0200, Christoph Hellwig wrote:
> > #define NFSDBG_FACILITY NFSDBG_PNFS_LD
> >
> > @@ -24,14 +25,17 @@ bl_free_device(struct pnfs_block_dev *dev)
> > kfree(dev->children);
> > } else {
> > if (dev->pr_registered) {
> > - const struct pr_ops *ops =
> > - file_bdev(dev->bdev_file)->bd_disk->fops->pr_ops;
>
> If you touch this it might be worth returnin early before the else
> above to reduce the indentation here a bit.
>
> > if (error)
> > - pr_err("failed to unregister PR key.\n");
> > + trace_bl_pr_key_unreg_err(bdev->bd_disk->disk_name,
> > + dev->pr_key, error);
> > + else
> > + trace_bl_pr_key_unreg(bdev->bd_disk->disk_name,
> > + dev->pr_key);
>
> I'd just pass the bdev to the tracepoint and derefence it there only
> when tracing is enabled.
I usually take that approach in hot paths, though this didn't seem
that performance critical and I assumed we didn't want to pull the
block device includes into everything that includes nfs4trace.h. But
I will look into that, it might not be that bad.
> Note that the disk_name isn't really what
> we'd want to trace anyway, as it misses the partition information.
Right, that part I was actually not sure about.
> The normal way to print the device name is the %pg printk specifier,
> but I'm not sure how to correctly use that for tracing which wants
> a string in the entry for binary tracing.
I'll see what can be done.
> > +++ b/fs/nfs/nfs4trace.c
> > @@ -29,5 +29,10 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(ff_layout_read_error);
> > EXPORT_TRACEPOINT_SYMBOL_GPL(ff_layout_write_error);
> > EXPORT_TRACEPOINT_SYMBOL_GPL(ff_layout_commit_error);
> >
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(bl_pr_key_reg);
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(bl_pr_key_reg_err);
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(bl_pr_key_unreg);
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(bl_pr_key_unreg_err);
>
> This is weird. The trace points for nfsd really should be in
> fs/nfsd/trace.h and not in fs/nfs/ as that would then pull in
> the client code into the server.
>
--
Chuck Lever
