Hi Tao,<div><br></div><div>The new design looks OK me.<br>
BTW, you have removed the section about "ll_lookup_lock" from the first
version document. So you will not change related logic, right? I think
this part should be fixed in new kernels.<br>
<br>
Cheers,<br>
Nasf<br><br><div class="gmail_quote">On Thu, Feb 9, 2012 at 3:15 PM, <span dir="ltr"><<a href="mailto:tao.peng@emc.com">tao.peng@emc.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> -----Original Message-----<br>
> From: Fan Yong [mailto:<a href="mailto:yong.fan@whamcloud.com">yong.fan@whamcloud.com</a>]<br>
</div><div><div class="h5">> Sent: Saturday, February 04, 2012 12:18 AM<br>
> To: Peng, Tao<br>
> Cc: <a href="mailto:adilger@whamcloud.com">adilger@whamcloud.com</a>; <a href="mailto:bergwolf@gmail.com">bergwolf@gmail.com</a>; Tang, Haiying; faibish, sorin; Liu, Xuezhao;<br>
> <a href="mailto:laisiyao@whamcloud.com">laisiyao@whamcloud.com</a>; <a href="mailto:green@whamcloud.com">green@whamcloud.com</a>; <a href="mailto:eeb@whamcloud.com">eeb@whamcloud.com</a>; <a href="mailto:bryon@whamcloud.com">bryon@whamcloud.com</a>; lustre-<br>
> <a href="mailto:devel@lists.lustre.org">devel@lists.lustre.org</a><br>
> Subject: Re: Lustre dcache clean up and distributed locking<br>
><br>
> On 2/3/12 3:37 PM, <a href="mailto:tao.peng@emc.com">tao.peng@emc.com</a> wrote:<br>
> >> -----Original Message-----<br>
> >> From: Fan Yong [mailto:<a href="mailto:yong.fan@whamcloud.com">yong.fan@whamcloud.com</a>]<br>
> >> Sent: Tuesday, January 31, 2012 8:02 PM<br>
> >> To: Peng, Tao<br>
> >> Cc: Andreas Dilger; <a href="mailto:bergwolf@gmail.com">bergwolf@gmail.com</a>; Tang, Haiying; faibish, sorin; Liu, Xuezhao;<br>
> >> <a href="mailto:laisiyao@whamcloud.com">laisiyao@whamcloud.com</a>; <a href="mailto:green@whamcloud.com">green@whamcloud.com</a>; <a href="mailto:eeb@whamcloud.com">eeb@whamcloud.com</a>; <a href="mailto:bryon@whamcloud.com">bryon@whamcloud.com</a>; lustre-<br>
> >> <a href="mailto:devel@lists.lustre.org">devel@lists.lustre.org</a><br>
> >> Subject: Re: Lustre dcache clean up and distributed locking<br>
> >><br>
> >> On 1/31/12 7:27 AM, Andreas Dilger wrote:<br>
> >>> On 2012-01-30, at 12:46 AM,<<a href="mailto:tao.peng@emc.com">tao.peng@emc.com</a>> wrote:<br>
> >>>> Sorry for the late reply. I got your email earlier but didn't have chance to read the document<br>
> due<br>
> >> to very limited internet access.<br>
> >>>> Thank you very much for taking detailed look at the document and giving so many valuable comments.<br>
> >>>><br>
> >>>> About adding new inode bitlock, I must admit that I missed hardlink case. Although the lock mode<br>
> >> can be changed to be the same as LOOKUP lock, I agree that inode bitlock cannot sufficiently<br>
> protect<br>
> >> every name/dentry separately for hardlinked files. And I agree, as you suggested in the document,<br>
> >> adding a name entry based lock can solve it.<br>
> >>>> IIUC, to add a name entry based lock, both client and server should be modified, and LDLM need to<br>
> >> be changed to manage the new lock information. Like new inode bitlock, the name entry based lock<br>
> will<br>
> >> as well introduce interoperability issues as a result of on wire protocol change. And in DNE<br>
> >> environment as Andreas explained, the original plan is to get LOOKUP on both master MDT and remote<br>
> MDT.<br>
> >> If we introduce name entry based lock, DNE case may need some change as well.<br>
> >>>> I will take a closer look at the approach. In the meanwhile, could you please confirm if it is<br>
> the<br>
> >> right way to go? The benefit of above complexity seems to be better distributed dentry locking<br>
> under<br>
> >> dcache framework.<br>
> >>>> What do others think? Andreas?<br>
> >>> Tao, before we go down the road of implementing such a change, there would have to be clear<br>
> benefits<br>
> >> to the performance and/or kernel integration before we could accept the interoperability issues and<br>
> >> added code complexity.<br>
> >>> The benefits of having a per-name lock, as I see them, are:<br>
> >>> - can be cancelled separately from other name locks<br>
> >>> - can be changed independently from the inode permissions (LOOKUP)<br>
> >>> - potential ease of integration with the Linux dcache<br>
> >>><br>
> >>> The drawbacks of introducing a per-name lock:<br>
> >>> - only useful for hard-linked files (uncommon case)<br>
> >>> - added code complexity for a new lock type<br>
> >>> - incompatibility with existing client/server<br>
> >>> - additional code to handle incompatibility transparently<br>
> >>> - lock enqueue may need extra RPC for each name lock (different resource)<br>
> >>> - lock cancellation may need an extra RPC for each resource<br>
> >>><br>
> >>> The way I see it is that the DENTRY lock would need to be a different resource for each name on a<br>
> >> file. The only way this is going to be useful AFAICS is if the client enqueues each DENTRY lock on<br>
> a<br>
> >> separate resource, so that it can be granted and cancelled independently of the LOOKUP lock (which<br>
> is<br>
> >> on the FID and would be common for all names that reference this inode). That would either need an<br>
> >> extra RPC for every filename lookup (to enqueue both the DENTRY and LOOKUP locks) or a new<br>
> >> LDLM_ENQUEUE RPC that can request and be granted two different locks (FID [seq, oid, ver, 0], and<br>
> >> FID+name hash [seq, oid, ver, hash]) in a single RPC.<br>
> >>> So, the main question is how much better the client dcache/VFS integration will be with the DCACHE<br>
> >> lock compared to the existing mechanism with the LOOKUP (FID) lock? Is it impossible (or very<br>
> >> difficult) to use the existing mechanism for the new lockless dcache code? Or is it a matter of<br>
> this<br>
> >> being a somewhat cleaner implementation, but not critical for moving forward?<br>
> >>> I'm aware that our dcache code has been too complex for some time, but I haven't looked recently<br>
> to<br>
> >> see how much of this is due to using/supporting old kernel APIs, and how much of it is due to the<br>
> use<br>
> >> of the LOOKUP lock on the FID protecting too much of the state on the client.<br>
> >>> Cheers, Andreas<br>
> >> Hi Tao,<br>
> >><br>
> >> To introduce either per-name lock or DENTRY lock is mainly for removing<br>
> >> Lustre special flag "DCACHE_LUSTRE_INVALID" to simply Lustre code. But<br>
> >> if such simplification causes more complex ldlm logic, we have to<br>
> >> consider whether it is worth to remove "DCACHE_LUSTRE_INVALID". It is<br>
> >> not impossible to make "DCACHE_LUSTRE_INVALID" accepted by linux kernel.<br>
> >> On the other hand, "DCACHE_LUSTRE_INVALID" can work together with<br>
> >> lockless dcache in new linux kernel also.<br>
> >><br>
> >> So to push the project move forward, we can divide it as two parts<br>
> >> (according to your document): Lustre dcache code cleanup, and some new<br>
> >> ldlm logic to remove "DCACHE_LUSTRE_INVALID". Before we reach an<br>
> >> agreement on the second part, we can start from the first part to<br>
> >> cleanup Lustre dcache code. How do you think?<br>
> >><br>
> > Thank you, Andreas and Yong. I tend to agree with you that we should avoid the complexity if there<br>
> is no obvious benefits. However, as we need to make sure the code is acceptable by kernel community, I<br>
> will try to consolidate a design for adding the new type of lock but just don't implement it at this<br>
> point. And if later we are asked (by kernel community developers) to do it, we just need to do the<br>
> implementation.<br>
><br>
> Hi Tao,<br>
><br>
> We has another way to remove Lustre special dentry flags<br>
> "DCACHE_LUSTRE_INVALID". To indicate the dentry in dcache without LOOKUP<br>
> lock protected but not unhashed yet, we can add new flags in<br>
> "ll_dentry_data" (pointed by "dentry::d_fsdata"). Since it is Lustre<br>
> private date, no need to worry about whether the new flags will conflict<br>
> with any other VFS standard "dentry::d_flags" or not, and maybe save<br>
> some effort to implement the complex DENTRY lock in your original design.<br>
><br>
</div></div>Hi Fan Yong,<br>
<br>
Thanks for your great suggestion. I updated the design doc with following changes, please help to review. Thank you!<br>
<br>
Changelog:<br>
1. Macro cleanups are removed. I prefer to do it separately while cleaning up other obsolete macros.<br>
2. Remove DENTRY LDLM lock proposal. Because DCACHE_LUSTRE_INVALID flag can be hidden from generic VFS layer, it is no longer necessary to introduce such complex change.<br>
3. Add a section to describe how to add LLD_LUSTRE_INVALID flag to ll_dentry_data structure to replace existing DCACHE_LUSTRE_INVALID flag.<br>
<br>
Cheers,<br>
Tao<br>
<div class="HOEnZb"><div class="h5">> Cheers,<br>
> Nasf<br>
><br>
> > Please stay tuned.<br>
> ><br>
> > Best,<br>
> > Tao<br>
> ><br>
> >> Cheers,<br>
> >> --<br>
> >> Nasf<br>
> >><br>
> >>><br>
> >>>>> -----Original Message-----<br>
> >>>>> From: Fan Yong [mailto:<a href="mailto:yong.fan@whamcloud.com">yong.fan@whamcloud.com</a>]<br>
> >>>>> Sent: Saturday, January 21, 2012 1:11 AM<br>
> >>>>> To: Peng, Tao; <a href="mailto:bergwolf@gmail.com">bergwolf@gmail.com</a><br>
> >>>>> Cc: Tang, Haiying; <a href="mailto:adilger@whamcloud.com">adilger@whamcloud.com</a>; faibish, sorin; Liu, Xuezhao; <a href="mailto:laisiyao@whamcloud.com">laisiyao@whamcloud.com</a>;<br>
> >>>>> <a href="mailto:green@whamcloud.com">green@whamcloud.com</a>; <a href="mailto:eeb@whamcloud.com">eeb@whamcloud.com</a>; Bryon Neitzel<br>
> >>>>> Subject: Re: Lustre dcache clean up and distributed locking<br>
> >>>>><br>
> >>>>> Hi Tao,<br>
> >>>>><br>
> >>>>> Excellent work. I just added some comments inside your document. Please check.<br>
> >>>>><br>
> >>>>><br>
> >>>>> Best Regards,<br>
> >>>>> Fan Yong<br>
> >>>>> Whamcloud, Inc.<br>
> >>>>><br>
> >>>>> On 1/20/12 9:34 AM, <a href="mailto:haiying.tang@emc.com">haiying.tang@emc.com</a> wrote:<br>
> >>>>>> Hi Andreas,<br>
> >>>>>><br>
> >>>>>> Tao is on vacation now. It might be difficult for him to check emails due to limited internet<br>
> >> access<br>
> >>>>> at hometown.<br>
> >>>>>> For urgent issue, you folks could send email to his gmail account <a href="mailto:bergwolf@gmail.com">bergwolf@gmail.com</a>.<br>
> >>>>>><br>
> >>>>>> Thanks,<br>
> >>>>>> Haiying<br>
> >>>>>><br>
> >>>>>> -----Original Message-----<br>
> >>>>>> From: Andreas Dilger [mailto:<a href="mailto:adilger@whamcloud.com">adilger@whamcloud.com</a>]<br>
> >>>>>> Sent: 2012年1月20日 1:39<br>
> >>>>>> To: Peng, Tao<br>
> >>>>>> Cc: faibish, sorin; Tang, Haiying; Liu, Xuezhao;<br>
> >>>>>> <a href="mailto:laisiyao@whamcloud.com">laisiyao@whamcloud.com</a>; <a href="mailto:yong.fan@whamcloud.com">yong.fan@whamcloud.com</a>; <a href="mailto:green@whamcloud.com">green@whamcloud.com</a>;<br>
> >>>>>> <a href="mailto:eeb@whamcloud.com">eeb@whamcloud.com</a><br>
> >>>>>> Subject: Re: Lustre dcache clean up and distributed locking<br>
> >>>>>><br>
> >>>>>> On 2012-01-17, at 3:21 AM,<<a href="mailto:tao.peng@emc.com">tao.peng@emc.com</a>> <<a href="mailto:tao.peng@emc.com">tao.peng@emc.com</a>> wrote:<br>
> >>>>>>> Thanks Siyao and Oleg for answering my dcache revalidation question on lustre mailing list. I<br>
> >>>>> updated the design to reflect it.<br>
> >>>>>>> Please see attachment.<br>
> >>>>>> Tao,<br>
> >>>>>> Fan Yong is also taking a more detailed look at your document and will<br>
> >>>>>> hopefully have a chance to reply before the New Year holidays.<br>
> >>>>>><br>
> >>>>>> Also, we are just working on landing the patches to add support for<br>
> >>>>>> Linux<br>
> >>>>>> 2.6.38 for the Lustre client. One of the patches relates to the<br>
> >>>>>> lockless dcache changes that were introduced in that kernel. If you<br>
> >>>>>> are interested to review this patch, and become more familiar with the<br>
> >>>>>> Lustre development process, you should visit <a href="http://review.whamcloud.com/1865" target="_blank">http://review.whamcloud.com/1865</a> for the patch.<br>
> >>>>>><br>
> >>>>>> You need to create an account in Gerrit using OpenID (Google, mostly),<br>
> >>>>>> and an account in our bug tracking system (<a href="http://jira.whamcloud.com" target="_blank">http://jira.whamcloud.com</a>)<br>
> >>>>>> if you haven't already.<br>
> >>>>>><br>
> >>>>>>>> -----Original Message-----<br>
> >>>>>>>> From: Andreas Dilger [mailto:<a href="mailto:adilger@whamcloud.com">adilger@whamcloud.com</a>]<br>
> >>>>>>>> Sent: Tuesday, January 17, 2012 4:16 PM<br>
> >>>>>>>> To: Peng, Tao<br>
> >>>>>>>> Cc: faibish, sorin; Tang, Haiying; Liu, Xuezhao; Lai Siyao; Fan<br>
> >>>>>>>> Yong; Oleg Drokin; Eric Barton<br>
> >>>>>>>> Subject: Re: Lustre dcache clean up and distributed locking<br>
> >>>>>>>><br>
> >>>>>>>> On 2012-01-16, at 9:25 PM,<<a href="mailto:tao.peng@emc.com">tao.peng@emc.com</a>> wrote:<br>
> >>>>>>>>> I finally started to work on Lustre dcache cleanup and locking.<br>
> >>>>>>>>> After reading Lustre, ocfs2 and VFS<br>
> >>>>>>>> dcache related code, I came to a design for cleaning up Lustre<br>
> >>>>>>>> dcache code and doing distributed locking under dcache. For<br>
> >>>>>>>> distributed locking, the main idea is to add a new inode bitlock<br>
> >>>>>>>> DENTRY lock to just protect valid dentry, instead of letting LOOKUP<br>
> >>>>>>>> lock handle multiple purpose, which makes client unable to know<br>
> >>>>>>>> whether file is deleted or not when server cancels LOOKUP lock. Instead, client holds PR mode<br>
> >>>>> DENTRY lock on all valid denties and when server cancels it, client knows the file is deleted.<br>
> >>>>>>>>> For details, please see the attachments (I attached both word and<br>
> >>>>>>>>> pdf versions as I am not sure if<br>
> >>>>>>>> which is more convenient for you:). And please help to review and comment. Thank you!<br>
> >>>>>>>><br>
> >>>>>>>> Hi Tao,<br>
> >>>>>>>> I'm passing this on to the engineers that are most familiar with the<br>
> >>>>>>>> DLM and client dcache code in Lustre. After a quick first read<br>
> >>>>>>>> through your document, your investigation of the client code is very<br>
> >>>>>>>> insightful and looks like it will be able to remove some of the significant complexity that<br>
> has<br>
> >>>>> grown over time in the llite code.<br>
> >>>>>>>> Cheers, Andreas<br>
> >>>>>>>> --<br>
> >>>>>>>> Andreas Dilger Whamcloud, Inc.<br>
> >>>>>>>> Principal Engineer <a href="http://www.whamcloud.com/" target="_blank">http://www.whamcloud.com/</a><br>
> >>>>>>>><br>
> >>>>>>>><br>
> >>>>>>> <dcache_distributed_locking-v2.docx><br>
> >>>>>> Cheers, Andreas<br>
> >>>>>> --<br>
> >>>>>> Andreas Dilger Whamcloud, Inc.<br>
> >>>>>> Principal Engineer <a href="http://www.whamcloud.com/" target="_blank">http://www.whamcloud.com/</a><br>
> >>>>>><br>
> >>>>>><br>
> >>>>>><br>
> >>>>>><br>
> >>>>>><br>
> >>> Cheers, Andreas<br>
> >>> --<br>
> >>> Andreas Dilger Whamcloud, Inc.<br>
> >>> Principal Engineer <a href="http://www.whamcloud.com/" target="_blank">http://www.whamcloud.com/</a><br>
> >>><br>
> >>><br>
> >>><br>
> >>><br>
><br>
<br>
</div></div></blockquote></div><br></div>