Re: interims VFS queue

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 28 Oct 2011 09:30:17 -0400
Christoph Hellwig <hch@infradead.org> wrote:

> As discussed at KS I've collected up the VFS patches from 3 month worth
> of linux-fsdevel archives.

vfs things..

1: What happened with Andi's "dio: optimize cache misses in the
   submission path"?  (Against which I have checkpatch fixes and
   #include linux/prefetch.h, btw)

2: I'm still sitting on Andy Whitcroft's "readlinkat: ensure we
   return ENOENT for the empty pathname for normal lookups" and its
   fixup.  I'd marked this as needed-in-3.1.  Help.

From: Andy Whitcroft <apw@canonical.com>
Subject: readlinkat: ensure we return ENOENT for the empty pathname for normal lookups

Since the commit below which added O_PATH support to the *at() calls, the
error return for readlink/readlinkat for the empty pathname has switched
from ENOENT to EINVAL:

  commit 65cfc6722361570bfe255698d9cd4dccaf47570d
  Author: Al Viro <viro@zeniv.linux.org.uk>
  Date:   Sun Mar 13 15:56:26 2011 -0400

    readlinkat(), fchownat() and fstatat() with empty relative pathnames

This is both unexpected for userspace and makes readlink/readlinkat
inconsistant with all other interfaces; and inconsistant with our stated
return for these pathnames.

As the readlinkat call does not have a flags parameter we cannot use the
AT_EMPTY_PATH approach used in the other calls.  Therefore expose whether
the original path is infact entry via a new user_path_at_empty() path
lookup function.  Use this to determine whether to default to EINVAL or
ENOENT for failures.

Addresses http://bugs.launchpad.net/bugs/817187

[akpm@linux-foundation.org: remove unused getname_flags()]
Signed-off-by: Andy Whitcroft <apw@canonical.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: <stable@kernel.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/namei.c            |   19 ++++++++++++++-----
 fs/stat.c             |    5 +++--
 include/linux/namei.h |    1 +
 3 files changed, 18 insertions(+), 7 deletions(-)

diff -puN fs/namei.c~readlinkat-ensure-we-return-enoent-for-the-empty-pathname-for-normal-lookups fs/namei.c
--- a/fs/namei.c~readlinkat-ensure-we-return-enoent-for-the-empty-pathname-for-normal-lookups
+++ a/fs/namei.c
@@ -137,7 +137,8 @@ static int do_getname(const char __user 
 	return retval;
 }
 
