Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform portability

[tao.peng]

From: Roman Grigoryev [roman_grigor...@xyratex.com]
Sent: Thursday, May 03, 2012 6:45 PM
To: Peng, Tao
Cc: adil...@whamcloud.com; wc-disc...@whamcloud.com; 
lustre-discuss@lists.lustre.org; lustre-de...@lists.lustre.org
Subject: Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform 
portability

On 05/03/2012 02:03 PM, tao.p...@emc.com wrote:
>>
>> I'm not know how linux developers approve patches. But, logically,
>> because Lustre is big enough project and use kernel subsystems then it
>> should be tested after kernel changes on kernel side too. In  other case
>> (without testing) we can observer situation when Lustre built ok but
>> doesn't work.

> Now I see what you are worrying about. All code submitted to Linux kernel
> will be tested, and also patches later merged. It's just that kernel is a
> different place than Lustre git tree. We are not allowed to put test scripts
> into kernel tree. So all test code will remain in Whamcloud tree. But we will
> run the same tests against kernel code as well. One of our goal is to keep 
> kernel
> client code in sync with Whamcloud tree as much as possible, so that the same
> code will be tested with older kernels as well.

> I just want to point, as I understand situation, keeping kernel client
> code in sync with Whamcloud tree without testing on latest
> kernels(include unstable) could lead to situation when client on new
> kernel doesn't work but built ok. Testing on older kernels doesn't say
> about work on new kernels, only give possibility.
I agree. We need to test latest kernel from time to time, and it can be a 
community effort. We will certainly test every patch we put in. And anyone 
interested can test the code as well. If you are suggesting some public testing 
infrastructure, maybe Whamcloud will be interested? IMHO, testing every patch 
in upstream kernel like Maloo does for Whamcloud tree, is a little luxury 
through.

Thanks,
Tao




___
Lustre-discuss mailing list
Lustre-discuss@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-discuss

Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform portability

[tao.peng]

> -Original Message-
> From: Roman Grigoryev [mailto:roman_grigor...@xyratex.com]
> Sent: Thursday, May 03, 2012 5:45 PM
> To: Peng, Tao
> Cc: adil...@whamcloud.com; wc-disc...@whamcloud.com; 
> lustre-discuss@lists.lustre.org; lustre-
> de...@lists.lustre.org
> Subject: Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform 
> portability
> 
> Hi Tao,
Hi Roman,

> 
> I'm not know how linux developers approve patches. But, logically,
> because Lustre is big enough project and use kernel subsystems then it
> should be tested after kernel changes on kernel side too. In  other case
> (without testing) we can observer situation when Lustre built ok but
> doesn't work.
Now I see what you are worrying about. All code submitted to Linux kernel will 
be tested, and also patches later merged. It's just that kernel is a different 
place than Lustre git tree. We are not allowed to put test scripts into kernel 
tree. So all test code will remain in Whamcloud tree. But we will run the same 
tests against kernel code as well. One of our goal is to keep kernel client 
code in sync with Whamcloud tree as much as possible, so that the same code 
will be tested with older kernels as well.

> 
> I don't know goal of adding Lustre to kernel and, possible,this
> situation could be acceptable from your point of view.
> 
Untested code is buggy and useless. We certainly don't want it either.

Thanks,
Tao

