Hi Chuck,
On Fri, Jun 21, 2024 at 12:22 PM <cel@xxxxxxxxxx> wrote:
>
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> An administrator cannot take action on these messages, but the
> reported errors might be helpful for troubleshooting. Transition
> them to trace points so these events appear in the trace log and
> can be easily lined up with other traced NFS client operations.
>
> Examples:
>
> append_writer-6147 [000] 80.247393: bl_pr_key_reg: device=sdb key=0x666dcdabf29514fe
> append_writer-6147 [000] 80.247842: bl_pr_key_unreg: device=sdb key=0x666dcdabf29514fe
>
> umount.nfs4-6172 [002] 84.950409: bl_pr_key_unreg_err: device=sdb key=0x666dcdabf29514fe error=24
>
> Christoph points out that:
> > ... 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.
>
> The trace points copy the pr_info() that they are replacing, and
> show only the parent device name and not the partition. I'm still
> looking into how to record both parts of the device name.
>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> fs/nfs/blocklayout/dev.c | 30 +++++++-------
> fs/nfs/nfs4trace.c | 7 ++++
> fs/nfs/nfs4trace.h | 88 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 111 insertions(+), 14 deletions(-)
>
> diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
> index 568f685dee4b..6c5d290ca81d 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
>
> @@ -26,12 +27,14 @@ bl_free_device(struct pnfs_block_dev *dev)
> if (test_and_clear_bit(PNFS_BDEV_REGISTERED, &dev->flags)) {
> struct block_device *bdev = file_bdev(dev->bdev_file);
> const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
> - int error;
> + int status;
>
> - error = ops->pr_register(file_bdev(dev->bdev_file),
> - dev->pr_key, 0, false);
> - if (error)
> - pr_err("failed to unregister PR key.\n");
> + status = ops->pr_register(bdev, dev->pr_key, 0, false);
> + if (status)
> + trace_bl_pr_key_unreg_err(bdev, dev->pr_key,
> + status);
> + else
> + trace_bl_pr_key_unreg(bdev, dev->pr_key);
> }
>
> if (dev->bdev_file)
> @@ -243,10 +246,10 @@ static bool bl_pr_register_scsi(struct pnfs_block_dev *dev)
>
> status = ops->pr_register(bdev, 0, dev->pr_key, true);
> if (status) {
> - pr_err("pNFS: failed to register key for block device %s.",
> - bdev->bd_disk->disk_name);
> + trace_bl_pr_key_reg_err(bdev, dev->pr_key, status);
> return false;
> }
> + trace_bl_pr_key_reg(bdev, dev->pr_key);
> return true;
> }
>
> @@ -351,8 +354,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))
> @@ -373,8 +377,9 @@ bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d,
> 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;
>
> @@ -383,13 +388,10 @@ 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;
> }
> diff --git a/fs/nfs/nfs4trace.c b/fs/nfs/nfs4trace.c
> index d22c6670f770..389941ccc9c9 100644
> --- a/fs/nfs/nfs4trace.c
> +++ b/fs/nfs/nfs4trace.c
> @@ -2,6 +2,8 @@
> /*
> * Copyright (c) 2013 Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> */
> +#include <uapi/linux/pr.h>
> +#include <linux/blkdev.h>
> #include <linux/nfs_fs.h>
> #include "nfs4_fs.h"
> #include "internal.h"
> @@ -29,5 +31,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..f2090a491fcb 100644
> --- a/fs/nfs/nfs4trace.h
> +++ b/fs/nfs/nfs4trace.h
> @@ -2153,6 +2153,94 @@ TRACE_EVENT(ff_layout_commit_error,
> )
> );
>
> +DECLARE_EVENT_CLASS(pnfs_bl_pr_key_class,
> + TP_PROTO(
> + const struct block_device *bdev,
> + u64 key
> + ),
> + TP_ARGS(bdev, key),
> + TP_STRUCT__entry(
> + __field(u64, key)
> + __field(dev_t, dev)
> + __string(device, bdev->bd_disk->disk_name)
> + ),
> + TP_fast_assign(
> + __entry->key = key;
> + __entry->dev = bdev->bd_dev;
^^^^^^^
b4 tells me this patch adds trailing whitespace at the line above:
Patch failed at 0004 nfs/blocklayout: SCSI layout trace points for
reservation key reg/unreg
/home/anna/Projects/linux-nfs.git/.git/rebase-apply/patch:135:
trailing whitespace.
__entry->dev = bdev->bd_dev;
/home/anna/Projects/linux-nfs.git/.git/rebase-apply/patch:188:
trailing whitespace.
__entry->dev = bdev->bd_dev;
error: patch failed: fs/nfs/blocklayout/dev.c:383
error: fs/nfs/blocklayout/dev.c: patch does not apply
> + __assign_str(device);
> + ),
> + TP_printk("dev=%d,%d (%s) key=0x%016llx",
> + MAJOR(__entry->dev), MINOR(__entry->dev),
> + __get_str(device), __entry->key
> + )
> +);
> +
> +#define DEFINE_NFS4_BLOCK_PRKEY_EVENT(name) \
> + DEFINE_EVENT(pnfs_bl_pr_key_class, name, \
> + TP_PROTO( \
> + const struct block_device *bdev, \
> + u64 key \
> + ), \
> + TP_ARGS(bdev, key))
> +DEFINE_NFS4_BLOCK_PRKEY_EVENT(bl_pr_key_reg);
> +DEFINE_NFS4_BLOCK_PRKEY_EVENT(bl_pr_key_unreg);
> +
> +/*
> + * From uapi/linux/pr.h
> + */
> +TRACE_DEFINE_ENUM(PR_STS_SUCCESS);
> +TRACE_DEFINE_ENUM(PR_STS_IOERR);
> +TRACE_DEFINE_ENUM(PR_STS_RESERVATION_CONFLICT);
> +TRACE_DEFINE_ENUM(PR_STS_RETRY_PATH_FAILURE);
> +TRACE_DEFINE_ENUM(PR_STS_PATH_FAST_FAILED);
> +TRACE_DEFINE_ENUM(PR_STS_PATH_FAILED);
> +
> +#define show_pr_status(x) \
> + __print_symbolic(x, \
> + { PR_STS_SUCCESS, "SUCCESS" }, \
> + { PR_STS_IOERR, "IOERR" }, \
> + { PR_STS_RESERVATION_CONFLICT, "RESERVATION_CONFLICT" }, \
> + { PR_STS_RETRY_PATH_FAILURE, "RETRY_PATH_FAILURE" }, \
> + { PR_STS_PATH_FAST_FAILED, "PATH_FAST_FAILED" }, \
> + { PR_STS_PATH_FAILED, "PATH_FAILED" })
> +
> +DECLARE_EVENT_CLASS(pnfs_bl_pr_key_err_class,
> + TP_PROTO(
> + const struct block_device *bdev,
> + u64 key,
> + int status
> + ),
> + TP_ARGS(bdev, key, status),
> + TP_STRUCT__entry(
> + __field(u64, key)
> + __field(dev_t, dev)
> + __field(unsigned long, status)
> + __string(device, bdev->bd_disk->disk_name)
> + ),
> + TP_fast_assign(
> + __entry->key = key;
> + __entry->dev = bdev->bd_dev;
^^^^^^
And for this line, too.
Thanks,
Anna
> + __entry->status = status;
> + __assign_str(device);
> + ),
> + TP_printk("dev=%d,%d (%s) key=0x%016llx status=%s",
> + MAJOR(__entry->dev), MINOR(__entry->dev),
> + __get_str(device), __entry->key,
> + show_pr_status(__entry->status)
> + )
> +);
> +
> +#define DEFINE_NFS4_BLOCK_PRKEY_ERR_EVENT(name) \
> + DEFINE_EVENT(pnfs_bl_pr_key_err_class, name, \
> + TP_PROTO( \
> + const struct block_device *bdev, \
> + u64 key, \
> + int status \
> + ), \
> + TP_ARGS(bdev, key, status))
> +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);
> --
> 2.45.1
>
>
