dlm: device interface
PATCH 5/7 dlm: device interface
This is a separate module from the dlm. It exports the dlm api to user space through a misc device. Applications use a library (libdlm) which communicates with the kernel through this device.
---
drivers/dlm/device.c | 1154 +++++++++++++++++++++++++++++++++++++++++++++ drivers/dlm/device.h | 20 include/linux/dlm_device.h | 86 +++ 3 files changed, 1260 insertions(+)
include/linux/dlm_device.h +86 -0
@@ -0,0 +1,86 @@ +/****************************************************************************** +******************************************************************************* +** +** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved. +** Copyright (C) 2004-2005 Red Hat, Inc. All rights reserved. +** +** This copyrighted material is made available to anyone wishing to use, +** modify, copy, or redistribute it subject to the terms and conditions +** of the GNU General Public License v.2. +** +******************************************************************************* +******************************************************************************/ + +/* This is the device interface for dlm, most users will use a library +* interface. +*/ + +/* Version of the device interface */ +#define DLM_DEVICE_VERSION_MAJOR 3 +#define DLM_DEVICE_VERSION_MINOR 0 +#define DLM_DEVICE_VERSION_PATCH 0 + +#ifndef __KERNEL +#define __user +#endif + +/* struct passed to the lock write */ +struct dlm_lock_params { +uint8_t mode; +uint16_t flags; +uint32_t lkid; +uint32_t parent; +struct dlm_range range; +uint8_t namelen; +void __user *castparam; +void __user *castaddr; +void __user *bastparam; +void __user *bastaddr; +struct dlm_lksb __user *lksb; +char lvb[DLM_LVB_LEN ]; +char name[1 ]; +}; + +struct dlm_lspace_params { +uint32_t flags; +uint32_t minor; +char name[1 ]; +}; + +struct dlm_write_request { +uint32_t version[3 ]; +uint8_t cmd; + +union { +struct dlm_lock_params lock; +struct dlm_lspace_params lspace; + } i; +}; + +/* struct read from the "device" fd, +consists mainly of userspace pointers for the library to use */ +struct dlm_lock_result { +uint32_t length; +void __user * user_astaddr; +void __user * user_astparam; +struct dlm_lksb __user * user_lksb; +struct dlm_lksb lksb; +uint8_t bast_mode; +/* Offsets may be zero if no data is present */ +uint32_t lvb_offset; +}; + +/* Commands passed to the device */ +#define DLM_USER_LOCK 1 +#define DLM_USER_UNLOCK 2 +#define DLM_USER_QUERY 3 +#define DLM_USER_CREATE_LOCKSPACE 4 +#define DLM_USER_REMOVE_LOCKSPACE 5 + +/* Arbitrary length restriction */ +#define MAX_LS_NAME_LEN 64 + +/* Lockspace flags */ +#define DLM_USER_LSFLG_AUTOFREE 1 +#define DLM_USER_LSFLG_FORCEFREE 2 +
drivers/dlm/device.c +1154 -0
1,154 lines not on this page · raw message
drivers/dlm/device.h +20 -0
@@ -0,0 +1,20 @@ +/****************************************************************************** +******************************************************************************* +** +** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved. +** Copyright (C) 2004-2005 Red Hat, Inc. All rights reserved. +** +** This copyrighted material is made available to anyone wishing to use, +** modify, copy, or redistribute it subject to the terms and conditions +** of the GNU General Public License v.2. +** +******************************************************************************* +******************************************************************************/ + +#ifndef __DEVICE_DOT_H__ +#define __DEVICE_DOT_H__ + +extern void dlm_device_free_devices (void ); +extern int dlm_device_init (void ); +extern void dlm_device_exit (void ); +#endif /* __DEVICE_DOT_H__ */
Quoted 8 lines · David Teigland
This is a separate module from the dlm. It exports the dlm api to user space through a misc device. Applications use a library (libdlm) which communicates with the kernel through this device.
Signed-off-by Dave Teigland <[email protected]>Signed-off-by Patrick Caulfield <[email protected]>
[...]
drivers/dlm/device.c line 150
Quoted 11 lines
drivers/dlm/device.c +11
-0+static void release_lockinfo (struct lock_info *li) +{ +put_file_info (li->li_file); + +down_write (&lockinfo_lock); +idr_remove (&lockinfo_idr, li->li_lksb.sb_lkid); +up_write (&lockinfo_lock); + +if (li->li_lksb.sb_lvbptr) +kfree (li->li_lksb.sb_lvbptr); +kfree (li);
checking li->li_lksb.sb_lvbptr for NULL here is redundant. kfree() checks for NULL itself - kfree(0) is perfectly valid, so just do
kfree(li->li_lksb.sb_lvbptr); kfree(li);
and get rid if the if (li->li_lksb.sb_lvbptr) bit.
drivers/dlm/device.c line 334
Quoted 7 lines
drivers/dlm/device.c +7
-0+static void bast_routine (void *param,int mode) +{ +struct lock_info *li = param; + +if (li && li->li_bastaddr) { +add_to_astqueue (li, li->li_bastaddr, li->li_bastparam,0 ); + }
^^^
superfluous bracket.[...]
Quoted 47 lines
drivers/dlm/device.c +47
-0+static void ast_routine (void *param) +{ +struct lock_info *li = param; + +/* Param may be NULL if a persistent lock is unlocked by someone else */ +if (!li) +return ; + +/* If this is a succesful conversion then activate the blocking ast +* args from the conversion request */ +if (!test_bit (LI_FLAG_FIRSTLOCK , &li->li_flags) && + li->li_lksb.sb_status ==0 ) { + + li->li_bastparam = li->li_pend_bastparam; + li->li_bastaddr = li->li_pend_bastaddr; + li->li_pend_bastaddr =NULL ; + } + +/* If it's an async request then post data to the user's AST queue. */ +if (li->li_castaddr) { +int lvb_updated =0 ; + +/* See if the lvb has been updated */ +if (dlm_lvb_operations[li->li_grmode+1 ][li->li_rqmode+1 ] ==1 ) + lvb_updated =1 ; + +if (li->li_lksb.sb_status ==0 ) + li->li_grmode = li->li_rqmode; + +/* Only queue AST if the device is still open */ +if (test_bit (1 , &li->li_file->fi_flags)) +add_to_astqueue (li, li->li_castaddr, li->li_castparam, lvb_updated); + +/* If it's a new lock operation that failed, then +* remove it from the owner queue and free the +* lock_info. +*/ +if (test_and_clear_bit (LI_FLAG_FIRSTLOCK , &li->li_flags) && + li->li_lksb.sb_status !=0 ) { + +/* Wait till dlm_lock() has finished */ +down (&li->li_firstlock); +up (&li->li_firstlock); + +spin_lock (&li->li_file->fi_li_lock); +list_del (&li->li_ownerqueue); +spin_unlock (&li->li_file->fi_li_lock);
drivers/dlm/device.c line 398
Quoted 10 lines
drivers/dlm/device.c +10
-0+release_lockinfo (li); +return ; + } +/* Free unlocks & queries */ +if (li->li_lksb.sb_status == -DLM_EUNLOCK || + li->li_cmd ==DLM_USER_QUERY ) { +release_lockinfo (li); + } + } +else {
else should be on same line as bracket according to Documentation/CodingStyle
if (foo) {
/* ... */
} else {
/* ... */
}[...]
drivers/dlm/device.c line 426
Quoted 6 lines
drivers/dlm/device.c +6
-0+/* Open on control device */ +static int dlm_ctl_open (struct inode *inode,struct file *file) +{ + file->private_data =NULL ; +return 0 ; +}
If you are always going to return zero, then why not just have the function return void instead?
drivers/dlm/device.c line 432
Quoted 5 lines
drivers/dlm/device.c +5
-0+/* Close on control device */ +static int dlm_ctl_close (struct inode *inode,struct file *file) +{ +return 0 ; +}
return void? and what's the purpose of this function? seems silly to me to have a function that does nothing but return 0 ever.
[...]
drivers/dlm/device.c line 601
Quoted 6 lines
drivers/dlm/device.c +6
-0+if (lsinfo->ls_lockspace) { +if (test_bit (LS_FLAG_AUTOFREE , &lsinfo->ls_flags)) { +//TODO this breaks! unregister_lockspace(lsinfo, 1); + } + } +else {
should be "} else {" - there are more cases of this elsewhere, but I'm not going to point them all out.
Quoted 10 lines
drivers/dlm/device.c +10
-0+static int do_user_lock (struct file_info *fi,uint8_t cmd,struct dlm_lock_params *kparams) +{ +struct lock_info *li; +int status; + +/* +* Validate things that we need to have correct. +*/ +if (!kparams->castaddr) +return -EINVAL ;
drivers/dlm/device.c line 848
Quoted 10 lines
drivers/dlm/device.c +10
-0+ +if (!kparams->lksb) +return -EINVAL ; + +/* Persistent child locks are not available yet */ +if ((kparams->flags &DLM_LKF_PERSISTENT ) && kparams->parent) +return -EINVAL ; + +/* For conversions, there should already be a lockinfo struct, +unless we are adopting an orphaned persistent lock */
^^^^Why indent this comment with two extra spaces and not just a
tab like the other ones?
drivers/dlm/device.c line 851
Quoted 3 lines
drivers/dlm/device.c +3
-0+if (kparams->flags &DLM_LKF_CONVERT ) { + + li =get_lockinfo (kparams->lkid);
Why the extra blank line between the if statement and the first statement inside the if?
Signature · 1 line
Jesper Juhl
Quoted 9 lines · Jesper Juhl
drivers/dlm/device.c line 432
Quoted 5 lines · David Teigland
drivers/dlm/device.c +5
-0+/* Close on control device */ +static int dlm_ctl_close (struct inode *inode,struct file *file) +{ +return 0 ; +}return void? and what's the purpose of this function? seems silly to me to have a function that does nothing but return 0 ever.
How about just adding "inline" calling it "documentation"?
Regards,
Daniel
Quoted 2 lines
+static struct
rw_semaphorelockinfo_lock;
why are you using a rwsem here? rw semaphores are *expensive*. Really expensive. Might well not be worth it unless you expect really frequent readers compared to writers *and* long hold times. Can you give an idea about how frequent the reader case is?
(and if it's really frequent, wouldn't RCU be a better solution?)
include/linux/dlm_device.h line 25
Quoted 3 lines · David Teigland
include/linux/dlm_device.h +3
-0+#ifndef __KERNEL +#define __user +#endif
What is this for?
include/linux/dlm_device.h line 32
Quoted 6 lines
include/linux/dlm_device.h +6
-0+/* struct passed to the lock write */ +struct dlm_lock_params { +uint8_t mode; +uint16_t flags; +uint32_t lkid; +uint32_t parent;
These are crossing the userspace/kernelspace boundry, please mark them __u8, __u16, and __u32.
include/linux/dlm_device.h line 41
Quoted 9 lines
include/linux/dlm_device.h +9
-0+struct dlm_range range; +uint8_t namelen; +void __user *castparam; +void __user *castaddr; +void __user *bastparam; +void __user *bastaddr; +struct dlm_lksb __user *lksb; +char lvb[DLM_LVB_LEN ]; +char name[1 ];
Mix of tabs and spaces.
Quoted 4 lines
+/*
+ * ioctls to create/remove lockspaces, and check how many
+ * outstanding ASTs there are against a particular LS.
+ */
Your comment is wrong, based on the code.
Quoted 12 lines
drivers/dlm/device.c +12
-0+static int dlm_ioctl (struct inode *inode,struct file *file, +uint command,ulong u) +{ +struct file_info *fi = file->private_data; +int status = -EINVAL ; +int count; +struct list_head *tmp_list; + +switch (command) { + +/* Are there any ASTs for us to read? +* Warning, this returns the number of messages (ASTs)
drivers/dlm/device.c line 640
Quoted 10 lines
drivers/dlm/device.c +10
-0+* in the queue, NOT the number of bytes to read +*/ +case FIONREAD : + count =0 ; +spin_lock (&fi->fi_ast_lock); +list_for_each (tmp_list, &fi->fi_ast_list) + count++; +spin_unlock (&fi->fi_ast_lock); + status =put_user (count, (int *)u); +break ;
Is this really needed?
thanks,
greg k-h