Re: [PATCH 2/2] dlm: Add down/up_write_non_owner to keep lockdep happy

Re: [PATCH 2/2] dlm: Add down/up_write_non_owner to keep lockdep happy

[Ingo Molnar]

* Steven Whitehouse <swhiteho@redhat.com> wrote:

> I looked at possibly changing this to use completions, but
> it seems that the usage here is not easily adapted to that.
> This patch adds suitable annotation to the write side of
> the ls_in_recovery semaphore so that we don't get nasty
> messages from lockdep when mounting a gfs2 filesystem.

What do those 'nasty messages' say? If they expose some bug and this
patch works around that bug by hiding it then NAK ...

        Ingo

[patch quoted below]

> 
> Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  fs/dlm/lockspace.c    |    2 +-
>  fs/dlm/member.c       |    2 +-
>  fs/dlm/recoverd.c     |    2 +-
>  include/linux/rwsem.h |    4 ++++
>  kernel/rwsem.c        |   16 ++++++++++++++++
>  5 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
> index 8dde538..fa0cc22 100644
> --- a/fs/dlm/lockspace.c
> +++ b/fs/dlm/lockspace.c
> @@ -551,7 +551,7 @@ static int new_lockspace(const char *name, int namelen, void **lockspace,
>  	INIT_LIST_HEAD(&ls->ls_root_list);
>  	init_rwsem(&ls->ls_root_sem);
>  
> -	down_write(&ls->ls_in_recovery);
> +	down_write_non_owner(&ls->ls_in_recovery);
>  
>  	spin_lock(&lslist_lock);
>  	ls->ls_create_count = 1;
> diff --git a/fs/dlm/member.c b/fs/dlm/member.c
> index b128775..99bd086 100644
> --- a/fs/dlm/member.c
> +++ b/fs/dlm/member.c
> @@ -318,7 +318,7 @@ int dlm_ls_stop(struct dlm_ls *ls)
>  	 */
>  
>  	if (new)
> -		down_write(&ls->ls_in_recovery);
> +		down_write_non_owner(&ls->ls_in_recovery);
>  
>  	/*
>  	 * The recoverd suspend/resume makes sure that dlm_recoverd (if
> diff --git a/fs/dlm/recoverd.c b/fs/dlm/recoverd.c
> index fd677c8..376479a 100644
> --- a/fs/dlm/recoverd.c
> +++ b/fs/dlm/recoverd.c
> @@ -40,7 +40,7 @@ static int enable_locking(struct dlm_ls *ls, uint64_t seq)
>  	if (ls->ls_recover_seq == seq) {
>  		set_bit(LSFL_RUNNING, &ls->ls_flags);
>  		/* unblocks processes waiting to enter the dlm */
> -		up_write(&ls->ls_in_recovery);
> +		up_write_non_owner(&ls->ls_in_recovery);
>  		error = 0;
>  	}
>  	spin_unlock(&ls->ls_recover_lock);
> diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
> index efd348f..34643df 100644
> --- a/include/linux/rwsem.h
> +++ b/include/linux/rwsem.h
> @@ -80,12 +80,16 @@ extern void down_write_nested(struct rw_semaphore *sem, int subclass);
>   *   proper abstraction for this case is completions. ]
>   */
>  extern void down_read_non_owner(struct rw_semaphore *sem);
> +extern void down_write_non_owner(struct rw_semaphore *sem);
>  extern void up_read_non_owner(struct rw_semaphore *sem);
> +extern void up_write_non_owner(struct rw_semaphore *sem);
>  #else
>  # define down_read_nested(sem, subclass)		down_read(sem)
>  # define down_write_nested(sem, subclass)	down_write(sem)
>  # define down_read_non_owner(sem)		down_read(sem)
> +# define down_write_non_owner(sem)		down_write(sem)
>  # define up_read_non_owner(sem)			up_read(sem)
> +# define up_write_non_owner(sem)		up_write(sem)
>  #endif
>  
>  #endif /* _LINUX_RWSEM_H */
> diff --git a/kernel/rwsem.c b/kernel/rwsem.c
> index cae050b..2c57eef 100644
> --- a/kernel/rwsem.c
> +++ b/kernel/rwsem.c
> @@ -126,6 +126,15 @@ void down_read_non_owner(struct rw_semaphore *sem)
>  
>  EXPORT_SYMBOL(down_read_non_owner);
>  
> +void down_write_non_owner(struct rw_semaphore *sem)
> +{
> +	might_sleep();
> +
> +	 __down_write(sem);
> +}
> +
> +EXPORT_SYMBOL(down_write_non_owner);
> +
>  void down_write_nested(struct rw_semaphore *sem, int subclass)
>  {
>  	might_sleep();
> @@ -143,6 +152,13 @@ void up_read_non_owner(struct rw_semaphore *sem)
>  
>  EXPORT_SYMBOL(up_read_non_owner);
>  
> +void up_write_non_owner(struct rw_semaphore *sem)
> +{
> +	__up_write(sem);
> +}
> +
> +EXPORT_SYMBOL(up_write_non_owner);
> +
>  #endif