Re: [PATCH 5 2/4] Return 32/64-bit dir name hash according to usage type

[Eric Sandeen <sandeen@redhat.com>]

On 4/24/12 11:10 AM, Bernd Schubert wrote:
> On 04/24/2012 12:42 AM, Andreas Dilger wrote:
>> On 2012-04-23, at 5:23 PM, Eric Sandeen wrote:
>>> I'm curious about the above as well as:
>>>
>>>         case SEEK_END:
>>>                 if (unlikely(offset>  0))
>>>                         goto out_err; /* not supported for directories */
>>>
>>> The previous .llseek handler, and the generic handler for other
>>> filesystems, allow seeking past the end of the dir AFAICT. (not
>>> sure why you'd want to, but I don't see that you'd get an error
>>> back).
>>>
>>> Is there a reason to uniquely exclude it in ext4?  Does that line up with POSIX?
>>
>> I don't know what the origin of this was... I don't think there is
>> a real reason for it except that it doesn't make any sense to do
>> so.
>>
> 
> I think I added that. According to pubs.opengroup.org:
> (http://pubs.opengroup.org/onlinepubs/009695399/functions/seekdir.html)
> 
> void seekdir(DIR *dirp, long loc);
> 
> <quote>
> 
> If the value of loc was not obtained from an earlier call to
> telldir(), or if a call to rewinddir() occurred between the call to
> telldir() and the call to seekdir(), the results of subsequent calls
> to readdir() are unspecified.
> 
> </quote>
> 
> 
> As telldir(), which should correlate to 'case SEEK_CUR' will not
> provide invalid values, the behaviour is undefined.
> 
> 
> Also,
> 
> 
> case SEEK_END:
> [...]
>                 if (dx_dir)
>                         offset += ext4_get_htree_eof(file);
>                 else
>                         offset += inode->i_size;
> [...]
> 
> 
>         if (!dx_dir) {
>                 if (offset > inode->i_sb->s_maxbytes)
>                         goto out_err;
>         } else if (offset > ext4_get_htree_eof(file))
>                 goto out_err;
> 
> 
> 
> 
> Hence, the additional:
> 
>          case SEEK_END:
>                  if (unlikely(offset>  0))
>                       goto out_err; /* not supported for directories */
> 
>     
> is just a shortcut to avoid useless calculations.
> 
> Unless I missed something, it only remains the question if could
> break existing applications relying on undefined behaviour. However,
> I have no idea how an application might trigger that?

(other lists removed at this point, this is ext4-specific)

I know I'm being a little pedantic w/ the late review here....

It seems like the only differences between ext4_dir_llseek and the old ext4_llseek are these:

1) For SEEK_END, we now return -EINVAL for a positive offset (i.e. past EOF)
2) For SEEK_END, we seek to ext4_get_htree_eof() not to inode->i_size
3) For SEEK_SET, we impose different limits for max offset
  - s_maxbytes / ext4_get_htree_eof for !dx/dx, vs. s_bitmap_maxbytes/s_maxbytes

Do any of these changes relate to the hash collision problem?  Are any of them uniquely
required for ext4, enough to warrant cut & paste of the vfs llseek code (again?)

What I'm getting at is: what are the reasons that we cannot use generic_file_llseek_size(),
maybe with a new argument to specify a non-standard location for SEEK_END.  Such
a change would require a solid explanation, but it'd probably go in if it meant
one less seek implementation to worry about.

-Eric