[pnfs] CVS: nfsv4

[Benny Halevy]

> That is why I had the comment "//??? if the right layout". I agree that 
> without that check it was broken but the change you made did not fix it. 
> Right now when a second request for a layout arrives from the 2nd client 
> for the same file the 
>         while (!list_empty(&fp->fi_layouts)) {
>                 lp = list_entry(fp->fi_layouts.next, 
> will find it and increment the hold count on lp but it will not add it to 
> the 2nd client chain, so when purging the 2nd client you will find nothing 
> on its chain to recall. The layout is on the file chain and the first 
> client that requested this layout. 

Right, the current implementation does not support multiple clients.
With my "fix" it works for a single one...

> right thing to do but I will not make any more changes until we talk on 
> Wednesday meeting and decide how to do it.

Ok. same here...

Marc Eshel wrote:
> Benny Halevy <bhalevy at panasas.com> wrote on 12/10/2006 08:08:26 AM:
> 
>> Marc Eshel wrote:
>>> Hi Benny,
>>> I am not sure that this is a bug. So you want to share the same layout 
> 
>>> struct on the server for all clients ? That might be more difficult to 
> 
>>> keep track of which client has which byte range. I had a layout struct 
> per 
>>> client which is also on the file struct. So if you need to see all 
> layouts 
>>> per client you run the chain of the client struct and if you need to 
> see 
>>> all the layouts per file you run the chain of the file struct.
>>> Marc.
>> Yes, we need a more efficient way to track the layouts eventually
>> but now the while() expression checked the client list while
>> the list operation is on the file's list.  The client had a layout
>> for a different file while the file's layout list was empty...
>  
> That is why I had the comment "//??? if the right layout". I agree that 
> without that check it was broken but the change you made did not fix it. 
> Right now when a second request for a layout arrives from the 2nd client 
> for the same file the 
>         while (!list_empty(&fp->fi_layouts)) {
>                 lp = list_entry(fp->fi_layouts.next, 
> will find it and increment the hold count on lp but it will not add it to 
> the 2nd client chain, so when purging the 2nd client you will find nothing 
> on its chain to recall. The layout is on the file chain and the first 
> client that requested this layout. 
> 
>> I completely agree that you should either check against the per-client
>> list and compare the file and byte range, or check against the
>> per-file list and match the client and byte range. I'm not sure
>> what will be more efficient though...
> 
> I had a separate layout struct for each client per file. You changed it to 
> have one layout for all clients per file. I don't think that this is the 
> right thing to do but I will not make any more changes until we talk on 
> Wednesday meeting and decide how to do it.
> 
>> If we keep a counter on both lists we can make the decision in run-time,
>> however, since  file sharing is less common I believe that the per-file
>> list should typically be shorter than or equal to the per-client list.
>> Having an efficient hash table rather than a list should help both cases 
> :)
>>>
>>> pnfs-bounces at linux-nfs.org wrote on 12/07/2006 11:06:01 PM:
>>>
>>>> CVSROOT:   /cvs
>>>> Module name:   nfsv4
>>>> Changes by:   bhalevy at citi.   2006/12/08 02:06:01
>>>>
>>>> Modified files:
>>>>    cvs/pnfs/fs/nfsd: nfs4state.c 
>>>>
>>>> Log message:
>>>> Fix bug in nfs4_pnfs_get_layout()
>>>>
>>>> Index: fs/nfsd/nfs4state.c
>>>> ===================================================================
>>>> RCS file: /cvs/nfsv4/cvs/pnfs/fs/nfsd/nfs4state.c,v
>>>> retrieving revision 1.21
>>>> diff -u -p -r1.21 nfs4state.c
>>>> --- fs/nfsd/nfs4state.c   6 Dec 2006 23:18:02 -0000   1.21
>>>> +++ fs/nfsd/nfs4state.c   8 Dec 2006 07:05:03 -0000
>>>> @@ -3551,7 +3551,7 @@ int nfs4_pnfs_get_layout(struct super_bl
>>>> goto out;
>>>>
>>>> lgp->lg_flags = 0;
>>>> -   while (!list_empty(&clp->cl_layouts)) {
>>>> +   while (!list_empty(&fp->fi_layouts)) {
>>>> lp = list_entry(fp->fi_layouts.next,
>>>> struct nfs4_layout, lo_perfile);
>>>> //??? if the right layout
>>>>
>>>> _______________________________________________
>>>> pNFS mailing list
>>>> pNFS at linux-nfs.org
>>>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>>> _______________________________________________
>>> pNFS mailing list
>>> pNFS at linux-nfs.org
>>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>