[PATCH 2/8] exofs: remove lock/unlock super

[PATCH 2/8] exofs: remove lock/unlock super

[Marco Stornelli]

From: Marco Stornelli <marco.stornelli@gmail.com>

Remove lock and unlock super operation.

Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
---
diff -Nurp linux-3.6-rc1-orig/fs/exofs/super.c linux-3.6-rc1/fs/exofs/super.c
--- linux-3.6-rc1-orig/fs/exofs/super.c	2012-08-16 09:37:31.000000000 +0200
+++ linux-3.6-rc1/fs/exofs/super.c	2012-08-16 09:45:31.000000000 +0200
@@ -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);

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

On Thu, 2012-08-16 at 12:00 +0200, Marco Stornelli wrote:
> From: Marco Stornelli <marco.stornelli@gmail.com>
> 
> Remove lock and unlock super operation.
> 
> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>

Acked-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Boaz Harrosh]

On 08/16/2012 03:20 PM, Artem Bityutskiy wrote:

> On Thu, 2012-08-16 at 12:00 +0200, Marco Stornelli wrote:
>> From: Marco Stornelli <marco.stornelli@gmail.com>
>>
>> Remove lock and unlock super operation.
>>
>> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
> 
> Acked-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 


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

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Thu, 2012-08-16 at 15:32 +0300, Boaz Harrosh wrote:
> On 08/16/2012 03:20 PM, Artem Bityutskiy wrote:
> 
> > On Thu, 2012-08-16 at 12:00 +0200, Marco Stornelli wrote:
> >> From: Marco Stornelli <marco.stornelli@gmail.com>
> >>
> >> Remove lock and unlock super operation.
> >>
> >> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
> > 
> > Acked-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> > 
> 
> 
> 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. 

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Boaz Harrosh]

On 08/16/2012 04:10 PM, Artem Bityutskiy wrote:

> On Thu, 2012-08-16 at 15:32 +0300, Boaz Harrosh wrote:
>> On 08/16/2012 03:20 PM, Artem Bityutskiy wrote:
>>
>>> On Thu, 2012-08-16 at 12:00 +0200, Marco Stornelli wrote:
>>>> From: Marco Stornelli <marco.stornelli@gmail.com>
>>>>
>>>> Remove lock and unlock super operation.
>>>>
>>>> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
>>>
>>> Acked-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>>>
>>
>>
>> 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. 
> 


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

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Boaz Harrosh]

On 08/16/2012 04:15 PM, Boaz Harrosh wrote:

> On 08/16/2012 04:10 PM, Artem Bityutskiy wrote:
> 
>> On Thu, 2012-08-16 at 15:32 +0300, Boaz Harrosh wrote:
>>> On 08/16/2012 03:20 PM, Artem Bityutskiy wrote:
>>>
>>>> On Thu, 2012-08-16 at 12:00 +0200, Marco Stornelli wrote:
>>>>> From: Marco Stornelli <marco.stornelli@gmail.com>
>>>>>
>>>>> Remove lock and unlock super operation.
>>>>>
>>>>> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
>>>>
>>>> Acked-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>>>>
>>>
>>>
>>> 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. 
>>
> 
> 
> 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

> 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

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

On Thu, 2012-08-16 at 16:15 +0300, Boaz Harrosh wrote:
> 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'.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Boaz Harrosh]

On 08/16/2012 04:30 PM, Artem Bityutskiy wrote:

> On Thu, 2012-08-16 at 16:15 +0300, Boaz Harrosh wrote:
>> 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

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Marco Stornelli]

Il 16/08/2012 14:32, Boaz Harrosh ha scritto:
> On 08/16/2012 03:20 PM, Artem Bityutskiy wrote:
>
>> On Thu, 2012-08-16 at 12:00 +0200, Marco Stornelli wrote:
>>> From: Marco Stornelli <marco.stornelli@gmail.com>
>>>
>>> Remove lock and unlock super operation.
>>>
>>> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
>>
>> Acked-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>>
>
>
> 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
>
> .
>

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

Re: [PATCH 2/8] exofs: remove lock/unlock super

[Boaz Harrosh]

On 08/16/2012 01:00 PM, Marco Stornelli wrote:

> From: Marco Stornelli <marco.stornelli@gmail.com>
> 
> Remove lock and unlock super operation.
> 


ACK-by: Boaz Harrosh <bharrosh@panasas.com>

> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>

> ---
> diff -Nurp linux-3.6-rc1-orig/fs/exofs/super.c linux-3.6-rc1/fs/exofs/super.c
> --- linux-3.6-rc1-orig/fs/exofs/super.c	2012-08-16 09:37:31.000000000 +0200
> +++ linux-3.6-rc1/fs/exofs/super.c	2012-08-16 09:45:31.000000000 +0200
> @@ -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);