[ogfs-dev]More comments on inital OpenDLM / OpenGFS patch

["Cahill, Ben M" <ben.m.cahill@intel.com>]

Hi Stanley,

Just a few more detailed comments:

1)  In deadman_block_others() and init_deadman(), I see that you use LKM_NOQUEUE in calls to dlmlock().  Don't these locks need to be queued (hopefully for a *long* time), so that the AST function, other_node_die(), would execute when a node dies?

2)  In init_deadman(), I see a return code of OGFS_DLM_FAILED in case of failure of kmalloc().  I think it might be better to return a standard NOMEM type of error code.  Reason:  Be specific about cause of failure, and use available standard error codes when possible.  Comments from others??

3)  In start_deadman_lock(), it would probably be good to set *first to either TRUE or FALSE in all cases.  Right now, it does not get set at all in case OGFS_DLM_FAILED.  Perhaps you could move *first = FALSE to be above the switch() statement?

4)  In opendlm_mount(), spelling errors in printk(): s/encount/enounter  s/collsion/collision

5)  In opendlm_get_lock, it would probably be good to memset the LVB to 0.

6)  Do you need the my_dlmlock_sync() function??  I'm wondering if you can use the ODLM dlmlock_sync() function instead??  I see that you're providing the "cancel" feature in my_dlmlock_sync() ... the "Programming Locking Applications" doc claims that dlmunlock() can be used to cancel a dlmlock_sync() request.

7)  In my_dlmlock_sync(), it would probably be good to set lock->done = 0 before calling dlmlock().  We should not trust the calling function to do that for us (or should we?).  Should we trust lock->cancel?  Could it cause a problem if we set lock->cancel = 0 within my_dlmlock_sync()??

8)  In block_others(), spelling error in printk(): s/Unkonwn/Unknown

9)  In opendlm_lockname{ }, what does __attribute__((packed)) do?

10)  In opendlm_lockname{ }, it might be more efficient (computationally) to change the order of the elements, placing the most unique one (lock_num) first.  Comparisons/searches of the opendlm_lockname{ } would fail more rapidly that way.

Thanks again!

-- Ben --

Opinions are mine, not Intel's

<<winmail.dat>>