Hi Piyush,
On Tue, Jun 16, 2009 at 10:46:40AM -0500, Piyush Shivam wrote:
> On 06/16/09 00:31, Marcel Telka wrote:
>> Hi Piyush,
>>
>> nfs4_srv_attr.c
>> ---------------
>>
>> 1) val is ulong_t. You cannot compare it with -1:
>>
>> 1554 ulong_t val;
>> 1582 if (val == -1) {
>> 1603 if (val == -1) {
>>
>
> It does look tricky. Like Frank explained, I am trying to distinguish
[...snip...]
> If you have another idea to make that distinction, I will be happy to
> consider it.
I have no problem with all you said above :-). That's correct. The problem is
that you cannot compare -1 with an ulong_t value. So please consider to
typecast -1 to ulong_t. Something like this:
if (val == (ulong_t)-1) {
My comment was about nothing more, nothing less, just typecast :-).
>
>
>>
>> 2)
>>
>> 1583 na->maxfilesize = MAXOFF32_T;
>> 1604 maxfilesize = MAXOFF32_T;
>>
>> Why MAXOFF32_T and not MAXOFFSET_T or MAXOFF_T or ...? Could you elaborate,
>> please? When _PC_FILESIZEBITS is supported you could return up to
>> UINT64_MAX,
>> which is a _bit_ bigger than MAXOFF32_T.
>>
>
> I don't have any strong opinion on the default size that is being
> returned. Initially, when I started looking at the bug, and saw that the
> use case was PCFS (not that it is the only file system where NFS may
> have to return a reasonable default max file size limit), I realized
> that for FAT32 file system, the maximum filesize is 4g -1, so 2g seemed
> reasonable. Then I went and looked at what the NFSv3 code did, and found
> the following:
>
> 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;
> ..
> }
>
> I did not see a need to make the default max file size inconsistent
> between NFSv3 and NFSv4. Of course, the above code may be quite old. If
> someone feels strongly that we need to make the default size UINT64_MAX,
> then I would suggest that we should change the default across V3 as
> well, unless we have a good case not to.
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.
>
>>
>> 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.
>> 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... :-)
--
Marcel Telka
Solaris RPE