> Thanks,
>   Roman
> 
> On 04/27/2012 04:33 PM, tao.p...@emc.com wrote:
> > Hi Roman,
> >
> > Not sure if I misunderstand your question, but we won't push lustre/tests/* 
> > files to kernel.
> >We only push client code and any tests will be executed out side of kernel
> >
> > Cheers,
> > Tao
> > 
> > From: Roman Grigoryev [roman_grigor...@xyratex.com]
> > Sent: Friday, April 27, 2012 6:25 PM
> > To: Peng, Tao
> > Cc: adil...@whamcloud.com; wc-disc...@whamcloud.com; 
> > lustre-discuss@lists.lustre.org; lustre-
> de...@lists.lustre.org
> > Subject: Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform 
> > portability
> >
> > Tao,Andreas,all,
> >
> > What is your plan in test/test framework changes from the point of view
> > of integration to kernel? As i know, kernel.org has his own test
> > infrastructure and his own test framework.
> >
> > I'm sorry if it's incorrect place for this question.
> >
> > Thanks,
> > Roman
> >
> > On 04/27/2012 02:15 PM, tao.p...@emc.com wrote:
> >> Hi Andreas,
> >>
> >>> -Original Message-
> >>> From: Andreas Dilger [mailto:adil...@whamcloud.com]
> >>> Sent: Friday, April 27, 2012 11:54 AM
> >>> To: Peng, Tao
> >>> Cc: ; ; 
> >>> 
> >>> Subject: Re: [wc-discuss] Re: Lustre and cross-platform portability
> >>>
> >>> On 2012-04-26, at 20:23,  wrote:
>  Thank you very much for bringing it up in LUG and getting all these 
>  positive support from
> community.
> >>>
> >>> Tao,
> >>> Yes it does look promising.
> >>>
> > To revive this thread, based on discussion at the LUG TWG:
> > - there was general consensus that cleaning up the Lustre client
> >  (and server) code was very desirable to do
> > - migrating libcfs to emulate the Linux kernel APIs is also usable.
> >  Ken mentioned that there is relatively little conflict between
> >  the Linux kernel and the MacOS kernel, and the same for WinNT, so
> >  they could use the same function names as Linux without problems.
>  I created LU-1346 (http://jira.whamcloud.com/browse/LU-1346) to track 
>  libcfs cleanup work.
> >>>
> >>> OK
> >>>
> > - there was no objection to converting the Lustre code from spaces
> >  to tabs.  My proposal was that build/checkpatch.pl could require
> >  tabs immediately, and new patches should be submitted with tabs
> >  on all new/modified lines (and optionally all lines on small
> >  functions to avoid messy formatting).  This will avoid issues
> >  with current patches in flight, and also avoid "git annotate"
> >  showing the jumbo replace-all-spaces-with-tabs patch for every
> >  line in Lustre, and I think a good fraction of lines will be
> >  updated in the next 9-12 months or more.  As we approach the
> >  actual time for upstream kernel submission is close, then we can
> >  make a final effort to clean up remaining lines in idle code
> >  (which will also be unlikely to conflict with existing work).
>  While tabs are the main coding style difference between Lustre and 
>  kernel, there are a few minor
> >>> change that is needed as well. I think we need to do following to match 
> >>> kernel coding style:
>  1. Lustre uses expandtab while kernel requires tabs
> >>>
> >>> Right.
> >>>
>  2. Lustre has vim syntax rules in most source files, which need to be 
>  removed
> >>>
> >>> They should be replaced with explicit vim and enacts syntax rules that 
> >>> have the kernel indent
> style
> >>> instead.  If we could get synta

Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform portability

[tao.peng]

Hi Roman,

Not sure if I misunderstand your question, but we won't push lustre/tests/* 
files to kernel. We only push client code and any tests will be executed out 
side of kernel.

Cheers,
Tao

From: Roman Grigoryev [roman_grigor...@xyratex.com]
Sent: Friday, April 27, 2012 6:25 PM
To: Peng, Tao
Cc: adil...@whamcloud.com; wc-disc...@whamcloud.com; 
lustre-discuss@lists.lustre.org; lustre-de...@lists.lustre.org
Subject: Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform 
portability

Tao,Andreas,all,

What is your plan in test/test framework changes from the point of view
of integration to kernel? As i know, kernel.org has his own test
infrastructure and his own test framework.

I'm sorry if it's incorrect place for this question.

Thanks,
Roman

On 04/27/2012 02:15 PM, tao.p...@emc.com wrote:
> Hi Andreas,
>
>> -Original Message-
>> From: Andreas Dilger [mailto:adil...@whamcloud.com]
>> Sent: Friday, April 27, 2012 11:54 AM
>> To: Peng, Tao
>> Cc: ; ; 
>> 
>> Subject: Re: [wc-discuss] Re: Lustre and cross-platform portability
>>
>> On 2012-04-26, at 20:23,  wrote:
>>> Thank you very much for bringing it up in LUG and getting all these 
>>> positive support from community.
>>
>> Tao,
>> Yes it does look promising.
>>
 To revive this thread, based on discussion at the LUG TWG:
 - there was general consensus that cleaning up the Lustre client
  (and server) code was very desirable to do
 - migrating libcfs to emulate the Linux kernel APIs is also usable.
  Ken mentioned that there is relatively little conflict between
  the Linux kernel and the MacOS kernel, and the same for WinNT, so
  they could use the same function names as Linux without problems.
>>> I created LU-1346 (http://jira.whamcloud.com/browse/LU-1346) to track 
>>> libcfs cleanup work.
>>
>> OK
>>
 - there was no objection to converting the Lustre code from spaces
  to tabs.  My proposal was that build/checkpatch.pl could require
  tabs immediately, and new patches should be submitted with tabs
  on all new/modified lines (and optionally all lines on small
  functions to avoid messy formatting).  This will avoid issues
  with current patches in flight, and also avoid "git annotate"
  showing the jumbo replace-all-spaces-with-tabs patch for every
  line in Lustre, and I think a good fraction of lines will be
  updated in the next 9-12 months or more.  As we approach the
  actual time for upstream kernel submission is close, then we can
  make a final effort to clean up remaining lines in idle code
  (which will also be unlikely to conflict with existing work).
>>> While tabs are the main coding style difference between Lustre and kernel, 
>>> there are a few minor
>> change that is needed as well. I think we need to do following to match 
>> kernel coding style:
>>> 1. Lustre uses expandtab while kernel requires tabs
>>
>> Right.
>>
>>> 2. Lustre has vim syntax rules in most source files, which need to be 
>>> removed
>>
>> They should be replaced with explicit vim and enacts syntax rules that have 
>> the kernel indent style
>> instead.  If we could get syntax rules that embodied more of the coding 
>> style than just indentation,
>> that would be even better.
>>
> But we do need to remove them before submitting to kernel, right? And if we 
> enforce checkpatch.pl on every patch applied, IMHO there is not much benefit 
> to have syntax rules on every file, not to mention that it is explicitly 
> forbidden in kernel coding style (Documentation/CodingStyle, Chapter 18:  
> Editor modelines and other cruft).
>
> BTW, instead of just enabling tabs, how about changing checkpatch.pl to 
> latest kernel version to make sure all future patches follow kernel coding 
> styles?
>
>>> 3. Lustre uses a slightly different comment style, which need to be changed 
>>> to kernel style
>>
>> This is DOxygen style formatting.  I had forgotten about this. We had in the 
>> past used this inline
>> formatting for producing some documentation, but I'd need to ask about 
>> whether there is still a need
>> for this today. In the meantime, please leave the comment style as-is.
>>
> OK.
>
>>
>>> I created LU-1347 (http://jira.whamcloud.com/browse/LU-1347) to track the 
>>> coding style changes.
>>>
 There is some hope that the kernel maintainers will not require
 all of the Lustre macros to be removed, but we can deal with this
 on a case-by-case basis.

>>> IMO, we can divide macros to three groups (or more?):
>>> 1. Old kernel support macros, kernel maintainers are clear that they won't 
>>> accept it.
>>
>> Yes, but we need to maintain this in the external Lustre tree for years 
>> after Lustre would be accepted
>> into mainline, since it would not be in vendor kernels (which a majority of 
>> Lustre users would be
>> using).
>>
>> For such compat macros we need to make an effort to keep the upstream code 

Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform portability

[tao.peng]

Hi Andreas,

> -Original Message-
> From: Andreas Dilger [mailto:adil...@whamcloud.com]
> Sent: Friday, April 27, 2012 11:54 AM
> To: Peng, Tao
> Cc: ; ; 
> 
> Subject: Re: [wc-discuss] Re: Lustre and cross-platform portability
> 
> On 2012-04-26, at 20:23,  wrote:
> > Thank you very much for bringing it up in LUG and getting all these 
> > positive support from community.
> 
> Tao,
> Yes it does look promising.
> 
> >> To revive this thread, based on discussion at the LUG TWG:
> >> - there was general consensus that cleaning up the Lustre client
> >>  (and server) code was very desirable to do
> >> - migrating libcfs to emulate the Linux kernel APIs is also usable.
> >>  Ken mentioned that there is relatively little conflict between
> >>  the Linux kernel and the MacOS kernel, and the same for WinNT, so
> >>  they could use the same function names as Linux without problems.
> > I created LU-1346 (http://jira.whamcloud.com/browse/LU-1346) to track 
> > libcfs cleanup work.
> 
> OK
> 
> >> - there was no objection to converting the Lustre code from spaces
> >>  to tabs.  My proposal was that build/checkpatch.pl could require
> >>  tabs immediately, and new patches should be submitted with tabs
> >>  on all new/modified lines (and optionally all lines on small
> >>  functions to avoid messy formatting).  This will avoid issues
> >>  with current patches in flight, and also avoid "git annotate"
> >>  showing the jumbo replace-all-spaces-with-tabs patch for every
> >>  line in Lustre, and I think a good fraction of lines will be
> >>  updated in the next 9-12 months or more.  As we approach the
> >>  actual time for upstream kernel submission is close, then we can
> >>  make a final effort to clean up remaining lines in idle code
> >>  (which will also be unlikely to conflict with existing work).
> > While tabs are the main coding style difference between Lustre and kernel, 
> > there are a few minor
> change that is needed as well. I think we need to do following to match 
> kernel coding style:
> > 1. Lustre uses expandtab while kernel requires tabs
> 
> Right.
> 
> > 2. Lustre has vim syntax rules in most source files, which need to be 
> > removed
> 
> They should be replaced with explicit vim and enacts syntax rules that have 
> the kernel indent style
> instead.  If we could get syntax rules that embodied more of the coding style 
> than just indentation,
> that would be even better.
> 
But we do need to remove them before submitting to kernel, right? And if we 
enforce checkpatch.pl on every patch applied, IMHO there is not much benefit to 
have syntax rules on every file, not to mention that it is explicitly forbidden 
in kernel coding style (Documentation/CodingStyle, Chapter 18:  Editor 
modelines and other cruft).

BTW, instead of just enabling tabs, how about changing checkpatch.pl to latest 
kernel version to make sure all future patches follow kernel coding styles?

> > 3. Lustre uses a slightly different comment style, which need to be changed 
> > to kernel style
> 
> This is DOxygen style formatting.  I had forgotten about this. We had in the 
> past used this inline
> formatting for producing some documentation, but I'd need to ask about 
> whether there is still a need
> for this today. In the meantime, please leave the comment style as-is.
> 
OK.

> 
> > I created LU-1347 (http://jira.whamcloud.com/browse/LU-1347) to track the 
> > coding style changes.
> >
> >> There is some hope that the kernel maintainers will not require
> >> all of the Lustre macros to be removed, but we can deal with this
> >> on a case-by-case basis.
> >>
> > IMO, we can divide macros to three groups (or more?):
> > 1. Old kernel support macros, kernel maintainers are clear that they won't 
> > accept it.
> 
> Yes, but we need to maintain this in the external Lustre tree for years after 
> Lustre would be accepted
> into mainline, since it would not be in vendor kernels (which a majority of 
> Lustre users would be
> using).
> 
> For such compat macros we need to make an effort to keep the upstream code as 
> close as possible to the
> external tree, so patches have the most chance of applying.
> 
I agree. We should minimize maintenance effort for it. And as you suggested, we 
can put as many of these compat macros into places like linux_compat.h as 
possible and have Lustre code use latest kernel APIs, so that most maintenance 
effort for old kernel support would be to keep linux_compat.h uptodate. For 
compact macros that cannot be cleaned up this way, we will have to pay the 
extra efforts. And of course the cleanup will be an incremental process and 
macros will be dealt with in a case-by-case basis.

> > 2. For macros to mark out server code, kernel maintainers can accept it. 
> > But I think we need to make
> sure we don't do it too intrusive.
> 
> Sure, and we also need to make sure the ongoing maintenance effort to keep 
> the code working is not too
> much either.
> 
> I'm OK with incremental patches

Re: [Lustre-discuss] [wc-discuss] Re: Lustre and cross-platform portability

[tao.peng]

Hi Andreas,

Thank you very much for bringing it up in LUG and getting all these positive 
support from community.

> -Original Message-
> From: Andreas Dilger [mailto:aedil...@gmail.com]
> Sent: Friday, April 27, 2012 7:48 AM
> To: wc-discuss
> Cc: Lustre Devel; lustre-discuss discuss
> Subject: [wc-discuss] Re: Lustre and cross-platform portability
> 
> To revive this thread, based on discussion at the LUG TWG:
> - there was general consensus that cleaning up the Lustre client
>   (and server) code was very desirable to do
> - migrating libcfs to emulate the Linux kernel APIs is also usable.
>   Ken mentioned that there is relatively little conflict between
>   the Linux kernel and the MacOS kernel, and the same for WinNT, so
>   they could use the same function names as Linux without problems.
I created LU-1346 (http://jira.whamcloud.com/browse/LU-1346) to track libcfs 
cleanup work.

> - there was no objection to converting the Lustre code from spaces
>   to tabs.  My proposal was that build/checkpatch.pl could require
>   tabs immediately, and new patches should be submitted with tabs
>   on all new/modified lines (and optionally all lines on small
>   functions to avoid messy formatting).  This will avoid issues
>   with current patches in flight, and also avoid "git annotate"
>   showing the jumbo replace-all-spaces-with-tabs patch for every
>   line in Lustre, and I think a good fraction of lines will be
>   updated in the next 9-12 months or more.  As we approach the
>   actual time for upstream kernel submission is close, then we can
>   make a final effort to clean up remaining lines in idle code
>   (which will also be unlikely to conflict with existing work).
While tabs are the main coding style difference between Lustre and kernel, 
there are a few minor change that is needed as well. I think we need to do 
following to match kernel coding style:
1. Lustre uses expandtab while kernel requires tabs
2. Lustre has vim syntax rules in most source files, which need to be removed
3. Lustre uses a slightly different comment style, which need to be changed to 
kernel style

I created LU-1347 (http://jira.whamcloud.com/browse/LU-1347) to track the 
coding style changes.

> 
> There is some hope that the kernel maintainers will not require
> all of the Lustre macros to be removed, but we can deal with this
> on a case-by-case basis.
> 
IMO, we can divide macros to three groups (or more?):
1. Old kernel support macros, kernel maintainers are clear that they won't 
accept it.
2. For macros to mark out server code, kernel maintainers can accept it. But I 
think we need to make sure we don't do it too intrusive.
3. Lustre feature macros, we can convert them to Kconfig macros.

Cheers,
Tao
> Cheers, Andreas
> 
> On 2012-03-14, at 7:31 PM, Andreas Dilger wrote:
> > Whamcloud and EMC are jointly investigating how to be able to contribute 
> > the Lustre client code into
> the upstream Linux kernel.
> >
> > As a prerequisite to this, EMC is working to clean up the Lustre client 
> > code to better match the
> kernel coding style, and one of the anticipated major obstacles to upstream 
> kernel submission is the
> heavy use of code abstraction via libcfs for portability to other operating 
> systems (most notably
> MacOS and WinNT, but also for liblustre, and potentially *BSD).
> >
> > I have no information that the WinNT project will ever be released by 
> > Oracle,
> 
> [revised] and the MacOS client needs significant work to update it to the 
> latest version of Lustre,
> and to get it landed into the Lustre repo,
> 
> > so the libcfs portability layer is potentially exacting a high cost in code 
> > maintenance and
> complexity (CLIO being a prime example) for no apparent benefit.  Similarly, 
> the liblustre client
> needs a portability layer for userspace, and suffers from the same apparent 
> lack of interest or users.
> >
> > I'd like to get some feedback from the Lustre community about removing the 
> > libcfs abstraction
> entirely, or possibly restructuring it to look like the Linux kernel API, and 
> having the other
> platforms code against it as a Linux portability layer, like ZFS on Linux 
> uses the Solaris Portability
> Layer (SPL) to avoid changing the core ZFS code.  A related topic is whether 
> it would be better to
> replace all cfs_* functions with standard Linux kernel functions en-masse, or 
> migrate away from cfs_*
> functions slowly?
> >
> > Also, we're planning on deprecating the liblustre client code, due to lack 
> > of interest/usage.  The
> current code is in disrepair, and we've been keeping it around for years 
> without any benefit, and
> while I was one of the strongest advocates for keeping it in our back pocket 
> in case of future needs,
> I don't see that materializing in the future.
> >
> > The liblustre code would be left in the tree for now, in case someone from 
> > the community is
> interested to get it working and maintain it, and it may be updated on a be

Re: [Lustre-discuss] question about dcache revalidate

[tao.peng]

Got it. Thanks.

Best,
Tao

From: Lai Siyao [mailto:laisi...@whamcloud.com]
Sent: Tuesday, January 17, 2012 5:44 PM
To: Peng, Tao
Cc: gr...@whamcloud.com; Lustre-discuss@lists.lustre.org
Subject: Re: [Lustre-discuss] question about dcache revalidate


On Tue, Jan 17, 2012 at 5:13 PM, mailto:tao.p...@emc.com>> 
wrote:
> On Jan 12, 2012, at 3:52 AM, Lai Siyao wrote:
>
> > No, to add a dentry to hash client needs holding LOOKUP lock, but lustre 
> > client unhash (see
> ll_unhash_aliases()) doesn't really remove dentry from hash, but set 
> LUSTRE_DCACHE_INVALID flag. So in
> the race you mentioned, another process may add the dentry but later the lock 
> is canceled soon, so at
> the time of revalidate, it needs to do a real lookup.
>
> In fact there's more to it.
> Certain operations like open, for example, don't actually get a lock from 
> server so if there was no
> lock to begin with, we add
> the "known invalid" entry to the hash and immediately mark it as invalid, no 
> later lock cancel needed
> to trigger this.
>
> Are you referring to ll_lookup_it_finish() that flag negative dentry 
> DCACHE_LUSTRE_INVALID and hash/unhash it immediately if client doesn't have 
> UDATE lock? Looks like such dentry is skipped during link path walk as 
> ll_dcompare checks DCACHE_LUSTRE_INVALID. Or am I looking at wrong place?

> Normally lustre client doesn't fetch any child lock for lookup(IT_OPEN), but 
> a child dentry will be inserted into hash, however this dentry is marked 
> INVALID, and it will be freed after file->release(), that means, no other 
> processes will use the dentry (in this case other processes will create 
> another dentry for this file and add to hash too). So Oleg means a dentry may 
> be inserted into hash without LOOKUP lock, but this dentry won't be used by 
> others, so it will not cause the race you mentioned.
I see. mdc_intent_open_pack() only asks for child OPEN and parent UPDATE lock. 
Thanks very much!

No, generally only nfsd-exported lustre client requests child OPEN lock.

Bye,
- Lai
___
Lustre-discuss mailing list
Lustre-discuss@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-discuss

Re: [Lustre-discuss] question about dcache revalidate

[tao.peng]

> On Jan 12, 2012, at 3:52 AM, Lai Siyao wrote:
>
> > No, to add a dentry to hash client needs holding LOOKUP lock, but lustre 
> > client unhash (see
> ll_unhash_aliases()) doesn't really remove dentry from hash, but set 
> LUSTRE_DCACHE_INVALID flag. So in
> the race you mentioned, another process may add the dentry but later the lock 
> is canceled soon, so at
> the time of revalidate, it needs to do a real lookup.
>
> In fact there's more to it.
> Certain operations like open, for example, don't actually get a lock from 
> server so if there was no
> lock to begin with, we add
> the "known invalid" entry to the hash and immediately mark it as invalid, no 
> later lock cancel needed
> to trigger this.
>
> Are you referring to ll_lookup_it_finish() that flag negative dentry 
> DCACHE_LUSTRE_INVALID and hash/unhash it immediately if client doesn't have 
> UDATE lock? Looks like such dentry is skipped during link path walk as 
> ll_dcompare checks DCACHE_LUSTRE_INVALID. Or am I looking at wrong place?
 
> Normally lustre client doesn't fetch any child lock for lookup(IT_OPEN), but 
> a child dentry will be inserted into hash, however this dentry is marked 
> INVALID, and it will be freed after file->release(), that means, no other 
> processes will use the dentry (in this case other processes will create 
> another dentry for this file and add to hash too). So Oleg means a dentry may 
> be inserted into hash without LOOKUP lock, but this dentry won't be used by 
> others, so it will not cause the race you mentioned.

I see. mdc_intent_open_pack() only asks for child OPEN and parent UPDATE lock. 
Thanks very much!

Best,
Tao
___
Lustre-discuss mailing list
Lustre-discuss@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-discuss

Re: [Lustre-discuss] question about dcache revalidate

[tao.peng]

Oleg hi,

> -Original Message-
> From: Oleg Drokin [mailto:gr...@whamcloud.com]
> Sent: Friday, January 13, 2012 12:04 AM
> To: Lai Siyao
> Cc: Peng, Tao; Lustre-discuss@lists.lustre.org
> Subject: Re: [Lustre-discuss] question about dcache revalidate
> 
> Hello!
> 
> On Jan 12, 2012, at 3:52 AM, Lai Siyao wrote:
> 
> > No, to add a dentry to hash client needs holding LOOKUP lock, but lustre 
> > client unhash (see
> ll_unhash_aliases()) doesn't really remove dentry from hash, but set 
> LUSTRE_DCACHE_INVALID flag. So in
> the race you mentioned, another process may add the dentry but later the lock 
> is canceled soon, so at
> the time of revalidate, it needs to do a real lookup.
> 
> In fact there's more to it.
> Certain operations like open, for example, don't actually get a lock from 
> server so if there was no
> lock to begin with, we add
> the "known invalid" entry to the hash and immediately mark it as invalid, no 
> later lock cancel needed
> to trigger this.
> 
Are you referring to ll_lookup_it_finish() that flag negative dentry 
DCACHE_LUSTRE_INVALID and hash/unhash it immediately if client doesn't have 
UDATE lock? Looks like such dentry is skipped during link path walk as 
ll_dcompare checks DCACHE_LUSTRE_INVALID. Or am I looking at wrong place?

Thanks,
Tao

> Bye,
> Oleg
> >
> > On Wed, Jan 11, 2012 at 10:06 PM,  wrote:
> > Hi,
> >
> > I was reading dcache.c and following comments in ll_revalidate_it() seem 
> > confusing. Does it mean
> llite can hash a positive dentry to dcache without taking inode LOOKUP lock?
> >
> > 589 /*
> > 590  * This part is here to combat evil-evil race in real_lookup on 
> > 2.6
> > 591  * kernels.  The race details are: We enter do_lookup() looking 
> > for some
> > 592  * name, there is nothing in dcache for this name yet and 
> > d_lookup()
> > 593  * returns NULL.  We proceed to real_lookup(), and while we do 
> > this,
> > 594  * another process does open on the same file we looking up 
> > (most simple
> > 595  * reproducer), open succeeds and the dentry is added. Now back 
> > to
> > 596  * us. In real_lookup() we do d_lookup() again and suddenly 
> > find the
> > 597  * dentry, so we call d_revalidate on it, but there is no lock, 
> > so
> > 598  * without this code we would return 0, but unpatched 
> > real_lookup just
> > 599  * returns -ENOENT in such a case instead of retrying the 
> > lookup. Once
> > 600  * this is dealt with in real_lookup(), all of this ugly mess 
> > can go and
> > 601  * we can just check locks in ->d_revalidate without doing any 
> > RPCs
> > 602  * ever.
> > 603  */
> >
> > Best Regards,
> > Tao
> >
> >
> > ___
> > Lustre-discuss mailing list
> > Lustre-discuss@lists.lustre.org
> > http://lists.lustre.org/mailman/listinfo/lustre-discuss
> >
> > ___
> > Lustre-discuss mailing list
> > Lustre-discuss@lists.lustre.org
> > http://lists.lustre.org/mailman/listinfo/lustre-discuss
> 
> --
> Oleg Drokin
> Senior Software Engineer
> Whamcloud, Inc.
> 

___
Lustre-discuss mailing list
Lustre-discuss@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-discuss

Re: [Lustre-discuss] question about dcache revalidate

[tao.peng]

I see. Thanks a lot for the explanation.

Cheers,
Tao

From: Lai Siyao [mailto:laisi...@whamcloud.com]
Sent: Thursday, January 12, 2012 4:53 PM
To: Peng, Tao
Cc: Lustre-discuss@lists.lustre.org
Subject: Re: [Lustre-discuss] question about dcache revalidate

No, to add a dentry to hash client needs holding LOOKUP lock, but lustre client 
unhash (see ll_unhash_aliases()) doesn't really remove dentry from hash, but 
set LUSTRE_DCACHE_INVALID flag. So in the race you mentioned, another process 
may add the dentry but later the lock is canceled soon, so at the time of 
revalidate, it needs to do a real lookup.
On Wed, Jan 11, 2012 at 10:06 PM, mailto:tao.p...@emc.com>> 
wrote:
Hi,

I was reading dcache.c and following comments in ll_revalidate_it() seem 
confusing. Does it mean llite can hash a positive dentry to dcache without 
taking inode LOOKUP lock?

589 /*
590  * This part is here to combat evil-evil race in real_lookup on 2.6
591  * kernels.  The race details are: We enter do_lookup() looking for 
some
592  * name, there is nothing in dcache for this name yet and d_lookup()
593  * returns NULL.  We proceed to real_lookup(), and while we do this,
594  * another process does open on the same file we looking up (most 
simple
595  * reproducer), open succeeds and the dentry is added. Now back to
596  * us. In real_lookup() we do d_lookup() again and suddenly find the
597  * dentry, so we call d_revalidate on it, but there is no lock, so
598  * without this code we would return 0, but unpatched real_lookup 
just
599  * returns -ENOENT in such a case instead of retrying the lookup. 
Once
600  * this is dealt with in real_lookup(), all of this ugly mess can 
go and
601  * we can just check locks in ->d_revalidate without doing any RPCs
602  * ever.
603  */

Best Regards,
Tao


___
Lustre-discuss mailing list
Lustre-discuss@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-discuss

___
Lustre-discuss mailing list
Lustre-discuss@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-discuss

[Lustre-discuss] question about dcache revalidate

[tao.peng]

Hi,

I was reading dcache.c and following comments in ll_revalidate_it() seem 
confusing. Does it mean llite can hash a positive dentry to dcache without 
taking inode LOOKUP lock?

589 /*
590  * This part is here to combat evil-evil race in real_lookup on 2.6
591  * kernels.  The race details are: We enter do_lookup() looking for 
some
592  * name, there is nothing in dcache for this name yet and d_lookup()
593  * returns NULL.  We proceed to real_lookup(), and while we do this,
594  * another process does open on the same file we looking up (most 
simple
595  * reproducer), open succeeds and the dentry is added. Now back to
596  * us. In real_lookup() we do d_lookup() again and suddenly find the
597  * dentry, so we call d_revalidate on it, but there is no lock, so
598  * without this code we would return 0, but unpatched real_lookup 
just
599  * returns -ENOENT in such a case instead of retrying the lookup. 
Once
600  * this is dealt with in real_lookup(), all of this ugly mess can 
go and
601  * we can just check locks in ->d_revalidate without doing any RPCs
602  * ever.
603  */

Best Regards,
Tao


___
Lustre-discuss mailing list
Lustre-discuss@lists.lustre.org
http://lists.lustre.org/mailman/listinfo/lustre-discuss