[PATCH 00/15] Remove BKL from default_llseek() and other issues (v2)

[PATCH 00/15] Remove BKL from default_llseek() and other issues (v2)

[Jan Blunck]

Alan,

I worked on the list of driver that need fixing and found some other issue that
I think are worth fixing. I post them in one big series because they are all
related to llseek in some way even it they are not directly related to the
removal of the big kernel lock from default_llseek():

 - the osst driver does not really support seeking but wants to work around
   broken userspace and therefore needs a succeeding llseek
 - use of f_pos in frv due to incomplete patch when removing file argument from
   proc_handler
 - filesystems that use BKL in readdir and don't have llseek set shouldn't fall
   back to default_llseek() but use generic_file_llseek() instead

I hope that the patches address your feedback well.

Comments?

Cheers,
	Jan

Jan Blunck (15):
  Introduce noop_llseek()
  osst: Use noop_llseek() instead of default_llseek()
  osst: Update ppos instead of using file->f_pos
  s390: tape_char should update ppos instead of using file->f_pos
  flash_read should update ppos instead of file->f_pos
  eeprom_read()/eeprom_write() should update ppos instead of
    file->f_pos
  sched_feat_write: Update ppos instead of file->f_pos
  airo: Use ppos instead of file->f_pos
  frv: remove "struct file *" argument from sysctl ->proc_handler
  mISDN: Remove unnecessary test on f_pos
  zcrypt: Use nonseekable_open()
  rtc-m41t80: Use nonseekable_open()
  Do not fallback to default_llseek() when readdir() uses BKL
  BKL: Remove BKL from default_llseek()
  BKL: Update documentation on llseek(\b)

 Documentation/filesystems/Locking   |    5 +++--
 arch/cris/arch-v10/drivers/eeprom.c |   34 +++++++++++++---------------------
 arch/frv/kernel/sysctl.c            |   18 ++++++++++--------
 drivers/isdn/mISDN/timerdev.c       |    2 --
 drivers/net/wireless/airo.c         |    2 +-
 drivers/rtc/rtc-m41t80.c            |    6 +-----
 drivers/s390/char/tape_char.c       |    4 ++--
 drivers/s390/crypto/zcrypt_api.c    |    2 +-
 drivers/sbus/char/flash.c           |    6 +++---
 drivers/scsi/osst.c                 |    9 +++++----
 fs/autofs/root.c                    |    1 +
 fs/freevxfs/vxfs_lookup.c           |    2 ++
 fs/isofs/dir.c                      |    1 +
 fs/ncpfs/dir.c                      |    1 +
 fs/qnx4/dir.c                       |    1 +
 fs/read_write.c                     |    8 ++++++--
 fs/reiserfs/dir.c                   |    1 +
 fs/smbfs/dir.c                      |    1 +
 fs/udf/dir.c                        |    1 +
 include/linux/fs.h                  |    1 +
 kernel/sched.c                      |    2 +-
 21 files changed, 56 insertions(+), 52 deletions(-)

[PATCH 01/15] Introduce noop_llseek()

[Jan Blunck]

The noop_llseek() is a llseek() operation that filesystems can use that
don't want to support seeking (leave the file->f_pos untouched) but still
want to let the syscall itself to succeed.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 fs/read_write.c    |    6 ++++++
 include/linux/fs.h |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 3ac2898..7a01d11 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -97,6 +97,12 @@ loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
 }
 EXPORT_SYMBOL(generic_file_llseek);
 
