Improve lseek scalability v3

Improve lseek scalability v3

[Andi Kleen]

Currently generic_file_llseek users synchronize all on the inode i_mutex,
which is very heavy handed because it affects even different processes.

This patchkit attempts to make generic_file_llseek (mostly) lockless.

For details see the individual patches.

v2: Forward ported to recent kernel. Add SEEK_DATA/HOLE support.
Fix a nasty SEEK_END bug in the previous version.
v3: No changes, except rebase. All reviews passed. Just reposting 
for merging.

-Andi

[PATCH 1/7] BTRFS: Fix lseek return value for error

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/btrfs/file.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 3c3abff..7ec0a24 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1818,19 +1818,17 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 	case SEEK_DATA:
 	case SEEK_HOLE:
 		ret = find_desired_extent(inode, &offset, origin);
-		if (ret) {
-			mutex_unlock(&inode->i_mutex);
-			return ret;
-		}
+		if (ret)
+			goto error;
 	}
 
 	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
 		ret = -EINVAL;
-		goto out;
+		goto error;
 	}
 	if (offset > inode->i_sb->s_maxbytes) {
 		ret = -EINVAL;
-		goto out;
+		goto error;
 	}
 
 	/* Special lock needed here? */
@@ -1841,6 +1839,9 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 out:
 	mutex_unlock(&inode->i_mutex);
 	return offset;
+error:
+	mutex_unlock(&inode->i_mutex);
+	return ret;
 }
 
 const struct file_operations btrfs_file_operations = {
-- 
1.7.4.4

[PATCH 2/7] VFS: Do (nearly) lockless generic_file_llseek

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The i_mutex lock use of generic _file_llseek hurts.  Independent processes
accessing the same file synchronize over a single lock, even though
they have no need for synchronization at all.

Under high utilization this can cause llseek to scale very poorly on larger
systems.

This patch does some rethinking of the llseek locking model:

First the 64bit f_pos is not necessarily atomic without locks
on 32bit systems. This can already cause races with read() today.
This was discussed on linux-kernel in the past and deemed acceptable.
The patch does not change that.

Let's look at the different seek variants:

SEEK_SET: Doesn't really need any locking.
If there's a race one writer wins, the other loses.

For 32bit the non atomic update races against read()
stay the same. Without a lock they can also happen
against write() now.  The read() race was deemed
acceptable in past discussions, and I think if it's
ok for read it's ok for write too.

=> Don't need a lock.

SEEK_END: This behaves like SEEK_SET plus it reads
the maximum size too. Reading the maximum size would have the
32bit atomic problem. But luckily we already have a way to read
the maximum size without locking (i_size_read), so we
can just use that instead.

Without i_mutex there is no synchronization with write() anymore,
however since the write() update is atomic on 64bit it just behaves
like another racy SEEK_SET.  On non atomic 32bit it's the same
as SEEK_SET.

=> Don't need a lock, but need to use i_size_read()

SEEK_CUR: This has a read-modify-write race window
on the same file. One could argue that any application
doing unsynchronized seeks on the same file is already broken.
But for the sake of not adding a regression here I'm
using the file->f_lock to synchronize this. Using this
lock is much better than the inode mutex because it doesn't
synchronize between processes.

=> So still need a lock, but can use a cheap local one.

This patch implements this new scheme in generic_file_llseek.
I dropped generic_file_llseek_unlocked and changed all callers.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/btrfs/file.c    |    2 +-
 fs/cifs/cifsfs.c   |    2 +-
 fs/gfs2/file.c     |    4 ++--
 fs/nfs/file.c      |    5 +++--
 fs/read_write.c    |   33 ++++++---------------------------
 include/linux/fs.h |    2 --
 6 files changed, 13 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 7ec0a24..62f56d4 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1813,7 +1813,7 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 	switch (origin) {
 	case SEEK_END:
 	case SEEK_CUR:
-		offset = generic_file_llseek_unlocked(file, offset, origin);
+		offset = generic_file_llseek(file, offset, origin);
 		goto out;
 	case SEEK_DATA:
 	case SEEK_HOLE:
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index f93eb94..e43641b 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -721,7 +721,7 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
 		if (rc < 0)
 			return (loff_t)rc;
 	}
-	return generic_file_llseek_unlocked(file, offset, origin);
+	return generic_file_llseek(file, offset, origin);
 }
 
 static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index edeb9e8..fe6bc02 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -63,11 +63,11 @@ static loff_t gfs2_llseek(struct file *file, loff_t offset, int origin)
 		error = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, LM_FLAG_ANY,
 					   &i_gh);
 		if (!error) {
-			error = generic_file_llseek_unlocked(file, offset, origin);
+			error = generic_file_llseek(file, offset, origin);
 			gfs2_glock_dq_uninit(&i_gh);
 		}
 	} else
-		error = generic_file_llseek_unlocked(file, offset, origin);
+		error = generic_file_llseek(file, offset, origin);
 
 	return error;
 }
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 28b8c3f..12623ab 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -198,11 +198,12 @@ static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
 		if (retval < 0)
 			return (loff_t)retval;
 
+		/* AK: should drop this lock. Unlikely to be needed. */
 		spin_lock(&inode->i_lock);
-		loff = generic_file_llseek_unlocked(filp, offset, origin);
+		loff = generic_file_llseek(filp, offset, origin);
 		spin_unlock(&inode->i_lock);
 	} else
-		loff = generic_file_llseek_unlocked(filp, offset, origin);
+		loff = generic_file_llseek(filp, offset, origin);
 	return loff;
 }
 
diff --git a/fs/read_write.c b/fs/read_write.c
index 179f1c3..24f0001 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -36,22 +36,23 @@ static inline int unsigned_offsets(struct file *file)
 }
 
 /**
- * generic_file_llseek_unlocked - lockless generic llseek implementation
+ * generic_file_llseek - generic llseek implementation for regular files
  * @file:	file structure to seek on
  * @offset:	file offset to seek to
  * @origin:	type of seek
  *
- * Updates the file offset to the value specified by @offset and @origin.
- * Locking must be provided by the caller.
+ * This is a generic implemenation of ->llseek useable for all normal local
+ * filesystems.  It just updates the file offset to the value specified by
+ * @offset and @origin under i_mutex.
  */
 loff_t
-generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
+generic_file_llseek(struct file *file, loff_t offset, int origin)
 {
 	struct inode *inode = file->f_mapping->host;
 
 	switch (origin) {
 	case SEEK_END:
-		offset += inode->i_size;
+		offset += i_size_read(inode);
 		break;
 	case SEEK_CUR:
 		/*
@@ -96,28 +97,6 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
 
 	return offset;
 }
-EXPORT_SYMBOL(generic_file_llseek_unlocked);
-
-/**
- * generic_file_llseek - generic llseek implementation for regular files
- * @file:	file structure to seek on
- * @offset:	file offset to seek to
- * @origin:	type of seek
- *
- * This is a generic implemenation of ->llseek useable for all normal local
- * filesystems.  It just updates the file offset to the value specified by
- * @offset and @origin under i_mutex.
- */
-loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
-{
-	loff_t rval;
-
-	mutex_lock(&file->f_dentry->d_inode->i_mutex);
-	rval = generic_file_llseek_unlocked(file, offset, origin);
-	mutex_unlock(&file->f_dentry->d_inode->i_mutex);
-
-	return rval;
-}
 EXPORT_SYMBOL(generic_file_llseek);
 
 /**
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c2bd68f..d9efdf7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2399,8 +2399,6 @@ file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping);
 extern loff_t noop_llseek(struct file *file, loff_t offset, int origin);
 extern loff_t no_llseek(struct file *file, loff_t offset, int origin);
 extern loff_t generic_file_llseek(struct file *file, loff_t offset, int origin);
-extern loff_t generic_file_llseek_unlocked(struct file *file, loff_t offset,
-			int origin);
 extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
 
-- 
1.7.4.4

[PATCH 3/7] VFS: Make generic lseek lockless safe

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

- use f_lock to protect SEEK_CUR
- use i_size_read to safely read file sizes on 32bit

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/read_write.c    |   52 +++++++++++++++++++++++++++++++++++-----------------
 include/linux/fs.h |    3 ++-
 2 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 24f0001..8e8aab3 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -35,6 +35,21 @@ static inline int unsigned_offsets(struct file *file)
 	return file->f_mode & FMODE_UNSIGNED_OFFSET;
 }
 
+static loff_t lseek_execute(struct file *file, struct inode *inode, loff_t offset,
+			    loff_t maxsize)
+{
+	if (offset < 0 && !unsigned_offsets(file))
+		return -EINVAL;
+	if (offset > maxsize)
+		return -EINVAL;
+
+	if (offset != file->f_pos) {
+		file->f_pos = offset;
+		file->f_version = 0;
+	}
+	return offset;
+}
+
 /**
  * generic_file_llseek - generic llseek implementation for regular files
  * @file:	file structure to seek on
@@ -44,6 +59,12 @@ static inline int unsigned_offsets(struct file *file)
  * This is a generic implemenation of ->llseek useable for all normal local
  * filesystems.  It just updates the file offset to the value specified by
  * @offset and @origin under i_mutex.
+ *
+ * Synchronization:
+ * SEEK_SET is unsynchronized (but atomic on 64bit platforms)
+ * SEEK_CUR is synchronized against other SEEK_CURs, but not read/writes.
+ * read/writes behave like SEEK_SET against seeks.
+ * SEEK_END
  */
 loff_t
 generic_file_llseek(struct file *file, loff_t offset, int origin)
@@ -63,14 +84,22 @@ generic_file_llseek(struct file *file, loff_t offset, int origin)
 		 */
 		if (offset == 0)
 			return file->f_pos;
-		offset += file->f_pos;
-		break;
+		/*
+		 * f_lock protects against read/modify/write race with other
+		 * SEEK_CURs. Note that parallel writes and reads behave
+		 * like SEEK_SET.
+		 */
+		spin_lock(&file->f_lock);
+		offset = lseek_execute(file, inode, file->f_pos + offset, 
+				       inode->i_sb->s_maxbytes);
+		spin_unlock(&file->f_lock);
+		return offset;
 	case SEEK_DATA:
 		/*
 		 * In the generic case the entire file is data, so as long as
 		 * offset isn't at the end of the file then the offset is data.
 		 */
-		if (offset >= inode->i_size)
+		if (offset >= i_size_read(inode))
 			return -ENXIO;
 		break;
 	case SEEK_HOLE:
@@ -78,24 +107,13 @@ generic_file_llseek(struct file *file, loff_t offset, int origin)
 		 * There is a virtual hole at the end of the file, so as long as
 		 * offset isn't i_size or larger, return i_size.
 		 */
-		if (offset >= inode->i_size)
+		if (offset >= i_size_read(inode))
 			return -ENXIO;
-		offset = inode->i_size;
+		offset = i_size_read(inode);
 		break;
 	}
 
-	if (offset < 0 && !unsigned_offsets(file))
-		return -EINVAL;
-	if (offset > inode->i_sb->s_maxbytes)
-		return -EINVAL;
-
-	/* Special lock needed here? */
-	if (offset != file->f_pos) {
-		file->f_pos = offset;
-		file->f_version = 0;
-	}
-
-	return offset;
+	return lseek_execute(file, inode, offset, inode->i_sb->s_maxbytes);
 }
 EXPORT_SYMBOL(generic_file_llseek);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d9efdf7..fdcf2eb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -965,7 +965,8 @@ struct file {
 #define f_dentry	f_path.dentry
 #define f_vfsmnt	f_path.mnt
 	const struct file_operations	*f_op;
-	spinlock_t		f_lock;  /* f_ep_links, f_flags, no IRQ */
+	spinlock_t		f_lock;  /* f_ep_links, f_flags, no IRQ,
+					    SEEK_SET */
 #ifdef CONFIG_SMP
 	int			f_sb_list_cpu;
 #endif
-- 
1.7.4.4

[PATCH 4/7] VFS: Add generic_file_llseek_size

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add a generic_file_llseek variant to the VFS that allows passing in
the maximum file size of the file system, instead of always
using maxbytes from the superblock.

This can be used to eliminate some cut'n'paste seek code in ext4.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/read_write.c    |   34 +++++++++++++++++++++++++++-------
 include/linux/fs.h |    2 ++
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 8e8aab3..3d55cc6 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -51,14 +51,14 @@ static loff_t lseek_execute(struct file *file, struct inode *inode, loff_t offse
 }
 
 /**
- * generic_file_llseek - generic llseek implementation for regular files
+ * generic_file_llseek_size - generic llseek implementation for regular files
  * @file:	file structure to seek on
  * @offset:	file offset to seek to
  * @origin:	type of seek
+ * @size:       max size of file system
  *
- * This is a generic implemenation of ->llseek useable for all normal local
- * filesystems.  It just updates the file offset to the value specified by
- * @offset and @origin under i_mutex.
+ * Variant of generic_file_llseek that allows passing in a custom
+ * file size.
  *
  * Synchronization:
  * SEEK_SET is unsynchronized (but atomic on 64bit platforms)
@@ -67,7 +67,8 @@ static loff_t lseek_execute(struct file *file, struct inode *inode, loff_t offse
  * SEEK_END
  */
 loff_t
-generic_file_llseek(struct file *file, loff_t offset, int origin)
+generic_file_llseek_size(struct file *file, loff_t offset, int origin, 
+                         loff_t maxsize)
 {
 	struct inode *inode = file->f_mapping->host;
 
@@ -91,7 +92,7 @@ generic_file_llseek(struct file *file, loff_t offset, int origin)
 		 */
 		spin_lock(&file->f_lock);
 		offset = lseek_execute(file, inode, file->f_pos + offset, 
-				       inode->i_sb->s_maxbytes);
+				       maxsize);
 		spin_unlock(&file->f_lock);
 		return offset;
 	case SEEK_DATA:
@@ -113,7 +114,26 @@ generic_file_llseek(struct file *file, loff_t offset, int origin)
 		break;
 	}
 
-	return lseek_execute(file, inode, offset, inode->i_sb->s_maxbytes);
+	return lseek_execute(file, inode, offset, maxsize);
+}
+EXPORT_SYMBOL(generic_file_llseek_size);
+
+/**
+ * generic_file_llseek - generic llseek implementation for regular files
+ * @file:	file structure to seek on
+ * @offset:	file offset to seek to
+ * @origin:	type of seek
+ *
+ * This is a generic implemenation of ->llseek useable for all normal local
+ * filesystems.  It just updates the file offset to the value specified by
+ * @offset and @origin under i_mutex.
+ */
+loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
+{
+	struct inode *inode = file->f_mapping->host;
+
+	return generic_file_llseek_size(file, offset, origin, 
+					inode->i_sb->s_maxbytes);
 }
 EXPORT_SYMBOL(generic_file_llseek);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index fdcf2eb..1a592c4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2400,6 +2400,8 @@ file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping);
 extern loff_t noop_llseek(struct file *file, loff_t offset, int origin);
 extern loff_t no_llseek(struct file *file, loff_t offset, int origin);
 extern loff_t generic_file_llseek(struct file *file, loff_t offset, int origin);
+extern loff_t generic_file_llseek_size(struct file *file, loff_t offset, int origin,
+				       loff_t maxsize);
 extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
 
-- 
1.7.4.4

[PATCH 5/7] LSEEK: EXT4: Replace cut'n'pasted llseek code with generic_file_llseek_size

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

This gives ext4 the benefits of unlocked llseek.

Cc: tytso@mit.edu
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/ext4/file.c |   47 +----------------------------------------------
 1 files changed, 1 insertions(+), 46 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index e4095e9..b9548f4 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -224,53 +224,8 @@ loff_t ext4_llseek(struct file *file, loff_t offset, int origin)
 		maxbytes = EXT4_SB(inode->i_sb)->s_bitmap_maxbytes;
 	else
 		maxbytes = inode->i_sb->s_maxbytes;
-	mutex_lock(&inode->i_mutex);
-	switch (origin) {
-	case SEEK_END:
-		offset += inode->i_size;
-		break;
-	case SEEK_CUR:
-		if (offset == 0) {
-			mutex_unlock(&inode->i_mutex);
-			return file->f_pos;
-		}
-		offset += file->f_pos;
-		break;
-	case SEEK_DATA:
-		/*
-		 * In the generic case the entire file is data, so as long as
-		 * offset isn't at the end of the file then the offset is data.
-		 */
-		if (offset >= inode->i_size) {
-			mutex_unlock(&inode->i_mutex);
-			return -ENXIO;
-		}
-		break;
-	case SEEK_HOLE:
-		/*
-		 * There is a virtual hole at the end of the file, so as long as
-		 * offset isn't i_size or larger, return i_size.
-		 */
-		if (offset >= inode->i_size) {
-			mutex_unlock(&inode->i_mutex);
-			return -ENXIO;
-		}
-		offset = inode->i_size;
-		break;
-	}
-
-	if (offset < 0 || offset > maxbytes) {
-		mutex_unlock(&inode->i_mutex);
-		return -EINVAL;
-	}
-
-	if (offset != file->f_pos) {
-		file->f_pos = offset;
-		file->f_version = 0;
-	}
-	mutex_unlock(&inode->i_mutex);
 
