Hi Piyush,
On Wed, Jun 17, 2009 at 10:20:46AM -0500, Piyush Shivam wrote:
> Hi Marcel,
>
> On 06/17/09 00:07, Marcel Telka wrote:
> <snip>
>> if (val == (ulong_t)-1) {
>>
>> My comment was about nothing more, nothing less, just typecast :-).\
>>
> OK. I will add the typecast.
Thanks.
>>> rfs3_fsinfo(..)
>>> {
>>> ..
>>> /*
>>> * Large file spec: want maxfilesize based on limit of
>>> * underlying filesystem. We can guess 2^31-1 if need be.
>>> */
>>> error = VOP_PATHCONF(vp, _PC_FILESIZEBITS, &l, cr, NULL);
>>>
>>> if (!error && l != 0 && l <= 64)
>>> resp->resok.maxfilesize = (1LL << (l-1)) - 1;
>>> else
>>> resp->resok.maxfilesize = MAXOFF32_T;
>>> ..
>>> }
>>>
>>
>> Ok. Thank you for the explanation. MAXOFF32_T now makes sense. The another
>> small problem I see now is that NFSv3 and NFSv4 could behave differently.
>> What
>> about unifiyng both NFSv3 and NFSv4 approaches?
>>
>> Is (l-1) above correct? I suspect no.
>>
> "l"-1 does seem like it requires a closer look. In my understanding,
Please let me show you different problem with l-1:
The code for NFSv4 is doing this:
VOP_PATHCONF(vp, _PC_FILESIZEBITS, &pc_val, cr);
pce->maxfilesize = ((1LL << pc_val) - 1);
The NFSv3 code is doing this:
VOP_PATHCONF(vp, _PC_FILESIZEBITS, &l, cr, NULL);
resp->resok.maxfilesize = (1LL << (l-1)) - 1;
Do you see the difference? For NFSv3 case we return only (approx.) half value of
max file size.
Another problem I see above with NFSv4 implementation is number of parameters
in VOP_PATHCONF() macro/call. There are only 4 instead of 5 as in NFSv3
implementation and in
VOP_PATHCONF() definition too:
http://grok.czech.sun.com:8080/source/xref/onnv-clone/usr/src/uts/common/sys/vnode.h#1066
How it would even compile without a warning?
> the behavior will not be different in the case "l" = -1, i.e., there is
> no underlying support for _PC_FILESIZEBITS. This is because, when doing
> the check "if (!error && l != 0 && l <= 64)", the check will fail even
> though "l" is -1. Since "l" is interpreted as unsigned long (rules of
> promotion will kick in while comparing a signed and an unsigned
> quantity), it is larger than 64, hence the else part will get invoked.
>
> But, if there is indeed an error and underlying support for
> _PC_FILESIZEBITS exists, then instead of bailing out, we will assign a
> default value, which is a bug (and a different behavior, like you said).
> I think it should be easy to add my v4 fix in v3 code as well.
>
>>>> 3) Why there is comment in the NFS4ATTR_GETIT case only? I am missing some
>>>> comment in the NFS4ATTR_VERIT case.
>>>>
>>> I did consider putting that comment in the VERIT case as well.
>>> However, I saw that the first set of comments is just a few lines
>>> above it, and I did not want to duplicate comments that are less
>>> than 15 lines or so apart. I can add comments in nfs4_srv_readdir.c
>>> if you think that will help.
>>>
>>
>> The problem is that a random reader in the middle of the code will not know
>> that the relevant comment is about 15 lines above (or in another file, in
>> nfs4_srv_readdir.c case).
>>
>> More comments in the code is better than less comments. Please consider to
>> copy'n'paste the comment to other two places.
>>
> OK. I will add the comments.
Thanks.
>>
>>>> 4) When _PC_FILESIZEBITS is not supported VOP_PATHCONF() will fail and
>>>> return
>>>> EINVAL (according to the comments section in the CR), so the codepath will
>>>> break at line 1574 and your fix won't be used to salvage the situation:
>>>>
>>>> 1571 error = VOP_PATHCONF(sarg->cs->vp, _PC_FILESIZEBITS,
>>>> &val,
>>>> 1572 sarg->cs->cr, NULL);
>>>> 1573 if (error)
>>>> 1574 break;
>>>> 1575 1576 /*
>>>> 1577 * If the underlying file system does not support
>>>> 1578 * _PC_FILESIZEBITS, return a reasonable default.
>>>> Note that
>>>> 1579 * error code on VOP_PATHCONF will be 0, even if the
>>>> underlying
>>>> 1580 * file system does not support _PC_FILESIZEBITS.
>>>> 1581 */
>>>> 1582 if (val == -1) {
>>>> 1583 na->maxfilesize = MAXOFF32_T;
>>>> 1584 } else {
>>>> 1585 if (val >= (sizeof (uint64_t) * 8))
>>>> 1586 na->maxfilesize = UINT64_MAX;
>>>> 1587 else
>>>> 1588 na->maxfilesize = ((1LL << val) - 1);
>>>> 1589 }
>>>>
>>> I believe that Frank has addressed this at length. Please let me know
>>> if they is still any confusion.
>>>
>>
>> It is clear now. Thanks. But please consider to change Comments section in
>> the
>> CR to remove any possible confusions for future. Obviously, this is not
>> critical for the fix... :-)
>>
> Sure, will do.
Thanks again :-).
>
> I will make the changes and repost the updated webrev soon.
>
> -Piyush
--
Marcel Telka
Solaris RPE