+loff_t noop_llseek(struct file *file, loff_t offset, int origin)
+{
+	return file->f_pos;
+}
+EXPORT_SYMBOL(noop_llseek);
+
 loff_t no_llseek(struct file *file, loff_t offset, int origin)
 {
 	return -ESPIPE;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2620a8c..0a0c1f4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2237,6 +2237,7 @@ extern long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
 
 extern void
 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,
-- 
1.6.4.2

[PATCH 02/15] osst: Use noop_llseek() instead of default_llseek()

[Jan Blunck]

__os_scsi_tape_open() suggests that llseek() doesn't work:
"We really want to do nonseekable_open(inode, filp); here, but some
 versions of tar incorrectly call lseek on tapes and bail out if that
 fails.  So we disallow pread() and pwrite(), but permit lseeks."

Instead of using the fallback default_llseek() the driver should use
noop_llseek() which leaves the file->f_pos untouched but succeeds.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/scsi/osst.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index acb8358..761ca9d 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -5619,6 +5619,7 @@ static const struct file_operations osst_fops = {
 	.open =         os_scsi_tape_open,
 	.flush =        os_scsi_tape_flush,
 	.release =      os_scsi_tape_close,
+	.llseek =	noop_llseek,
 };
 
 static int osst_supports(struct scsi_device * SDp)
-- 
1.6.4.2

[PATCH 03/15] osst: Update ppos instead of using file->f_pos

[Jan Blunck]

osst_read()/osst_write() modify file->f_pos directly instead of the ppos
given to them. The VFS later updates the file->f_pos and overwrites it
with the value of ppos.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/scsi/osst.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index 761ca9d..960e39c 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -3586,7 +3586,7 @@ if (SRpnt) printk(KERN_ERR "%s:A: Not supposed to have SRpnt at line %d\n", name
 		if (i == (-ENOSPC)) {
 			transfer = STp->buffer->writing;	/* FIXME -- check this logic */
 			if (transfer <= do_count) {
-				filp->f_pos += do_count - transfer;
+				*ppos += do_count - transfer;
 				count -= do_count - transfer;
 				if (STps->drv_block >= 0) {
 					STps->drv_block += (do_count - transfer) / STp->block_size;
@@ -3624,7 +3624,7 @@ if (SRpnt) printk(KERN_ERR "%s:A: Not supposed to have SRpnt at line %d\n", name
 			goto out;
 		}
 
-		filp->f_pos += do_count;
+		*ppos += do_count;
 		b_point += do_count;
 		count -= do_count;
 		if (STps->drv_block >= 0) {
@@ -3646,7 +3646,7 @@ if (SRpnt) printk(KERN_ERR "%s:A: Not supposed to have SRpnt at line %d\n", name
 		if (STps->drv_block >= 0) {
 			STps->drv_block += blks;
 		}
-		filp->f_pos += count;
+		*ppos += count;
 		count = 0;
 	}
 
@@ -3822,7 +3822,7 @@ static ssize_t osst_read(struct file * filp, char __user * buf, size_t count, lo
 			}
 			STp->logical_blk_num += transfer / STp->block_size;
 			STps->drv_block      += transfer / STp->block_size;
-			filp->f_pos          += transfer;
+			*ppos          += transfer;
 			buf                  += transfer;
 			total                += transfer;
 		}
-- 
1.6.4.2

[PATCH 04/15] s390: tape_char should update ppos instead of using file->f_pos

[Jan Blunck]

tapechar_read()/tapechar_write() modify file->f_pos directly instead of
the ppos given to them. The VFS later updates the file->f_pos and
overwrites it with the unchanged value of ppos.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/s390/char/tape_char.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/char/tape_char.c b/drivers/s390/char/tape_char.c
index 31566c5..3514f6e 100644
--- a/drivers/s390/char/tape_char.c
+++ b/drivers/s390/char/tape_char.c
@@ -170,7 +170,7 @@ tapechar_read(struct file *filp, char __user *data, size_t count, loff_t *ppos)
 	if (rc == 0) {
 		rc = block_size - request->rescnt;
 		DBF_EVENT(6, "TCHAR:rbytes:  %x\n", rc);
-		filp->f_pos += rc;
+		*ppos += rc;
 		/* Copy data from idal buffer to user space. */
 		if (idal_buffer_to_user(device->char_data.idal_buf,
 					data, rc) != 0)
@@ -238,7 +238,7 @@ tapechar_write(struct file *filp, const char __user *data, size_t count, loff_t
 			break;
 		DBF_EVENT(6, "TCHAR:wbytes: %lx\n",
 			  block_size - request->rescnt);
-		filp->f_pos += block_size - request->rescnt;
+		*ppos += block_size - request->rescnt;
 		written += block_size - request->rescnt;
 		if (request->rescnt != 0)
 			break;
-- 
1.6.4.2

[PATCH 05/15] flash_read should update ppos instead of file->f_pos

[Jan Blunck]

flash_read() updates file->f_pos directly instead of the ppos given. The
VFS later updates the file->f_pos and overwrites it with the unchanged
value of ppos.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/sbus/char/flash.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/sbus/char/flash.c b/drivers/sbus/char/flash.c
index 4108347..12cd438 100644
--- a/drivers/sbus/char/flash.c
+++ b/drivers/sbus/char/flash.c
@@ -106,9 +106,9 @@ static ssize_t
 flash_read(struct file * file, char __user * buf,
 	   size_t count, loff_t *ppos)
 {
-	unsigned long p = file->f_pos;
+	loff_t p = *ppos;
 	int i;
-	
+
 	if (count > flash.read_size - p)
 		count = flash.read_size - p;
 
@@ -119,7 +119,7 @@ flash_read(struct file * file, char __user * buf,
 		buf++;
 	}
 
-	file->f_pos += count;
+	*ppos += count;
 	return count;
 }
 
-- 
1.6.4.2

[PATCH 06/15] eeprom_read()/eeprom_write() should update ppos instead of file->f_pos

[Jan Blunck]

eeprom_read()/eeprom_write() modify file->f_pos directly instead of the
ppos given. The VFS later updates the file->f_pos and overwrites it with
the unchanged value of ppos. Since nothing touches the struct file in
eeprom_read()/eeprom_write() now, the on stack allocated struct file in
eeprom_read_buf()/eeprom_write_buf() can be removed as well.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 arch/cris/arch-v10/drivers/eeprom.c |   34 +++++++++++++---------------------
 1 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/arch/cris/arch-v10/drivers/eeprom.c b/arch/cris/arch-v10/drivers/eeprom.c
index 1f2ae90..68d9e88 100644
--- a/arch/cris/arch-v10/drivers/eeprom.c
+++ b/arch/cris/arch-v10/drivers/eeprom.c
@@ -439,20 +439,18 @@ static loff_t eeprom_lseek(struct file * file, loff_t offset, int orig)
 
 static int eeprom_read_buf(loff_t addr, char * buf, int count)
 {
-  struct file f;
-
-  f.f_pos = addr;
-  return eeprom_read(&f, buf, count, &addr);
+  return eeprom_read(NULL, buf, count, &addr);
 }
 
 
 
 /* Reads data from eeprom. */
 
-static ssize_t eeprom_read(struct file * file, char * buf, size_t count, loff_t *off)
+static ssize_t eeprom_read(struct file * file, char * buf, size_t count,
+			   loff_t *ppos)
 {
   int read=0;
-  unsigned long p = file->f_pos;
+  loff_t p = *ppos;
 
   unsigned char page;
 
@@ -498,11 +496,9 @@ static ssize_t eeprom_read(struct file * file, char * buf, size_t count, loff_t
 
   /* go on with the actual read */
   read = read_from_eeprom( buf, count);
-  
+
   if(read > 0)
-  {
-    file->f_pos += read;
-  }
+    *ppos += read;
 
   eeprom.busy--;
   wake_up_interruptible(&eeprom.wait_q);
@@ -513,18 +509,14 @@ static ssize_t eeprom_read(struct file * file, char * buf, size_t count, loff_t
 
 static int eeprom_write_buf(loff_t addr, const char * buf, int count)
 {
-  struct file f;
-
-  f.f_pos = addr;
-  
-  return eeprom_write(&f, buf, count, &addr);
+  return eeprom_write(NULL, buf, count, &addr);
 }
 
 
 /* Writes data to eeprom. */
 
 static ssize_t eeprom_write(struct file * file, const char * buf, size_t count,
-                            loff_t *off)
+			    loff_t *ppos)
 {
   int i, written, restart=1;
   unsigned long p;
@@ -543,9 +535,9 @@ static ssize_t eeprom_write(struct file * file, const char * buf, size_t count,
   {
     restart = 0;
     written = 0;
-    p = file->f_pos;
-   
-    
+    p = *ppos;
+
+
     while( (written < count) && (p < eeprom.size))
     {
       /* address the eeprom */
@@ -671,10 +663,10 @@ static ssize_t eeprom_write(struct file * file, const char * buf, size_t count,
 
   eeprom.busy--;
   wake_up_interruptible(&eeprom.wait_q);
-  if (written == 0 && file->f_pos >= eeprom.size){
+  if (written == 0 && *ppos >= eeprom.size){
     return -ENOSPC;
   }
-  file->f_pos += written;
+  *ppos += written;
   return written;
 }
 
-- 
1.6.4.2

[PATCH 07/15] sched_feat_write: Update ppos instead of file->f_pos

[Jan Blunck]

sched_feat_write() should update ppos instead of file->f_pos.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 kernel/sched.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 3c11ae0..0114845 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -772,7 +772,7 @@ sched_feat_write(struct file *filp, const char __user *ubuf,
 	if (!sched_feat_names[i])
 		return -EINVAL;
 
-	filp->f_pos += cnt;
+	*ppos += cnt;
 
 	return cnt;
 }
-- 
1.6.4.2

[PATCH 08/15] airo: Use ppos instead of file->f_pos

[Jan Blunck]

When updating the priv->writelen value the ppos given to proc_write()
should be used instead of file->f_pos. This might lead to a wrong writelen
value when pwrite() is used.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/net/wireless/airo.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index abf896a..ae7dea8 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -4659,7 +4659,7 @@ static ssize_t proc_write( struct file *file,
 	if (copy_from_user(priv->wbuffer + pos, buffer, len))
 		return -EFAULT;
 	if ( pos + len > priv->writelen )
-		priv->writelen = len + file->f_pos;
+		priv->writelen = pos + len;
 	*offset = pos + len;
 	return len;
 }
-- 
1.6.4.2

[PATCH 09/15] frv: remove "struct file *" argument from sysctl ->proc_handler

[Jan Blunck]

Seems that Alexey Dobriyan missed this usage of the file argument when
removing it from ->proc_handler in commit 8d65af789f3e2cf4cfbdbf71a0f7a61ebcd41d38.

Signed-off-by: Jan Blunck <jblunck@suse.de>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/frv/kernel/sysctl.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/frv/kernel/sysctl.c b/arch/frv/kernel/sysctl.c
index 3e9d7e0..bd6cc5c 100644
--- a/arch/frv/kernel/sysctl.c
+++ b/arch/frv/kernel/sysctl.c
@@ -47,8 +47,9 @@ static void frv_change_dcache_mode(unsigned long newmode)
 /*
  * handle requests to dynamically switch the write caching mode delivered by /proc
  */
-static int procctl_frv_cachemode(ctl_table *table, int write, struct file *filp,
-				 void __user *buffer, size_t *lenp, loff_t *ppos)
+static int procctl_frv_cachemode(ctl_table *table, int write,
+				 void __user *buffer, size_t *lenp,
+				 loff_t *ppos)
 {
 	unsigned long hsr0;
 	char buff[8];
@@ -85,7 +86,7 @@ static int procctl_frv_cachemode(ctl_table *table, int write, struct file *filp,
 	}
 
 	/* read the state */
-	if (filp->f_pos > 0) {
+	if (*ppos > 0) {
 		*lenp = 0;
 		return 0;
 	}
@@ -111,7 +112,7 @@ static int procctl_frv_cachemode(ctl_table *table, int write, struct file *filp,
 		return -EFAULT;
 
 	*lenp = len;
-	filp->f_pos = len;
+	*ppos = len;
 	return 0;
 
 } /* end procctl_frv_cachemode() */
@@ -121,8 +122,9 @@ static int procctl_frv_cachemode(ctl_table *table, int write, struct file *filp,
  * permit the mm_struct the nominated process is using have its MMU context ID pinned
  */
 #ifdef CONFIG_MMU
-static int procctl_frv_pin_cxnr(ctl_table *table, int write, struct file *filp,
-				void __user *buffer, size_t *lenp, loff_t *ppos)
+static int procctl_frv_pin_cxnr(ctl_table *table, int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos)
 {
 	pid_t pid;
 	char buff[16], *p;
@@ -151,7 +153,7 @@ static int procctl_frv_pin_cxnr(ctl_table *table, int write, struct file *filp,
 	}
 
 	/* read the currently pinned CXN */
-	if (filp->f_pos > 0) {
+	if (*ppos > 0) {
 		*lenp = 0;
 		return 0;
 	}
@@ -164,7 +166,7 @@ static int procctl_frv_pin_cxnr(ctl_table *table, int write, struct file *filp,
 		return -EFAULT;
 
 	*lenp = len;
-	filp->f_pos = len;
+	*ppos = len;
 	return 0;
 
 } /* end procctl_frv_pin_cxnr() */
-- 
1.6.4.2

[PATCH 10/15] mISDN: Remove unnecessary test on f_pos

[Jan Blunck]

This test is not doing anything since it is always false if the mISDN_read()
is called from vfs_read(). Besides that the driver uses nonseekable_open()
and is not using off or file->f_pos anywhere.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/isdn/mISDN/timerdev.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/isdn/mISDN/timerdev.c b/drivers/isdn/mISDN/timerdev.c
index 5b7e9bf..0a8b440 100644
--- a/drivers/isdn/mISDN/timerdev.c
+++ b/drivers/isdn/mISDN/timerdev.c
@@ -96,8 +96,6 @@ mISDN_read(struct file *filep, char __user *buf, size_t count, loff_t *off)
 	if (*debug & DEBUG_TIMER)
 		printk(KERN_DEBUG "%s(%p, %p, %d, %p)\n", __func__,
 			filep, buf, (int)count, off);
-	if (*off != filep->f_pos)
-		return -ESPIPE;
 
 	if (list_empty(&dev->expired) && (dev->work == 0)) {
 		if (filep->f_flags & O_NONBLOCK)
-- 
1.6.4.2

[PATCH 11/15] zcrypt: Use nonseekable_open()

[Jan Blunck]

zcrypt doesn't even allow to read/write the files it opens. So seeking on
it shouldn't fall back to defaults either but should be disallowed as well.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/s390/crypto/zcrypt_api.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
index 65b6a96..4ff1820 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -302,7 +302,7 @@ static int zcrypt_open(struct inode *inode, struct file *filp)
 	lock_kernel();
 	atomic_inc(&zcrypt_open_count);
 	unlock_kernel();
-	return 0;
+	return nonseekable_open(inode, filp);
 }
 
 /**
-- 
1.6.4.2

[PATCH 12/15] rtc-m41t80: Use nonseekable_open()

[Jan Blunck]

Use nonseekable_open() for this since seeking is not supported anyway.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/rtc/rtc-m41t80.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 60fe266..1e2ed76 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -595,10 +595,6 @@ static void wdt_disable(void)
 static ssize_t wdt_write(struct file *file, const char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	/*  Can't seek (pwrite) on this device
-	if (ppos != &file->f_pos)
-	return -ESPIPE;
-	*/
 	if (count) {
 		wdt_ping();
 		return 1;
@@ -695,7 +691,7 @@ static int wdt_open(struct inode *inode, struct file *file)
 		 */
 		wdt_is_open = 1;
 		unlock_kernel();
-		return 0;
+		return nonseekable_open(inode, file);
 	}
 	return -ENODEV;
 }
-- 
1.6.4.2

[PATCH 13/15] Do not fallback to default_llseek() when readdir() uses BKL

[Jan Blunck]

Do not use the fallback default_llseek() if the readdir operation of the
filesystem still uses the big kernel lock. Since llseek() modifies
file->f_pos of the directory directly it may need locking to not confuse
readdir which usually uses file->f_pos directly as well. Since the special
characteristics of the BKL (unlocked on schedule) are not necessary in this
case, the inode mutex can be used for locking as provided by
generic_file_llseek(). This is only possible since all filesystems, except
reiserfs, either use a directory as a flat file or with disk address
offsets. Reiserfs on the other hand uses a 32bit hash off the filename as
the offset so generic_file_llseek() can get used as well since the hash is
always smaller than sb->s_maxbytes (= (512 << 32) - blocksize).

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 fs/autofs/root.c          |    1 +
 fs/freevxfs/vxfs_lookup.c |    2 ++
 fs/isofs/dir.c            |    1 +
 fs/ncpfs/dir.c            |    1 +
 fs/qnx4/dir.c             |    1 +
 fs/reiserfs/dir.c         |    1 +
 fs/smbfs/dir.c            |    1 +
 fs/udf/dir.c              |    1 +
 8 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/fs/autofs/root.c b/fs/autofs/root.c
index 4a1401c..d3aec77 100644
--- a/fs/autofs/root.c
+++ b/fs/autofs/root.c
@@ -30,6 +30,7 @@ const struct file_operations autofs_root_operations = {
 	.read		= generic_read_dir,
 	.readdir	= autofs_root_readdir,
 	.ioctl		= autofs_root_ioctl,
+	.llseek		= generic_file_llseek,
 };
 
 const struct inode_operations autofs_root_inode_operations = {
diff --git a/fs/freevxfs/vxfs_lookup.c b/fs/freevxfs/vxfs_lookup.c
index aee049c..0ec7bb2 100644
--- a/fs/freevxfs/vxfs_lookup.c
+++ b/fs/freevxfs/vxfs_lookup.c
@@ -57,6 +57,8 @@ const struct inode_operations vxfs_dir_inode_ops = {
 };
 
 const struct file_operations vxfs_dir_operations = {
+	.llseek =		generic_file_llseek,
+	.read =			generic_read_dir,
 	.readdir =		vxfs_readdir,
 };
 
diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
index 8ba5441..4816bd3 100644
--- a/fs/isofs/dir.c
+++ b/fs/isofs/dir.c
@@ -271,6 +271,7 @@ static int isofs_readdir(struct file *filp,
 
 const struct file_operations isofs_dir_operations =
 {
+	.llseek = generic_file_llseek,
 	.read = generic_read_dir,
 	.readdir = isofs_readdir,
 };
diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index b8b5b30..c9cad9a 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -50,6 +50,7 @@ extern int ncp_symlink(struct inode *, struct dentry *, const char *);
 		      
 const struct file_operations ncp_dir_operations =
 {
+	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.readdir	= ncp_readdir,
 	.ioctl		= ncp_ioctl,
diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index 86cc39c..da40f9b 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -77,6 +77,7 @@ out:
 
 const struct file_operations qnx4_dir_operations =
 {
+	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.readdir	= qnx4_readdir,
 	.fsync		= simple_fsync,
diff --git a/fs/reiserfs/dir.c b/fs/reiserfs/dir.c
index 6d2668f..fa616a1 100644
--- a/fs/reiserfs/dir.c
+++ b/fs/reiserfs/dir.c
@@ -17,6 +17,7 @@ static int reiserfs_dir_fsync(struct file *filp, struct dentry *dentry,
 			      int datasync);
 
 const struct file_operations reiserfs_dir_operations = {
+	.llseek = generic_file_llseek,
 	.read = generic_read_dir,
 	.readdir = reiserfs_readdir,
 	.fsync = reiserfs_dir_fsync,
diff --git a/fs/smbfs/dir.c b/fs/smbfs/dir.c
index 3e4803b..cd11c54 100644
--- a/fs/smbfs/dir.c
+++ b/fs/smbfs/dir.c
@@ -37,6 +37,7 @@ static int smb_link(struct dentry *, struct inode *, struct dentry *);
 
 const struct file_operations smb_dir_operations =
 {
+	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.readdir	= smb_readdir,
 	.ioctl		= smb_ioctl,
diff --git a/fs/udf/dir.c b/fs/udf/dir.c
index 61d9a76..475cc49 100644
--- a/fs/udf/dir.c
+++ b/fs/udf/dir.c
@@ -207,6 +207,7 @@ static int udf_readdir(struct file *filp, void *dirent, filldir_t filldir)
 
 /* readdir and lookup functions */
 const struct file_operations udf_dir_operations = {
+	.llseek			= generic_file_llseek,
 	.read			= generic_read_dir,
 	.readdir		= udf_readdir,
 	.ioctl			= udf_ioctl,
-- 
1.6.4.2

[PATCH 14/15] BKL: Remove BKL from default_llseek()

[Jan Blunck]

Using the BKL in llseek() does not protect the inode's i_size from
modification since the i_size is protected by a seqlock nowadays. Since
default_llseek() is already using the i_size_read() wrapper it is not the
BKL which is serializing the access here.
The access to file->f_pos is not protected by the BKL either since its
access in vfs_write()/vfs_read() is not protected by any lock. If the BKL
is not protecting anything here it can clearly get removed.

Signed-off-by: Jan Blunck <jblunck@suse.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Frédéric Weisbecker <fweisbec@gmail.com>
Cc: John Kacur <jkacur@redhat.com>
---
 fs/read_write.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 7a01d11..cbe54e4 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -113,7 +113,6 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin)
 {
 	loff_t retval;
 
-	lock_kernel();
 	switch (origin) {
 		case SEEK_END:
 			offset += i_size_read(file->f_path.dentry->d_inode);
@@ -134,7 +133,6 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin)
 		retval = offset;
 	}
 out:
-	unlock_kernel();
 	return retval;
 }
 EXPORT_SYMBOL(default_llseek);
-- 
1.6.4.2

[PATCH 15/15] BKL: Update documentation on llseek( \b)

[Jan Blunck]

The inode's i_size is not protected by the big kernel lock. Therefore it
does not make sense to recommend taking the BKL in filesystems llseek
operations. Instead it should use the inode's mutex or use just use
i_size_read() instead. Add a note that this is not protecting file->f_pos.

Signed-off-by: Jan Blunck <jblunck@suse.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Frédéric Weisbecker <fweisbec@gmail.com>
Cc: John Kacur <jkacur@redhat.com>
---
 Documentation/filesystems/Locking |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 18b9d0c..25159d4 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -429,8 +429,9 @@ check_flags:		no
 implementations.  If your fs is not using generic_file_llseek, you
 need to acquire and release the appropriate locks in your ->llseek().
 For many filesystems, it is probably safe to acquire the inode
-semaphore.  Note some filesystems (i.e. remote ones) provide no
-protection for i_size so you will need to use the BKL.
+mutex or just to use i_size_read() instead.
+Note: this does not protect the file->f_pos against concurrent modifications
+since this is something the userspace has to take care about.
 
 Note: ext2_release() was *the* source of contention on fs-intensive
 loads and dropping BKL on ->release() helps to get rid of that (we still
-- 
1.6.4.2

Re: [PATCH 15/15] BKL: Update documentation on llseek(\b)

[Alan Cox]

On Fri, 20 Nov 2009 17:40:45 +0100
Jan Blunck <jblunck@suse.de> wrote:

> The inode's i_size is not protected by the big kernel lock. Therefore it
> does not make sense to recommend taking the BKL in filesystems llseek
> operations. Instead it should use the inode's mutex or use just use
> i_size_read() instead. Add a note that this is not protecting file->f_pos.

Looks good to me reading through it.

Alan

Re: [PATCH 01/15] Introduce noop_llseek()

[Jamie Lokier]

Jan Blunck wrote:
> The noop_llseek() is a llseek() operation that filesystems can use that
> don't want to support seeking (leave the file->f_pos untouched) but still
> want to let the syscall itself to succeed.

This is weird behaviour: if you want to allow llseek() to succeed but
don't really support seeking, why does the device even care about the
value of file->f_pos?

-- Jamie

Re: [PATCH 02/15] osst: Use noop_llseek() instead of default_llseek()

[Jamie Lokier]

Jan Blunck wrote:
> __os_scsi_tape_open() suggests that llseek() doesn't work:
> "We really want to do nonseekable_open(inode, filp); here, but some
>  versions of tar incorrectly call lseek on tapes and bail out if that
>  fails.  So we disallow pread() and pwrite(), but permit lseeks."
> 
> Instead of using the fallback default_llseek() the driver should use
> noop_llseek() which leaves the file->f_pos untouched but succeeds.

st.c has the same comment, but I didn't see a patch for st.c in the series.

-- Jamie

Re: [PATCH 01/15] Introduce noop_llseek()

[Jan Blunck]

On Fri, Nov 20, Jamie Lokier wrote:

> Jan Blunck wrote:
> > The noop_llseek() is a llseek() operation that filesystems can use that
> > don't want to support seeking (leave the file->f_pos untouched) but still
> > want to let the syscall itself to succeed.
> 
> This is weird behaviour: if you want to allow llseek() to succeed but
> don't really support seeking, why does the device even care about the
> value of file->f_pos?

The device itself does not care about it but it is userspace that is expecting
the seek to succeed. There is a comment in osst that at least there seems to
be a borken version of tar that wants to seek on the device even it that does
not have any effect.

Regards,
	Jan

-- 
Jan Blunck <jblunck@suse.de>

Re: [PATCH 03/15] osst: Update ppos instead of using file->f_pos

[Jamie Lokier]

Jan Blunck wrote:
> osst_read()/osst_write() modify file->f_pos directly instead of the ppos
> given to them. The VFS later updates the file->f_pos and overwrites it
> with the value of ppos.

I notice st.c doesn't use or update file->f_pos (or *ppos), so
userspace probably won't be caring about f_pos from osst.c (they're
both SCSI tape drivers).  And osst.c doesn't use the value, it just
increases it with each transfer.  It doesn't even reset the value to
zero when rewinding the tape, so it's not that meaningful.

So how about just removing those modifications to file->f_pos from osst.c?

-- Jamie

Re: [PATCH 03/15] osst: Update ppos instead of using file->f_pos

[Jamie Lokier]

Jamie Lokier wrote:
> Jan Blunck wrote:
> > osst_read()/osst_write() modify file->f_pos directly instead of the ppos
> > given to them. The VFS later updates the file->f_pos and overwrites it
> > with the value of ppos.
> 
> I notice st.c doesn't use or update file->f_pos (or *ppos), so
> userspace probably won't be caring about f_pos from osst.c (they're
> both SCSI tape drivers).  And osst.c doesn't use the value, it just
> increases it with each transfer.  It doesn't even reset the value to
> zero when rewinding the tape, so it's not that meaningful.
> 
> So how about just removing those modifications to file->f_pos from osst.c?

Or alternatively, perhaps they are missing from st.c.  I don't know,
but userspace can't have been all that dependent on the value :-)

-- Jamie

Re: [PATCH 02/15] osst: Use noop_llseek() instead of default_llseek()

[Jan Blunck]

On Fri, Nov 20, Jamie Lokier wrote:

> Jan Blunck wrote:
> > __os_scsi_tape_open() suggests that llseek() doesn't work:
> > "We really want to do nonseekable_open(inode, filp); here, but some
> >  versions of tar incorrectly call lseek on tapes and bail out if that
> >  fails.  So we disallow pread() and pwrite(), but permit lseeks."
> > 
> > Instead of using the fallback default_llseek() the driver should use
> > noop_llseek() which leaves the file->f_pos untouched but succeeds.
> 
> st.c has the same comment, but I didn't see a patch for st.c in the series.
> 

Right, that would make sense.

I wonder if anyone has this weird tar and could test if it actually works.

-- 
st: Use noop_llseek() instead of default_llseek()

st_open() suggests that llseek() doesn't work:
"We really want to do nonseekable_open(inode, filp); here, but some
versions of tar incorrectly call lseek on tapes and bail out if that
fails.  So we disallow pread() and pwrite(), but permit lseeks."

Instead of using the fallback default_llseek() the driver should use
noop_llseek() which leaves the file->f_pos untouched but succeeds.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 12d58a7..9f03668 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -3958,6 +3958,7 @@ static const struct file_operations st_fops =
 	.open =		st_open,
 	.flush =	st_flush,
 	.release =	st_release,
+	.llseek =	noop_llseek,
 };
 
 static int st_probe(struct device *dev)

Re: [PATCH 13/15] Do not fallback to default_llseek() when readdir() uses BKL

[Jan Kara]

On Fri 20-11-09 17:40:43, Jan Blunck wrote:
> Do not use the fallback default_llseek() if the readdir operation of the
> filesystem still uses the big kernel lock. Since llseek() modifies
> file->f_pos of the directory directly it may need locking to not confuse
> readdir which usually uses file->f_pos directly as well. Since the special
> characteristics of the BKL (unlocked on schedule) are not necessary in this
> case, the inode mutex can be used for locking as provided by
> generic_file_llseek(). This is only possible since all filesystems, except
> reiserfs, either use a directory as a flat file or with disk address
> offsets. Reiserfs on the other hand uses a 32bit hash off the filename as
> the offset so generic_file_llseek() can get used as well since the hash is
> always smaller than sb->s_maxbytes (= (512 << 32) - blocksize).
> 
> Signed-off-by: Jan Blunck <jblunck@suse.de>
> ---
>  fs/udf/dir.c              |    1 +
For the UDF part:

  Acked-by: Jan Kara <jack@suse.cz>

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 01/15] Introduce noop_llseek()

[Arnd Bergmann]

On Friday 20 November 2009, Jan Blunck wrote:
> On Fri, Nov 20, Jamie Lokier wrote:
> 
> > Jan Blunck wrote:
> > > The noop_llseek() is a llseek() operation that filesystems can use that
> > > don't want to support seeking (leave the file->f_pos untouched) but still
> > > want to let the syscall itself to succeed.
> > 
> > This is weird behaviour: if you want to allow llseek() to succeed but
> > don't really support seeking, why does the device even care about the
> > value of file->f_pos?
> 
> The device itself does not care about it but it is userspace that is expecting
> the seek to succeed. There is a comment in osst that at least there seems to
> be a borken version of tar that wants to seek on the device even it that does
> not have any effect.

Looking at the question from the other side -- if the device and the user
don't care about file->f_pos, what's wrong with calling generic_file_llseek
instead of noop_llseek?

	Arnd <><

Re: [PATCH 13/15] Do not fallback to default_llseek() when readdir() uses BKL

[Anders Larsen]

On 2009-11-20 17:40:43, Jan Blunck wrote:
> Do not use the fallback default_llseek() if the readdir operation of  
> the
> filesystem still uses the big kernel lock. Since llseek() modifies
> file->f_pos of the directory directly it may need locking to not  
> confuse
> readdir which usually uses file->f_pos directly as well. Since the  
> special
> characteristics of the BKL (unlocked on schedule) are not necessary  
> in this
> case, the inode mutex can be used for locking as provided by
> generic_file_llseek(). This is only possible since all filesystems,  
> except
> reiserfs, either use a directory as a flat file or with disk address
> offsets. Reiserfs on the other hand uses a 32bit hash off the  
> filename as
> the offset so generic_file_llseek() can get used as well since the  
> hash is
> always smaller than sb->s_maxbytes (= (512 << 32) - blocksize).
> 
> Signed-off-by: Jan Blunck <jblunck@suse.de>
> ---

wrt fs/qnx4:
Acked-by: Anders Larsen <al@alarsen.net>

[tip:sched/core] sched_feat_write(): Update ppos instead of file->f_pos

[tip-bot for Jan Blunck]

Commit-ID:  429947248f814e90f416ab4f68a871ab628000c3
Gitweb:     http://git.kernel.org/tip/429947248f814e90f416ab4f68a871ab628000c3
Author:     Jan Blunck <jblunck@suse.de>
AuthorDate: Fri, 20 Nov 2009 17:40:37 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 Nov 2009 19:38:03 +0100

sched_feat_write(): Update ppos instead of file->f_pos

sched_feat_write() should update ppos instead of file->f_pos.

(This reduces some BKL dependencies of this code.)

Signed-off-by: Jan Blunck <jblunck@suse.de>
Cc: jkacur@redhat.com
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jamie Lokier <jamie@shareable.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
LKML-Reference: <1258735245-25826-8-git-send-email-jblunck@suse.de>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index ab9a034..93474a7 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -771,7 +771,7 @@ sched_feat_write(struct file *filp, const char __user *ubuf,
 	if (!sched_feat_names[i])
 		return -EINVAL;
 
-	filp->f_pos += cnt;
+	*ppos += cnt;
 
 	return cnt;
 }