-	return offset;
+	return generic_file_llseek_size(file, offset, origin, maxbytes);
 }
 
 const struct file_operations ext4_file_operations = {
-- 
1.7.4.4

[PATCH 6/7] LSEEK: NFS: Drop unnecessary locking in llseek

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

This makes NFS follow the standard generic_file_llseek locking scheme.

Cc: Trond.Myklebust@netapp.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/nfs/file.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 12623ab..91c01f0 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -180,8 +180,6 @@ force_reval:
 
 static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
 {
-	loff_t loff;
-
 	dprintk("NFS: llseek file(%s/%s, %lld, %d)\n",
 			filp->f_path.dentry->d_parent->d_name.name,
 			filp->f_path.dentry->d_name.name,
@@ -197,14 +195,9 @@ static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
 		int retval = nfs_revalidate_file_size(inode, filp);
 		if (retval < 0)
 			return (loff_t)retval;
+	}
 
-		/* AK: should drop this lock. Unlikely to be needed. */
-		spin_lock(&inode->i_lock);
-		loff = generic_file_llseek(filp, offset, origin);
-		spin_unlock(&inode->i_lock);
-	} else
-		loff = generic_file_llseek(filp, offset, origin);
-	return loff;
+	return generic_file_llseek(filp, offset, origin);
 }
 
 /*
-- 
1.7.4.4

[PATCH 7/7] LSEEK: BTRFS: Avoid i_mutex for SEEK_{CUR,SET,END}

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Don't need the i_mutex for those cases, only for SEEK_HOLE/DATA.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/btrfs/file.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 62f56d4..ae7f37e 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1809,18 +1809,13 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 	struct inode *inode = file->f_mapping->host;
 	int ret;
 
+	if (origin != SEEK_DATA && origin != SEEK_HOLE)
+		return generic_file_llseek(file, offset, origin);
+
 	mutex_lock(&inode->i_mutex);
-	switch (origin) {
-	case SEEK_END:
-	case SEEK_CUR:
-		offset = generic_file_llseek(file, offset, origin);
-		goto out;
-	case SEEK_DATA:
-	case SEEK_HOLE:
-		ret = find_desired_extent(inode, &offset, origin);
-		if (ret)
-			goto error;
-	}
+	ret = find_desired_extent(inode, &offset, origin);
+	if (ret)
+		goto error;
 
 	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
 		ret = -EINVAL;
@@ -1836,7 +1831,6 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 		file->f_pos = offset;
 		file->f_version = 0;
 	}
-out:
 	mutex_unlock(&inode->i_mutex);
 	return offset;
 error:
-- 
1.7.4.4

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Thomas Gleixner]

On Thu, 15 Sep 2011, Andi Kleen wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c

How informative! Can you explain what's the actual fix is ???

Just get it:

  Changelogs are important and not just a random collection of
  characters required by some random tool

Thanks,

	tglx

Re: Improve lseek scalability v3

[Matthew Wilcox]

On Thu, Sep 15, 2011 at 04:06:46PM -0700, Andi Kleen wrote:
> Currently generic_file_llseek users synchronize all on the inode i_mutex,
> which is very heavy handed because it affects even different processes.
> 
> This patchkit attempts to make generic_file_llseek (mostly) lockless.

Yes, but, are there any real workloads which care?  I know will-it-scale
says that lseek doesn't scale, but any real app which has a seek-heavy
workload is surely using pread()/pwrite() by now ... after all, they
were in UNIX98 so they've been a standard for 13 years.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Improve lseek scalability v3

[Josef Bacik]

On Fri, Sep 16, 2011 at 07:00:55AM -0600, Matthew Wilcox wrote:
> On Thu, Sep 15, 2011 at 04:06:46PM -0700, Andi Kleen wrote:
> > Currently generic_file_llseek users synchronize all on the inode i_mutex,
> > which is very heavy handed because it affects even different processes.
> > 
> > This patchkit attempts to make generic_file_llseek (mostly) lockless.
> 
> Yes, but, are there any real workloads which care?  I know will-it-scale
> says that lseek doesn't scale, but any real app which has a seek-heavy
> workload is surely using pread()/pwrite() by now ... after all, they
> were in UNIX98 so they've been a standard for 13 years.
>

Apparently postgresql uses lseek heavily

http://rhaas.blogspot.com/2011/08/linux-and-glibc-scalability.html

Thanks,

Josef 

Re: Improve lseek scalability v3

[Andres Freund]

On Friday 16 Sep 2011 15:00:55 Matthew Wilcox wrote:
> On Thu, Sep 15, 2011 at 04:06:46PM -0700, Andi Kleen wrote:
> > Currently generic_file_llseek users synchronize all on the inode i_mutex,
> > which is very heavy handed because it affects even different processes.
> > 
> > This patchkit attempts to make generic_file_llseek (mostly) lockless.
> 
> Yes, but, are there any real workloads which care?  I know will-it-scale
> says that lseek doesn't scale, but any real app which has a seek-heavy
> workload is surely using pread()/pwrite() by now ... after all, they
> were in UNIX98 so they've been a standard for 13 years.
I sent an email containing benchmarks from Robert Haas regarding the Subject. 
Looking at lkml.org I can't see it right now, Will recheck when I am at home.

He replaced lseek(SEEK_END) with fstat() and got speedups up to 8.7 times the 
lseek performance.
The workload was 64 clients hammering postgres with a simple readonly workload 
(pgbench -S).

For reference see the thread in the postgres archives which also links to 
performance data: http://archives.postgresql.org/message-
id/CA+TgmoawRfpan35wzvgHkSJ0+i-W=VkJpKnRxK2kTDR+HsanWA@mail.gmail.com

You likely won't be able to see the bottlenecks with any of the released 
postgres versions as there are bottlenecks fixed in HEAD that throttle way 
before that.

In the other mail I wrote that I forward ported v1 of this patch to v3.1-rc1 
and tested it on the biggest machine I could easily reboot into an 
experimental kernel.
That machine was only a 2 socket x 4 cores (+ht) nehalem machine though. The 
benefits I measured at it were between 1-3% if memory serves right. Its not 
surprising though that the problem is not that visible at such a comparatively 
low concurrency.
In Roberts Tests the concurrency difference started to show at around 40 
clients.

I looked at all the patches (as I said V1 some weeks back) and they looked 
reasonable to me.
My aforementioned machine runs v1 rebased onto newer kernels for the last 
weeks without problems.

Greetings,

Andres

Re: Improve lseek scalability v3

[Andi Kleen]

> I sent an email containing benchmarks from Robert Haas regarding the Subject. 
> Looking at lkml.org I can't see it right now, Will recheck when I am at home.

I never saw that. Thanks for the data.

> You likely won't be able to see the bottlenecks with any of the released 
> postgres versions as there are bottlenecks fixed in HEAD that throttle way 
> before that.

FWIW we did some postgres testing on a 4 socket, but only with the heavily patched 
MOSBENCH version, which fixes various locking problems. Didn't run it with that
patchkit though.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: Improve lseek scalability v3

[Andres Freund]

On Friday 16 Sep 2011 15:00:55 Matthew Wilcox wrote:
> On Thu, Sep 15, 2011 at 04:06:46PM -0700, Andi Kleen wrote:
> > Currently generic_file_llseek users synchronize all on the inode i_mutex,
> > which is very heavy handed because it affects even different processes.
> > 
> > This patchkit attempts to make generic_file_llseek (mostly) lockless.
> 
> Yes, but, are there any real workloads which care?  I know will-it-scale
> says that lseek doesn't scale, but any real app which has a seek-heavy
> workload is surely using pread()/pwrite() by now ... after all, they
> were in UNIX98 so they've been a standard for 13 years.
Postgres uses llseek() to check wether the filesize has changed, 
pread()/pwrite() won't help with that.
An unlocked llseek will likely be faster than fstat(). Especially if the 
latter invokes some LSM module which lseek doesn't do.

Now postgres certainly could start synchronizing the filesize in shared memory 
between processes, but ...

Andres

Re: Improve lseek scalability v3

[Andres Freund]

On Friday 16 Sep 2011 16:23:19 Andi Kleen wrote:
> > I sent an email containing benchmarks from Robert Haas regarding the
> > Subject. Looking at lkml.org I can't see it right now, Will recheck when
> > I am at home.
> I never saw that.
Hm. Not good. I see message in my imap folder marked as replied. I don't keep 
mail-server logs that long though...

> Thanks for the data.
Thats Roberts work though.

> > You likely won't be able to see the bottlenecks with any of the released
> > postgres versions as there are bottlenecks fixed in HEAD that throttle
> > way before that.
> FWIW we did some postgres testing on a 4 socket, but only with the heavily
> patched MOSBENCH version, which fixes various locking problems. Didn't run
> it with that patchkit though.
I don't really now what MOSBENCH changed but I think some of the fixes Robert 
are unlikely to be contained in there because they are more than just a rework 
of locking itself but algorithmic changes on another level.
(Should it interest you its its likely to be  
84e37126770dd6de903dad88ce150a49b63b5ef9, 
b4fbe392f8ff6ff1a66b488eb7197eef9e1770a4, 
3cba8999b343648c4c528432ab3d51400194e93b 
8e5ac74c1249820ca55481223a95b9124b4a4f95, ...)

I also think youre only likely to notice those problems if youre running a 
readonly test. The write-path is currently far to slow to see problems in that 
area.

Greetings,

Andres

Re: Improve lseek scalability v3

[Matthew Wilcox]

On Fri, Sep 16, 2011 at 04:16:49PM +0200, Andres Freund wrote:
> I sent an email containing benchmarks from Robert Haas regarding the Subject. 
> Looking at lkml.org I can't see it right now, Will recheck when I am at home.
> 
> He replaced lseek(SEEK_END) with fstat() and got speedups up to 8.7 times the 
> lseek performance.
> The workload was 64 clients hammering postgres with a simple readonly workload 
> (pgbench -S).

Yay!  Data!

> For reference see the thread in the postgres archives which also links to 
> performance data: http://archives.postgresql.org/message-
> id/CA+TgmoawRfpan35wzvgHkSJ0+i-W=VkJpKnRxK2kTDR+HsanWA@mail.gmail.com

So both fstat and lseek do more work than postgres wants.  lseek modifies
the file pointer while fstat copies all kinds of unnecessary information
into userspace.  I imagine this is the source of the slowdown seen in
the 1-client case.

There have been various proposals to make the amount of information returned
by fstat limited to the 'cheap' (for various definitions of 'cheap') fields.

I'd like to dig into the requirement for knowing the file size a little
better.  According to the blog entry it's used for "the query planner".
Does the query planner need to know the exact number of bytes in the file,
or is it after an order-of-magnitude?  Or to-the-nearest-gigabyte?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Christoph Hellwig]

On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c

I think this should go to Chris/Linus ASAP.  But a slightly better
patch description wouldn't hurt either.

Also any reason you captialize BTRFS?

> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  fs/btrfs/file.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 3c3abff..7ec0a24 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1818,19 +1818,17 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
>  	case SEEK_DATA:
>  	case SEEK_HOLE:
>  		ret = find_desired_extent(inode, &offset, origin);
> -		if (ret) {
> -			mutex_unlock(&inode->i_mutex);
> -			return ret;
> -		}
> +		if (ret)
> +			goto error;
>  	}
>  
>  	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
>  		ret = -EINVAL;
> -		goto out;
> +		goto error;
>  	}
>  	if (offset > inode->i_sb->s_maxbytes) {
>  		ret = -EINVAL;
> -		goto out;
> +		goto error;
>  	}
>  
>  	/* Special lock needed here? */
> @@ -1841,6 +1839,9 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
>  out:
>  	mutex_unlock(&inode->i_mutex);
>  	return offset;
> +error:
> +	mutex_unlock(&inode->i_mutex);
> +	return ret;
>  }
>  
>  const struct file_operations btrfs_file_operations = {
> -- 
> 1.7.4.4
> 
> --
> 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
---end quoted text---

Re: [PATCH 4/7] VFS: Add generic_file_llseek_size

[Christoph Hellwig]

On Thu, Sep 15, 2011 at 04:06:50PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Add a generic_file_llseek variant to the VFS that allows passing in
> the maximum file size of the file system, instead of always
> using maxbytes from the superblock.
> 
> This can be used to eliminate some cut'n'paste seek code in ext4.

I'd reorder this to the front of the series.  Also I think this
helper could be used to implement default_llseek.

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Andi Kleen]

