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