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

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

[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

Bye,
    Oleg

===== fs/reiserfs/super.c 1.66 vs edited =====
--- 1.66/fs/reiserfs/super.c	Sat Jun 21 00:16:06 2003
+++ edited/fs/reiserfs/super.c	Tue Aug  5 12:22:10 2003
@@ -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;
 }
 

Re: [PATCH] [2.6] reiserfs: fix locking in reiserfs_remount

[Mike Fedyk]

On Wed, Aug 06, 2003 at 01:38:58PM +0400, Oleg Drokin wrote:
> 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?

Re: [PATCH] [2.6] reiserfs: fix locking in reiserfs_remount

[Oleg Drokin]

Hello!

On Wed, Aug 06, 2003 at 10:28:13AM -0700, Mike Fedyk wrote:
> >     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.

> 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

Re: [PATCH] [2.6] reiserfs: fix locking in reiserfs_remount

[Hans Reiser]

Oleg Drokin wrote:

>Hello!
>
>On Wed, Aug 06, 2003 at 10:28:13AM -0700, Mike Fedyk wrote:
>  
>
>>>    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.
>
>  
>
>>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
>-
>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/
>
>
>  
>
Is it known who removed it?

-- 
Hans

Re: [PATCH] [2.6] reiserfs: fix locking in reiserfs_remount

[Oleg Drokin]

Hello!

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

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

Re: [PATCH] [2.6] reiserfs: fix locking in reiserfs_remount

[Andrew Morton]

Oleg Drokin <green@namesys.com> wrote:
>
> Hello!
> 
> On Thu, Aug 07, 2003 at 05:24:43PM +0400, Hans Reiser wrote:
> 
> > >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(-)

diff -puN fs/super.c~remount_fs-lock_kernel fs/super.c
--- 25/fs/super.c~remount_fs-lock_kernel	2003-08-07 08:17:38.000000000 -0700
+++ 25-akpm/fs/super.c	2003-08-07 08:23:42.000000000 -0700
@@ -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);
 	}

_

Re: [PATCH] [2.6] reiserfs: fix locking in reiserfs_remount

[Oleg Drokin]

Hello!

On Thu, Aug 07, 2003 at 08:24:40AM -0700, Andrew Morton wrote:
> > 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