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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



First of all, many thanks for your carefully reviewing :)

On Wed, 2003-09-10 at 04:08, Cahill, Ben M wrote:
> 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?
The purpose of using LKM_NOQUEUE: If I am the first node, we will get
the lock immediately, otherwise I am not the first node. And it is only
used when try to grab other node's deadman lock.

> 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??
Sure. You are right.

> 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?
OK.

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

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

> 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.
I hope to use dlmlock_sync also :) But it could not be used by kernel
mode client :(

> 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()??
Yeah, I've found this bug and fixed it in my debug version codes.
Thanks :)

> 8)  In block_others(), spelling error in printk(): s/Unkonwn/Unknown
Thanks :)
> 
> 9)  In opendlm_lockname{ }, what does __attribute__((packed)) do?
It will pack all fields against compiler's optimizing.
Because the length of lock name is limited, we must ensure the size the
struct opendlm_lockname. 

> 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.
Sure.



Best Regrads,
Stan
> Thanks again!
> 
> -- Ben --
> 
> Opinions are mine, not Intel's
-- 
Opinions expressed are those of the author and do not represent Intel
Corporation
"gpg --recv-keys --keyserver wwwkeys.pgp.net E1390A7F"
{E1390A7F:3AD1 1B0C 2019 E183 0CFF  55E8 369A 8B75 E139 0A7F}



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Opengfs-devel mailing list
Opengfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opengfs-devel

[Index of Archives]     [Kernel]     [Security]     [Bugtraq]     [MIPS Linux]     [ARM Linux]     [Linux Clusters]     [Linux RAID]     [Yosemite Hiking]
  Powered by Linux