Re: [PATCH 2/5] vfs: Add tracepoints for inode_excl_inuse_trylock/unlock

[Amir Goldstein <amir73il@gmail.com>]

On Mon, Jan 31, 2022 at 5:13 PM David Howells <dhowells@redhat.com> wrote:
>
> Add tracepoints for inode_excl_inuse_trylock/unlock() to record successful
> and lock, failed lock, successful unlock and unlock when it wasn't locked.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Amir Goldstein <amir73il@gmail.com>
> cc: Miklos Szeredi <miklos@szeredi.hu>
> cc: linux-unionfs@vger.kernel.org
> cc: linux-cachefs@redhat.com
> ---
>
>  fs/inode.c           |   21 +++++++++++++++++----
>  fs/overlayfs/super.c |   10 ++++++----
>  include/linux/fs.h   |    9 +++++++--
>  3 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 954719f66113..61b93a89853f 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -22,6 +22,8 @@
>  #include <linux/iversion.h>
>  #include <trace/events/writeback.h>
>  #include "internal.h"
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/vfs.h>
>
>  /*
>   * Inode locking rules:
> @@ -2409,11 +2411,14 @@ EXPORT_SYMBOL(current_time);
>  /**
>   * inode_excl_inuse_trylock - Try to exclusively lock an inode for kernel access
>   * @dentry: Reference to the inode to be locked
> + * @o: Private reference for the kernel service
> + * @who: Which kernel service is trying to gain the lock
>   *
>   * Try to gain exclusive access to an inode for a kernel service, returning
>   * true if successful.
>   */
> -bool inode_excl_inuse_trylock(struct dentry *dentry)
> +bool inode_excl_inuse_trylock(struct dentry *dentry, unsigned int o,
> +                             enum inode_excl_inuse_by who)
>  {
>         struct inode *inode = d_inode(dentry);
>         bool locked = false;
> @@ -2421,7 +2426,10 @@ bool inode_excl_inuse_trylock(struct dentry *dentry)
>         spin_lock(&inode->i_lock);
>         if (!(inode->i_state & I_EXCL_INUSE)) {
>                 inode->i_state |= I_EXCL_INUSE;
> +               trace_inode_excl_inuse_lock(inode, o, who);
>                 locked = true;
> +       } else {
> +               trace_inode_excl_inuse_lock_failed(inode, o, who);
>         }
>         spin_unlock(&inode->i_lock);
>
> @@ -2432,18 +2440,23 @@ EXPORT_SYMBOL(inode_excl_inuse_trylock);
>  /**
>   * inode_excl_inuse_unlock - Unlock exclusive kernel access to an inode
>   * @dentry: Reference to the inode to be unlocked
> + * @o: Private reference for the kernel service
>   *
>   * Drop exclusive access to an inode for a kernel service.  A warning is given
>   * if the inode was not marked for exclusive access.
>   */
> -void inode_excl_inuse_unlock(struct dentry *dentry)
> +void inode_excl_inuse_unlock(struct dentry *dentry, unsigned int o)
>  {
>         if (dentry) {
>                 struct inode *inode = d_inode(dentry);
>
>                 spin_lock(&inode->i_lock);
> -               WARN_ON(!(inode->i_state & I_EXCL_INUSE));
> -               inode->i_state &= ~I_EXCL_INUSE;
> +               if (WARN_ON(!(inode->i_state & I_EXCL_INUSE))) {
> +                       trace_inode_excl_inuse_unlock_bad(inode, o);
> +               } else {
> +                       inode->i_state &= ~I_EXCL_INUSE;
> +                       trace_inode_excl_inuse_unlock(inode, o);
> +               }
>                 spin_unlock(&inode->i_lock);
>         }
>  }
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 5c3361a2dc7c..6434ae11496d 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -224,10 +224,10 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>         dput(ofs->indexdir);
>         dput(ofs->workdir);
>         if (ofs->workdir_locked)
> -               inode_excl_inuse_unlock(ofs->workbasedir);
> +               inode_excl_inuse_unlock(ofs->workbasedir, 0);
>         dput(ofs->workbasedir);
>         if (ofs->upperdir_locked)
> -               inode_excl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root);
> +               inode_excl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root, 0);
>
>         /* Hack!  Reuse ofs->layers as a vfsmount array before freeing it */
>         mounts = (struct vfsmount **) ofs->layers;
> @@ -1239,7 +1239,8 @@ static int ovl_get_upper(struct super_block *sb, struct ovl_fs *ofs,
>         if (upper_mnt->mnt_sb->s_flags & SB_NOSEC)
>                 sb->s_flags |= SB_NOSEC;
>
> -       if (inode_excl_inuse_trylock(ovl_upper_mnt(ofs)->mnt_root)) {
> +       if (inode_excl_inuse_trylock(ovl_upper_mnt(ofs)->mnt_root, 0,
> +                                    inode_excl_inuse_by_overlayfs)) {
>                 ofs->upperdir_locked = true;
>         } else {
>                 err = ovl_report_in_use(ofs, "upperdir");
> @@ -1499,7 +1500,8 @@ static int ovl_get_workdir(struct super_block *sb, struct ovl_fs *ofs,
>
>         ofs->workbasedir = dget(workpath.dentry);
>
> -       if (inode_excl_inuse_trylock(ofs->workbasedir)) {
> +       if (inode_excl_inuse_trylock(ofs->workbasedir, 0,
> +                                    inode_excl_inuse_by_overlayfs)) {

More incentive to keep the ovl_* wrappers.

Thanks,
Amir.