Skip to contentSkip to conversation

[2.6] reiserfs: fix locking in reiserfs_remount

PATCH In review Oleg Drokin 4 participants 7 messages Aug 6, 2003 – Aug 7, 2003 VFS

lore
Share & tools
lore

Oleg Drokin

author
#0 lore raw

PATCH 1/1 [2.6] reiserfs: fix locking in reiserfs_remount

Hello!

Since reiserfs_remount can be called without BKL held, we better take BKL in there. Please apply the below patch. It is against 2.6.0-test2

Bye,
Oleg

===== fs/reiserfs/super.c 1.66 vs edited =====

fs/reiserfs/super.c +12 -3
@@ -761,6 +761,7 @@
   if (!reiserfs_parse_options(s, arg, &mount_options, &blocks, NULL))
     return -EINVAL;
   
+  reiserfs_write_lock(s);
   handle_attrs(s);
 
   /* Add options that are safe here */

@@ -778,17 +779,22 @@
 
   if(blocks) {
     int rc = reiserfs_resize(s, blocks);
-    if (rc != 0)
+    if (rc != 0) {
+      reiserfs_write_unlock(s);
       return rc;
+    }
   }
 
   if (*mount_flags & MS_RDONLY) {
     /* remount read-only */
-    if (s->s_flags & MS_RDONLY)
+    if (s->s_flags & MS_RDONLY) {
       /* it is read-only already */
+      reiserfs_write_unlock(s);
       return 0;
+    }
     /* try to remount file system with read-only permissions */
     if (sb_umount_state(rs) == REISERFS_VALID_FS || REISERFS_SB(s)->s_mount_state != REISERFS_VALID_FS) {
+      reiserfs_write_unlock(s);
       return 0;
     }
 

@@ -800,8 +806,10 @@
     s->s_dirt = 0;
   } else {
     /* remount read-write */
-    if (!(s->s_flags & MS_RDONLY))
+    if (!(s->s_flags & MS_RDONLY)) {
+        reiserfs_write_unlock(s);
 	return 0; /* We are read-write already */
+    }
 
     REISERFS_SB(s)->s_mount_state = sb_umount_state(rs) ;
     s->s_flags &= ~MS_RDONLY ; /* now it is safe to call journal_begin */

@@ -824,6 +832,7 @@
   if (!( *mount_flags & MS_RDONLY ) )
     finish_unfinished( s );
 
+  reiserfs_write_unlock(s);
   return 0;
 }
#1 lore raw
Quoted 4 lines · Oleg Drokin

Hello!

Since reiserfs_remount can be called without BKL held, we better take BKL in there. Please apply the below patch. It is against 2.6.0-test2

is the BKL in reiserfs_write_unlock()?

Do we need to be adding more BKL usage, instead of the same or less at this point?

author
#2 lore raw

Hello!

Quoted 3 lines · Mike Fedyk
Quoted 2 lines

Since reiserfs_remount can be called without BKL held, we better take BKL in there. Please apply the below patch. It is against 2.6.0-test2

is the BKL in reiserfs_write_unlock()?

Yes.

Quoted 2 lines

Do we need to be adding more BKL usage, instead of the same or less at this point?

Reiserfs needs BKL for it's journal operations. This is not "more", for some time BKL was taken in the VFS, then whoever removed that, forgot to propagate BKL down to actual fs methods that need the BKL.

Bye,
Oleg

#3 lore raw
Quoted 26 lines · Oleg Drokin

Hello!

Quoted 7 lines · Mike Fedyk
Quoted 4 lines

Since reiserfs_remount can be called without BKL held, we better take BKL in there. Please apply the below patch. It is against 2.6.0-test2

is the BKL in reiserfs_write_unlock()?

Yes.

Quoted 4 lines

Do we need to be adding more BKL usage, instead of the same or less at this point?

Reiserfs needs BKL for it's journal operations. This is not "more", for some time BKL was taken in the VFS, then whoever removed that, forgot to propagate BKL down to actual fs methods that need the BKL.

Quoted 11 lines

Bye,
Oleg
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Is it known who removed it?

Signature · 1 line
Hans
author
#4 lore raw

Hello!

Quoted 4 lines · Hans Reiser
Quoted 3 lines

Reiserfs needs BKL for it's journal operations. This is not "more", for some time BKL was taken in the VFS, then whoever removed that, forgot to propagate BKL down to actual fs methods that need the BKL.

Is it known who removed it?

I think it was Andrew. At least this new emergency remount path without BKL was introduced by his patch without any extra attribution.

Bye,
Oleg

#5 lore raw
Quoted 5 lines · Oleg Drokin

Hello!

On Thu, Aug 07, 2003 at 05:24:43PM +0400, Hans Reiser wrote:

Quoted 7 lines
Quoted 4 lines
Quoted 3 lines

Reiserfs needs BKL for it's journal operations. This is not "more", for some time BKL was taken in the VFS, then whoever removed that, forgot to propagate BKL down to actual fs methods that need the BKL.

Is it known who removed it?

I think it was Andrew. At least this new emergency remount path without BKL was introduced by his patch without any extra attribution.

Is that the only path? It appears that way to me.

The Locking document says that ->remoutn_fs is called under lock_kernel(), so this should be fixed at the VFS level.

 fs/super.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletion(-)
fs/super.c renamed from fs/super.c~remount_fs-lock_kernel +9 -1
diff -puN fs/super.c~remount_fs-lock_kernel fs/super.c
@@ -507,8 +507,16 @@ static void do_emergency_remount(unsigne
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 		down_read(&sb->s_umount);
-		if (sb->s_root && sb->s_bdev && !(sb->s_flags & MS_RDONLY))
+		if (sb->s_root && sb->s_bdev && !(sb->s_flags & MS_RDONLY)) {
+			/*
+			 * ->remount_fs needs lock_kernel().
+			 *
+			 * What lock protects sb->s_flags??
+			 */
+			lock_kernel();
 			do_remount_sb(sb, MS_RDONLY, NULL, 1);
+			unlock_kernel();
+		}
 		drop_super(sb);
 		spin_lock(&sb_lock);
 	}

_

author
#6 lore raw

Hello!

Quoted 5 lines · Andrew Morton
Quoted 2 lines

I think it was Andrew. At least this new emergency remount path without BKL was introduced by his patch without any extra attribution.

Is that the only path? It appears that way to me.
The Locking document says that ->remoutn_fs is called under lock_kernel(), so this should be fixed at the VFS level.

Hm. Indeed.

Bye,
Oleg