[pnfs] [PATCH 1/5] pnfs: Several non-critical fixups.

[Dean Hildebrand]

Benny Halevy wrote:
> On May. 07, 2008, 18:29 -0700, Dean Hildebrand <seattleplus at gmail.com> wrote:
>   
>> Benny Halevy wrote:
>>     
>>> On May. 07, 2008, 15:13 -0700, Dean Hildebrand <seattleplus at gmail.com> wrote:
>>>   
>>>       
>>>> Benny Halevy wrote:
>>>>     
>>>>         
>>>>> On May. 06, 2008, 16:45 -0700, Dean Hildebrand <seattleplus at gmail.com> wrote:
>>>>> <snip>
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>>>> @@ -499,10 +499,11 @@ decode_and_add_ds(uint32_t **pp, struct filelayout_mount_type *mt)
>>>>>>  	 */
>>>>>>  	if (!ds->ds_clp) {
>>>>>>  		err = nfs4_pnfs_ds_create(mds_srv, ds);
>>>>>> -		printk(KERN_ERR
>>>>>> -		       "%s nfs4_pnfs_ds_create returned %d\n", __func__, err);
>>>>>> -		if (err)
>>>>>> +		if (err) {
>>>>>> +			printk(KERN_ERR "%s nfs4_pnfs_ds_create error %d\n",
>>>>>> +			       __func__, err);
>>>>>>  			goto out_err;
>>>>>> +		}
>>>>>>  	}
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> Why is this non-critical?
>>>>> This looks like a real bug fix to me.... 
>>>>>   
>>>>>   
>>>>>       
>>>>>           
>>>>>> @@ -1184,7 +1184,7 @@ pnfs_update_layout_commit(struct inode *inode,
>>>>>>  	struct nfs_page *nfs_page = nfs_list_entry(head->next);
>>>>>>  	int status;
>>>>>>  
>>>>>> -	dprintk("--> %s inode %p layout range: %Zd@%Lu\n", __func__, inode,
>>>>>> +	dprintk("--> %s inode %p layout range: %Zd@%Zu\n", __func__, inode,
>>>>>>  				(size_t)(npages * PAGE_SIZE),
>>>>>>  				(loff_t)idx_start * PAGE_SIZE);
>>>>>>  
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> Why is that?
>>>>> Do you get a warning in your environment?
>>>>> How about this instead?
>>>>>
>>>>>  	dprintk("--> %s inode %p layout range: %Zd@%Lu\n", __func__, inode,
>>>>>  				(size_t)(npages * PAGE_SIZE),
>>>>> -				(loff_t)idx_start * PAGE_SIZE);
>>>>> +				(u64)idx_start * PAGE_SIZE);
>>>>>   
>>>>>       
>>>>>           
>>>> I don't get any warnings on 64 bit, but good catch.  There is a problem, 
>>>> but I think the main culprit is the parenthesis.  pgoff_t is unsigned 
>>>> long, so casting it to loff_t is actually wrong, and so is u64 it 
>>>>     
>>>>         
>>> You want to do that to prevent overflow when multiplying with PAGE_SIZE
>>> on 32-bit machines...
>>>   
>>>       
>> I'm not sure what you mean by *that*. So you like my updated patch?
>>     
>
> Not really :-(
>
> (unsigned long)(idx_start * PAGE_SIZE) can overflow when unsigned long
> is 32-bit wide so you want to cast idx_start to u64.
>   
I see.  I think it should be %llu then, and the indenting needs to be fixed.
Dean
> Benny
>
>
>   
>> Dean
>>     
>>> Benny
>>>
>>>   
>>>       
>>>> seems.  So I think the real format is %lu.  There is also a indentation 
>>>> error.
>>>>    Let me send a new patch.
>>>> Dean
>>>>     
>>>>         
>>>   
>>>       
>
>