On Fri, Sep 16, 2011 at 11:48:15AM -0400, Christoph Hellwig wrote:
> On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c
> 
> I think this should go to Chris/Linus ASAP.  But a slightly better
> patch description wouldn't hurt either.

Josef acked it earlier, but with a missing Chris the btrfs merge
pipeline seems to be disfunctional at the moment.

-Andi

Re: Improve lseek scalability v3

[Andres Freund]

Hi,
On Friday 16 Sep 2011 17:36:20 Matthew Wilcox wrote:
> On Fri, Sep 16, 2011 at 04:16:49PM +0200, Andres Freund wrote:
> > I sent an email containing benchmarks from Robert Haas regarding the
> > Subject. Looking at lkml.org I can't see it right now, Will recheck when
> > I am at home.
> > 
> > He replaced lseek(SEEK_END) with fstat() and got speedups up to 8.7 times
> > the lseek performance.
> > The workload was 64 clients hammering postgres with a simple readonly
> > workload (pgbench -S).
> Yay!  Data!

> > For reference see the thread in the postgres archives which also links to
> > performance data: http://archives.postgresql.org/message-
> > id/CA+TgmoawRfpan35wzvgHkSJ0+i-W=VkJpKnRxK2kTDR+HsanWA@mail.gmail.com
> So both fstat and lseek do more work than postgres wants.  lseek modifies
> the file pointer while fstat copies all kinds of unnecessary information
> into userspace.  I imagine this is the source of the slowdown seen in
> the 1-client case.
Yes, that was my theory as well.

> I'd like to dig into the requirement for knowing the file size a little
> better.  According to the blog entry it's used for "the query planner".
Its used for multiple things - one of which is the query planner.
The query planner needs to know how many tuples a table has to produce a 
sensible plan. For that is has stats which tell 1. how big is the table 2. how 
many tuples does the table have. Those statistics are only updated every now 
and then though.
So it uses those old stats to check how many tuples are normally stored on a 
page and then uses that to extrapolate the number of tuples from the current 
nr of pages (which is computed by lseek(SEEK_END) over the 1GB segements of a 
table).

I am not sure how interested you are on the relevant postgres internals?

> Does the query planner need to know the exact number of bytes in the file,
> or is it after an order-of-magnitude?  Or to-the-nearest-gigabyte?
It depends on where the information is used. For some of the uses it needs to 
be exact (the assumed size is rechecked after acquiring a lock preventing 
extension) at other places I guess it would be ok if the accuracy got lower 
with bigger files (those files won't ever get bigger than 1GB).
But I have a hard time seeing an implementation where the approximate size 
would be faster to get than just the filesize? 

Andres

Re: [HACKERS] Improve lseek scalability v3

[Alvaro Herrera]

Excerpts from Andres Freund's message of vie sep 16 14:27:33 -0300 2011:
> Hi,
> On Friday 16 Sep 2011 17:36:20 Matthew Wilcox wrote:

> > Does the query planner need to know the exact number of bytes in the file,
> > or is it after an order-of-magnitude?  Or to-the-nearest-gigabyte?
> It depends on where the information is used. For some of the uses it needs to 
> be exact (the assumed size is rechecked after acquiring a lock preventing 
> extension) at other places I guess it would be ok if the accuracy got lower 
> with bigger files (those files won't ever get bigger than 1GB).

One other thing we're interested in is portability.  I mean, even if
Linux were to introduce a new hypothetical syscall that was able to
return the file size at a ridiculously low cost, we probably wouldn't
use it because it'd be Linux-specific.  So an improvement of lseek()
seems to be the best option.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: Improve lseek scalability v3

[Alvaro Herrera]

Excerpts from Andres Freund's message of vie sep 16 14:27:33 -0300 2011:
> Hi,
> On Friday 16 Sep 2011 17:36:20 Matthew Wilcox wrote:

> > Does the query planner need to know the exact number of bytes in the file,
> > or is it after an order-of-magnitude?  Or to-the-nearest-gigabyte?
> It depends on where the information is used. For some of the uses it needs to 
> be exact (the assumed size is rechecked after acquiring a lock preventing 
> extension) at other places I guess it would be ok if the accuracy got lower 
> with bigger files (those files won't ever get bigger than 1GB).

One other thing we're interested in is portability.  I mean, even if
Linux were to introduce a new hypothetical syscall that was able to
return the file size at a ridiculously low cost, we probably wouldn't
use it because it'd be Linux-specific.  So an improvement of lseek()
seems to be the best option.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve lseek scalability v3

[Andi Kleen]

> One other thing we're interested in is portability.  I mean, even if
> Linux were to introduce a new hypothetical syscall that was able to
> return the file size at a ridiculously low cost, we probably wouldn't
> use it because it'd be Linux-specific.  So an improvement of lseek()
> seems to be the best option.

Fully agreed. It doesn't make any sense at all to implement special
syscalls just to workaround a basic system call not scaling.

-Andi

Re: Improve lseek scalability v3

[Benjamin LaHaise]

On Fri, Sep 16, 2011 at 07:27:33PM +0200, Andres Freund wrote:
> many tuples does the table have. Those statistics are only updated every now 
> and then though.
> So it uses those old stats to check how many tuples are normally stored on a 
> page and then uses that to extrapolate the number of tuples from the current 
> nr of pages (which is computed by lseek(SEEK_END) over the 1GB segements of a 
> table).
> 
> I am not sure how interested you are on the relevant postgres internals?

For such tables, can't Postgres track the size of the file internally?  I'm 
assuming it's keeping file descriptors open on the tables it manages, in 
which case when it writes to a file to extend it, the internally stored size 
could be updated.  Not making a syscall at all would scale far better than 
even a modified lseek() will perform.

Granted, I've not looked at the Postgres code at all.

		-ben

Re: Improve lseek scalability v3

[Andres Freund]

On Friday, September 16, 2011 10:08:17 PM Benjamin LaHaise wrote:
> On Fri, Sep 16, 2011 at 07:27:33PM +0200, Andres Freund wrote:
> > many tuples does the table have. Those statistics are only updated every
> > now and then though.
> > So it uses those old stats to check how many tuples are normally stored
> > on a page and then uses that to extrapolate the number of tuples from
> > the current nr of pages (which is computed by lseek(SEEK_END) over the
> > 1GB segements of a table).
> > 
> > I am not sure how interested you are on the relevant postgres internals?
> 
> For such tables, can't Postgres track the size of the file internally?  I'm
> assuming it's keeping file descriptors open on the tables it manages, in
> which case when it writes to a file to extend it, the internally stored
> size could be updated.  Not making a syscall at all would scale far better
> than even a modified lseek() will perform.
Yes, it tracks the fds internally. The problem is that postgres is process 
based so those tables are not reachable by all processes. It could start 
tracking those in shared memory but the synchronization overhead for that 
would likely be more expensive than the syscall overhead (Given that the 
fdsets are possibly (and realistically) disjunct between the individual 
backends you would have to reserve enough shared memory for a fully seperate 
fds between each process... Which would complicate efficient lookup).

Also with fstat() instead of lseek() there was no bottleneck anymore, so I 
don't think the benefits would warrant that.

Greetings,

Andres

Re: [HACKERS] Improve lseek scalability v3

[Andres Freund]

On Friday, September 16, 2011 11:02:38 PM Andres Freund wrote:
> Also with fstat() instead of lseek() there was no bottleneck anymore, so I 
> don't think the benefits would warrant that.
At least thats what I observed on a 4 x 6 machine without the patch applied 
(can't reboot it). That shouldn't be concurrency relevant so...

Andres

Re: Improve lseek scalability v3

[Greg Stark]

On Fri, Sep 16, 2011 at 9:08 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> For such tables, can't Postgres track the size of the file internally?  I'm
> assuming it's keeping file descriptors open on the tables it manages, in
> which case when it writes to a file to extend it, the internally stored size
> could be updated.  Not making a syscall at all would scale far better than
> even a modified lseek() will perform.

There's no hardwired limit on how many tables you can have in a
database, it's not limited by the number of file descriptors. Postgres
would have to keep some kind of LRU for recently opened files and
their sizes or something like that. There would probably still be a
lot of lseeks/fstats going on.

Generally keeping a Postgres cached value for the size would then have
a reliability issue. It's much safer to have a single authoritative
value -- the actual length of the file -- than have the same value
stored in two locations and then need to worry about them getting out
of sync. If a write fails when extending the file due to a filesystem
running out of space then Postgres might not know how to update its
internal cached state accurately for example.

There's no question it could be done but it's not clear it would
necessarily be much faster than a lock-free lseek/fstat.

On Fri, Sep 16, 2011 at 6:27 PM, Andres Freund <andres@anarazel.de> wrote:
> It depends on where the information is used. For some of the uses it needs to
> be exact (the assumed size is rechecked after acquiring a lock preventing
> extension)

Fwiw this might give the wrong impression. I don't believe scans
acquire a lock preventing extension -- that is another process can be
concurrently extending the file at the same time as the scan is
proceeding. The scan only locks out truncation (vacuum). Any blocks
added by another process are ignored by the scan because they can only
contain records invisible to that transaction. This does depend on the
lseek/fstat being done after the transaction snapshot is taken which
is possibly "rechecking" the value taken by the query planner but
they're really two independent things.


-- 
greg

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Jeff Liu]

I once posted a similar patch for this issue which can be found at:
http://www.spinics.net/lists/linux-btrfs/msg12169.html

with an additional improvement if the offset is larger or equal to the
file size, return -ENXIO in directly:

if (offset >= inode->i_size) {
                        mutex_unlock(&inode->i_mutex);
                        return -ENXIO;
                }


Thanks,
-Jeff
On 09/16/2011 11:48 PM, Christoph Hellwig wrote:

> On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c
> 
> I think this should go to Chris/Linus ASAP.  But a slightly better
> patch description wouldn't hurt either.
> 
> Also any reason you captialize BTRFS?
> 
>>
>> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>> ---
>>  fs/btrfs/file.c |   13 +++++++------
>>  1 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index 3c3abff..7ec0a24 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -1818,19 +1818,17 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
>>  	case SEEK_DATA:
>>  	case SEEK_HOLE:
>>  		ret = find_desired_extent(inode, &offset, origin);
>> -		if (ret) {
>> -			mutex_unlock(&inode->i_mutex);
>> -			return ret;
>> -		}
>> +		if (ret)
>> +			goto error;
>>  	}
>>  
>>  	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
>>  		ret = -EINVAL;
>> -		goto out;
>> +		goto error;
>>  	}
>>  	if (offset > inode->i_sb->s_maxbytes) {
>>  		ret = -EINVAL;
>> -		goto out;
>> +		goto error;
>>  	}
>>  
>>  	/* Special lock needed here? */
>> @@ -1841,6 +1839,9 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
>>  out:
>>  	mutex_unlock(&inode->i_mutex);
>>  	return offset;
>> +error:
>> +	mutex_unlock(&inode->i_mutex);
>> +	return ret;
>>  }
>>  
>>  const struct file_operations btrfs_file_operations = {
>> -- 
>> 1.7.4.4
>>
>> --
>> 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
> ---end quoted text---
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Andreas Dilger]

