[patch] remove BKL from inode_setattr

[patch] remove BKL from inode_setattr

[Andrew Morton]

Since April 05 of this year we've been holding the BKL across the
vmtruncate call out of inode_setattr().  By accident it seems.

This does not affect unlink().  It affects ftruncate() and open(O_TRUNC).

Given that the drop_inode() path does not take the BKL, I would
suggest that it is safe to assume that the various filesystem's
truncate code is safe without this additional VFS-level lock_kernel(),
and that it can be simply removed.

Sound sane?


--- 2.5.42/fs/attr.c~truncate-bkl	Sun Oct 13 20:04:06 2002
+++ 2.5.42-akpm/fs/attr.c	Sun Oct 13 22:01:15 2002
@@ -67,7 +67,6 @@ int inode_setattr(struct inode * inode, 
 	unsigned int ia_valid = attr->ia_valid;
 	int error = 0;
 	
-	lock_kernel();
 	if (ia_valid & ATTR_SIZE) {
 		error = vmtruncate(inode, attr->ia_size);
 		if (error)
@@ -91,7 +90,6 @@ int inode_setattr(struct inode * inode, 
 	}
 	mark_inode_dirty(inode);
 out:
-	unlock_kernel();
 	return error;
 }
 

.

Re: [patch] remove BKL from inode_setattr

[Hugh Dickins]

On Sun, 13 Oct 2002, Andrew Morton wrote:
> 
> Since April 05 of this year we've been holding the BKL across the
> vmtruncate call out of inode_setattr().  By accident it seems.

But until then, the lock_kernel was one level up in notify_change().

> This does not affect unlink().  It affects ftruncate() and open(O_TRUNC).

And the patch you give affects chown, chgrp, chmod, utime also:
removing a synchronization point if nothing more.  Could that matter?

> Given that the drop_inode() path does not take the BKL, I would
> suggest that it is safe to assume that the various filesystem's
> truncate code is safe without this additional VFS-level lock_kernel(),
> and that it can be simply removed.

Isn't doing it when the references have gone rather easier/safer
than when they remain?  I'm not sure your argument holds.

> Sound sane?

Of course I want you to be right!  But I don't see that you've made
a strong enough case yet.  Please show I'm being stupid, someone.

Hugh

Re: [patch] remove BKL from inode_setattr

[Andrew Morton]

Hugh Dickins wrote:
> 
> On Sun, 13 Oct 2002, Andrew Morton wrote:
> >
> > Since April 05 of this year we've been holding the BKL across the
> > vmtruncate call out of inode_setattr().  By accident it seems.
> 
> But until then, the lock_kernel was one level up in notify_change().

OK.

> > This does not affect unlink().  It affects ftruncate() and open(O_TRUNC).
> 
> And the patch you give affects chown, chgrp, chmod, utime also:
> removing a synchronization point if nothing more.  Could that matter?

I don't think so.  If

	lock_kernel();
	foo = bar;
	unlock_kernel();

is racy then so is

	foo = bar;
 
> > Given that the drop_inode() path does not take the BKL, I would
> > suggest that it is safe to assume that the various filesystem's
> > truncate code is safe without this additional VFS-level lock_kernel(),
> > and that it can be simply removed.
> 
> Isn't doing it when the references have gone rather easier/safer
> than when they remain?  I'm not sure your argument holds.

well we hold i_sem.  So races against operations against the same
file are pretty much blotted out.  Only reads I guess.  It's races
against operations on different files which are the concern.  And
we're already exposed to them.

The number of filsystems which do not take the bkl in truncate/setattr
is in fact quite small.  Here's the patch which removes all doubt:




 fs/affs/file.c          |   13 ++++++++-----
 fs/attr.c               |    2 --
 fs/cifs/inode.c         |    7 ++++++-
 fs/jfs/file.c           |    3 +++
 fs/reiserfs/file.c      |    2 ++
 fs/smbfs/proc.c         |   18 +++++++++++++++---
 fs/sysv/itree.c         |    6 +++++-
 fs/xfs/linux/xfs_iops.c |   11 +++++++++--
 8 files changed, 48 insertions(+), 14 deletions(-)

--- 2.5.42/fs/attr.c~truncate-bkl	Sun Oct 13 20:04:06 2002
+++ 2.5.42-akpm/fs/attr.c	Sun Oct 13 21:04:22 2002
@@ -67,7 +67,6 @@ int inode_setattr(struct inode * inode, 
 	unsigned int ia_valid = attr->ia_valid;
 	int error = 0;
 	
-	lock_kernel();
 	if (ia_valid & ATTR_SIZE) {
 		error = vmtruncate(inode, attr->ia_size);
 		if (error)
@@ -91,7 +90,6 @@ int inode_setattr(struct inode * inode, 
 	}
 	mark_inode_dirty(inode);
 out:
-	unlock_kernel();
 	return error;
 }
 
--- 2.5.42/fs/affs/file.c~truncate-bkl	Sun Oct 13 21:02:43 2002
+++ 2.5.42-akpm/fs/affs/file.c	Sun Oct 13 21:03:46 2002
@@ -832,6 +832,7 @@ affs_truncate(struct inode *inode)
 	pr_debug("AFFS: truncate(inode=%d, oldsize=%u, newsize=%u)\n",
 		 (u32)inode->i_ino, (u32)AFFS_I(inode)->mmu_private, (u32)inode->i_size);
 
+	lock_kernel();
 	last_blk = 0;
 	ext = 0;
 	if (inode->i_size) {
@@ -847,7 +848,7 @@ affs_truncate(struct inode *inode)
 
 		page = grab_cache_page(mapping, size >> PAGE_CACHE_SHIFT);
 		if (!page)
-			return;
+			goto out;
 		size = (size & (PAGE_CACHE_SIZE - 1)) + 1;
 		res = mapping->a_ops->prepare_write(NULL, page, size, size);
 		if (!res)
@@ -855,16 +856,16 @@ affs_truncate(struct inode *inode)
 		unlock_page(page);
 		page_cache_release(page);
 		mark_inode_dirty(inode);
-		return;
+		goto out;
 	} else if (inode->i_size == AFFS_I(inode)->mmu_private)
-		return;
+		goto out;
 
 	// lock cache
 	ext_bh = affs_get_extblock(inode, ext);
 	if (IS_ERR(ext_bh)) {
 		affs_warning(sb, "truncate", "unexpected read error for ext block %u (%d)",
 			     ext, PTR_ERR(ext_bh));
-		return;
+		goto out;
 	}
 	if (AFFS_I(inode)->i_lc) {
 		/* clear linear cache */
@@ -910,7 +911,7 @@ affs_truncate(struct inode *inode)
 			if (IS_ERR(ext_bh)) {
 				affs_warning(sb, "truncate", "unexpected read error for last block %u (%d)",
 					     ext, PTR_ERR(ext_bh));
-				return;
+				goto out;
 			}
 			tmp = be32_to_cpu(AFFS_DATA_HEAD(bh)->next);
 			AFFS_DATA_HEAD(bh)->next = 0;
@@ -936,4 +937,6 @@ affs_truncate(struct inode *inode)
 		affs_brelse(ext_bh);
 	}
 	affs_free_prealloc(inode);
+out:
+	unlock_kernel();
 }
--- 2.5.42/fs/cifs/inode.c~truncate-bkl	Sun Oct 13 21:05:31 2002
+++ 2.5.42-akpm/fs/cifs/inode.c	Sun Oct 13 21:05:55 2002
@@ -20,6 +20,7 @@
  */
 #include <linux/fs.h>
 #include <linux/stat.h>
+#include <linux/smp_lock.h>
 #include <asm/div64.h>
 #include "cifsfs.h"
 #include "cifspdu.h"
@@ -517,6 +518,8 @@ cifs_truncate_file(struct inode *inode)
 	struct dentry *dirent;
 	char *full_path = NULL;   
 
+    lock_kernel();
+
 	xid = GetXid();
 
 	cifs_sb = CIFS_SB(inode->i_sb);
@@ -527,7 +530,7 @@ cifs_truncate_file(struct inode *inode)
 		       ("Can not get pathname from empty dentry in inode 0x%p ",
 			inode));
 		FreeXid(xid);
