interims VFS queue

interims VFS queue

[Christoph Hellwig]

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

The major changes are lseek locking and O_DIRECT cache footprint
optimizations from Andi Kleen, which Al already had queued up in his
non-public queue before he caught the flu.  In addition to that
it's just fairly small patches.  If I missed anything important please
send it my way until Al is back.


There is a few promiment things intentionally missing:

 - ->d_prune support from Sage.  I though we finally had this one nailed
   down a few weeks ago, but it crashes btrfs when running xfstests, so
   we'll need another round.
 - Miklos' remount r/o fixes.  I really wanted them, but found a few
   minor issue and want to give them more QA first.  Hopefully we can
   get them in during a second pull for 3.2, even more hopefull Al will
   be back in time as he hasn't replied back to Miklos' answers to his
   comments.
 - the block queue caching part of Andis O_DIRECT patches.  With the
   current mess we have with the block queues I'd rather defer that
   part for now, hopefull we can still put it in later.
 - the btrfs lseek cleanups as part of the lseek series - I think they
   are correct, but I fear we might run into a merge mess with them
   as Chris said he picked them up for the btrfs tree a while ago.

The tree is at

  git://git.kernel.org/pub/scm/linux/kernel/git/hch/vfs-queue.git for-next

and I'd prefer if Stephen could add it to linux-next for a day or two
before pulling it into mainline.

The git request-pull information is below:



The following changes since commit c3b92c8787367a8bb53d57d9789b558f1295cc96:

  Linux 3.1 (2011-10-24 09:10:05 +0200)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/hch/vfs-queue.git for-next

Andi Kleen (11):
      direct-io: separate fields only used in the submission path from struct dio
      direct-io: fix a wrong comment
      direct-io: rearrange fields in dio/dio_submit to avoid holes
      direct-io: use a slab cache for struct dio
      direct-io: separate map_bh from dio
      direct-io: inline the complete submission path
      direct-io: merge direct_io_walker into __blockdev_direct_IO
      vfs: do (nearly) lockless generic_file_llseek
      vfs: add generic_file_llseek_size
      ext4: replace cut'n'pasted llseek code with generic_file_llseek_size
      nfs: drop unnecessary locking in llseek

Andreas Gruenbacher (3):
      vfs: indicate that the permission functions take all the MAY_* flags
      vfs: pass all mask flags check_acl and posix_acl_permission
      vfs: add a comment to inode_permission()

Aneesh Kumar K.V (1):
      vfs: add hex format for MAY_* flag values

Bryan Schumaker (1):
      vfs: add "device" tag to /proc/self/mountstats

Christoph Hellwig (1):
      vfs: fix spinning prevention in prune_icache_sb

Eric W. Biederman (1):
      compat: sync compat_stats with statfs.

J. Bruce Fields (1):
      leases: fix write-open/read-lease race

Jeff Layton (1):
      vfs: iov_iter: have iov_iter_advance decrement nr_segs appropriately

Wang Sheng-Hui (1):
      cleanup: vfs: small comment fix for block_invalidatepage

 arch/mips/include/asm/compat.h    |    3 +-
 arch/parisc/include/asm/compat.h  |    3 +-
 arch/powerpc/include/asm/compat.h |    3 +-
 arch/s390/include/asm/compat.h    |    3 +-
 arch/sparc/include/asm/compat.h   |    3 +-
 arch/x86/include/asm/compat.h     |    3 +-
 fs/btrfs/file.c                   |    2 +-
 fs/buffer.c                       |    4 +-
 fs/cifs/cifsfs.c                  |    2 +-
 fs/compat.c                       |    7 +-
 fs/direct-io.c                    |  646 +++++++++++++++++++------------------
 fs/ext4/file.c                    |   47 +---
 fs/gfs2/file.c                    |    4 +-
 fs/inode.c                        |    2 +-
 fs/namei.c                        |   17 +-
 fs/namespace.c                    |    1 +
 fs/nfs/file.c                     |   10 +-
 fs/open.c                         |    4 +
 fs/posix_acl.c                    |    2 +
 fs/read_write.c                   |   74 +++--
 include/linux/fs.h                |   28 +-
 mm/filemap.c                      |    3 +
 22 files changed, 440 insertions(+), 431 deletions(-)

