[pnfs] [PATCH 02/28] pnfs: fix pnfs_{read|write}pages

[Benny Halevy]

Fredric Isaman wrote:
>
>
> On Thu, 13 Mar 2008, Benny Halevy wrote:
>
>> On Mar. 13, 2008, 20:09 +0200, Fredric Isaman 
>> <iisaman at citi.umich.edu> wrote:
>>> On Wed, 12 Mar 2008, Benny Halevy wrote:
>>>
>>>> On Mar. 12, 2008, 18:23 +0200, Fredric Isaman 
>>>> <iisaman at citi.umich.edu> wrote:
>>>>> On Wed, 12 Mar 2008, Benny Halevy wrote:
>>>>>
>>>>>> On Mar. 11, 2008, 21:31 +0200, Fred Isaman 
>>>>>> <iisaman at citi.umich.edu> wrote:
>>>>>>> THIS WILL BREAK OTHER LAYOUT DRIVERS that rely on old behavior.
>>>>>> Fred, do you have the cycles to fix the files and objects LDs
>>>>>> for the new behavior so we can merge this patch onto the pnfs 
>>>>>> branch?
>>>>>>
>>>>> Yes, I'll work on that.
>>>>>
>>>>>      Fred
>>>> Thanks!
>>>>
>>>>
>>>
>>>
>>> OK, I've been glancing through the code for the other layout drivers.
>>> Basically, in the functions ld_io_ops->{read|write}_pagelist, the
>>> filelayout driver only returns 0, while the panlayout only returns 0 
>>> or a
>>> negative error.  So why do we have the checks for positive return
>>> value at all?
>>
>> In the past the objects LD used to return a positive count on 
>> synchronous
>> completion of the I/O operation (it had nothing to do with falling back
>> to NFS I/O)
>>
>>>
>>> In addition, to clarify the intent of some of the panobject code:
>>>
>>> Does sam_read/sam_write return PAN_ERR_IN_PROGRESS iff called with a
>>> completion callback?
>>
>> Not exactly.  It returns that if called with a callback function
>> and the operation continues asynchronously.  If it's already
>> completed (either successfully or aborted with an error) it will
>> return the appropriate success/error status.
>>
>
> In that case, don't you have the potential for the code in the async 
> case to call the done routines twice, first as the callback, then (if 
> the async call finishes quickly) again in the (rc != 
> PAN_ERR_IN_PROGRESS) condition?

No, rc != PAN_ERR_IN_PROGRESS is final so the callback won't be called 
in this case.
Or, said differently, the callback will be called iff rc == 
PAN_ERR_IN_PROGRESS.

Benny

>
>>> Why the check against PNFS_ISSYNC in panlayout_*_done? Don't you still
>>> need to deallocate the memory used by the request?
>>
>> Yes.  Historically we wouldn't call the callback on the synchronous
>> return path.  This is no longer true and this code needs to be fixed 
>> accordingly.
>>
>>>
>>> On that note, it seems that PNFS_ISSYNC is never currently set/used at
>>> all.  Is this true?
>>
>> True.  It used to be set in the 2.6.18.3 codebase in the .{read,write}
>> paths.
>>
>
> Is this going to be resurrected by Dean's O_SYNC work, or should a 
> patch be sent in removing references to it?
>
>
>     Fred
>
>> Benny
>>
>>>
>>>      Fred
>>>
>>>
>>
>>
>> -- 
>> Benny Halevy
>> Software Architect
>> Tel/Fax: +972-3-647-8340
>> Mobile: +972-54-802-8340
>> US:      +1-412-203-3187
>> bhalevy at panasas.com
>>
>> Panasas, Inc.
>> The Leader in Parallel Storage
>> www.panasas.com
>>