[pnfs] [0/2] encode empty bitmap in getdeviceinfo [was: Re: [PATCH 08/10] pnfsd: Update getdeviceinfo for draft-19]

[Benny Halevy]

On Feb. 12, 2008, 15:24 +0200, Benny Halevy <bhalevy at panasas.com> wrote:
> On Feb. 12, 2008, 15:05 +0200, Peter Staubach <staubach at redhat.com> wrote:
>> Benny Halevy wrote:
>>> On Feb. 12, 2008, 0:47 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
>>>   
>>>> Benny Halevy wrote:
>>>>     
>>>>> On Feb. 10, 2008, 12:04 +0200, Benny Halevy <bhalevy at panasas.com> wrote:
>>>>>   
>>>>>       
>>>>>> On Feb. 09, 2008, 10:26 +0200, Marc Eshel <eshel at almaden.ibm.com> wrote:
>>>>>>     
>>>>>>         
>>>>>>> @@ -3422,8 +3436,13 @@ nfsd4_encode_getdevinfo(struct nfsd4_compoundres *resp,
>>>>>>>  	/* Call file system to retrieve and encode device */
>>>>>>>  	nfserr = sb->s_export_op->get_device_info(sb, &args);
>>>>>>>  	if (nfserr) {
>>>>>>> +		/* Rewind to the beginning */
>>>>>>> +		p = p_in;
>>>>>>> +		ADJUST_ARGS();
>>>>>>> +		if (nfserr == -ETOOSMALL)
>>>>>>> +			goto toosmall;
>>>>>>>  		printk(KERN_ERR "%s: export ERROR %d\n", __func__, nfserr);
>>>>>>> -		return nfserrno(nfserr);
>>>>>>> +		goto out;
>>>>>>>  	}
>>>>>>>  
>>>>>>>  	/* The file system should never write 0 bytes without
>>>>>>> @@ -3436,7 +3455,22 @@ nfsd4_encode_getdevinfo(struct nfsd4_compoundres *resp,
>>>>>>>  	 */
>>>>>>>  	p += XDR_QUADLEN(args.xdr.bytes_written);
>>>>>>>  	ADJUST_ARGS();
>>>>>>> -	return nfs_ok;
>>>>>>> +
>>>>>>> +	/* Encode supported device notifications */
>>>>>>> +	RESERVE_SPACE(8);
>>>>>>> +	WRITE32(1);
>>>>>>> +	WRITE32(args.notify_types);
>>>>>>>       
>>>>>>>           
>>>>>> Is this cleared by the proc layer until we support deviceid notifications?
>>>>>> Currently it seem like you're just echoing whatever the client asked for
>>>>>> which is wrong.  Maybe it'd be better to split the gd_notify_types member
>>>>>> into two: one for the request and the other, which will be set to zero
>>>>>> by default, for the response.
>>>>>>     
>>>>>>         
>>>>> Scratch that, the file system must set it appropriately in get_device_info.
>>>>>   
>>>>>       
>>>> Now that I realize that the array bitmap can have length 0 (meaning 
>>>> there is no bitmap), should this code be:
>>>>
>>>> if (args.notify_types) {
>>>> 	RESERVE_SPACE(8);
>>>> 	WRITE32(1);
>>>> 	WRITE32(args.notify_types);
>>>>
>>>> } else {
>>>>
>>>> 	RESERVE_SPACE(4);
>>>> 	WRITE32(0);
>>>> }
>>>>
>>>>
>>>>     
>>> It's possible but not necessary.
>>> The zero bitmap is equivalent to the empty one.
>>> Basically we're trading code simplicity and a few cpu instructions
>>> with 4 more bytes on the wire.  Since GETDEVICEINFO is an infrequent
>>> operation I vote for simpler code.
>> It is simpler code here, maybe, but what about the decode side?
>> It will take longer to decode than an "empty" bitmap as opposed
>> to a zero length one.
> 
> True,  The decoder has to check for the length anyway and
> not decoding the extra word will save a few cycles.
> 
>> This also just seems sloppy to me.  The packets over the wire
>> should be as concise and precise as possible.  This can be
>> achieved with a simple test and branch and the perceived
>> complexity of the resulting code is debatable.  It looks pretty
>> straightforward to me what it is doing.
> 
> I concur and will make the change.

The two patches in reply to this message implement xdr encoding of an empty
notifications bitmap rather than one containing a single zeroed entry
for the client request and server reply, respectively.

I'd like to squash them into the ibm-pnfs-d19-v2 patchset and close
it be merging it into the pnfs-d19 branch (I'll make tags pointing at
the historical ibm-pnfs-d19-v{1,2} branches for reference)

Benny

> 
> Thanks,
> 
> Benny
> 
>>     Thanx...
>>
>>        ps
> 
> _______________________________________________
> pNFS mailing list
> pNFS at linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs