[Ocfs2-devel] [PATCH] ocfs2: make lockres lookup faster

[Sunil Mushran <sunil.mushran@oracle.com>]

The dlm interface allows different sized locknames. And the locknames can be
binary. That we use mostly ascii is just coincidental. Yes, mostly. The 
dentry
lock is partially binary. Also, $RECOVERY is used only during recovery.

So the only interesting bit from my pov would be:

-		if (memcmp(res->lockname.name + 1, name + 1, len - 1))
+		if (memcmp(res->lockname.name, name, len))

Will just this change improve performance? How long a hash list would need
to be for us to see an appreciable improvement?

Sunil


Wengang Wang wrote:
> Lockres lookup is within the dlm->spinlock. We'd better finish the lookup as
> fast as possible especially when the machine is with more cpus.
>
> Existing lookup is comparing charactors starting on a non-aligned address which
> takes more time. This patch improves the performance mostly by changing comparing
> on non-aligned address to comparing on aligned address. Also it makes all lockres
> have same name length so that comparing length is not needed. And thus the extra
> comparing on the first charactor is not needed any longer.
>
> This patch changes recovery lockres name length from 9 to 31. This change doesn't
> have much badness.
>
> Per my test on the loop comparations in user space, This change at most can get
> 15.7% faster.
>
> Questions:
> 1. Is there other special lockres name with non-31 length?
> 2. If all lockres name length is changed to 32(including the tailing '\n'), it
> gets at most 19% faster, but increase 1 byte network transfer for very request.
> I don't know whether this is worthy.
>
> Drawbacks:
> 1. It changes locking version which makes rolling upgrade impossible.
>
> #I didn't test the patch yet.
>
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  fs/ocfs2/dlm/dlmcommon.h    |    5 +++--
>  fs/ocfs2/dlm/dlmdomain.c    |    6 +-----
>  fs/ocfs2/ocfs2_lockingver.h |    5 ++++-
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmcommon.h b/fs/ocfs2/dlm/dlmcommon.h
> index 0102be3..c41ebf5 100644
> --- a/fs/ocfs2/dlm/dlmcommon.h
> +++ b/fs/ocfs2/dlm/dlmcommon.h
> @@ -91,8 +91,9 @@ enum dlm_ast_type {
>  			 LKM_CANCEL | LKM_INVVALBLK | LKM_FORCE | \
>  			 LKM_RECOVERY | LKM_LOCAL | LKM_NOQUEUE)
>  
> -#define DLM_RECOVERY_LOCK_NAME       "$RECOVERY"
> -#define DLM_RECOVERY_LOCK_NAME_LEN   9
> +#define DLM_RECOVERY_LOCK_NAME       "$RECOVERY0000000000000000000000"
> +/* make the length of recovery lock name the same as normal ones */
> +#define DLM_RECOVERY_LOCK_NAME_LEN   31
>  
>  static inline int dlm_is_recovery_lock(const char *lock_name, int name_len)
>  {
> diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
> index 988c905..7062d48 100644
> --- a/fs/ocfs2/dlm/dlmdomain.c
> +++ b/fs/ocfs2/dlm/dlmdomain.c
> @@ -191,11 +191,7 @@ struct dlm_lock_resource * __dlm_lookup_lockres_full(struct dlm_ctxt *dlm,
>  	hlist_for_each(list, bucket) {
>  		struct dlm_lock_resource *res = hlist_entry(list,
>  			struct dlm_lock_resource, hash_node);
> -		if (res->lockname.name[0] != name[0])
> -			continue;
> -		if (unlikely(res->lockname.len != len))
> -			continue;
> -		if (memcmp(res->lockname.name + 1, name + 1, len - 1))
> +		if (memcmp(res->lockname.name, name, len))
>  			continue;
>  		dlm_lockres_get(res);
>  		return res;
> diff --git a/fs/ocfs2/ocfs2_lockingver.h b/fs/ocfs2/ocfs2_lockingver.h
> index 2e45c8d..7fd9260 100644
> --- a/fs/ocfs2/ocfs2_lockingver.h
> +++ b/fs/ocfs2/ocfs2_lockingver.h
> @@ -25,8 +25,11 @@
>   * more details.
>   *
>   * 1.0 - Initial locking version from ocfs2 1.4.
> + * 1.1 - Change recovery lock name from "$recovery" to
> + *       "$recovery00000000000000000000000". --make it 31 bytes long, same as
> + *       the length of normal lock name.
>   */
>  #define OCFS2_LOCKING_PROTOCOL_MAJOR 1
> -#define OCFS2_LOCKING_PROTOCOL_MINOR 0
> +#define OCFS2_LOCKING_PROTOCOL_MINOR 1
>  
>  #endif  /* OCFS2_LOCKINGVER_H */