diff mbox series

[v2,4/4] nfs/blocklayout: SCSI layout trace points for reservation key reg/unreg

Message ID 20240621162227.215412-10-cel@kernel.org (mailing list archive)
State New
Headers show
Series Fixes for pNFS SCSI layout PR key registration | expand

Commit Message

Chuck Lever June 21, 2024, 4:22 p.m. UTC 
From: Chuck Lever <chuck.lever@oracle.com>

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@oracle.com>
---
 fs/nfs/blocklayout/dev.c | 30 +++++++-------
 fs/nfs/nfs4trace.c       |  7 ++++
 fs/nfs/nfs4trace.h       | 88 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 14 deletions(-)

Comments

Anna Schumaker June 21, 2024, 5:21 p.m. UTC | #1 
Hi Chuck,

On Fri, Jun 21, 2024 at 12:22 PM <cel@kernel.org> wrote:
>
> From: Chuck Lever <chuck.lever@oracle.com>
>
> 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@oracle.com>
> ---
>  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@netapp.com>
>   */
> +#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
>
>
Chuck Lever III June 21, 2024, 5:46 p.m. UTC | #2 
> On Jun 21, 2024, at 1:21 PM, Anna Schumaker <schumaker.anna@gmail.com> wrote:
> 
> Hi Chuck,
> 
> On Fri, Jun 21, 2024 at 12:22 PM <cel@kernel.org> wrote:
>> 
>> From: Chuck Lever <chuck.lever@oracle.com>
>> 
>> 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@oracle.com>
>> ---
>> 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@netapp.com>
>>  */
>> +#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

Fixed in my tree. Will appear in v3 of this series.


>> +               __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


--
Chuck Lever
Christoph Hellwig June 22, 2024, 5:09 a.m. UTC | #3 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

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@netapp.com>
  */
+#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;   
+		__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;   
+		__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);