| Message ID | 20240619173929.177818-7-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Snapshot of fixes for SCSI PR key registration | expand |
> #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. Note that the disk_name isn't really what we'd want to trace anyway, as it misses the partition information. 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. > +++ 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.
On Thu, Jun 20, 2024 at 06:50:46AM +0200, Christoph Hellwig wrote: > 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. Sorry. This is of course client code and I just did not have enough coffee yet.
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. >
diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c index 519c310c745d..b3828e5ee079 100644 --- a/fs/nfs/blocklayout/dev.c +++ b/fs/nfs/blocklayout/dev.c @@ -10,6 +10,7 @@ #include <linux/pr.h> #include "blocklayout.h" +#include "../nfs4trace.h" #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; + struct block_device *bdev = file_bdev(dev->bdev_file); + const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops; int error; - error = ops->pr_register(file_bdev(dev->bdev_file), - dev->pr_key, 0, false); + error = ops->pr_register(bdev, dev->pr_key, 0, false); 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); } if (dev->bdev_file) @@ -327,8 +331,9 @@ bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d, struct pnfs_block_volume *volumes, int idx, gfp_t gfp_mask) { struct pnfs_block_volume *v = &volumes[idx]; - struct file *bdev_file; + struct block_device *bdev; const struct pr_ops *ops; + struct file *bdev_file; int error; if (!bl_validate_designator(v)) @@ -346,8 +351,9 @@ bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d, if (IS_ERR(bdev_file)) return PTR_ERR(bdev_file); d->bdev_file = bdev_file; + bdev = file_bdev(bdev_file); - d->len = bdev_nr_bytes(file_bdev(d->bdev_file)); + d->len = bdev_nr_bytes(bdev); d->map = bl_map_simple; d->pr_key = v->scsi.pr_key; @@ -356,23 +362,20 @@ bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d, goto out_blkdev_put; } - pr_info("pNFS: using block device %s (reservation key 0x%llx)\n", - file_bdev(d->bdev_file)->bd_disk->disk_name, d->pr_key); - - ops = file_bdev(d->bdev_file)->bd_disk->fops->pr_ops; + ops = bdev->bd_disk->fops->pr_ops; if (!ops) { pr_err("pNFS: block device %s does not support reservations.", - file_bdev(d->bdev_file)->bd_disk->disk_name); + bdev->bd_disk->disk_name); error = -EINVAL; goto out_blkdev_put; } - error = ops->pr_register(file_bdev(d->bdev_file), 0, d->pr_key, true); + error = ops->pr_register(bdev, 0, d->pr_key, true); if (error) { - pr_err("pNFS: failed to register key for block device %s.", - file_bdev(d->bdev_file)->bd_disk->disk_name); + trace_bl_pr_key_reg_err(bdev->bd_disk->disk_name, d->pr_key, error); goto out_blkdev_put; } + trace_bl_pr_key_reg(bdev->bd_disk->disk_name, d->pr_key); d->pr_registered = true; return 0; diff --git a/fs/nfs/nfs4trace.c b/fs/nfs/nfs4trace.c index d22c6670f770..74eff35fbc90 100644 --- a/fs/nfs/nfs4trace.c +++ 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); + EXPORT_TRACEPOINT_SYMBOL_GPL(fl_getdevinfo); #endif diff --git a/fs/nfs/nfs4trace.h b/fs/nfs/nfs4trace.h index 4de8780a7c48..2af75b8e018d 100644 --- a/fs/nfs/nfs4trace.h +++ b/fs/nfs/nfs4trace.h @@ -2153,6 +2153,68 @@ TRACE_EVENT(ff_layout_commit_error, ) ); +DECLARE_EVENT_CLASS(pnfs_bl_pr_key_class, + TP_PROTO( + const char *name, + u64 key + ), + TP_ARGS(name, key), + TP_STRUCT__entry( + __string(name, name) + __field(u64, key) + ), + TP_fast_assign( + __assign_str(name); + __entry->key = key; + ), + TP_printk("device=%s key=0x%llx", + __get_str(name), __entry->key + ) +); + +#define DEFINE_NFS4_BLOCK_PRKEY_EVENT(name) \ + DEFINE_EVENT(pnfs_bl_pr_key_class, name, \ + TP_PROTO( \ + const char *name, \ + u64 key \ + ), \ + TP_ARGS(name, key)) +DEFINE_NFS4_BLOCK_PRKEY_EVENT(bl_pr_key_reg); +DEFINE_NFS4_BLOCK_PRKEY_EVENT(bl_pr_key_unreg); + +DECLARE_EVENT_CLASS(pnfs_bl_pr_key_err_class, + TP_PROTO( + const char *name, + u64 key, + int error + ), + TP_ARGS(name, key, error), + TP_STRUCT__entry( + __string(name, name) + __field(u64, key) + __field(int, error) + ), + TP_fast_assign( + __assign_str(name); + __entry->key = key; + __entry->error = error; + ), + TP_printk("device=%s key=0x%llx error=%d", + __get_str(name), __entry->key, __entry->error + ) +); + +#define DEFINE_NFS4_BLOCK_PRKEY_ERR_EVENT(name) \ + DEFINE_EVENT(pnfs_bl_pr_key_err_class, name, \ + TP_PROTO( \ + const char *name, \ + u64 key, \ + int error \ + ), \ + TP_ARGS(name, key, error)) +DEFINE_NFS4_BLOCK_PRKEY_ERR_EVENT(bl_pr_key_reg_err); +DEFINE_NFS4_BLOCK_PRKEY_ERR_EVENT(bl_pr_key_unreg_err); + #ifdef CONFIG_NFS_V4_2 TRACE_DEFINE_ENUM(NFS4_CONTENT_DATA); TRACE_DEFINE_ENUM(NFS4_CONTENT_HOLE);