On 2011-09-17, at 12:10 AM, Jeff Liu <jeff.liu@oracle.com> wrote:
> I once posted a similar patch for this issue which can be found at:
> http://www.spinics.net/lists/linux-btrfs/msg12169.html
> 
> with an additional improvement if the offset is larger or equal to the
> file size, return -ENXIO in directly:
> 
>                if (offset >= inode->i_size) {
>                        mutex_unlock(&inode->i_mutex);
>                        return -ENXIO;
>                }

Except that is wrong, because it would then be impossible to write sparse files. 

> Thanks,
> -Jeff
> On 09/16/2011 11:48 PM, Christoph Hellwig wrote:
> 
>> On Thu, Sep 15, 2011 at 04:06:47PM -0700, Andi Kleen wrote:
>>> From: Andi Kleen <ak@linux.intel.com>
>>> 
>>> Introduced by 9a4327ca1f45f82edad7dc0a4e52ce9316e0950c
>> 
>> I think this should go to Chris/Linus ASAP.  But a slightly better
>> patch description wouldn't hurt either.
>> 
>> Also any reason you captialize BTRFS?
>> 
>>> 
>>> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>>> ---
>>> fs/btrfs/file.c |   13 +++++++------
>>> 1 files changed, 7 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>>> index 3c3abff..7ec0a24 100644
>>> --- a/fs/btrfs/file.c
>>> +++ b/fs/btrfs/file.c
>>> @@ -1818,19 +1818,17 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
>>>    case SEEK_DATA:
>>>    case SEEK_HOLE:
>>>        ret = find_desired_extent(inode, &offset, origin);
>>> -        if (ret) {
>>> -            mutex_unlock(&inode->i_mutex);
>>> -            return ret;
>>> -        }
>>> +        if (ret)
>>> +            goto error;
>>>    }
>>> 
>>>    if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
>>>        ret = -EINVAL;
>>> -        goto out;
>>> +        goto error;
>>>    }
>>>    if (offset > inode->i_sb->s_maxbytes) {
>>>        ret = -EINVAL;
>>> -        goto out;
>>> +        goto error;
>>>    }
>>> 
>>>    /* Special lock needed here? */
>>> @@ -1841,6 +1839,9 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
>>> out:
>>>    mutex_unlock(&inode->i_mutex);
>>>    return offset;
>>> +error:
>>> +    mutex_unlock(&inode->i_mutex);
>>> +    return ret;
>>> }
>>> 
>>> const struct file_operations btrfs_file_operations = {
>>> -- 
>>> 1.7.4.4
>>> 
>>> --
>>> 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
>> ---end quoted text---
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> 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: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Andi Kleen]

> > with an additional improvement if the offset is larger or equal to the
> > file size, return -ENXIO in directly:
> > 
> >                if (offset >= inode->i_size) {
> >                        mutex_unlock(&inode->i_mutex);
> >                        return -ENXIO;
> >                }
> 
> Except that is wrong, because it would then be impossible to write sparse files. 

And also i_size must be always read with i_size_read()

Anyways clearly there's a problem in btrfs land with merging fixes in time.
Is anyone collecting patches while Chris is gone?

-Andi

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Jeff Liu]

Hi Andreas and Andi,

Thanks for your comments.

On 09/18/2011 09:46 AM, Andi Kleen wrote:

>>> with an additional improvement if the offset is larger or equal to the
>>> file size, return -ENXIO in directly:
>>>
>>>                if (offset >= inode->i_size) {
>>>                        mutex_unlock(&inode->i_mutex);
>>>                        return -ENXIO;
>>>                }
>>
>> Except that is wrong, because it would then be impossible to write sparse files. 

Per my tryout, except that, if the offset >= source file size, call
lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return
the total file size rather than -ENXIO.  however, our desired result it
-ENXIO in this case, Am I right?

> 
> And also i_size must be always read with i_size_read()

Thanks for pointing this out!
Would you please kindly review the revised as below?

Signed-off-by: Jie Liu <jeff.liu@oracle.com>

---
 fs/btrfs/file.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index e7872e4..40c1ef3 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1813,6 +1813,11 @@ static loff_t btrfs_file_llseek(struct file
*file, loff_t offset, int origin)
 		goto out;
 	case SEEK_DATA:
 	case SEEK_HOLE:
+		if (offset >= i_size_read(inode)) {
+			mutex_unlock(&inode->i_mutex);
+			return -ENXIO;
+		}
+
 		ret = find_desired_extent(inode, &offset, origin);
 		if (ret) {
 			mutex_unlock(&inode->i_mutex);
@@ -1821,11 +1826,11 @@ static loff_t btrfs_file_llseek(struct file
*file, loff_t offset, int origin)
 	}

 	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
-		ret = -EINVAL;
+		offset = -EINVAL;
 		goto out;
 	}
 	if (offset > inode->i_sb->s_maxbytes) {
-		ret = -EINVAL;
+		offset = -EINVAL;
 		goto out;
 	}

-- 
1.7.4.1

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Marco Stornelli]

