exofs: remove lock/unlock super

PATCH In review Marco Stornelli 3 participants 10 messages Aug 16, 2012 VFS

lore

Share & tools

lore

Marco Stornelli

author

2012-08-16 10:00 UTC #0 lore raw

PATCH 2/8 exofs: remove lock/unlock super

From: Marco Stornelli <[email protected]>

Remove lock and unlock super operation.

Signed-off-by Marco Stornelli <[email protected]>

---

fs/exofs/super.c +0 -2

diff -Nurp linux-3.6-rc1-orig/fs/exofs/super.c linux-3.6-rc1/fs/exofs/super.c
@@ -384,8 +384,6 @@ static int exofs_sync_fs(struct super_bl
 	if (unlikely(ret))
 		goto out;
 -	lock_super(sb);
-
 	ios->length = offsetof(struct exofs_fscb, s_dev_table_oid);
 	memset(fscb, 0, ios->length);
 	fscb->s_nextid = cpu_to_le64(sbi->s_nextid);

@@ -403,8 +401,6 @@ static int exofs_sync_fs(struct super_bl
 	else
 		sb->s_dirt = 0;
 -
-	unlock_super(sb);
 out:
 	EXOFS_DBGMSG("s_nextid=0x%llx ret=%d\n", _LLU(sbi->s_nextid), ret);
 	ore_put_io_state(ios);

Artem Bityutskiy

Acked-by on v1 2/8

2012-08-16 12:16 UTC #1 lore raw

Quoted 5 lines · Marco Stornelli

From: Marco Stornelli <[email protected]>
Remove lock and unlock super operation.
Signed-off-by Marco Stornelli <[email protected]>

Acked-by Artem Bityutskiy <[email protected]>

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Boaz Harrosh

2012-08-16 12:32 UTC #2 lore raw

Quoted 9 lines · Artem Bityutskiy

Quoted 5 lines · Marco Stornelli
From: Marco Stornelli <[email protected]>
Remove lock and unlock super operation.
Signed-off-by Marco Stornelli <[email protected]>
Acked-by Artem Bityutskiy <[email protected]>

Are you sure? It used to be that exofs_sync_fs() could be called concurrently.

What about two "bash -c sync" calls or a sync and an unmount in parallel. anything protecting that?

If so then sure, but please let me test first.
Boaz

Artem Bityutskiy

2012-08-16 13:05 UTC #3 lore raw

Quoted 13 lines · Boaz Harrosh

Quoted 9 lines · Artem Bityutskiy
Quoted 5 lines · Marco Stornelli
From: Marco Stornelli <[email protected]>
Remove lock and unlock super operation.
Signed-off-by Marco Stornelli <[email protected]>
Acked-by Artem Bityutskiy <[email protected]>

Quoted 7 lines

Are you sure? It used to be that exofs_sync_fs() could be called concurrently.
What about two "bash -c sync" calls or a sync and an unmount in parallel. anything protecting that?
If so then sure, but please let me test first.

Umm, actually we will probably end up writing the same twice without the lock.

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Boaz Harrosh

2012-08-16 13:15 UTC #4 lore raw

Quoted 17 lines · Artem Bityutskiy

Quoted 16 lines · Boaz Harrosh
Quoted 9 lines · Artem Bityutskiy
Quoted 5 lines · Marco Stornelli
From: Marco Stornelli <[email protected]>
Remove lock and unlock super operation.
Signed-off-by Marco Stornelli <[email protected]>
Acked-by Artem Bityutskiy <[email protected]>
Are you sure? It used to be that exofs_sync_fs() could be called concurrently.

Quoted 8 lines

Quoted 4 lines
What about two "bash -c sync" calls or a sync and an unmount in parallel. anything protecting that?
If so then sure, but please let me test first.
Umm, actually we will probably end up writing the same twice without the lock.

No we are not allowed to run exofs_sync_fs() concurrently because it uses a per-alllocated scratch buffer to do it's stuff so you can end up with data corruption on disk.

And we cannot use a spin-lock because we might sleep in ore_write()

There are some optimizations I can do here, but lets for now just do the sb->s_lock thing, and I might decide to completely revamp the all thing later.

Thanks
Boaz

Boaz Harrosh

2012-08-16 13:23 UTC #5 lore raw

Quoted 24 lines · Boaz Harrosh

Quoted 22 lines · Artem Bityutskiy
Quoted 20 lines · Boaz Harrosh
Quoted 9 lines · Artem Bityutskiy
Quoted 5 lines · Marco Stornelli
From: Marco Stornelli <[email protected]>
Remove lock and unlock super operation.
Signed-off-by Marco Stornelli <[email protected]>
Acked-by Artem Bityutskiy <[email protected]>
Are you sure? It used to be that exofs_sync_fs() could be called concurrently.
What about two "bash -c sync" calls or a sync and an unmount in parallel. anything protecting that?
If so then sure, but please let me test first.

Quoted 9 lines

Quoted 3 lines
Umm, actually we will probably end up writing the same twice without the lock.
No we are not allowed to run exofs_sync_fs() concurrently because it uses a per-alllocated scratch buffer to do it's stuff so you can end up with data corruption on disk.

I take that back. We kmalloc. (We used to not too).

In theory the counter in question could change mid flight and the IOs can be submitted out of order, But the chance for that is very very slim. So It's fine. It's a deprecated counter anyway, so I'm OK to drop it.

I'll completely remove it the next time I increment the sb version number.

Thanks
Boaz

Quoted 8 lines

And we cannot use a spin-lock because we might sleep in ore_write()
There are some optimizations I can do here, but lets for now just do the sb->s_lock thing, and I might decide to completely revamp the all thing later.
Thanks
Boaz

Artem Bityutskiy

2012-08-16 13:25 UTC #6 lore raw

In reply to Boaz Harrosh

Quoted 3 lines · Boaz Harrosh

There are some optimizations I can do here, but lets for now just do the sb->s_lock thing, and I might decide to completely revamp the all thing later.

OK. But I guess an exofs-specific mutex could be used instead. We do not have to depend on 'sb->s_lock'.

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Boaz Harrosh

2012-08-16 13:29 UTC #7 lore raw

Quoted 8 lines · Artem Bityutskiy

Quoted 3 lines · Boaz Harrosh
There are some optimizations I can do here, but lets for now just do the sb->s_lock thing, and I might decide to completely revamp the all thing later.
OK. But I guess an exofs-specific mutex could be used instead. We do not have to depend on 'sb->s_lock'.

sorry I meant sbi->s_lock. But I'm fine with the complete drop now

Thanks
Boaz

Marco Stornelli

author

2012-08-16 16:09 UTC #8 lore raw

In reply to Boaz Harrosh

Il 16/08/2012 14:32, Boaz Harrosh ha scritto:

Quoted 16 lines · Artem Bityutskiy

Quoted 10 lines · Marco Stornelli
Quoted 5 lines
From: Marco Stornelli <[email protected]>
Remove lock and unlock super operation.
Signed-off-by Marco Stornelli <[email protected]>
Acked-by Artem Bityutskiy <[email protected]>
Are you sure? It used to be that exofs_sync_fs() could be called concurrently.

Quoted 8 lines

What about two "bash -c sync" calls or a sync and an unmount in parallel. anything protecting that?
If so then sure, but please let me test first.
Boaz
.

Yes Boaz, I agree, it's the reason I asked for a comments for exofs and hpfs. The only reason to have a lock here is to serialize operations in parallel on the same super block.

Marco

Boaz Harrosh

2012-08-16 13:23 UTC #9 lore raw

In reply to Marco Stornelli

Quoted 4 lines · Marco Stornelli

From: Marco Stornelli <[email protected]>
Remove lock and unlock super operation.

ACK-by Boaz Harrosh <[email protected]>

Quoted 22 lines

Signed-off-by Marco Stornelli <[email protected]>

---
diff -Nurp linux-3.6-rc1-orig/fs/exofs/super.c linux-3.6-rc1/fs/exofs/super.c

fs/exofs/super.c +0 -2

@@ -384,8 +384,6 @@ static int exofs_sync_fs(struct super_bl
 	if (unlikely(ret))
 		goto out;
 -	lock_super(sb);
-
 	ios->length = offsetof(struct exofs_fscb, s_dev_table_oid);
 	memset(fscb, 0, ios->length);
 	fscb->s_nextid = cpu_to_le64(sbi->s_nextid);

@@ -403,8 +401,6 @@ static int exofs_sync_fs(struct super_bl
 	else
 		sb->s_dirt = 0;
 -
-	unlock_super(sb);
 out:
 	EXOFS_DBGMSG("s_nextid=0x%llx ret=%d\n", _LLU(sbi->s_nextid), ret);
 	ore_put_io_state(ios);