Re: interims VFS queue

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

Hi Christoph,

On Fri, 28 Oct 2011 09:30:17 -0400 Christoph Hellwig <hch@infradead.org> wrote:
>
> The tree is at
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/hch/vfs-queue.git for-next
> 
> and I'd prefer if Stephen could add it to linux-next for a day or two
> before pulling it into mainline.

I can do that, however I will be travelling from tomorrow until Nov 1, so
the next linux-next release will be then (hopefully).  If that is not too
late (there should still be 5 or 6 days of the merge window left), then
I will put it in that release.  If it goes to Linus in the meantime, then
that will save me some work :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: interims VFS queue

[Linus Torvalds]

On Fri, Oct 28, 2011 at 11:43 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> I can do that, however I will be travelling from tomorrow until Nov 1, so
> the next linux-next release will be then (hopefully).  If that is not too
> late (there should still be 5 or 6 days of the merge window left), then
> I will put it in that release.  If it goes to Linus in the meantime, then
> that will save me some work :-)

I decided to pull it, because it's going to get more testing in my
tree anyway. And I was pulling filesystems, so pulling the VFS patches
at the same time just fit.

                     Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: interims VFS queue

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]

Hi Linus,

On Fri, 28 Oct 2011 12:08:55 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> I decided to pull it, because it's going to get more testing in my
> tree anyway. And I was pulling filesystems, so pulling the VFS patches
> at the same time just fit.

Yep, just noticed that.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: interims VFS queue

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

Hi Christoph,

On Sat, 29 Oct 2011 05:43:10 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Fri, 28 Oct 2011 09:30:17 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> >
> > The tree is at
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/hch/vfs-queue.git for-next
> > 
> > and I'd prefer if Stephen could add it to linux-next for a day or two
> > before pulling it into mainline.
> 
> I can do that, however I will be travelling from tomorrow until Nov 1, so
> the next linux-next release will be then (hopefully).  If that is not too
> late (there should still be 5 or 6 days of the merge window left), then
> I will put it in that release.  If it goes to Linus in the meantime, then
> that will save me some work :-)

Looks like Linus already took it.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: interims VFS queue

[Andrew Morton]

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;
_

Re: interims VFS queue

[Christoph Hellwig]

On Fri, Oct 28, 2011 at 03:09:36PM -0700, Andrew Morton wrote:
> 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)

It has mostly been merged, including all whitespace fixes and some
folding.  What is missing is the last two patchees because I don't
want to put the request_queue caching in until the remaining
lifetime issues for the request queue are sorted out, and the last
patch (which would need prefetch.h) depends on it.

> 
> 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:

The fix is defintively required, I'll look over it in a bit more detail
and will push it with the next round.

> 
> 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.

That was my assumption, too. 

> 
> 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?

This also needs all the other bits I mentioned during review.

> 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.

Tao, can you please resend it against current Linus' tree after making
sure the upated version is still fine?

caching the request queue was Re: interims VFS queue

[Andi Kleen]

On Sat, Oct 29, 2011 at 06:58:56AM -0400, Christoph Hellwig wrote:
> On Fri, Oct 28, 2011 at 03:09:36PM -0700, Andrew Morton wrote:
> > 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)
> 
> It has mostly been merged, including all whitespace fixes and some
> folding.  What is missing is the last two patchees because I don't
> want to put the request_queue caching in until the remaining
> lifetime issues for the request queue are sorted out, and the last
> patch (which would need prefetch.h) depends on it.

I asked Jens in person and he seemed ok with with the queue 
caching. Jens can you ack?


-Andi

Re: caching the request queue was Re: interims VFS queue

[Vivek Goyal]

