[PATCH] UBIFS: kill BKL

[PATCH] UBIFS: kill BKL

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

The BKL was pushed down from VFS to the file-systems. It used
to serialize mount/unmount/remount. UBIFS must be safe if several
file-systems are mounted/unmounted/re-mounted at the same time,
so kill kick the BKL out of UBIFS.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/super.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 26d2e0d..4ad992f 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1726,8 +1726,6 @@ static void ubifs_put_super(struct super_block *sb)
 	ubifs_msg("un-mount UBI device %d, volume %d", c->vi.ubi_num,
 		  c->vi.vol_id);
 
-	lock_kernel();
-
 	/*
 	 * The following asserts are only valid if there has not been a failure
 	 * of the media. For example, there will be dirty inodes if we failed
@@ -1792,8 +1790,6 @@ static void ubifs_put_super(struct super_block *sb)
 	ubi_close_volume(c->ubi);
 	mutex_unlock(&c->umount_mutex);
 	kfree(c);
-
-	unlock_kernel();
 }
 
 static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
@@ -1809,24 +1805,18 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
 		return err;
 	}
 
-	lock_kernel();
 	if ((sb->s_flags & MS_RDONLY) && !(*flags & MS_RDONLY)) {
 		if (c->ro_media) {
 			ubifs_msg("cannot re-mount due to prior errors");
-			unlock_kernel();
 			return -EROFS;
 		}
 		err = ubifs_remount_rw(c);
-		if (err) {
-			unlock_kernel();
+		if (err)
 			return err;
-		}
 	} else if (!(sb->s_flags & MS_RDONLY) && (*flags & MS_RDONLY)) {
-		if (c->ro_media) {
+		if (c->ro_media)
 			ubifs_msg("cannot re-mount due to prior errors");
-			unlock_kernel();
 			return -EROFS;
-		}
 		ubifs_remount_ro(c);
 	}
 
@@ -1839,7 +1829,6 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
 	}
 
 	ubifs_assert(c->lst.taken_empty_lebs > 0);
-	unlock_kernel();
 	return 0;
 }
 
-- 
1.6.0.6

Re: [PATCH] UBIFS: kill BKL

[Adrian Hunter]

Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> The BKL was pushed down from VFS to the file-systems. It used
> to serialize mount/unmount/remount. UBIFS must be safe if several
> file-systems are mounted/unmounted/re-mounted at the same time,
> so kill kick the BKL out of UBIFS.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---

Looks like protection is always provided by sb->s_umount

Missing {} below, btw.

>  fs/ubifs/super.c |   15 ++-------------
>  1 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 26d2e0d..4ad992f 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1726,8 +1726,6 @@ static void ubifs_put_super(struct super_block *sb)
>  	ubifs_msg("un-mount UBI device %d, volume %d", c->vi.ubi_num,
>  		  c->vi.vol_id);
>  
> -	lock_kernel();
> -
>  	/*
>  	 * The following asserts are only valid if there has not been a failure
>  	 * of the media. For example, there will be dirty inodes if we failed
> @@ -1792,8 +1790,6 @@ static void ubifs_put_super(struct super_block *sb)
>  	ubi_close_volume(c->ubi);
>  	mutex_unlock(&c->umount_mutex);
>  	kfree(c);
> -
> -	unlock_kernel();
>  }
>  
>  static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
> @@ -1809,24 +1805,18 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
>  		return err;
>  	}
>  
> -	lock_kernel();
>  	if ((sb->s_flags & MS_RDONLY) && !(*flags & MS_RDONLY)) {
>  		if (c->ro_media) {
>  			ubifs_msg("cannot re-mount due to prior errors");
> -			unlock_kernel();
>  			return -EROFS;
>  		}
>  		err = ubifs_remount_rw(c);
> -		if (err) {
> -			unlock_kernel();
> +		if (err)
>  			return err;
> -		}
>  	} else if (!(sb->s_flags & MS_RDONLY) && (*flags & MS_RDONLY)) {
> -		if (c->ro_media) {
> +		if (c->ro_media)

Missing {}

>  			ubifs_msg("cannot re-mount due to prior errors");
> -			unlock_kernel();
>  			return -EROFS;
> -		}
>  		ubifs_remount_ro(c);
>  	}
>  
> @@ -1839,7 +1829,6 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
>  	}
>  
>  	ubifs_assert(c->lst.taken_empty_lebs > 0);
> -	unlock_kernel();
>  	return 0;
>  }
>  

Re: [PATCH] UBIFS: kill BKL

[Artem Bityutskiy]

On 07/20/2009 09:16 AM, Adrian Hunter wrote:
> Artem Bityutskiy wrote:
>> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
>>
>> The BKL was pushed down from VFS to the file-systems. It used
>> to serialize mount/unmount/remount. UBIFS must be safe if several
>> file-systems are mounted/unmounted/re-mounted at the same time,
>> so kill kick the BKL out of UBIFS.
>>
>> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
>> ---
>
> Looks like protection is always provided by sb->s_umount
>
> Missing {} below, btw.

Ugrh. Right. Will fix shortly.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
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] UBIFS: kill BKL

[Artem Bityutskiy]

On 07/20/2009 09:16 AM, Hunter Adrian (Nokia-D/Helsinki) wrote:
> Artem Bityutskiy wrote:
>> From: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
>>
>> The BKL was pushed down from VFS to the file-systems. It used
>> to serialize mount/unmount/remount. UBIFS must be safe if several
>> file-systems are mounted/unmounted/re-mounted at the same time,
>> so kill kick the BKL out of UBIFS.
>>
>> Signed-off-by: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
>> ---
>
> Looks like protection is always provided by sb->s_umount

Err, then what would be a possible reason we would need BKL?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
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] UBIFS: kill BKL

[Adrian Hunter]

Bityutskiy Artem (Nokia-D/Helsinki) wrote:
> On 07/20/2009 09:16 AM, Hunter Adrian (Nokia-D/Helsinki) wrote:
>> Artem Bityutskiy wrote:
>>> From: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
>>>
>>> The BKL was pushed down from VFS to the file-systems. It used
>>> to serialize mount/unmount/remount. UBIFS must be safe if several
>>> file-systems are mounted/unmounted/re-mounted at the same time,
>>> so kill kick the BKL out of UBIFS.
>>>
>>> Signed-off-by: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
>>> ---
>> Looks like protection is always provided by sb->s_umount
> 
> Err, then what would be a possible reason we would need BKL?

Say if we had used BKL instead of c->umount_mutex.
i.e. if we had relied on BKL in some other code
to provide synchronisation with unmounting/remounting etc

Re: [PATCH] UBIFS: kill BKL

[Artem Bityutskiy]

On Mon, 2009-07-20 at 09:16 +0300, Adrian Hunter wrote:
> Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > 
> > The BKL was pushed down from VFS to the file-systems. It used
> > to serialize mount/unmount/remount. UBIFS must be safe if several
> > file-systems are mounted/unmounted/re-mounted at the same time,
> > so kill kick the BKL out of UBIFS.
> > 
> > Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > ---
> 
> Looks like protection is always provided by sb->s_umount

Not for 2 or more instances of fs mounted/umounted/remounted at
the same time, which I meant in my comment. But I've fixed the
comment.

> Missing {} below, btw.

New version is below.

>From 347a38db88429400f0f479dc4d7de2b673999433 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Sun, 19 Jul 2009 13:51:04 +0300
Subject: [PATCH] UBIFS: kill BKL

The BKL was pushed down from VFS to the file-systems. It used
to serialize mount/unmount/remount and prevented more than one
instance of the same file-system from doing
mount/umount/remount at the same time. But it is OK for UBIFS
and it does not need any additional locking for these cases.
Thus, kick the BKL out of UBIFS.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
Changelog: fixed parentheses
           remove #include <linux/smp_lock.h>

 fs/ubifs/super.c |   13 +------------
 1 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 26d2e0d..13e7ed4 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -36,7 +36,6 @@
 #include <linux/mount.h>
 #include <linux/math64.h>
 #include <linux/writeback.h>
-#include <linux/smp_lock.h>
 #include "ubifs.h"
 
 /*
@@ -1726,8 +1725,6 @@ static void ubifs_put_super(struct super_block *sb)
 	ubifs_msg("un-mount UBI device %d, volume %d", c->vi.ubi_num,
 		  c->vi.vol_id);
 
-	lock_kernel();
-
 	/*
 	 * The following asserts are only valid if there has not been a failure
 	 * of the media. For example, there will be dirty inodes if we failed
@@ -1792,8 +1789,6 @@ static void ubifs_put_super(struct super_block *sb)
 	ubi_close_volume(c->ubi);
 	mutex_unlock(&c->umount_mutex);
 	kfree(c);
-
-	unlock_kernel();
 }
 
 static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
@@ -1809,22 +1804,17 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
 		return err;
 	}
 
-	lock_kernel();
 	if ((sb->s_flags & MS_RDONLY) && !(*flags & MS_RDONLY)) {
 		if (c->ro_media) {
 			ubifs_msg("cannot re-mount due to prior errors");
-			unlock_kernel();
 			return -EROFS;
 		}
 		err = ubifs_remount_rw(c);
-		if (err) {
-			unlock_kernel();
+		if (err)
 			return err;
-		}
 	} else if (!(sb->s_flags & MS_RDONLY) && (*flags & MS_RDONLY)) {
 		if (c->ro_media) {
 			ubifs_msg("cannot re-mount due to prior errors");
-			unlock_kernel();
 			return -EROFS;
 		}
 		ubifs_remount_ro(c);
@@ -1839,7 +1829,6 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
 	}
 
 	ubifs_assert(c->lst.taken_empty_lebs > 0);
-	unlock_kernel();
 	return 0;
 }
 
-- 
1.6.0.6

Re: [PATCH] UBIFS: kill BKL

[Christoph Hellwig]

On Sun, Jul 19, 2009 at 01:55:22PM +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> The BKL was pushed down from VFS to the file-systems. It used
> to serialize mount/unmount/remount. UBIFS must be safe if several
> file-systems are mounted/unmounted/re-mounted at the same time,
> so kill kick the BKL out of UBIFS.

On something slightly related - anohter part of the super_ops
rework in 2.6.31 was that we now always push data out before calling
into ->sync_fs to synchronize the metadata.

This means the generic_sync_sb_inodes call in ubifs_sync_fs should go
away now.

Re: [PATCH] UBIFS: kill BKL

[Artem Bityutskiy]

On Mon, 2009-07-20 at 08:51 -0400, Christoph Hellwig wrote:
> On something slightly related - anohter part of the super_ops
> rework in 2.6.31 was that we now always push data out before calling
> into ->sync_fs to synchronize the metadata.
> 
> This means the generic_sync_sb_inodes call in ubifs_sync_fs should go
> away now.

Yes, indeed '__sync_filesystem()' invokes 'sync_inodes_sb()' before
'->sync_sb()'. Thanks for pointing this out. Below is the patch.

>From c14ed92bf8639c8533f8e923859b136d0671db1b Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Mon, 20 Jul 2009 17:56:19 +0300
Subject: [PATCH] UBIFS: remove unneeded call from ubifs_sync_fs

Nowadays VFS always synchronizes all dirty inodes and pages before
calling '->sync_fs()', so remove unneeded 'generic_sync_sb_inodes()'
from 'ubifs_sync_fs()'. It used to be needed, but not any longer.

Pointed-out-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/super.c |   24 +++++++-----------------
 1 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 13e7ed4..b541bd7 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -437,12 +437,6 @@ static int ubifs_sync_fs(struct super_block *sb, int wait)
 {
 	int i, err;
 	struct ubifs_info *c = sb->s_fs_info;
-	struct writeback_control wbc = {
-		.sync_mode   = WB_SYNC_ALL,
-		.range_start = 0,
-		.range_end   = LLONG_MAX,
-		.nr_to_write = LONG_MAX,
-	};
 
 	/*
 	 * Zero @wait is just an advisory thing to help the file system shove
@@ -453,17 +447,6 @@ static int ubifs_sync_fs(struct super_block *sb, int wait)
 		return 0;
 
 	/*
-	 * VFS calls '->sync_fs()' before synchronizing all dirty inodes and
-	 * pages, so synchronize them first, then commit the journal. Strictly
-	 * speaking, it is not necessary to commit the journal here,
-	 * synchronizing write-buffers would be enough. But committing makes
-	 * UBIFS free space predictions much more accurate, so we want to let
-	 * the user be able to get more accurate results of 'statfs()' after
-	 * they synchronize the file system.
-	 */
-	generic_sync_sb_inodes(sb, &wbc);
-
-	/*
 	 * Synchronize write buffers, because 'ubifs_run_commit()' does not
 	 * do this if it waits for an already running commit.
 	 */
@@ -473,6 +456,13 @@ static int ubifs_sync_fs(struct super_block *sb, int wait)
 			return err;
 	}
 
+	/*
+	 * Strictly speaking, it is not necessary to commit the journal here,
+	 * synchronizing write-buffers would be enough. But committing makes
+	 * UBIFS free space predictions much more accurate, so we want to let
+	 * the user be able to get more accurate results of 'statfs()' after
+	 * they synchronize the file system.
+	 */
 	err = ubifs_run_commit(c);
 	if (err)
 		return err;
-- 
1.6.0.6

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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