-		return;
+		goto out;
 	}
 	dirent = list_entry(inode->i_dentry.next, struct dentry, d_alias);
 	if (dirent) {
@@ -556,6 +559,8 @@ cifs_truncate_file(struct inode *inode)
 	if (full_path)
 		kfree(full_path);
 	FreeXid(xid);
+out:
+    unlock_kernel();
 	return;
 }
 
--- 2.5.42/fs/jfs/file.c~truncate-bkl	Sun Oct 13 21:11:06 2002
+++ 2.5.42-akpm/fs/jfs/file.c	Sun Oct 13 21:11:11 2002
@@ -18,6 +18,7 @@
  */
 
 #include <linux/fs.h>
+#include <linux/smp_lock.h>
 #include "jfs_incore.h"
 #include "jfs_dmap.h"
 #include "jfs_txnmgr.h"
@@ -90,9 +91,11 @@ static void jfs_truncate(struct inode *i
 {
 	jFYI(1, ("jfs_truncate: size = 0x%lx\n", (ulong) ip->i_size));
 
+	lock_kernel();
 	IWRITE_LOCK(ip);
 	jfs_truncate_nolock(ip, ip->i_size);
 	IWRITE_UNLOCK(ip);
+	unlock_kernel();
 }
 
 static int jfs_open(struct inode *inode, struct file *file)
--- 2.5.42/fs/reiserfs/file.c~truncate-bkl	Sun Oct 13 21:15:53 2002
+++ 2.5.42-akpm/fs/reiserfs/file.c	Sun Oct 13 21:16:17 2002
@@ -66,7 +66,9 @@ static int reiserfs_file_release (struct
 }
 
 static void reiserfs_vfs_truncate_file(struct inode *inode) {
+    lock_kernel() ;
     reiserfs_truncate_file(inode, 1) ;
+    unlock_kernel() ;
 }
 
 /* Sync a reiserfs file. */
--- 2.5.42/fs/smbfs/proc.c~truncate-bkl	Sun Oct 13 21:18:13 2002
+++ 2.5.42-akpm/fs/smbfs/proc.c	Sun Oct 13 21:19:41 2002
@@ -1740,12 +1740,17 @@ out:
 static int
 smb_proc_trunc32(struct inode *inode, loff_t length)
 {
+	int ret;
+
 	/*
 	 * Writing 0bytes is old-SMB magic for truncating files.
 	 * MAX_NON_LFS should prevent this from being called with a too
 	 * large offset.
 	 */
-	return smb_proc_write(inode, length, 0, NULL);
+	lock_kernel();
+	ret = smb_proc_write(inode, length, 0, NULL);
+	unlock_kernel();
+	return ret;
 }
 
 static int
@@ -1757,6 +1762,7 @@ smb_proc_trunc64(struct inode *inode, lo
 	char *data;
 	struct smb_request *req;
 
+	lock_kernel();
 	result = -ENOMEM;
 	if (! (req = smb_alloc_request(server, 14)))
 		goto out;
@@ -1787,14 +1793,19 @@ smb_proc_trunc64(struct inode *inode, lo
 out_free:
 	smb_rput(req);
 out:
+	unlock_kernel();
 	return result;
 }
 
 static int
 smb_proc_trunc95(struct inode *inode, loff_t length)
 {
-	struct smb_sb_info *server = server_from_inode(inode);
-	int result = smb_proc_trunc32(inode, length);
+	struct smb_sb_info *server;
+	int result;
+ 
+	lock_kernel();
+	server = server_from_inode(inode);
+	result = smb_proc_trunc32(inode, length);
  
 	/*
 	 * win9x doesn't appear to update the size immediately.
@@ -1804,6 +1815,7 @@ smb_proc_trunc95(struct inode *inode, lo
 	 * FIXME: is this still necessary?
 	 */
 	smb_proc_flush(server, SMB_I(inode)->fileid);
+	unlock_kernel();
 	return result;
 }
 
--- 2.5.42/fs/sysv/itree.c~truncate-bkl	Sun Oct 13 21:20:07 2002
+++ 2.5.42-akpm/fs/sysv/itree.c	Sun Oct 13 21:20:43 2002
@@ -373,6 +373,8 @@ void sysv_truncate (struct inode * inode
 	    S_ISLNK(inode->i_mode)))
 		return;
 
+	lock_kernel();
+
 	blocksize = inode->i_sb->s_blocksize;
 	iblock = (inode->i_size + blocksize-1)
 					>> inode->i_sb->s_blocksize_bits;
@@ -381,7 +383,7 @@ void sysv_truncate (struct inode * inode
 
 	n = block_to_path(inode, iblock, offsets);
 	if (n == 0)
-		return;
+		goto out;
 
 	if (n == 1) {
 		free_data(inode, i_data+offsets[0], i_data + DIRECT);
@@ -421,6 +423,8 @@ do_indirects:
 		sysv_sync_inode (inode);
 	else
 		mark_inode_dirty(inode);
+out:
+	unlock_kernel();
 }
 
 static unsigned sysv_nblocks(struct super_block *s, loff_t size)
--- 2.5.42/fs/xfs/linux/xfs_iops.c~truncate-bkl	Sun Oct 13 21:22:15 2002
+++ 2.5.42-akpm/fs/xfs/linux/xfs_iops.c	Sun Oct 13 21:23:26 2002
@@ -32,6 +32,7 @@
 
 #include <xfs.h>
 #include <linux/xattr.h>
+#include <linux/smp_lock.h>
 
 
 /*
@@ -482,6 +483,8 @@ linvfs_setattr(
 	int		error;
 	int		flags = 0;
 
+	lock_kernel();
+
 	memset(&vattr, 0, sizeof(vattr_t));
 	if (ia_valid & ATTR_UID) {
 		vattr.va_mask |= AT_UID;
@@ -521,8 +524,10 @@ linvfs_setattr(
 		flags = ATTR_UTIME;
 
 	VOP_SETATTR(vp, &vattr, flags, NULL, error);
-	if (error)
-		return(-error); /* Positive error up from XFS */
+	if (error) {
+		error = -error;	/* Positive error up from XFS */
+		goto out;
+	}
 	if (ia_valid & ATTR_SIZE) {
 		error = vmtruncate(inode, attr->ia_size);
 	}
@@ -531,6 +536,8 @@ linvfs_setattr(
 		vn_revalidate(vp, 0);
 		mark_inode_dirty_sync(inode);
 	}
+out:
+	unlock_kernel();
 	return error;
 }
 

.

Re: [patch] remove BKL from inode_setattr

[Steve Lord]

On Mon, 2002-10-14 at 01:34, Andrew Morton wrote:

> 
> The number of filsystems which do not take the bkl in truncate/setattr
> is in fact quite small.  Here's the patch which removes all doubt:
> 
> 
> 
> 
>  fs/affs/file.c          |   13 ++++++++-----
>  fs/attr.c               |    2 --
>  fs/cifs/inode.c         |    7 ++++++-
>  fs/jfs/file.c           |    3 +++
>  fs/reiserfs/file.c      |    2 ++
>  fs/smbfs/proc.c         |   18 +++++++++++++++---
>  fs/sysv/itree.c         |    6 +++++-
>  fs/xfs/linux/xfs_iops.c |   11 +++++++++--
>  8 files changed, 48 insertions(+), 14 deletions(-)

XFS deliberately does not take the BKL - anywhere. Our setattr
code is doing its own locking. You just added the BKL to a 
bunch of xfs operations which do not need it. Now, vmtruncate
may need it, itself, but if vmtruncate does not, then the xfs
callout from vmtruncate certainly does not.

Steve

-- 

Steve Lord                                      voice: +1-651-683-3511
Principal Engineer, Filesystem Software         email: lord@sgi.com

Re: [patch] remove BKL from inode_setattr

[Dave Kleikamp]

On Monday 14 October 2002 01:34, Andrew Morton wrote:

> --- 2.5.42/fs/jfs/file.c~truncate-bkl	Sun Oct 13 21:11:06 2002
> +++ 2.5.42-akpm/fs/jfs/file.c	Sun Oct 13 21:11:11 2002
> @@ -18,6 +18,7 @@
>   */
>
>  #include <linux/fs.h>
> +#include <linux/smp_lock.h>
>  #include "jfs_incore.h"
>  #include "jfs_dmap.h"
>  #include "jfs_txnmgr.h"
> @@ -90,9 +91,11 @@ static void jfs_truncate(struct inode *i
>  {
>  	jFYI(1, ("jfs_truncate: size = 0x%lx\n", (ulong) ip->i_size));
>
> +	lock_kernel();
>  	IWRITE_LOCK(ip);
>  	jfs_truncate_nolock(ip, ip->i_size);
>  	IWRITE_UNLOCK(ip);
> +	unlock_kernel();
>  }
>
>  static int jfs_open(struct inode *inode, struct file *file)

JFS does not need the BKL.  It does it's own locking.

-- 
David Kleikamp
IBM Linux Technology Center

Re: [patch] remove BKL from inode_setattr

[Andrew Morton]

Steve Lord wrote:
> 
> On Mon, 2002-10-14 at 01:34, Andrew Morton wrote:
> 
> >
> > The number of filsystems which do not take the bkl in truncate/setattr
> > is in fact quite small.  Here's the patch which removes all doubt:
> >
> >
> >
> >
> >  fs/affs/file.c          |   13 ++++++++-----
> >  fs/attr.c               |    2 --
> >  fs/cifs/inode.c         |    7 ++++++-
> >  fs/jfs/file.c           |    3 +++
> >  fs/reiserfs/file.c      |    2 ++
> >  fs/smbfs/proc.c         |   18 +++++++++++++++---
> >  fs/sysv/itree.c         |    6 +++++-
> >  fs/xfs/linux/xfs_iops.c |   11 +++++++++--
> >  8 files changed, 48 insertions(+), 14 deletions(-)
> 
> XFS deliberately does not take the BKL - anywhere. Our setattr
> code is doing its own locking. You just added the BKL to a
> bunch of xfs operations which do not need it. Now, vmtruncate
> may need it, itself, but if vmtruncate does not, then the xfs
> callout from vmtruncate certainly does not.
> 

Sorry, but that is standard "bkl migration" methodology.  You had it
before, so you get it after.  It is not my role to change XFS locking.

Anyway, I don't think these patches are going anywhere.

Re: [patch] remove BKL from inode_setattr

[Steve Lord]

On Mon, 2002-10-14 at 11:41, Andrew Morton wrote:
> > 
> > XFS deliberately does not take the BKL - anywhere. Our setattr
> > code is doing its own locking. You just added the BKL to a
> > bunch of xfs operations which do not need it. Now, vmtruncate
> > may need it, itself, but if vmtruncate does not, then the xfs
> > callout from vmtruncate certainly does not.
> > 
> 
> Sorry, but that is standard "bkl migration" methodology.  You had it
> before, so you get it after.  It is not my role to change XFS locking.

But you did .... my point was, XFS does not use the BKL at all, has
never needed it and never will. The setattr call you added it to
meant you added it to chown, chmod etc. When the BKL was migrated
down below the vfs layer in all those places I deliberately did not
add it to the XFS calls.

> 
> Anyway, I don't think these patches are going anywhere.

No problem,

Steve

-- 

Steve Lord                                      voice: +1-651-683-3511
Principal Engineer, Filesystem Software         email: lord@sgi.com