On Sat, Oct 29, 2011 at 01:49:16PM +0200, Andi Kleen wrote:
> On Sat, Oct 29, 2011 at 06:58:56AM -0400, Christoph Hellwig wrote:
> > On Fri, Oct 28, 2011 at 03:09:36PM -0700, Andrew Morton wrote:
> > > 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)
> > 
> > It has mostly been merged, including all whitespace fixes and some
> > folding.  What is missing is the last two patchees because I don't
> > want to put the request_queue caching in until the remaining
> > lifetime issues for the request queue are sorted out, and the last
> > patch (which would need prefetch.h) depends on it.
> 
> I asked Jens in person and he seemed ok with with the queue 
> caching. Jens can you ack?

If you are caching queue in block device, you probably need to take
queue reference too using blk_get_queue().

Thanks
Vivek

Re: interims VFS queue

[Tao Ma]

On 10/29/2011 06:58 PM, Christoph Hellwig wrote:
> On Fri, Oct 28, 2011 at 03:09:36PM -0700, Andrew Morton wrote:
>> 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)
> 
> It has mostly been merged, including all whitespace fixes and some
> folding.  What is missing is the last two patchees because I don't
> want to put the request_queue caching in until the remaining
> lifetime issues for the request queue are sorted out, and the last
> patch (which would need prefetch.h) depends on it.
> 
>>
>> 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:
> 
> The fix is defintively required, I'll look over it in a bit more detail
> and will push it with the next round.
> 
>>
>> 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.
> 
> That was my assumption, too. 
> 
>>
>> 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?
> 
> This also needs all the other bits I mentioned during review.
> 
>> 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.
> 
> Tao, can you please resend it against current Linus' tree after making
> sure the upated version is still fine?
OK, I will try to rebase it and do some test against it.

Thanks.
Tao

[PATCH for 3.2] fs/direct-io.c: Calculate fs_count correctly in get_more_blocks.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

In get_more_blocks, we use dio_count to calculate fs_count and do some
tricky things to increase fs_count if dio_count isn't aligned. But
actually it still has some cornor case 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 extent twice for file systems like ext4). So we'd better call
get_block just once with the proper fs_count.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
Christoph, this is the updated version. I have done some tests against it and
it works. And the only difference between the old and this one is the change
from dio to sdio. Thanks.

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

diff --git a/fs/direct-io.c b/fs/direct-io.c
index d740ab6..5582183 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -580,9 +580,8 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio,
 {
 	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 *dio, struct dio_submit *sdio,
 	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;
-- 
1.7.1

Re: [PATCH for 3.2] fs/direct-io.c: Calculate fs_count correctly in get_more_blocks.

[Jeff Moyer]

Tao Ma <tm@tao.ma> writes:

> From: Tao Ma <boyu.mt@taobao.com>
>
> In get_more_blocks, we use dio_count to calculate fs_count and do some
> tricky things to increase fs_count if dio_count isn't aligned. But
> actually it still has some cornor case 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 extent twice for file systems like ext4). So we'd better call
> get_block just once with the proper fs_count.

This description was *really* hard for me to understand.  It seems to me
that right now there's an inefficiency in the code.  It's not clear
whether you're claiming that it was introduced recently, though.  Was
it, or has this problem been around for a while?

How did you notice this?  Was there any evidence of a problem, such as
performance overhead or less than ideal file layout?

Anyway, I agree that the code does not correctly calculate the number of
file system blocks in a request.  I also agree that your patch fixes
that issue.

Please ammend the description and then you can add my:

Acked-by: Jeff Moyer <jmoyer@redhat.com>

Cheers,
Jeff

Re: [PATCH for 3.2] fs/direct-io.c: Calculate fs_count correctly in get_more_blocks.

[Tao Ma]

On 11/01/2011 02:12 AM, Jeff Moyer wrote:
> Tao Ma <tm@tao.ma> writes:
> 
>> From: Tao Ma <boyu.mt@taobao.com>
>>
>> In get_more_blocks, we use dio_count to calculate fs_count and do some
>> tricky things to increase fs_count if dio_count isn't aligned. But
>> actually it still has some cornor case 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 extent twice for file systems like ext4). So we'd better call
>> get_block just once with the proper fs_count.
> 
> This description was *really* hard for me to understand.  It seems to me
> that right now there's an inefficiency in the code.  It's not clear
> whether you're claiming that it was introduced recently, though.  Was
> it, or has this problem been around for a while?
Actually it is there a long time ago. And the good thing is that it
isn't a bug, only some performance overhead.
> 
> How did you notice this?  Was there any evidence of a problem, such as
> performance overhead or less than ideal file layout?
I found it when I dig into some ext4 issues. The ext4 can't create the
whole 8K(in the above case) and ext4 has to create the blocks 2 times
for just one direct i/o write. In some of our test, it costs.

