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.
>>> 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.
>
"l"-1 does seem like it requires a closer look. In my understanding,
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.
>
>>> 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.
I will make the changes and repost the updated webrev soon.
-Piyush
