| Message ID | 4F95C109.1030401@itwm.fraunhofer.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 4/23/12 3:52 PM, Bernd Schubert wrote: > On 04/23/2012 10:37 PM, Eric Sandeen wrote: >> On 4/22/12 7:51 AM, Bernd Schubert wrote: >>> On 04/20/2012 10:04 PM, Eric Sandeen wrote: >>>> On 1/9/12 7:21 AM, Bernd Schubert wrote: >>>>> From: Fan Yong <yong.fan@whamcloud.com> >>>>> >>>>> Traditionally ext2/3/4 has returned a 32-bit hash value from llseek() >>>>> to appease NFSv2, which can only handle a 32-bit cookie for seekdir() >>>>> and telldir(). However, this causes problems if there are 32-bit hash >>>>> collisions, since the NFSv2 server can get stuck resending the same >>>>> entries from the directory repeatedly. >>>>> >>>>> Allow ext4 to return a full 64-bit hash (both major and minor) for >>>>> telldir to decrease the chance of hash collisions. This still needs >>>>> integration on the NFS side. >>>>> >>>>> Patch-updated-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de> >>>>> (blame me if something is not correct) >>>> >>>> Bernd, I've merged this to ext3. Bruce thought maybe you were working >>>> on the same. Should I send mine? >>> >>> That is perfectly fine with me. >>> >>>> >>>> Also... >>>> >>>>> +/* >>>>> + * ext4_dir_llseek() based on generic_file_llseek() to handle both >>>>> + * non-htree and htree directories, where the "offset" is in terms >>>>> + * of the filename hash value instead of the byte offset. >>>>> + * >>>>> + * NOTE: offsets obtained *before* ext4_set_inode_flag(dir, EXT4_INODE_INDEX) >>>>> + * will be invalid once the directory was converted into a dx directory >>>>> + */ >>>>> +loff_t ext4_dir_llseek(struct file *file, loff_t offset, int origin) >>>> >>>> ext4_llseek() worries about max offset for direct/indirect vs. extent-mapped >>>> files. Do we need to worry about the same thing in this function? >>> >>> Hrmm, I just checked it and I think either is wrong. We only have to >>> care about non-dx directories, so ext4_readdir() applies, which limits >>> filp->f_pos < inode->i_size. >>> Going to send a patch tomorrow. Thanks for spotting this! >> >> The other thing I'm wondering is whether, in light of >> >> ef3d0fd27e90f67e35da516dafc1482c82939a60 vfs: do (nearly) lockless generic_file_llseek >> >> taking the i_mutex in ext4_dir_llseek could be a perf regression vs what was there before? Is there anything about the new function which requires stronger locking? >> >> I may be missing something obvious about the nfs interaction, not sure. >> > > Oh, good point. I was just about to send a small patch, but reading > through the lockless commit will take some time - its already too late > for me for today. Will work on that tomorrow. Thanks again for your review! Sorry it's so late :( -Eric > Cheers, > Bernd > > > > diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c > index b867862..3a4988e2 100644 > --- a/fs/ext4/dir.c > +++ b/fs/ext4/dir.c > @@ -363,7 +363,7 @@ loff_t ext4_dir_llseek(struct file *file, loff_t > offset, int origin) > goto out_err; > > if (!dx_dir) { > - if (offset > inode->i_sb->s_maxbytes) > + if (offset > i_size_read(inode)) > goto out_err; > } else if (offset > ext4_get_htree_eof(file)) > goto out_err; > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/23/12 3:52 PM, Bernd Schubert wrote: > On 04/23/2012 10:37 PM, Eric Sandeen wrote: ... > diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c > index b867862..3a4988e2 100644 > --- a/fs/ext4/dir.c > +++ b/fs/ext4/dir.c > @@ -363,7 +363,7 @@ loff_t ext4_dir_llseek(struct file *file, loff_t > offset, int origin) > goto out_err; > > if (!dx_dir) { > - if (offset > inode->i_sb->s_maxbytes) > + if (offset > i_size_read(inode)) > goto out_err; > } else if (offset > ext4_get_htree_eof(file)) > goto out_err; 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? -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Cheers, Andreas -- Andreas Dilger Whamcloud, Inc. Principal Lustre Engineer http://www.whamcloud.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Thanks, Bernd -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/24/2012 09:21 PM, Eric Sandeen wrote: > 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.... That is fine, lets better be pedantic now than cause trouble to ext4 users... > > 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) I definitely introduces that one, as I cannot see how an application might ever run into it. Especially as ext4 directories cannot shrink. So if an application tries to exceed the directory size limit, it looks to me as some of attempt to break something or as an error in the application. However, if there should be the slightest chance to break existing applications relying on that, we need to remove that. I thought about 2) and 3) it on my way home and I think I remembered the reason for it. > 2) For SEEK_END, we seek to ext4_get_htree_eof() not to inode->i_size Lets assume an application wants to seek to the last directory entry. If it would seek to inode->i_size and then would attempt another readdir from that offset, we probably would succeed, as inode->i_size is probably just an arbitrary value in between two hashes, or even smaller than the very first hash value, so the next readdir() probably even would read the very first directory entry. I think i_size and ext4_get_htree_eof() makes a very big difference here. > 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 Its a bit too late for me to check that today (and I'm almost starving...), but is it possible that s_maxbytes is smaller than ext4_get_htree_eof? So is possible that valid hash values get larger than s_maxbytes? I will check that tomorrow morning. > > 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. I think we probably need to extent generic_file_llseek_size() by a parameter 'max_fs_limit' (well something like that name, I don't find a better one now) and then it should be possible to use it. Cheers, Bernd -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/24/12 4:28 PM, Andreas Dilger wrote: > On 2012-04-24, at 2:21 PM, Eric Sandeen wrote: >> 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. > > So, when we were looking at this code, it makes sense that if dir seek is being done for telldir/seekdir that the parameters for ext4 are hash functions, so they should be compared against hash limits instead of the file size. > > This probably makes sense for other filesystems that use hash cookies instead of byte offsets to have a similar dir seek implementation, but I thought there might be a controversy about this and I'm happy to get it into ext4 as a starting point. That makes sense... but I think the generic code could be expanded to handle this, if we used the existing size parameter to specify max seekable offset in the dir, and a new parameter to cause SEEK_END to behave in a special way (not depending on i_size?) -Eric > Cheers, Andreas > -- > Andreas Dilger Whamcloud, Inc. > Principal Lustre Engineer http://www.whamcloud.com/ > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-04-24, at 2:21 PM, Eric Sandeen wrote: > 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. So, when we were looking at this code, it makes sense that if dir seek is being done for telldir/seekdir that the parameters for ext4 are hash functions, so they should be compared against hash limits instead of the file size. This probably makes sense for other filesystems that use hash cookies instead of byte offsets to have a similar dir seek implementation, but I thought there might be a controversy about this and I'm happy to get it into ext4 as a starting point. Cheers, Andreas -- Andreas Dilger Whamcloud, Inc. Principal Lustre Engineer http://www.whamcloud.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-04-24, at 4:07 PM, Bernd Schubert wrote: >> 1) For SEEK_END, we now return -EINVAL for a positive offset (i.e. past EOF) > > I definitely introduces that one, as I cannot see how an application > might ever run into it. Especially as ext4 directories cannot shrink. So > if an application tries to exceed the directory size limit, it looks to > me as some of attempt to break something or as an error in the > application. However, if there should be the slightest chance to break > existing applications relying on that, we need to remove that. I think the other reason to avoid SEEK_END + n is that since SEEK_END for a hash offset is (signed) MAX_LONG, so if one seeks beyond that it will wrap to a negative offset. Cheers, Andreas -- Andreas Dilger Whamcloud, Inc. Principal Lustre Engineer http://www.whamcloud.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/24/12 5:24 PM, Andreas Dilger wrote: > On 2012-04-24, at 4:07 PM, Bernd Schubert wrote: >>> 1) For SEEK_END, we now return -EINVAL for a positive offset (i.e. past EOF) >> >> I definitely introduces that one, as I cannot see how an application >> might ever run into it. Especially as ext4 directories cannot shrink. So >> if an application tries to exceed the directory size limit, it looks to >> me as some of attempt to break something or as an error in the >> application. However, if there should be the slightest chance to break >> existing applications relying on that, we need to remove that. > > I think the other reason to avoid SEEK_END + n is that since SEEK_END > for a hash offset is (signed) MAX_LONG, so if one seeks beyond that > it will wrap to a negative offset. Makes sense. Wishing this had been done as a separate patch, though, since it's really addressing a separate issue from the $SUBJECT, and could have used specific documentation of the change. Nitpicky I know, but it helps. -Eric > Cheers, Andreas > -- > Andreas Dilger Whamcloud, Inc. > Principal Lustre Engineer http://www.whamcloud.com/ > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/25/2012 05:05 PM, Eric Sandeen wrote: > On 4/24/12 5:24 PM, Andreas Dilger wrote: >> On 2012-04-24, at 4:07 PM, Bernd Schubert wrote: >>>> 1) For SEEK_END, we now return -EINVAL for a positive offset (i.e. past EOF) >>> >>> I definitely introduces that one, as I cannot see how an application >>> might ever run into it. Especially as ext4 directories cannot shrink. So >>> if an application tries to exceed the directory size limit, it looks to >>> me as some of attempt to break something or as an error in the >>> application. However, if there should be the slightest chance to break >>> existing applications relying on that, we need to remove that. >> >> I think the other reason to avoid SEEK_END + n is that since SEEK_END >> for a hash offset is (signed) MAX_LONG, so if one seeks beyond that >> it will wrap to a negative offset. > > Makes sense. > > Wishing this had been done as a separate patch, though, since it's really addressing a separate issue from the $SUBJECT, and could have used specific documentation of the change. Nitpicky I know, but it helps. Sorry, my fault. Maybe we should simply document it in the code? And how do we proceed in general. Shall I write a patch to use generic_file_llseek() and update that function to take more arguments? I don't think that would go into 3.4. Thanks, Bernd -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/25/12 10:12 AM, Bernd Schubert wrote: > On 04/25/2012 05:05 PM, Eric Sandeen wrote: >> On 4/24/12 5:24 PM, Andreas Dilger wrote: >>> On 2012-04-24, at 4:07 PM, Bernd Schubert wrote: >>>>> 1) For SEEK_END, we now return -EINVAL for a positive offset >>>>> (i.e. past EOF) >>>> >>>> I definitely introduces that one, as I cannot see how an >>>> application might ever run into it. Especially as ext4 >>>> directories cannot shrink. So if an application tries to exceed >>>> the directory size limit, it looks to me as some of attempt to >>>> break something or as an error in the application. However, if >>>> there should be the slightest chance to break existing >>>> applications relying on that, we need to remove that. >>> >>> I think the other reason to avoid SEEK_END + n is that since >>> SEEK_END for a hash offset is (signed) MAX_LONG, so if one seeks >>> beyond that it will wrap to a negative offset. >> >> Makes sense. >> >> Wishing this had been done as a separate patch, though, since it's >> really addressing a separate issue from the $SUBJECT, and could >> have used specific documentation of the change. Nitpicky I know, >> but it helps. > > Sorry, my fault. No worries, I should have reviewed sooner too :) > Maybe we should simply document it in the code? And > how do we proceed in general. Shall I write a patch to use > generic_file_llseek() and update that function to take more > arguments? I don't think that would go into 3.4. Unless there is obviously _wrong_ behavior to be fixed I don't think it's needed for 3.4. We could maybe do one patch to make it lockless again, if there's good confidence in that, since it's sort of a "regression." Trying to munge things into the upstream seek function would be post-3.4, I'm sure, if it turns out it can be done at all. Thanks, -Eric > Thanks, Bernd -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c index b867862..3a4988e2 100644 --- a/fs/ext4/dir.c +++ b/fs/ext4/dir.c @@ -363,7 +363,7 @@ loff_t ext4_dir_llseek(struct file *file, loff_t offset, int origin) goto out_err; if (!dx_dir) { - if (offset > inode->i_sb->s_maxbytes) + if (offset > i_size_read(inode)) goto out_err; } else if (offset > ext4_get_htree_eof(file))