> 
> Anyway, I agree that the code does not correctly calculate the number of
> file system blocks in a request.  I also agree that your patch fixes
> that issue.
> 
> Please ammend the description and then you can add my:
So how about the following commit log(please feel free to modify it if I
still don't describe it correctly).

In get_more_blocks, we use dio_count to calculate fs_count to let the
file system map(maybe also create) blocks. And some tricky things are
done to increase fs_count if dio_count isn't aligned.

But actually it still has some cornor case that can't be coverd. See the
following example:
./dio_write foo -s 1024 -w 4096(direct write 4096 bytes at offset 1024).

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). So the
underlying file system is called twice and leads to some performance
overhead. So fix it by calculating fs_count correctly and let the file
system knows what we really want to write.

Thanks
Tao

> 
> Acked-by: Jeff Moyer <jmoyer@redhat.com>
> 
> Cheers,
> Jeff
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 for 3.2] fs/direct-io.c: Calculate fs_count correctly in get_more_blocks.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

In get_more_blocks, we use dio_count to calculate fs_count to let the
file system map(maybe also create) blocks. And some tricky things are
done to increase fs_count if dio_count isn't aligned.

But actually it still has some cornor case that can't be coverd. See the
following example:
./dio_write foo -s 1024 -w 4096(direct write 4096 bytes at offset 1024).

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). So the
underlying file system is called twice and leads to some performance
overhead. So fix it by calculating fs_count correctly and let the file
system knows what we really want to write.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 fs/direct-io.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index d740ab6..5582183 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -580,9 +580,8 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio,
 {
 	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 *dio, struct dio_submit *sdio,
 	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;
-- 
1.7.1

Re: [PATCH V2 for 3.2] fs/direct-io.c: Calculate fs_count correctly in get_more_blocks.

[Christoph Hellwig]

The patch looks good to me, but given that it neither fixes a bug
nor a regression I'd probably call it 3.3 material at this point.

Re: [PATCH V2 for 3.2] fs/direct-io.c: Calculate fs_count correctly in get_more_blocks.

[Tao Ma]

On 11/02/2011 03:36 PM, Christoph Hellwig wrote:
> The patch looks good to me, but given that it neither fixes a bug
> nor a regression I'd probably call it 3.3 material at this point.
OK, so you will take it or I should ask Andrew to take it for now?

Thanks
Tao

Re: interims VFS queue

[Aneesh Kumar K.V]

On Fri, 28 Oct 2011 15:09:36 -0700, Andrew Morton <akpm@linux-foundation.org> wrote:
> 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.


I had discussed this with Al Viro. He mentioned that he wanted to do it
differently. 
 
-aneesh

Re: interims VFS queue

[Christoph Hellwig]

On Sat, Oct 29, 2011 at 07:18:29PM +0530, Aneesh Kumar K.V wrote:
> > 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.
> 
> 
> I had discussed this with Al Viro. He mentioned that he wanted to do it
> differently. 

We will have to put a fix in, given that it is a regression in syscall
behaviour from a previous version.  I don't overly like the impact
the current patch has, but unless we can come up with something better
rather quickly I think we have to put Andy's patch in for now.

Re: interims VFS queue

[Hans Verkuil]

On Saturday, October 29, 2011 00:09:36 Andrew Morton wrote:
> 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.

I don't believe Mauro merged this in the v4l tree yet. I've added Mauro to
the CC list since this is now in his hands. I believe he also talked to some
people at the kernel summit about this to get additional review(s), but I may
have misunderstood that part.

Regards,

	Hans

Re: interims VFS queue, part2

[Christoph Hellwig]

Here are more VFS bits:

 - a simple leak fix for the simple_fill_super failure path.
 - Andy's fix for a user space visible error code regression in
   readlinkat.  This one is a bit ugly, and Anessh mentioned that Al
   didn't particularly like the fix, but I think we need to fix the user
   visible effects ASAP, and can clean up the implementation later.
 - the first part of Miklos' remount r/o fixes.  This is mostly
   finishing the i_link abstraction Al started a while ago, and while
   it touches a few lines it's entirely mechanical.  The second part
   is marking a filesystem as read-only even if ->remount_fs failed
   in case of an emergency remount r/o from sysrq.  I'm still looking
   into the rest of his series.
 - Sage's ->d_prune dentry operation.  This was skipped last time
   because it hit the only possible bug it could introduce.  I'm
   confident this new version is fine, and it's really needed for
   ceph to get cache coherency right.  I've ordered it at last in
   case you do not want it anymore at this stage.

The series passes xfstests without regressions on btrfs, ext3, ext4 and
xfs.

The following changes since commit 994c0e992522c123298b4a91b72f5e67ba2d1123:

  Merge branch 'next/soc' of git://git.linaro.org/people/arnd/arm-soc (2011-11-01 21:08:03 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/hch/vfs-queue.git for-next

Andy Whitcroft (1):
      readlinkat: ensure we return ENOENT for the empty pathname for normal lookups

Konstantin Khlebnikov (1):
      vfs: fix dentry leak in simple_fill_super()

Miklos Szeredi (8):
      vfs: ignore error on forced remount
      hypfs: remove unnecessary nlink setting
      jfs: remove unnecessary nlink setting
      ocfs2: remove unnecessary nlink setting
      logfs: remove unnecessary nlink setting
      filesystems: add missing nlink wrappers
      filesystems: add set_nlink()
      vfs: protect i_nlink

Sage Weil (1):
      vfs: add d_prune dentry operation

 Documentation/filesystems/Locking |    1 +
 arch/s390/hypfs/inode.c           |    6 +---
 drivers/mtd/mtdchar.c             |    2 +-
 drivers/staging/pohmelfs/inode.c  |    2 +-
 fs/9p/vfs_inode.c                 |    4 +-
 fs/9p/vfs_inode_dotl.c            |    4 +-
 fs/adfs/inode.c                   |    2 +-
 fs/affs/amigaffs.c                |    4 +-
 fs/affs/inode.c                   |    8 +++---
 fs/affs/namei.c                   |    6 ++--
 fs/afs/fsclient.c                 |    2 +-
 fs/afs/inode.c                    |    4 +-
 fs/autofs4/inode.c                |    2 +-
 fs/befs/linuxvfs.c                |    2 +-
 fs/bfs/dir.c                      |    2 +-
 fs/bfs/inode.c                    |    2 +-
 fs/binfmt_misc.c                  |    2 +-
 fs/btrfs/delayed-inode.c          |    2 +-
 fs/btrfs/disk-io.c                |    2 +-
 fs/btrfs/inode.c                  |    4 +-
 fs/btrfs/tree-log.c               |    2 +-
 fs/ceph/caps.c                    |    2 +-
 fs/ceph/inode.c                   |    2 +-
 fs/cifs/inode.c                   |    6 ++--
 fs/cifs/link.c                    |    2 +-
 fs/coda/coda_linux.c              |    2 +-
 fs/coda/dir.c                     |    2 +-
 fs/dcache.c                       |   40 ++++++++++++++++++++++++++++++++----
 fs/devpts/inode.c                 |    4 +-
 fs/ecryptfs/inode.c               |   12 +++++-----
 fs/efs/inode.c                    |    2 +-
 fs/exofs/inode.c                  |    2 +-
 fs/ext2/ialloc.c                  |    2 +-
 fs/ext2/inode.c                   |    2 +-
 fs/ext3/ialloc.c                  |    2 +-
 fs/ext3/inode.c                   |    2 +-
 fs/ext3/namei.c                   |    6 ++--
 fs/ext4/ialloc.c                  |    2 +-
 fs/ext4/inode.c                   |    2 +-
 fs/ext4/migrate.c                 |    2 +-
 fs/ext4/namei.c                   |    8 +++---
 fs/fat/inode.c                    |    4 +-
 fs/fat/namei_msdos.c              |    2 +-
 fs/fat/namei_vfat.c               |    2 +-
 fs/freevxfs/vxfs_inode.c          |    2 +-
 fs/fuse/control.c                 |    2 +-
 fs/fuse/inode.c                   |    2 +-
 fs/gfs2/glops.c                   |    2 +-
 fs/hfs/dir.c                      |    4 +-
 fs/hfs/inode.c                    |    4 +-
 fs/hfsplus/dir.c                  |    4 +-
 fs/hfsplus/inode.c                |   10 ++++----
 fs/hostfs/hostfs_kern.c           |    2 +-
 fs/hpfs/dir.c                     |    2 +-
 fs/hpfs/inode.c                   |   10 ++++----
 fs/hpfs/namei.c                   |    8 +++---
 fs/hppfs/hppfs.c                  |    2 +-
 fs/hugetlbfs/inode.c              |    2 +-
 fs/inode.c                        |    2 +-
 fs/isofs/inode.c                  |    4 +-
 fs/isofs/rock.c                   |    4 +-
 fs/jffs2/dir.c                    |    6 ++--
 fs/jffs2/fs.c                     |    6 ++--
 fs/jfs/jfs_imap.c                 |    6 ++--
 fs/jfs/jfs_inode.c                |    2 +-
 fs/jfs/namei.c                    |   12 +++++-----
 fs/jfs/super.c                    |    1 -
 fs/libfs.c                        |    6 +++-
 fs/logfs/dir.c                    |    8 +++---
 fs/logfs/inode.c                  |    3 +-
 fs/logfs/readwrite.c              |    2 +-
 fs/minix/inode.c                  |    4 +-
 fs/namei.c                        |   18 ++++++++++++----
 fs/ncpfs/inode.c                  |    2 +-
 fs/nfs/inode.c                    |    6 ++--
 fs/nilfs2/inode.c                 |    4 +-
 fs/nilfs2/namei.c                 |    2 +-
 fs/ntfs/inode.c                   |    8 +++---
 fs/ocfs2/dir.c                    |    4 +-
 fs/ocfs2/dlmglue.c                |    2 +-
 fs/ocfs2/inode.c                  |    4 +-
 fs/ocfs2/namei.c                  |   18 +++++++---------
 fs/openpromfs/inode.c             |    4 +-
 fs/proc/base.c                    |   12 +++++-----
 fs/proc/generic.c                 |    2 +-
 fs/proc/inode.c                   |    2 +-
 fs/proc/proc_sysctl.c             |    2 +-
 fs/qnx4/inode.c                   |    2 +-
 fs/reiserfs/inode.c               |   10 ++++----
 fs/reiserfs/namei.c               |   16 +++++++-------
 fs/romfs/super.c                  |    2 +-
 fs/squashfs/inode.c               |   18 ++++++++--------
 fs/stack.c                        |    2 +-
 fs/stat.c                         |    5 ++-
 fs/super.c                        |    9 ++++++-
 fs/sysfs/inode.c                  |    2 +-
 fs/sysv/inode.c                   |    2 +-
 fs/ubifs/super.c                  |    2 +-
 fs/ubifs/xattr.c                  |    4 +-
 fs/udf/inode.c                    |    8 ++++--
 fs/udf/namei.c                    |   15 +++++--------
 fs/ufs/ialloc.c                   |    2 +-
 fs/ufs/inode.c                    |    4 +-
 fs/xfs/xfs_iops.c                 |    2 +-
 include/linux/dcache.h            |    8 ++++--
 include/linux/fs.h                |   31 +++++++++++++++++++++++++---
 include/linux/namei.h             |    1 +
 mm/shmem.c                        |    2 +-
 108 files changed, 301 insertions(+), 235 deletions(-)