-static char *getname_flags(const char __user * filename, int flags)
+static char *getname_flags_empty(const char __user * filename,
+							int flags, int *empty)
 {
 	char *tmp, *result;
 
@@ -148,6 +149,8 @@ static char *getname_flags(const char __
 
 		result = tmp;
 		if (retval < 0) {
+			if (retval == -ENOENT && empty)
+				*empty = 1;
 			if (retval != -ENOENT || !(flags & LOOKUP_EMPTY)) {
 				__putname(tmp);
 				result = ERR_PTR(retval);
@@ -160,7 +163,7 @@ static char *getname_flags(const char __
 
 char *getname(const char __user * filename)
 {
-	return getname_flags(filename, 0);
+	return getname_flags_empty(filename, 0, 0);
 }
 
 #ifdef CONFIG_AUDITSYSCALL
@@ -1798,11 +1801,11 @@ struct dentry *lookup_one_len(const char
 	return __lookup_hash(&this, base, NULL);
 }
 
-int user_path_at(int dfd, const char __user *name, unsigned flags,
-		 struct path *path)
+int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
+		 struct path *path, int *empty)
 {
 	struct nameidata nd;
-	char *tmp = getname_flags(name, flags);
+	char *tmp = getname_flags_empty(name, flags, empty);
 	int err = PTR_ERR(tmp);
 	if (!IS_ERR(tmp)) {
 
@@ -1816,6 +1819,12 @@ int user_path_at(int dfd, const char __u
 	return err;
 }
 
+int user_path_at(int dfd, const char __user *name, unsigned flags,
+		 struct path *path)
+{
+	return user_path_at_empty(dfd, name, flags, path, 0);
+}
+
 static int user_path_parent(int dfd, const char __user *path,
 			struct nameidata *nd, char **name)
 {
diff -puN fs/stat.c~readlinkat-ensure-we-return-enoent-for-the-empty-pathname-for-normal-lookups fs/stat.c
--- a/fs/stat.c~readlinkat-ensure-we-return-enoent-for-the-empty-pathname-for-normal-lookups
+++ a/fs/stat.c
@@ -294,15 +294,16 @@ SYSCALL_DEFINE4(readlinkat, int, dfd, co
 {
 	struct path path;
 	int error;
+	int empty = 0;
 
 	if (bufsiz <= 0)
 		return -EINVAL;
 
-	error = user_path_at(dfd, pathname, LOOKUP_EMPTY, &path);
+	error = user_path_at_empty(dfd, pathname, LOOKUP_EMPTY, &path, &empty);
 	if (!error) {
 		struct inode *inode = path.dentry->d_inode;
 
-		error = -EINVAL;
+		error = (empty) ? -ENOENT : -EINVAL;
 		if (inode->i_op->readlink) {
 			error = security_inode_readlink(path.dentry);
 			if (!error) {
diff -puN include/linux/namei.h~readlinkat-ensure-we-return-enoent-for-the-empty-pathname-for-normal-lookups include/linux/namei.h
--- a/include/linux/namei.h~readlinkat-ensure-we-return-enoent-for-the-empty-pathname-for-normal-lookups
+++ a/include/linux/namei.h
@@ -67,6 +67,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LA
 #define LOOKUP_EMPTY		0x4000
 
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
+extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
 
 #define user_path(name, path) user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, path)
 #define user_lpath(name, path) user_path_at(AT_FDCWD, name, 0, path)
_



3: I'm also sitting on Hans Verkuil's "poll: add
   poll_requested_events() function".  This is being merged via the v4l
   tree, based on looks-ok-to-akpm.  AFAIK nobody else has looked at
   it.  This should be in linux-next via the v4l tree by now, but it
   isn't, so something might have gone wrong.


4: I'm still sitting on Steve Rago's "fcntl(F_SETFL): allow setting
   of O_SYNC".  I have a comment here that you said it "needs an
   audit".  AFAIK nothing has happened.  Should I toss it?


From: Steve Rago <sar@nec-labs.com>
Subject: fcntl(F_SETFL): allow setting of O_SYNC

This has probably been a problem since day 1 (I ran into this running the
2.4 kernel years ago; finally got around to fixing it).  The problem is
that fcntl(fd, F_SETFL, flags|O_SYNC) appears to work, but silently
ignores the O_SYNC flag.  Opening the file with O_SYNC works okay, but
setting it later on via fcntl doesn't work.

akpm: will cause surprise slowdowns of applications such as
http://koders.com/c/fidA34D8D5EE9AA5D0AB0F3C604678E2E935E5B0246.aspx?s=dupa

akpm: maybe we should sync the file when someone sets O_SYNC.

Signed-off-by: Steve Rago <sar@nec-labs.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/fcntl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN fs/fcntl.c~fcntlf_setfl-allow-setting-of-o_sync fs/fcntl.c
--- a/fs/fcntl.c~fcntlf_setfl-allow-setting-of-o_sync
+++ a/fs/fcntl.c
@@ -143,7 +143,7 @@ SYSCALL_DEFINE1(dup, unsigned int, filde
 	return ret;
 }
 
-#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME)
+#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME | O_SYNC)
 
 static int setfl(int fd, struct file * filp, unsigned long arg)
 {
_



5: Tao Ma's "fs/direct-io.c: calculate fs_count correctly in
   get_more_blocks()" was missed and needs rework as a result of this
   merge.  Here's what I now have.  It compiles.


From: Tao Ma <boyu.mt@taobao.com>
Subject: fs/direct-io.c: calculate fs_count correctly in get_more_blocks()

In get_more_blocks(), we use dio_count to calcuate fs_count and do some
tricky things to increase fs_count if dio_count isn't aligned.  But
actually it still has some corner cases that can't be coverd.  See the
following example:

	dio_write foo -s 1024 -w 4096

(direct write 4096 bytes at offset 1024).  The same goes if the offset
isn't aligned to fs_blocksize.

In this case, the old calculation counts fs_count to be 1, but actually we
will write into 2 different blocks (if fs_blocksize=4096).  The old code
just works, since it will call get_block twice (and may have to allocate
and create extents twice for filesystems like ext4).  So we'd better call
get_block just once with the proper fs_count.

Signed-off-by: Tao Ma <boyu.mt@taobao.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/direct-io.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff -puN fs/direct-io.c~fs-direct-ioc-calcuate-fs_count-correctly-in-get_more_blocks fs/direct-io.c
--- a/fs/direct-io.c~fs-direct-ioc-calcuate-fs_count-correctly-in-get_more_blocks
+++ a/fs/direct-io.c
@@ -580,9 +580,8 @@ static int get_more_blocks(struct dio *d
 {
 	int ret;
 	sector_t fs_startblk;	/* Into file, in filesystem-sized blocks */
+	sector_t fs_endblk;	/* Into file, in filesystem-sized blocks */
 	unsigned long fs_count;	/* Number of filesystem-sized blocks */
-	unsigned long dio_count;/* Number of dio_block-sized blocks */
-	unsigned long blkmask;
 	int create;
 
 	/*
@@ -593,11 +592,9 @@ static int get_more_blocks(struct dio *d
 	if (ret == 0) {
 		BUG_ON(sdio->block_in_file >= sdio->final_block_in_request);
 		fs_startblk = sdio->block_in_file >> sdio->blkfactor;
-		dio_count = sdio->final_block_in_request - sdio->block_in_file;
-		fs_count = dio_count >> sdio->blkfactor;
-		blkmask = (1 << sdio->blkfactor) - 1;
-		if (dio_count & blkmask)	
-			fs_count++;
+		fs_endblk = (sdio->final_block_in_request - 1) >>
+					sdio->blkfactor;
+		fs_count = fs_endblk - fs_startblk + 1;
 
 		map_bh->b_state = 0;
 		map_bh->b_size = fs_count << dio->inode->i_blkbits;
_