Il 18/09/2011 09:29, Jeff Liu ha scritto:
> Hi Andreas and Andi,
>
> Thanks for your comments.
>
> On 09/18/2011 09:46 AM, Andi Kleen wrote:
>
>>>> with an additional improvement if the offset is larger or equal to the
>>>> file size, return -ENXIO in directly:
>>>>
>>>>                 if (offset>= inode->i_size) {
>>>>                         mutex_unlock(&inode->i_mutex);
>>>>                         return -ENXIO;
>>>>                 }
>>>
>>> Except that is wrong, because it would then be impossible to write sparse files.
>
> Per my tryout, except that, if the offset>= source file size, call
> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return
> the total file size rather than -ENXIO.  however, our desired result it
> -ENXIO in this case, Am I right?
>

Yes, ENXIO should be the operation result.

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Jeff liu]

=D4=DA 2011-9-18=A3=AC=CF=C2=CE=E74:42=A3=AC Marco Stornelli =D0=B4=B5=C0=
=A3=BA

> Il 18/09/2011 09:29, Jeff Liu ha scritto:
>> Hi Andreas and Andi,
>>=20
>> Thanks for your comments.
>>=20
>> On 09/18/2011 09:46 AM, Andi Kleen wrote:
>>=20
>>>>> with an additional improvement if the offset is larger or equal t=
o the
>>>>> file size, return -ENXIO in directly:
>>>>>=20
>>>>>                if (offset>=3D inode->i_size) {
>>>>>                        mutex_unlock(&inode->i_mutex);
>>>>>                        return -ENXIO;
>>>>>                }
>>>>=20
>>>> Except that is wrong, because it would then be impossible to write=
 sparse files.
>>=20
>> Per my tryout, except that, if the offset>=3D source file size, call
>> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always ret=
urn
>> the total file size rather than -ENXIO.  however, our desired result=
 it
>> -ENXIO in this case, Am I right?
>>=20
>=20
> Yes, ENXIO should be the operation result.

Thanks for your kind confirmation.


-Jeff

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Jeff liu]

在 2011-9-18，下午4:42， Marco Stornelli 写道：

> Il 18/09/2011 09:29, Jeff Liu ha scritto:
>> Hi Andreas and Andi,
>> 
>> Thanks for your comments.
>> 
>> On 09/18/2011 09:46 AM, Andi Kleen wrote:
>> 
>>>>> with an additional improvement if the offset is larger or equal to the
>>>>> file size, return -ENXIO in directly:
>>>>> 
>>>>>                if (offset>= inode->i_size) {
>>>>>                        mutex_unlock(&inode->i_mutex);
>>>>>                        return -ENXIO;
>>>>>                }
>>>> 
>>>> Except that is wrong, because it would then be impossible to write sparse files.
>> 
>> Per my tryout, except that, if the offset>= source file size, call
>> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return
>> the total file size rather than -ENXIO.  however, our desired result it
>> -ENXIO in this case, Am I right?
>> 
> 
> Yes, ENXIO should be the operation result.

Thanks for your kind confirmation.


-Jeff

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Chris Mason]

Excerpts from Jeff liu's message of 2011-09-18 06:33:38 -0400:
>=20
> =E5=9C=A8 2011-9-18=EF=BC=8C=E4=B8=8B=E5=8D=884:42=EF=BC=8C Marco Sto=
rnelli =E5=86=99=E9=81=93=EF=BC=9A
>=20
> > Il 18/09/2011 09:29, Jeff Liu ha scritto:
> >> Hi Andreas and Andi,
> >>=20
> >> Thanks for your comments.
> >>=20
> >> On 09/18/2011 09:46 AM, Andi Kleen wrote:
> >>=20
> >>>>> with an additional improvement if the offset is larger or equal=
 to the
> >>>>> file size, return -ENXIO in directly:
> >>>>>=20
> >>>>>                if (offset>=3D inode->i_size) {
> >>>>>                        mutex_unlock(&inode->i_mutex);
> >>>>>                        return -ENXIO;
> >>>>>                }
> >>>>=20
> >>>> Except that is wrong, because it would then be impossible to wri=
te sparse files.
> >>=20
> >> Per my tryout, except that, if the offset>=3D source file size, ca=
ll
> >> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always r=
eturn
> >> the total file size rather than -ENXIO.  however, our desired resu=
lt it
> >> -ENXIO in this case, Am I right?
> >>=20
> >=20
> > Yes, ENXIO should be the operation result.
>=20
> Thanks for your kind confirmation.

Thanks everyone, I've put Jeff's last version of this in my queue.

-chris

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Chris Mason]

Excerpts from Jeff liu's message of 2011-09-18 06:33:38 -0400:
> 
> 在 2011-9-18，下午4:42， Marco Stornelli 写道：
> 
> > Il 18/09/2011 09:29, Jeff Liu ha scritto:
> >> Hi Andreas and Andi,
> >> 
> >> Thanks for your comments.
> >> 
> >> On 09/18/2011 09:46 AM, Andi Kleen wrote:
> >> 
> >>>>> with an additional improvement if the offset is larger or equal to the
> >>>>> file size, return -ENXIO in directly:
> >>>>> 
> >>>>>                if (offset>= inode->i_size) {
> >>>>>                        mutex_unlock(&inode->i_mutex);
> >>>>>                        return -ENXIO;
> >>>>>                }
> >>>> 
> >>>> Except that is wrong, because it would then be impossible to write sparse files.
> >> 
> >> Per my tryout, except that, if the offset>= source file size, call
> >> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return
> >> the total file size rather than -ENXIO.  however, our desired result it
> >> -ENXIO in this case, Am I right?
> >> 
> > 
> > Yes, ENXIO should be the operation result.
> 
> Thanks for your kind confirmation.

Thanks everyone, I've put Jeff's last version of this in my queue.

-chris

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Chris Mason]

Excerpts from Jeff liu's message of 2011-09-18 06:33:38 -0400:
> 
> 在 2011-9-18，下午4:42， Marco Stornelli 写道：
> 
> > Il 18/09/2011 09:29, Jeff Liu ha scritto:
> >> Hi Andreas and Andi,
> >> 
> >> Thanks for your comments.
> >> 
> >> On 09/18/2011 09:46 AM, Andi Kleen wrote:
> >> 
> >>>>> with an additional improvement if the offset is larger or equal to the
> >>>>> file size, return -ENXIO in directly:
> >>>>> 
> >>>>>                if (offset>= inode->i_size) {
> >>>>>                        mutex_unlock(&inode->i_mutex);
> >>>>>                        return -ENXIO;
> >>>>>                }
> >>>> 
> >>>> Except that is wrong, because it would then be impossible to write sparse files.
> >> 
> >> Per my tryout, except that, if the offset>= source file size, call
> >> lseek(fd, offset, SEEK_DATA/SEEK_HOLE) against Btrfs will always return
> >> the total file size rather than -ENXIO.  however, our desired result it
> >> -ENXIO in this case, Am I right?
> >> 
> > 
> > Yes, ENXIO should be the operation result.
> 
> Thanks for your kind confirmation.

Thanks everyone, I've put Jeff's last version of this in my queue.

-chris

Re: [HACKERS] Improve lseek scalability v3

[Stephen Frost]

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

* Benjamin LaHaise (bcrl@kvack.org) wrote:
> For such tables, can't Postgres track the size of the file internally?  I'm 
> assuming it's keeping file descriptors open on the tables it manages, in 
> which case when it writes to a file to extend it, the internally stored size 
> could be updated.  Not making a syscall at all would scale far better than 
> even a modified lseek() will perform.

We'd have to have it in shared memory and have a lock around it, it
wouldn't be cheap at all.

	Thanks,
		
		Stephen

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

Re: Improve lseek scalability v3

[Stephen Frost]

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

* Benjamin LaHaise (bcrl@kvack.org) wrote:
> For such tables, can't Postgres track the size of the file internally?  I'm 
> assuming it's keeping file descriptors open on the tables it manages, in 
> which case when it writes to a file to extend it, the internally stored size 
> could be updated.  Not making a syscall at all would scale far better than 
> even a modified lseek() will perform.

We'd have to have it in shared memory and have a lock around it, it
wouldn't be cheap at all.

	Thanks,
		
		Stephen

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

Re: [HACKERS] Improve lseek scalability v3

[Matthew Wilcox]

On Mon, Sep 19, 2011 at 08:31:00AM -0400, Stephen Frost wrote:
> * Benjamin LaHaise (bcrl@kvack.org) wrote:
> > For such tables, can't Postgres track the size of the file internally?  I'm 
> > assuming it's keeping file descriptors open on the tables it manages, in 
> > which case when it writes to a file to extend it, the internally stored size 
> > could be updated.  Not making a syscall at all would scale far better than 
> > even a modified lseek() will perform.
> 
> We'd have to have it in shared memory and have a lock around it, it
> wouldn't be cheap at all.

Yep, that makes perfect sense.  After all, the kernel does basically the
same thing to maintain this information; why should we have userspace
duplicating the same infrastructure?

I must admit, I'd never heard of this usage of lseek to get the current
size of a file before; I'd assumed everybody used fstat.  Given this
legitimate reason for a high-frequency calling of lseek, I withdraw my
earlier objection to the patch series.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [HACKERS] Improve lseek scalability v3

[Robert Haas]

On Mon, Sep 19, 2011 at 8:31 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Benjamin LaHaise (bcrl@kvack.org) wrote:
>> For such tables, can't Postgres track the size of the file internally?  I'm
>> assuming it's keeping file descriptors open on the tables it manages, in
>> which case when it writes to a file to extend it, the internally stored size
>> could be updated.  Not making a syscall at all would scale far better than
>> even a modified lseek() will perform.
>
> We'd have to have it in shared memory and have a lock around it, it
> wouldn't be cheap at all.

