| Message ID | 1540434176-14349-1-git-send-email-yuyufen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tmpfs: let lseek return ENXIO with a negative offset | expand |
> On Oct 24, 2018, at 8:22 PM, Yufen Yu <yuyufen@huawei.com> wrote: > > For now, the others filesystems, such as ext4, f2fs, ubifs, > all of them return ENXIO when lseek with a negative offset. > It is better to let tmpfs return ENXIO too. After that, tmpfs > can also pass generic/448. > > Signed-off-by: Yufen Yu <yuyufen@huawei.com> > --- > mm/shmem.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index 0376c124..f37bf06 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2608,9 +2608,7 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence) > inode_lock(inode); > /* We're holding i_mutex so we can access i_size directly */ > > - if (offset < 0) > - offset = -EINVAL; > - else if (offset >= inode->i_size) > + if (offset < 0 || offset >= inode->i_size) > offset = -ENXIO; > else { > start = offset >> PAGE_SHIFT; > -- It's not at all clear what the proper thing to do is if a negative offset is passed. The man page for lseek(2) states: SEEK_DATA Adjust the file offset to the next location in the file greater than or equal to offset containing data. If offset points to data, then the file offset is set to offset. SEEK_HOLE Adjust the file offset to the next hole in the file greater than or equal to offset. If offset points into the middle of a hole, then the file offset is set to offset. If there is no hole past offset, then the file offset is adjusted to the end of the file (i.e., there is an implicit hole at the end of any file). This seems to indicate that if passed a negative offset, a whence of either SEEK_DATA or SEEK_HOLE should operate the same as if passed an offset of 0. ENXIO just seems to be the wrong error code to return for a passed negative offset in these cases (also from lseek(2)): ENXIO whence is SEEK_DATA or SEEK_HOLE, and the file offset is beyond the end of the file. but EINVAL isn't technically appropriate either: EINVAL whence is not valid. Or: the resulting file offset would be negative, or beyond the end of a seekable device. At the very least it seems the man page should be updated to reflect that ENXIO may be returned if whence is SEEK_DATA or SEEK_HOLE and the passed offset is negative. William Kucharski
On Thu, 25 Oct 2018 10:22:56 +0800 Yufen Yu <yuyufen@huawei.com> wrote: > For now, the others filesystems, such as ext4, f2fs, ubifs, > all of them return ENXIO when lseek with a negative offset. When using SEEK_DATA and/or SEEK_HOLE, yes? > It is better to let tmpfs return ENXIO too. After that, tmpfs > can also pass generic/448. generic/448 is, I assume, part of xfstests? So I'll rewrite the changelog as follows. Please review carefully. Subject: tmpfs: make lseek(SEEK_DATA/SEK_HOLE) return ENXIO with a negative offset Other filesystems such as ext4, f2fs and ubifs all return ENXIO when lseek (SEEK_DATA or SEEK_HOLE) requests a negative offset. man 2 lseek says : EINVAL whence is not valid. Or: the resulting file offset would be : negative, or beyond the end of a seekable device. : : ENXIO whence is SEEK_DATA or SEEK_HOLE, and the file offset is beyond : the end of the file. Make tmpfs return ENXIO under these circumstances as well. After this, tmpfs also passes xfstests's generic/448.
> On Nov 7, 2018, at 4:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > man 2 lseek says > > : EINVAL whence is not valid. Or: the resulting file offset would be > : negative, or beyond the end of a seekable device. > : > : ENXIO whence is SEEK_DATA or SEEK_HOLE, and the file offset is beyond > : the end of the file. > > > Make tmpfs return ENXIO under these circumstances as well. After this, > tmpfs also passes xfstests's generic/448. As I objected to last week, despite the fact that other file systems do this, is this in fact the desired behavior? I'll let you reread that message rather than repeat it in its entirety here, but certainly a negative offset is not "beyond the end of the file," and the end result is errno is set to ENXIO for a reason that does not match what the lseek(2) man page describes. I also mentioned if a negative offset is used with SEEK_CUR or SEEK_WHENCE, arguably the negative offset should actually be treated as "0" given lseek(2) also states: SEEK_DATA Adjust the file offset to the next location in the file greater than or equal to offset containing data. If offset points to data, then the file offset is set to offset. SEEK_HOLE Adjust the file offset to the next hole in the file greater than or equal to offset. If offset points into the middle of a hole, then the file offset is set to offset. If there is no hole past offset, then the file offset is adjusted to the end of the file (i.e., there is an implicit hole at the end of any file). Since the "next location" or "next hole" will never be at a negative offset, the "greater than" clause of both descriptions would mean the resulting offset should be treated as if it were passed as zero. However, if xfstest-compliant behavior is desired, the lseek(2) man page description for ENXIO should be updated to something like: ENXIO whence is SEEK_DATA or SEEK_HOLE, and the file offset is negative or beyond the end of the file. I don't mean to be pedantic, but I also know how frustrating it can be when a system call returns with errno set for a reason that doesn't correspond to the man page. Thanks, William Kucharski
On Thu, 8 Nov 2018 03:46:35 -0700 William Kucharski <william.kucharski@oracle.com> wrote: > > > > On Nov 7, 2018, at 4:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > > > man 2 lseek says > > > > : EINVAL whence is not valid. Or: the resulting file offset would be > > : negative, or beyond the end of a seekable device. > > : > > : ENXIO whence is SEEK_DATA or SEEK_HOLE, and the file offset is beyond > > : the end of the file. > > > > > > Make tmpfs return ENXIO under these circumstances as well. After this, > > tmpfs also passes xfstests's generic/448. > > As I objected to last week, despite the fact that other file systems do this, is > this in fact the desired behavior? > > I'll let you reread that message rather than repeat it in its entirety here, but > certainly a negative offset is not "beyond the end of the file," and the end > result is errno is set to ENXIO for a reason that does not match what the > lseek(2) man page describes. > > I also mentioned if a negative offset is used with SEEK_CUR or SEEK_WHENCE, > arguably the negative offset should actually be treated as "0" given lseek(2) > also states: > > SEEK_DATA > Adjust the file offset to the next location in the file > greater than or equal to offset containing data. If offset > points to data, then the file offset is set to offset. > > SEEK_HOLE > Adjust the file offset to the next hole in the file greater > than or equal to offset. If offset points into the middle of > a hole, then the file offset is set to offset. If there is no > hole past offset, then the file offset is adjusted to the end > of the file (i.e., there is an implicit hole at the end of any > file). > > Since the "next location" or "next hole" will never be at a negative offset, the > "greater than" clause of both descriptions would mean the resulting offset should > be treated as if it were passed as zero. > > However, if xfstest-compliant behavior is desired, the lseek(2) man page > description for ENXIO should be updated to something like: > > ENXIO whence is SEEK_DATA or SEEK_HOLE, and the file offset is negative or > beyond the end of the file. > > I don't mean to be pedantic, but I also know how frustrating it can be when a system > call returns with errno set for a reason that doesn't correspond to the man page. I think that at this stage we should make tmpfs behaviour match the other filesystems. If the manpage doesn't match the kernel's behaviour for this linux-specific feature(?) then we should fix the manpage. If we find that the behaviour should actually change (and there's a way of doing that in a reasonably back-compatible manner) then let's change all filesystems and the manpage. OK?
> On Nov 8, 2018, at 4:07 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > I think that at this stage we should make tmpfs behaviour match the > other filesystems. > > If the manpage doesn't match the kernel's behaviour for this > linux-specific feature(?) then we should fix the manpage. > > If we find that the behaviour should actually change (and there's a way > of doing that in a reasonably back-compatible manner) then let's change > all filesystems and the manpage. > > OK? I did a little research, and according to lseek(2): SEEK_DATA and SEEK_HOLE are nonstandard extensions also present in Solaris, FreeBSD, and DragonFly BSD; they are proposed for inclusion in the next POSIX revision (Issue 8). I wrote a brief test program that operated on a file of 1024 zeroes and found that on an ext4 file system, lseek(2) matches Solaris' behavior on ZFS: Solaris/ZFS =========== lseek(3, -1, SEEK_HOLE) error, errno 6 (ENXIO) lseek(3, 0, SEEK_HOLE) returned 1024 lseek(3, 1, SEEK_HOLE) returned 1024 lseek(3, -1, SEEK_DATA) error, errno 6 (ENXIO) lseek(3, 0, SEEK_DATA) returned 0 lseek(3, 1, SEEK_DATA) returned 1 Linux/ext4 ========== lseek(3, -1, SEEK_HOLE) error, errno 6 (ENXIO) lseek(3, 0, SEEK_HOLE) returned 1024 lseek(3, 1, SEEK_HOLE) returned 1024 lseek(3, -1, SEEK_DATA) error, errno 6 (ENXIO) lseek(3, 0, SEEK_DATA) returned 0 lseek(3, 1, SEEK_DATA) returned 1 That validates the xfstest expectations that a negative passed offset should return ENXIO. I suggest the man page wording be changed to read: ENXIO whence is SEEK_DATA or SEEK_HOLE, and the file offset is negative or beyond the end of the file. unless you'd prefer an alternative statement. Thanks!!
diff --git a/mm/shmem.c b/mm/shmem.c index 0376c124..f37bf06 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2608,9 +2608,7 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence) inode_lock(inode); /* We're holding i_mutex so we can access i_size directly */ - if (offset < 0) - offset = -EINVAL; - else if (offset >= inode->i_size) + if (offset < 0 || offset >= inode->i_size) offset = -ENXIO; else { start = offset >> PAGE_SHIFT;
For now, the others filesystems, such as ext4, f2fs, ubifs, all of them return ENXIO when lseek with a negative offset. It is better to let tmpfs return ENXIO too. After that, tmpfs can also pass generic/448. Signed-off-by: Yufen Yu <yuyufen@huawei.com> --- mm/shmem.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)