In theory, we could implement a lock-free cache.  But I still think it
would be better to see this fixed on the kernel side.  If we had some
evidence that all of those lseek() calls were a performance problem
even when the i_mutex is not seriously contended, then that would be a
good argument for doing this in user-space, but I haven't seen any
such evidence.  On the other hand, the numbers I posted show that when
i_mutex IS contended, it can cause a throughput regression of up to
90%.  That seems worth fixing.  If it turns out that lseek() is too
expensive even in the uncontended case or with the i_mutex contention
removed (or if the Linux community is unwilling to accept the proposed
fix), then we can (and should) look at further optimizing it within
PostgreSQL.  My guess, though, is that an unlocked lseek will be fast
enough that we won't need to worry about installing our own caching
infrastructure (or at least, there will be plenty of more significant
performance problems to hunt down first).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Andi Kleen]

> Thanks everyone, I've put Jeff's last version of this in my queue.

Can you post the version you merged? The previous ones all had issues.

-Andi

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Chris Mason]

Excerpts from Andi Kleen's message of 2011-09-19 13:52:03 -0400:
> > Thanks everyone, I've put Jeff's last version of this in my queue.
> 
> Can you post the version you merged? The previous ones all had issues.

https://github.com/chrismason/linux/commit/48802c8ae2a9d618ec734a61283d645ad527e06c

This was the last one sent, I thought it combined all the fixes.


commit 48802c8ae2a9d618ec734a61283d645ad527e06c
Author: Jeff Liu <jeff.liu@oracle.com>
Date:   Sun Sep 18 10:34:02 2011 -0400

    BTRFS: Fix lseek return value for error
    
    The recent reworking of btrfs' lseek lead to incorrect
    values being returned.  This adds checks for seeking
    beyond EOF in SEEK_HOLE and makes sure the error
    values come back correct.
    
    Andi Kleen also sent in similar patches.
    
    Signed-off-by: Jie Liu <jeff.liu@oracle.com>
    Reported-by: Andi Kleen <ak@linux.intel.com>
    Signed-off-by: Chris Mason <chris.mason@oracle.com>

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 3c3abff..a381cd2 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1817,6 +1817,11 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 		goto out;
 	case SEEK_DATA:
 	case SEEK_HOLE:
+		if (offset >= i_size_read(inode)) {
+			mutex_unlock(&inode->i_mutex);
+			return -ENXIO;
+		}
+
 		ret = find_desired_extent(inode, &offset, origin);
 		if (ret) {
 			mutex_unlock(&inode->i_mutex);
@@ -1825,11 +1830,11 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 	}
 
 	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
-		ret = -EINVAL;
+		offset = -EINVAL;
 		goto out;
 	}
 	if (offset > inode->i_sb->s_maxbytes) {
-		ret = -EINVAL;
+		offset = -EINVAL;
 		goto out;
 	}
 

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Andi Kleen]

On Mon, Sep 19, 2011 at 03:30:02PM -0400, Chris Mason wrote:
> Excerpts from Andi Kleen's message of 2011-09-19 13:52:03 -0400:
> > > Thanks everyone, I've put Jeff's last version of this in my queue.
> > 
> > Can you post the version you merged? The previous ones all had issues.
> 
> https://github.com/chrismason/linux/commit/48802c8ae2a9d618ec734a61283d645ad527e06c
> 
> This was the last one sent, I thought it combined all the fixes.

Ok looks good, but it will be all obsolete once my patchkit lseek
get in (except for the SEEK_DATA/HOLE hunk)

-Andi

Re: [PATCH 1/7] BTRFS: Fix lseek return value for error

[Chris Mason]

Excerpts from Andi Kleen's message of 2011-09-19 15:59:52 -0400:
> On Mon, Sep 19, 2011 at 03:30:02PM -0400, Chris Mason wrote:
> > Excerpts from Andi Kleen's message of 2011-09-19 13:52:03 -0400:
> > > > Thanks everyone, I've put Jeff's last version of this in my queue.
> > > 
> > > Can you post the version you merged? The previous ones all had issues.
> > 
> > https://github.com/chrismason/linux/commit/48802c8ae2a9d618ec734a61283d645ad527e06c
> > 
> > This was the last one sent, I thought it combined all the fixes.
> 
> Ok looks good, but it will be all obsolete once my patchkit lseek
> get in (except for the SEEK_DATA/HOLE hunk)

Yeah, it's similar to yours except for the hole hunk.  Your gotos are
fine by me, whatever makes your patch cleaner.

-chris

Re: [HACKERS] Improve lseek scalability v3

[Marco Stornelli]

2011/9/19 Matthew Wilcox <matthew@wil.cx>:
> On Mon, Sep 19, 2011 at 08:31:00AM -0400, Stephen Frost wrote:
>> * Benjamin LaHaise (bcrl@kvack.org) wrote:
>> > For such tables, can't Postgres track the size of the file internally?  I'm
>> > assuming it's keeping file descriptors open on the tables it manages, in
>> > which case when it writes to a file to extend it, the internally stored size
>> > could be updated.  Not making a syscall at all would scale far better than
>> > even a modified lseek() will perform.
>>
>> We'd have to have it in shared memory and have a lock around it, it
>> wouldn't be cheap at all.
>
> Yep, that makes perfect sense.  After all, the kernel does basically the
> same thing to maintain this information; why should we have userspace
> duplicating the same infrastructure?
>
> I must admit, I'd never heard of this usage of lseek to get the current
> size of a file before; I'd assumed everybody used fstat.  Given this
> legitimate reason for a high-frequency calling of lseek, I withdraw my
> earlier objection to the patch series.
>
> --
> Matthew Wilcox                          Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."
> --
> 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
>

I really don't understand the approach here. An improvement is an
improvement, do we need a use case to add an improvement to the
kernel? We are not talking about to add a new syscall or to do an ABI
change in this case. So my absolute ack to these patches.

Marco

Re: [HACKERS] Improve lseek scalability v3

[Marco Stornelli]

2011/9/19 Matthew Wilcox <matthew@wil.cx>:
> On Mon, Sep 19, 2011 at 08:31:00AM -0400, Stephen Frost wrote:
>> * Benjamin LaHaise (bcrl@kvack.org) wrote:
>> > For such tables, can't Postgres track the size of the file internally?  I'm
>> > assuming it's keeping file descriptors open on the tables it manages, in
>> > which case when it writes to a file to extend it, the internally stored size
>> > could be updated.  Not making a syscall at all would scale far better than
>> > even a modified lseek() will perform.
>>
>> We'd have to have it in shared memory and have a lock around it, it
>> wouldn't be cheap at all.
>
> Yep, that makes perfect sense.  After all, the kernel does basically the
> same thing to maintain this information; why should we have userspace
> duplicating the same infrastructure?
>
> I must admit, I'd never heard of this usage of lseek to get the current
> size of a file before; I'd assumed everybody used fstat.  Given this
> legitimate reason for a high-frequency calling of lseek, I withdraw my
> earlier objection to the patch series.
>
> --
> Matthew Wilcox                          Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."
> --
> 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
>

I really don't understand the approach here. An improvement is an
improvement, do we need a use case to add an improvement to the
kernel? We are not talking about to add a new syscall or to do an ABI
change in this case. So my absolute ack to these patches.

Marco
--
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: Improve lseek scalability v3

[Andres Freund]

Hi,

On Friday, September 16, 2011 01:06:46 AM Andi Kleen wrote:
> v3: No changes, except rebase. All reviews passed. Just reposting
> for merging.
Is anything/anyone still objecting to this patchset?

I just retested it ontop of v3.1-rc8 minus the btrfs parts (which don't apply 
cleanly anymore because a modified version of 1/7 was merged) and it works 
fine for some hours of fs heavy db using benchmarking/development.

Following is a seemingly trivial forward-port of 7/7. But since I have 
about no clue in fs development and even less about brfts - which I never used -
take it with a grain of salt.
It seems a bit ugly to have the mutex_unlock at three places btw. A 2nd patch
fixes that, no idea whether its worth the churn.
Both are compile tested only.

Even at this (2 x E5520 (4 cores)) machine there seems to be a benefit of 
about 1.5%. Not enough cores to get into the actually problematic performance 
areas as presented by Robert though.
The variance between runs is a bit too high to call it reliable though.

Thanks,

Andres


PS: I have no clue what to do with the s-o-b and changelog when forward 
porting a patch... So I just copied the original message - which seems wrong.

[PATCH 1/2] LSEEK: BTRFS: Avoid i_mutex for SEEK_{CUR,SET,END}

[Andres Freund]

Don't need the i_mutex for those cases, only for SEEK_HOLE/DATA.

Really-From: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Andres Freund <andres@anarazel.de>
---
 fs/btrfs/file.c |   27 +++++++++++----------------
 1 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 7a13337..5bc7116 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1809,24 +1809,19 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 	struct inode *inode = file->f_mapping->host;
 	int ret;
 
+	if (origin != SEEK_DATA && origin != SEEK_HOLE)
+		return generic_file_llseek(file, offset, origin);
+
 	mutex_lock(&inode->i_mutex);
-	switch (origin) {
-	case SEEK_END:
-	case SEEK_CUR:
-		offset = generic_file_llseek(file, offset, origin);
-		goto out;
-	case SEEK_DATA:
-	case SEEK_HOLE:
-		if (offset >= i_size_read(inode)) {
-			mutex_unlock(&inode->i_mutex);
-			return -ENXIO;
-		}
+	if (offset >= i_size_read(inode)) {
+		mutex_unlock(&inode->i_mutex);
+		return -ENXIO;
+	}
 
-		ret = find_desired_extent(inode, &offset, origin);
-		if (ret) {
-			mutex_unlock(&inode->i_mutex);
-			return ret;
-		}
+	ret = find_desired_extent(inode, &offset, origin);
+	if (ret) {
+		mutex_unlock(&inode->i_mutex);
+		return ret;
 	}
 
 	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
-- 
1.7.6.409.ge7a85.dirty

[PATCH 2/2] btrfs: Don't have multiple paths to error out in btrfs_file_llseek

[Andres Freund]

Using multiple paths seems to invite overlooking one when adding new
stuff in the future.

Signed-of-by: Andres Freund <andres@anarazel.de>
---
 fs/btrfs/file.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 5bc7116..701c633 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1814,14 +1814,14 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
 
 	mutex_lock(&inode->i_mutex);
 	if (offset >= i_size_read(inode)) {
-		mutex_unlock(&inode->i_mutex);
-		return -ENXIO;
+		offset = -ENXIO;
+		goto out;
 	}
 
 	ret = find_desired_extent(inode, &offset, origin);
 	if (ret) {
-		mutex_unlock(&inode->i_mutex);
-		return ret;
+		offset = ret;
+		goto out;
 	}
 
 	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
-- 
1.7.6.409.ge7a85.dirty

Re: Improve lseek scalability v3

[Andi Kleen]

Thanks for testing. According to Viro the patchkit is in his queue, so hopefully
next merge window.
-Andi

Re: [PATCH 1/2] LSEEK: BTRFS: Avoid i_mutex for SEEK_{CUR,SET,END}

[Christoph Hellwig]

Andres, can you check with Chris that the btrfs changes made it to
his tree?  The core lseek changes from Andi are in mainline now, but
I think these bits are better off going through Chrises btrfs tree.

On Sat, Oct 01, 2011 at 10:49:27PM +0200, Andres Freund wrote:
> 
> Don't need the i_mutex for those cases, only for SEEK_HOLE/DATA.
> 
> Really-From: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Andres Freund <andres@anarazel.de>
> ---
>  fs/btrfs/file.c |   27 +++++++++++----------------
>  1 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 7a13337..5bc7116 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1809,24 +1809,19 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
>  	struct inode *inode = file->f_mapping->host;
>  	int ret;
>  
> +	if (origin != SEEK_DATA && origin != SEEK_HOLE)
> +		return generic_file_llseek(file, offset, origin);
> +
>  	mutex_lock(&inode->i_mutex);
> -	switch (origin) {
> -	case SEEK_END:
> -	case SEEK_CUR:
> -		offset = generic_file_llseek(file, offset, origin);
> -		goto out;
> -	case SEEK_DATA:
> -	case SEEK_HOLE:
> -		if (offset >= i_size_read(inode)) {
> -			mutex_unlock(&inode->i_mutex);
> -			return -ENXIO;
> -		}
> +	if (offset >= i_size_read(inode)) {
> +		mutex_unlock(&inode->i_mutex);
> +		return -ENXIO;
> +	}
>  
> -		ret = find_desired_extent(inode, &offset, origin);
> -		if (ret) {
> -			mutex_unlock(&inode->i_mutex);
> -			return ret;
> -		}
> +	ret = find_desired_extent(inode, &offset, origin);
> +	if (ret) {
> +		mutex_unlock(&inode->i_mutex);
> +		return ret;
>  	}
>  
>  	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
> -- 
> 1.7.6.409.ge7a85.dirty
> 
> --
> 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
---end quoted text---

Re: [PATCH 1/2] LSEEK: BTRFS: Avoid i_mutex for SEEK_{CUR,SET,END}

[Chris Mason]

On Wed, Nov 02, 2011 at 04:29:15AM -0400, Christoph Hellwig wrote:
> Andres, can you check with Chris that the btrfs changes made it to
> his tree?  The core lseek changes from Andi are in mainline now, but
> I think these bits are better off going through Chrises btrfs tree.

I'm pulling these in, thanks!

-chris

> 
> On Sat, Oct 01, 2011 at 10:49:27PM +0200, Andres Freund wrote:
> > 
> > Don't need the i_mutex for those cases, only for SEEK_HOLE/DATA.
> > 
> > Really-From: Andi Kleen <ak@linux.intel.com>
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > Signed-off-by: Andres Freund <andres@anarazel.de>
> > ---
> >  fs/btrfs/file.c |   27 +++++++++++----------------
> >  1 files changed, 11 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 7a13337..5bc7116 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1809,24 +1809,19 @@ static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin)
> >  	struct inode *inode = file->f_mapping->host;
> >  	int ret;
> >  
> > +	if (origin != SEEK_DATA && origin != SEEK_HOLE)
> > +		return generic_file_llseek(file, offset, origin);
> > +
> >  	mutex_lock(&inode->i_mutex);
> > -	switch (origin) {
> > -	case SEEK_END:
> > -	case SEEK_CUR:
> > -		offset = generic_file_llseek(file, offset, origin);
> > -		goto out;
> > -	case SEEK_DATA:
> > -	case SEEK_HOLE:
> > -		if (offset >= i_size_read(inode)) {
> > -			mutex_unlock(&inode->i_mutex);
> > -			return -ENXIO;
> > -		}
> > +	if (offset >= i_size_read(inode)) {
> > +		mutex_unlock(&inode->i_mutex);
> > +		return -ENXIO;
> > +	}
> >  
> > -		ret = find_desired_extent(inode, &offset, origin);
> > -		if (ret) {
> > -			mutex_unlock(&inode->i_mutex);
> > -			return ret;
> > -		}
> > +	ret = find_desired_extent(inode, &offset, origin);
> > +	if (ret) {
> > +		mutex_unlock(&inode->i_mutex);
> > +		return ret;
> >  	}
> >  
> >  	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
> > -- 
> > 1.7.6.409.ge7a85.dirty
> > 
> > --
> > 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
> ---end quoted text---

Re: [PATCH 1/2] LSEEK: BTRFS: Avoid i_mutex for SEEK_{CUR,SET,END}

[Andres Freund]

On Saturday, November 05, 2011 04:27:49 PM Chris Mason wrote:
> On Wed, Nov 02, 2011 at 04:29:15AM -0400, Christoph Hellwig wrote:
> > Andres, can you check with Chris that the btrfs changes made it to
> > his tree?  The core lseek changes from Andi are in mainline now, but
> > I think these bits are better off going through Chrises btrfs tree.
> 
> I'm pulling these in, thanks!
I just stumbled uppon this email when opening up my email client which 
triggered me to recheck the issue - do you have pulled those? A very quick 
look didn't intoicate so.

Andres
> > On Sat, Oct 01, 2011 at 10:49:27PM +0200, Andres Freund wrote:
> > > Don't need the i_mutex for those cases, only for SEEK_HOLE/DATA.
> > > 
> > > Really-From: Andi Kleen <ak@linux.intel.com>
> > > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > > Signed-off-by: Andres Freund <andres@anarazel.de>
> > > ---
> > > 
> > >  fs/btrfs/file.c |   27 +++++++++++----------------
> > >  1 files changed, 11 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > > index 7a13337..5bc7116 100644
> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -1809,24 +1809,19 @@ static loff_t btrfs_file_llseek(struct file
> > > *file, loff_t offset, int origin)
> > > 
> > >  	struct inode *inode = file->f_mapping->host;
> > >  	int ret;
> > > 
> > > +	if (origin != SEEK_DATA && origin != SEEK_HOLE)
> > > +		return generic_file_llseek(file, offset, origin);
> > > +
> > > 
> > >  	mutex_lock(&inode->i_mutex);
> > > 
> > > -	switch (origin) {
> > > -	case SEEK_END:
> > > -	case SEEK_CUR:
> > > -		offset = generic_file_llseek(file, offset, origin);
> > > -		goto out;
> > > -	case SEEK_DATA:
> > > -	case SEEK_HOLE:
> > > -		if (offset >= i_size_read(inode)) {
> > > -			mutex_unlock(&inode->i_mutex);
> > > -			return -ENXIO;
> > > -		}
> > > +	if (offset >= i_size_read(inode)) {
> > > +		mutex_unlock(&inode->i_mutex);
> > > +		return -ENXIO;
> > > +	}
> > > 
> > > -		ret = find_desired_extent(inode, &offset, origin);
> > > -		if (ret) {
> > > -			mutex_unlock(&inode->i_mutex);
> > > -			return ret;
> > > -		}
> > > +	ret = find_desired_extent(inode, &offset, origin);
> > > +	if (ret) {
> > > +		mutex_unlock(&inode->i_mutex);
> > > +		return ret;
> > > 
> > >  	}
> > >  	
> > >  	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
> > 
> > ---end quoted text---
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/